Callback use of user_data is inconsistent or unavailable across SDK #71

New issue

Open

opened 2021-02-01 12:57:44 -06:00 by ndinsmore · 1 comment

ndinsmore commented

2021-02-01 12:57:44 -06:00

(Migrated from github.com)

The use of user_data in callbacks is of importance because it can allow callback to a class method when using C++/OOP via a non capturing Lambda function, which is great.

There is an inconsistency with how the user_data is implemented or if it is implemented in some cases.
The two different schemes are the alarm scheme where user_data is passed as a second argument to the callback:

typedef int64_t (*alarm_callback_t)(alarm_id_t id, void *user_data);

And the repeating timer scheme where the user_data is passed to the callback inside the repeating_timer_t:

typedef bool (*repeating_timer_callback_t)(repeating_timer_t *rt);
struct repeating_timer {
    int64_t delay_us;
    alarm_pool_t *pool;
    alarm_id_t alarm_id;
    repeating_timer_callback_t callback;
    void *user_data;
};

Having a consistent scheme between the two would be great, with the alarm scheme being more idiomatic from what I can tell.

Also of concern is the lack of user_data on the following callbacks and types.

typedef void (*gpio_irq_callback_t)(uint gpio, uint32_t events); 

typedef void(* resus_callback_t )(void);

typedef void (*rtc_callback_t)(void);

typedef void(* hardware_alarm_callback_t )(uint alarm_num)

With the lack on IRQs my primary concern as it makes for example a Sensor class for a hall effect sensor difficult.

The use of user_data in callbacks is of importance because it can allow callback to a class method when using C++/OOP via a non capturing Lambda function, which is great. There is an inconsistency with how the user_data is implemented or if it is implemented in some cases. The two different schemes are the alarm scheme where user_data is passed as a second argument to the callback: ```c typedef int64_t (*alarm_callback_t)(alarm_id_t id, void *user_data); ``` And the repeating timer scheme where the user_data is passed to the callback inside the repeating_timer_t: ```c typedef bool (*repeating_timer_callback_t)(repeating_timer_t *rt); struct repeating_timer { int64_t delay_us; alarm_pool_t *pool; alarm_id_t alarm_id; repeating_timer_callback_t callback; void *user_data; }; ``` Having a consistent scheme between the two would be great, with the alarm scheme being more idiomatic from what I can tell. Also of concern is the lack of user_data on the following callbacks and types. ```c typedef void (*gpio_irq_callback_t)(uint gpio, uint32_t events); typedef void(* resus_callback_t )(void); typedef void (*rtc_callback_t)(void); typedef void(* hardware_alarm_callback_t )(uint alarm_num) ``` With the lack on IRQs my primary concern as it makes for example a Sensor class for a hall effect sensor difficult.

kilograham commented

2021-04-08 17:22:45 -05:00

(Migrated from github.com)

The use of user_data in callbacks is of importance because it can allow callback to a class method when using C++/OOP via a non capturing Lambda function, which is great.

There is an inconsistency with how the user_data is implemented or if it is implemented in some cases.
The two different schemes are the alarm scheme where user_data is passed as a second argument to the callback:
typedef int64_t (*alarm_callback_t)(alarm_id_t id, void *user_data);
And the repeating timer scheme where the user_data is passed to the callback inside the repeating_timer_t:
typedef bool (*repeating_timer_callback_t)(repeating_timer_t *rt);
struct repeating_timer {
    int64_t delay_us;
    alarm_pool_t *pool;
    alarm_id_t alarm_id;
    repeating_timer_callback_t callback;
    void *user_data;
};
Having a consistent scheme between the two would be great, with the alarm scheme being more idiomatic from what I can tell.

The reason for the repeating timer owning the callback here, is that the repeating timer can be wholly user owned state (it seemed superfluous to allocate another object on top of the alarm). It then seemed equally superfluous to extract the user data out of the structure and marshall it as an argument

Also of concern is the lack of user_data on the following callbacks and types.
typedef void (*gpio_irq_callback_t)(uint gpio, uint32_t events); 

yes, the hold gpio irq API should not have made it in in its current form; it needs to be replaced.

typedef void(* resus_callback_t )(void);

typedef void (*rtc_callback_t)(void);

typedef void(* hardware_alarm_callback_t )(uint alarm_num)
With the lack on IRQs my primary concern as it makes for example a Sensor class for a hall effect sensor difficult.

We certainly don't associate any state with IRQ handlers themselves, however that can and should be handled/provided by any abstraction which provides multiplexed routing based on the underlying IRQ, for sure.

> The use of user_data in callbacks is of importance because it can allow callback to a class method when using C++/OOP via a non capturing Lambda function, which is great. > > There is an inconsistency with how the user_data is implemented or if it is implemented in some cases. > The two different schemes are the alarm scheme where user_data is passed as a second argument to the callback: > > ```c > typedef int64_t (*alarm_callback_t)(alarm_id_t id, void *user_data); > ``` > > And the repeating timer scheme where the user_data is passed to the callback inside the repeating_timer_t: > > ```c > typedef bool (*repeating_timer_callback_t)(repeating_timer_t *rt); > struct repeating_timer { > int64_t delay_us; > alarm_pool_t *pool; > alarm_id_t alarm_id; > repeating_timer_callback_t callback; > void *user_data; > }; > ``` > > Having a consistent scheme between the two would be great, with the alarm scheme being more idiomatic from what I can tell. > The reason for the repeating timer owning the callback here, is that the repeating timer can be wholly user owned state (it seemed superfluous to allocate another object on top of the alarm). It then seemed equally superfluous to extract the user data out of the structure and marshall it as an argument > Also of concern is the lack of user_data on the following callbacks and types. > > ```c > typedef void (*gpio_irq_callback_t)(uint gpio, uint32_t events); > yes, the hold gpio irq API should not have made it in in its current form; it needs to be replaced. > typedef void(* resus_callback_t )(void); > > typedef void (*rtc_callback_t)(void); > > typedef void(* hardware_alarm_callback_t )(uint alarm_num) > ``` > > With the lack on IRQs my primary concern as it makes for example a Sensor class for a hall effect sensor difficult. We certainly don't associate any state with IRQ handlers themselves, however that can and should be handled/provided by any abstraction which provides multiplexed routing based on the underlying IRQ, for sure.