Send dbus signals from the main thread - #322
Conversation
Callbacks set via WPC_register_for_data and WPC_register_for_stack_status are called from c-mesh-api's dispatch thread. Earlier, these callbacks were sending the dbus signals as well. This presented potential problems because sd_bus is not thread safe, and in this case the main thread and the dispatch thread could access the same dbus connection (m_bus). The callbacks now only pass the data to a ring buffer (event_queue.h) which the main thread reads from. The event queue uses an eventfd to signal when a new event was added to it. A main loop is now added as described in sd_bus_get_fd(3) manual. The loop polls the dbus and event queue file descriptors, and sends the dbus signals if necessary. Additionally, WPC_close() is moved to be before other close functions in order to ensure dispatch thread exits before we close the event queue.
GwendalRaoul
left a comment
There was a problem hiding this comment.
Overall good for me, but could be good to abstract a bit more the event_queue.
So any module can queue it's own event with its id that it will get back when handling it when dispatch from main thread.
It will the case where we want to add a new event.
| return -errno; | ||
| } | ||
|
|
||
| r = sd_bus_process(m_bus, NULL); |
There was a problem hiding this comment.
nothing to check from dbus_fd ? I guess it will be handled internally in sd_bus_process so even if their was nothing on dbus_fd, it will not complain
There was a problem hiding this comment.
I don't remember much but now I checked it again, and sometimes sd_bus_get_timeout returns 0 and dbus_fd.revents is empty when polled, but sd_bus_process is expected to be called.
For sd_bus_get_timeout the documentation mentions the following:
"The returned timeout may be zero, in which case a subsequent I/O polling call should be invoked in non-blocking mode."
So when the timeout is 0, the correct way would be to skip poll and call sd_bus_process. In practice ppoll with zero timeout should return immediately, and then we just call sd_bus_process without checking dbus_fd (it would be empty anyway).
I'll need to check this further to make sure sd_bus_process would be fine when nothing is on the bus. Anyway better to document it as well.
There was a problem hiding this comment.
This should be fine, when we call it like this in practice we cover also the case when sd_bus_get_timeout returned 0. We then do ppoll with zero timeout and call sd_bus_process.
It was a bit confusing but it seems sd_bus_process is safe to call when there is nothing to do. In fact, this loop from systemd calls sd_bus_process repeatedly as long as it returns > 0 ("pending events") and only does poll when it returns 0 ("nothing to do").
Btw now sd_bus_process calls don't seem consistent with the process_pending_events, because in each iteration of the main loop, we drain the outgoing events, but don't drain the sd_bus_process (like in the stdio-bridge example above). But I suppose it's better like this, because we expect there to be more uplink data events compared to dbus method calls for example?
| uint8_t dst_ep; | ||
| uint8_t hop_count; | ||
| uint8_t payload[EVENT_DATA_MAX_PAYLOAD]; | ||
| } event_data_received_t; |
There was a problem hiding this comment.
Should the event_queue know exactly the format of event?
Could we keep it more generic and use abstarct type that are casted when used?
So event type allow to dispatch to write module that will cast the event to its correct type ?
There was a problem hiding this comment.
Good idea, makes sense to decouple the data format from the event queue!
There was a problem hiding this comment.
I ended up just moving them out to their own header file. We should have the event type identifier in a common place anyway (for the switch in main.c and the debug logs in event queue as a bonus).
Related to #320.
Callbacks set via WPC_register_for_data and WPC_register_for_stack_status are called from c-mesh-api's dispatch thread.
Earlier, these callbacks were sending the dbus signals as well. This presented potential problems because sd_bus is not thread safe, and in this case the main thread and the dispatch thread could access the same dbus connection (m_bus).
The callbacks now only pass the data to a ring buffer (event_queue.h) which the main thread reads from. The event queue uses an eventfd to signal when a new event was added to it.
A main loop is now added as described in sd_bus_get_fd(3) manual. The loop polls the dbus and event queue file descriptors, and sends the dbus signals if necessary.
Additionally, WPC_close() is moved to be before other close functions in order to ensure dispatch thread exits before we close the event queue.