Fix(NetworkEvents): Don't skip event callbacks in NetworkEvents::remo… #10337
Conversation
…veEvent Fixes Issue 10318 Includes pull request 10321 that fixes 10316 This change: * Adds code to find the event callbacks * Issues error when duplicate callbacks insertion attempts are made * Issues error when callbacks are not found during removal
👋 Hello LeeLeahy2, we appreciate your contribution to this project! Click to see more instructions ...
Review and merge process you can expect ...
|
|
small conflict cause by the update of |
Test Results 56 files - 83 56 suites - 83 4m 18s ⏱️ - 1h 37m 38s Results for commit 33a77d8. ± Comparison against base commit 9e60bbe. This pull request removes 9 tests.♻️ This comment has been updated with latest results. |
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]
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
I would log duplicate addition and not found removal only as warning. |
|
@LeeLeahy2 I agree with @JAndrassy . Will you please change the logs to warning? |
…veEvent Fixes Issue 10318 Includes pull request 10321 that fixes 10316 This change: * Adds code to find the event callbacks * Issues warning when duplicate callbacks insertion attempts are made * Issues warning when callbacks are not found during removal
|
The pull request was updated to change log_e to log_w in onEvent, onSysEvent and removeEvent. The initial choice of log_e was due to the comment in 10319. |
|
|
||
| for (i = 0; i < cbEventList.size(); i++) { | ||
| NetworkEventCbList_t entry = cbEventList[i]; | ||
| if (getStdFunctionAddress(entry.fcb) == getStdFunctionAddress(cbEvent) && entry.event == event) { |
There was a problem hiding this comment.
Why not place the result of getStdFunctionAddress(cbEvent) outside the for loop?
There was a problem hiding this comment.
wasn't before, it's OK not to change that now also
|
This PR causes #10365 issue |
Description of Change
Summary:
The fix for 10316 is required to get beyond the crash and detect Issue 10318.
Moved code from removeEvent to scan for the matching event into findEvent routines. The findEvent routines return the "id" or zero if the handler was not found.
Reworked removeEvent routines to use findEvent. Added log_e in the event callback handler not found path in removeEvent.
Added code to onEvent and onSysEvent routines to check for duplicate entries using findEvent. Added log_e that displays when the duplicate event is found.
Tests scenarios
Used the updated test sketch below
Related links
Closes 10318
Test Sketch