feat(NetworkEvent lib) refactor network events handling #11115
Conversation
👋 Hello vortigont, we appreciate your contribution to this project! 📘 Please review the project's Contributions Guide for key guidelines on code, documentation, testing, and more. 🖊️ Please also make sure you have read and signed the Contributor License Agreement for this project. Click to see more instructions ...
Review and merge process you can expect ...
|
Test Results 76 files 76 suites 12m 42s ⏱️ Results for commit 7fb791b. ♻️ This comment has been updated with latest results. |
ESP IDF has very similar implementation of event bus ti propagate messages to subscribers, it consists of same set of componets - a queue and a task to execute subscriber's callbacks. Reusing it for Arduino networking events could give some benefits: - a unified event exchange bus between arduino and IDF components - save resources for extra Task stack and Queue to handle Arduino's own event bus
- use handlers to avoid dublicate event subscriptions - replace ARDUINO_EVENT_MAX with ARDUINO_EVENT_ANY to match with ESP event loop
- replace legacy functions with handler instance based - replace if's with simplified switch/case - use post without arduino_event_t where appropriate, avoid useless mallocs on event bus
add a new functional callback type NetworkEventReceiver which accepts two argumnets: an event id and a pointer to optional struct with trailer data. Using this callback would allow to propagate event messages more efficiently. Mandatory allocation for arduino_event_info_t and arduino_event_t objects won't needed and simple events could be passed with only (int32_t)arduino_event_id_t wich fits nice to ESP event loop add NetworkEvents::postEvent() overload with optional arduino_event_info_t* struct use std::variant for various types of callback handlers, event picker depending on callback variant will provide an optimized calls avoiding extra copies when not needed.
vortigont
force-pushed
the
netevent_esploop
branch
from
aafc841 to
b45e8d0
Compare
- make event handlers and callbacks as class members, remove static pointer to class instance - replace legacy esp_event_handler_unregister() with instance/hamdler via esp_event_handler_instance_unregister() - replace a bunch of if's with switch/case for better readability - removed mandatory struct arduino_event_t when sending arduino events, use post(event_id,*data)
- make _onApArduinoEvent event handler as class members, remove static pointer to APClass class instance - replace legacy esp_event_handler_unregister() with instance/hamdler via esp_event_handler_instance_unregister() - replace a bunch of if's with switch/case for better readability - removed mandatory struct arduino_event_t when sending arduino events, use post(event_id,*data)
vortigont
force-pushed
the
netevent_esploop
branch
from
b45e8d0 to
7fb791b
Compare
Memory usage test (comparing PR against master branch)The table below shows the summary of memory usage change (decrease - increase) in bytes and percentage for each target.
Click to expand the detailed deltas report [usage change in BYTES]
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@vortigont I understand your reasons for the changes, but I prefer to keep Arduino's events and handlers totally separate from IDF for a few reasons. Mainly because Arduino is used by all different kinds of people that do all different kinds of things in the callbacks. An Arduino callback should never interfere with the rest of the system at any point and even is user decides to block it, background tasks should always continue to run normally. That is why Arduino runs at very low priority as well. Using C++ standard mutex is also discouraged in FreeRTOS environment (we have discussed this in the Async Discord, maybe you have missed it). Generally all code in Arduino is written with the presumption that the user will do something that is not supposed to and that should not break the rest of the chip, so we are OK with "wasting" a few KB of RAM to run own task to handle all events. We are open for other improvements though :) |
Arduino events are in reality copies of already existing IDF events that span multiple components and that are just collected in on place, plus some internal handling to update states and such, because most do not listen to events and expect to get the states otherwise |
|
yeah, users are tend to misbehave :) you point is reasonable. Although nothing prevents user to attach to the same default event bus in his code. As for mutexes, I must have skipped it, sorry, will catch up. |
|
There is always a bell ringing when i see the chance to save some RAM :-) |
|
@Jason2866 nah... I was not for the RAM saving here at all, it's just the thing everybody loves in MCU world, we do have that using NetworkEventCb = void (*)(arduino_event_id_t event);
using NetworkEventFuncCb = std::function<void(arduino_event_id_t event, arduino_event_info_t info)>;
using NetworkEventSysCb = void (*)(arduino_event_t *event); and kind of not-so-nice scheme to register/unregister by function pointers while IDF is using instance handlers and keep by-ptr as legacy. OK, ok, Well, anyway it was a nice exercise for me to understand the relation and exchange between Arduino's network events and IDF's origin. |
the main idea here is to reuse ESP IDF's event bus to propagate notifications for Arduino network events too.
Currently
NetworkEventslib uses it's own instances of queue and Task to deliver event among componects, but ESP IDF already has a very similar implementation and what is currently happening is we resend messages from one queue to another.Reusing IDF's event loop for Arduino networking events could give some benefits:
- a unified event exchange bus between arduino and IDF components
- save resources for extra Task stack and Queue to handle Arduino's own event bus
I've come further and refactored other code related to NetworkEvents to close some gaps and unify messaging.
Since code changes are quite large, would appreciate any feedbacks if this worth the efforts to continue.
@me-no-dev @P-R-O-C-H-Y
Relates to #10805