fix (NetworkEvents lib) remove checks for duplicated event handlers#10376
Merged
me-no-dev merged 8 commits intoespressif:masterfrom Oct 24, 2024
Merged
fix (NetworkEvents lib) remove checks for duplicated event handlers#10376me-no-dev merged 8 commits intoespressif:masterfrom
me-no-dev merged 8 commits intoespressif:masterfrom
Conversation
…llbacks removing event callback via std::function pointer does not work as expected for lambdas (issue espressif#10365) here mark NetworkEvents::removeEvent(NetworkEventFuncCb cbEvent, arduino_event_id_t event = ARDUINO_EVENT_MAX) as deprecated in favor of removing by callback's id for NetworkEvents::onEvent remove checking for dublicate event handler, this does not work for lambdas too remove NetworkEvents::find methods as unnecessary move cbEventList container inside the class declare NetworkEventCbList_t as a cpp struct with constructor, allows using std::vector.emplace() when adding new items to container optimize NetworkEvents::remove() calls to use erase-remove idiom for std::vector
prevent checkForEvent loop to be callable from outside the task's thread
- rename NetworkEvents::cbEventList as private member NetworkEvents_cbEventList - NetworkEvents::getStatusBits() add const qualifier - turn statics into constexpr - add indexes to enum::arduino_event_id_t to make events indexing consistent for SOCs with/without WiFi also leave some index gaps to minimize renumbering on adding new events - add doxygen help to NetworkEvents:: methods - declare NetworkEvents::eventName() as static, that could be used without creating class scope - potential mem leak in postEvent
Contributor
👋 Hello vortigont, we appreciate your contribution to this project! Click to see more instructions ...
Review and merge process you can expect ...
|
Contributor
Test Results 56 files 56 suites 4m 11s ⏱️ Results for commit ab1d55d. ♻️ This comment has been updated with latest results. |
Contributor
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]
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
…tions provide thread safety for dual core SoCs since std::mutex brings additional componetns of libstdc++ lib it impacts resulting image size significantly (around 50k) Might be enabled on-demand if thread-safety is required
vortigont
force-pushed
the
netwrk_evnt_lib_refctr
branch
from
28b93b9 to
1a2d988
Compare
VojtechBartoska
added
Area: WiFi
SuGlider
approved these changes
Oct 23, 2024
Collaborator
SuGlider
left a comment
SuGlider
left a comment
There was a problem hiding this comment.
LGTM!
@vortigont - thanks for the good contribution!
me-no-dev
approved these changes
Oct 24, 2024
me-no-dev
added
Status: Pending Merge
Member
|
There are some spelling errors: https://github.com/espressif/arduino-esp32/actions/runs/11498055926/job/32003053726?pr=10376#step:8:25 |
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A refactoring for
NetworkEventsclassThis fixes #10365 issue and supersedes #10337, it removes pointer comparison checks for function pointer handlers but adds more safety to the code working with callback handlers container.
std::functionpointer, it does not work as expected for lambdasNetworkEvents::removeEvent(NetworkEventFuncCb)as deprecated in favor of removing by callback's idNetworkEvents::onEventremove checking for duplicate event handler, this does not work for lambdas tooNetworkEvents::findmethods as unnecessary in favor of usingstditeratorsNetworkEventsclassNetworkEventCbList_tas a cpp struct with constructor, allows usingstd::vector.emplace()when adding new items to containerNetworkEvents::remove()calls to use erase-remove idiom forstd::vectorNetworkEventsclassarduino_event_id_tto make events indexing consistent for SOCs with/without WiFialso leave some index gaps to minimize renumbering on adding new events
NetworkEvents::methodsNetworkEvents::eventName()as static, that could be used without creating class scopepostEvent()