esp_event: event finalization callback function

[wuyuanyi]

Hi, I am looking into using the esp_event to exchange data between my components. Currently, I am facing such an issue: my data is a non-trivial c++ class (e.g., stl containers), hence relying on the pass-by-copy mechanism of the event loop does not work for me. The alternative is to allocate the object on heap and pass by pointers. However, there is no guarantee that this memory could be released.

In my opionion, the event loop library can provide finalization callback mechanism for each event, so that the resource allocated by the event could be released after handling.

Related post: https://esp32.com/viewtopic.php?t=16720

[ESP_igrr]

Hi wuyuanyi,

What you describe should be possible. We could introduce something like esp_event_post_moved which would take ownership of the pointer and would accept the deletion callback argument instead of event_data_size.
Internally the deletion callback (finalizer) could be stored in an esp_event_post_instance_t structure. For events posted using the "normal" copying esp_event_post that would simply be a pointer to "free".

A few caveats which come to mind:

1. Originally we have designed esp_event with copy semantics to reduce the likelihood of users borrowing a pointer to data allocated on the stack, and sending it to another task. Providing this esp_event_post_moved function seems to invite such kind of mistakes, so we'll have to figure out how to make sure that users understand when not to use it.
2. Increased number of APIs: we already have several esp_event_post APIs: {post (to the default event loop), post_to (to the specified event loop)} x {post from task, post from ISR}. If we add another dimension ({copy, move}) then we'll have 8 similar APIs in total! Making sure the users don't get confused with all the option is something we need to think of.
3. The event queue occupies more RAM (from 12 bytes to 16 bytes per event, if CONFIG_ESP_EVENT_POST_FROM_ISR is disabled).

Those caveats aside, if you were to submit a PR with such a feature (and better yet, tests and docs!) we would most likely end up accepting it.

[wuyuanyi]

Hi wuyuanyi,

What you describe should be possible. We could introduce something like esp_event_post_moved which would take ownership of the pointer and would accept the deletion callback argument instead of event_data_size.
Internally the deletion callback (finalizer) could be stored in an esp_event_post_instance_t structure. For events posted using the "normal" copying esp_event_post that would simply be a pointer to "free".

A few caveats which come to mind:

1. Originally we have designed esp_event with copy semantics to reduce the likelihood of users borrowing a pointer to data allocated on the stack, and sending it to another task. Providing this esp_event_post_moved function seems to invite such kind of mistakes, so we'll have to figure out how to make sure that users understand when not to use it.
2. Increased number of APIs: we already have several esp_event_post APIs: {post (to the default event loop), post_to (to the specified event loop)} x {post from task, post from ISR}. If we add another dimension ({copy, move}) then we'll have 8 similar APIs in total! Making sure the users don't get confused with all the option is something we need to think of.
3. The event queue occupies more RAM (from 12 bytes to 16 bytes per event, if CONFIG_ESP_EVENT_POST_FROM_ISR is disabled).

Those caveats aside, if you were to submit a PR with such a feature (and better yet, tests and docs!) we would most likely end up accepting it.

Hi Igrr,
Thanks for your comments. What do you think about such a less intrusive approach (in terms of API change), by defining a structure
`
struct esp_event_payload_s {
void* data;
void (*free_fcn)(void* );
//// optionally, also allow passing the user context as a void*?
}
`
that represents a user-allocated event payload whose lifetime will be managed by the event-loop. It is associated with a special `event_data_size` value (e.g., 0x7fffffff), so that when posted, the loop only copy the pointer to the `struct esp_event_payload_s` rather than its content. There should be an extra flag added to `esp_event_post_instance_t` to indicate if this is the special struct esp_event_payload_s payload. When the event has been processed, if the flag is set, the loop invokes the `free_fcn` in the payload instead of the `free` to release the resource.

This way we do not need more API but it looks a bit more hacky. Also, would it be possible to tell if the pointer is on stack or heap-allocated? This may be helpful to prevent user passing in a local variable that will definitely crash the program.

I would be happy to contribute to the doc test etc, but I am not very familiar with the esp-idf internal working flow. I will take a look when I get some free time.

[ESP_igrr]

I think this approach with the "magic" size value would result in less readable code of the application. You could probably add a function-like macro to "hide" this detail, and make the calling code look nicer, but then it's not going to be any different to introducing a new API.

Also, would it be possible to tell if the pointer is on stack or heap-allocated?

In general, no: task stacks are usually allocated from the heap when the task is created. And even if you could determine that something is on the heap (but not on the stack), that would not guarantee that the object lives long enough for the event dispatching to happen.

[wuyuanyi]

I think this approach with the "magic" size value would result in less readable code of the application. You could probably add a function-like macro to "hide" this detail, and make the calling code look nicer, but then it's not going to be any different to introducing a new API.

Also, would it be possible to tell if the pointer is on stack or heap-allocated?

In general, no: task stacks are usually allocated from the heap when the task is created. And even if you could determine that something is on the heap (but not on the stack), that would not guarantee that the object lives long enough for the event dispatching to happen.

It makes sense. Thanks.