As i recently wanted to have a timer as a class member and a naive
class Foo {
APP_TIMER_DEF(my_timer);
};
did not work I started to look into the source code. What I found where some very strange design decisions.
Let's start with a look at what the APP_TIMER_DEF macro actually does (app_timer.h line 107-109):
#define APP_TIMER_DEF(timer_id) \
static app_timer_t timer_id##_data = { {0} }; \
static const app_timer_id_t timer_id = &timer_id##_data
So this macro declares a variable of type app_timer_t (a struct with an uint32_t array as it's only member) with the name supplied to the macro appended with _data as its name and initialized with 0. It also declares a const variable of type app_timer_id_t and initializes it with a pointer to the variable it just declared. The first variable is essentially hidden from the user of the API.
As it turns out app_timer_id_t is just a typedef for a pointer to app_timer_t (app_timer.h line 100):
typedef app_timer_t * app_timer_id_t;
I would call that already bad practice but it gets even worse (or at least stranger).
Let us look next at the declaration of app_timer_create (app_timer.h line 209-212):
uint32_t app_timer_create(app_timer_id_t const * p_timer_id,
app_timer_mode_t mode,
app_timer_timeout_handler_t timeout_handler);
The first argument looks innocent enough at first but remember app_timer_id_t is just a typedef for a pointer to app_timer_t, so if we resolve the typedef p_time_id is of type app_timer_t* const*. Yes, that is a pointer to a const pointer to app_timer_t.
That seems strange, why would it make sense to have a pointer to a pointer? I assume that the typedef hiding the pointer led the nordic developer to making the mistake of making this double pointer without noticing it (which is one of the reasons why I consider such typedefs bad practice).
To check if there is a good reason for this pointer to pointer thing I also took a look at the definition of the function. The only use of the argument p_timer_id except for a error check to ensure it's !NULL is this (app_timer.c line 1009):
timer_node_t * p_node = (timer_node_t *)*p_timer_id;
Here the outer pointer gets dereferenced and the inner pointer gets casted to a pointer of a different type. So as far as I can see there is no reason to not just directly use a pointer to app_timer_t instead.
I also took a look at the rest of the implementation of the timer system but still couldn't find a reason for these design decisions.
So my question is: What is the reason for that macro, the typedef and the strange pointer to a pointer?
Because if there is no good reason for this I would propose to simplify the API so that users declare a variable of type app_timer_t directly instead of using the APP_TIMER_DEF macro, that all functions use pointers to app_timer_t as arguments and the app_timer_id_t type gets removed.
For reference I'm using SDK version 10.0.0 but there is no difference in version 11.0.0.
