[notifier] signal change to active/pending dataset + simplify 'Notifier::EventToString' #5657
Conversation
This file contains 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
abtink
force-pushed
the
notifier/dataset-changed
branch
from
cf8bdf4
to
0718349
Compare
abtink
force-pushed
the
notifier/dataset-changed
branch
from
0718349
to
6753436
Compare
Size Report of OpenThread
|
Codecov Report
@@ Coverage Diff @@
## master #5657 +/- ##
==========================================
- Coverage 80.74% 77.64% -3.10%
==========================================
Files 378 378
Lines 48331 48093 -238
==========================================
- Hits 39024 37341 -1683
- Misses 9307 10752 +1445
|
Irving-cl
approved these changes
Oct 16, 2020
| @@ -92,6 +92,8 @@ enum Event | |||
| kEventThreadBackboneRouterStateChanged = OT_CHANGED_THREAD_BACKBONE_ROUTER_STATE, ///< Backbone Router state changed | |||
| kEventThreadBackboneRouterLocalChanged = OT_CHANGED_THREAD_BACKBONE_ROUTER_LOCAL, ///< Local Backbone Router changed | |||
| kEventJoinerStateChanged = OT_CHANGED_JOINER_STATE, ///< Joiner state changed | |||
| kEventActiveDatasetChanged = OT_CHANGED_ACTIVE_DATASET, ///< Active Dataset changed | |||
| kEventPendingDatasetChanged = OT_CHANGED_PENDING_DATASET, ///< Pending Dataset changed | |||
There was a problem hiding this comment.
Just a question: It seems that these events are always used alone when calling Signal instead of a bit mask way. So why not defining these values simply as 0-29? For uint32_t we can add at most 2 more events in the future.
There was a problem hiding this comment.
The state changed callback provides multiple events via a bitmask.
openthread/src/core/common/notifier.cpp
Lines 104 to 109 in 5c7850e
There was a problem hiding this comment.
We do use them as bitmask (which can contain multiple events) in the callbacks, e.g., in otStateChangedCallback() or in HandleNotifierEvents().
Signal() method uses a single event but multiple events can be collected and emitted together (Notifier
has its own Tasklet from which it would emit the collected events).
I agree we may face a limit due to use of uint32_t. We have been careful about adding events and only adding ones that would be useful. In the case here, I think knowing when Datasets get changed would be helpful (for different situations).
We have intentionally defined otChangedFlags type which is used as parameter type in otStateChangedCallback(). It is currentlly defined in instance.h (public header) as:
typedef uint32_t otChangedFlags;
typedef void (*otStateChangedCallback)(otChangedFlags aFlags, void *aContext);
This would allow us in future to change the otChangedFlags to uint64_t to get more possible OT_CHANGED events and hopefully minimize impact on users of the API.
Inside the core, we have Events type which acts as a set/list of events and provides helper methods to check whether it Contains(Event) and/or ContainsAny() or ContainsAll() of a collection of events.
This commit changes `EventToString()` to use an array instead of `switch`. This helps with code size due to enumerator cases being `uint32_t` bitmask values and therefore compliers may not be able to optimize the `switch` statement as a table.
abtink
force-pushed
the
notifier/dataset-changed
branch
from
6753436
to
d112503
Compare
jwhui
approved these changes
Oct 19, 2020
jwhui
pushed a commit
that referenced
this pull request
Oct 19, 2020
simonlingoogle
pushed a commit
to simonlingoogle/openthread
that referenced
this pull request
Nov 12, 2020
Bug: 171270296 * origin/github/master: [notifier] simplify 'EventToString()' (openthread#5657) [notifier] signal change to active or pending Operational Dataset (openthread#5657) [coap] add helper method 'Message::SetTokenFromMessage()' (openthread#5661) [dataset-manager] misc enhancements (openthread#5655) [simulation] add configuration MAX_NETWORK_SIZE (openthread#5565) [lib-url] update 'Makefile' to include 'src/core' in header path search (openthread#5659) [code-utils] enhance 'VerifyOrExit' to make action optional (openthread#5659) [autoconf] remove bashisms from configure.ac (openthread#5658) [otns] emit COAP send/receive events to OTNS (openthread#5656) [logging] update logging to use new helper macros (openthread#5650) [common] macros to get first/second/rest of variadic arguments (openthread#5650) [continuous-integration] kill ot-rcp and ot-cli in pty check (openthread#5654) [cli] add context for the user commands (openthread#5587) [thread-cert] turn off OTBR NAT64 in 1.2 Backbone tests (openthread#5652) [low-power] add an Enh-ACK frame link layer counter (openthread#5583) [thread-cert] refactor case 9.2.12 and 9.2.13 using pktverify (openthread#5442) [coap] allow max URI path length (openthread#5645) [harness-automation] update readme (openthread#5646) [common] use non-copyable for more instance-unique classes (openthread#5639) [linux] fix missing correct mtu (openthread#5642) [docs] update Doxygen menu (openthread#5641) [thread-cert] enhance case 5.6.7 for data version increment (openthread#5643) [cc1352,cc2652] add check before claiming transmit done (openthread#5640) [cli] add command to set/get external FEM's LNA gain (openthread#5563) [ip6] no anycast as source address (openthread#5632) [debug] prefer user provided assert to system assert (openthread#5624) Change-Id: Ia7e971ebfef8a78dda9e818698e46831b41d1ec7
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR contains two related commits:
[notifier] signal change to active or pending Operational Dataset
[notifier] simplify 'EventToString()'
This commit changes
EventToString()to use an array instead ofswitch. This helps with code size due to enumerator cases beinguint32_tbitmask values and therefore compliers may not be able tooptimize the
switchstatement as a table.