event manager: Implement event manager #38611
Conversation
github-actions
bot
added
area: API
include/event_manager.h
Outdated
| /* | ||
| * Copyright (c) 2018 Nordic Semiconductor ASA | ||
| * | ||
| * SPDX-License-Identifier: LicenseRef-Nordic-5-Clause |
There was a problem hiding this comment.
Please replace with SPDX-License-Identifier: Apache-2.0
include/profiler.h
Outdated
| /* | ||
| * Copyright (c) 2018 Nordic Semiconductor ASA | ||
| * | ||
| * SPDX-License-Identifier: LicenseRef-Nordic-5-Clause |
There was a problem hiding this comment.
Please replace with SPDX-License-Identifier: Apache-2.0
subsys/event_manager/CMakeLists.txt
Outdated
| # | ||
| # Copyright (c) 2018 Nordic Semiconductor | ||
| # | ||
| # SPDX-License-Identifier: LicenseRef-Nordic-5-Clause |
There was a problem hiding this comment.
Please replace with SPDX-License-Identifier: Apache-2.0
subsys/event_manager/Kconfig
Outdated
| # | ||
| # Copyright (c) 2018 Nordic Semiconductor | ||
| # | ||
| # SPDX-License-Identifier: LicenseRef-Nordic-5-Clause |
There was a problem hiding this comment.
Please replace with SPDX-License-Identifier: Apache-2.0
subsys/event_manager/event_manager.c
Outdated
| /* | ||
| * Copyright (c) 2018 Nordic Semiconductor ASA | ||
| * | ||
| * SPDX-License-Identifier: LicenseRef-Nordic-5-Clause |
There was a problem hiding this comment.
Please replace with SPDX-License-Identifier: Apache-2.0
| /* | ||
| * Copyright (c) 2018 Nordic Semiconductor ASA | ||
| * | ||
| * SPDX-License-Identifier: LicenseRef-Nordic-5-Clause |
There was a problem hiding this comment.
Please replace with SPDX-License-Identifier: Apache-2.0
| /* | ||
| * Copyright (c) 2019 Nordic Semiconductor ASA | ||
| * | ||
| * SPDX-License-Identifier: LicenseRef-Nordic-5-Clause |
There was a problem hiding this comment.
Please replace with SPDX-License-Identifier: Apache-2.0
zycz
requested review from
MaureenHelm,
andyross,
galak and
nashif
as code owners
|
Couple of comments:
As for event manager design, there are few things that could be added later on:
|
Profiler is a tool for monitoring events both from event manager and custom ones. https://developer.nordicsemi.com/nRF_Connect_SDK/doc/latest/nrf/libraries/others/profiler.html#profiler
Ok, it is now fixed |
nordic-krch
left a comment
nordic-krch
left a comment
There was a problem hiding this comment.
few more comments, what about test? Would be good to get that in as well.
There was a problem hiding this comment.
sample/application_development/event_manager
There was a problem hiding this comment.
I am not sure why are we adding this for this board specifically? (and why the board is listing all the samples it supports in the first place)
subsys/event_manager/CMakeLists.txt
Outdated
There was a problem hiding this comment.
It's required because of event_manager_priv.h. It woould be better then to create include/event_manager and move there all headers (including profiler.h)and remove this line. After that profiler.h name can stay because it will be name-spaced with event manager.
include/profiler.h
Outdated
There was a problem hiding this comment.
I cannot find this option in Kconfig.
include/profiler.h
Outdated
There was a problem hiding this comment.
see other guards in zephyr, they contain path to the include file.
include/profiler.h
Outdated
There was a problem hiding this comment.
all those must be prefixed with event_manager_
include/profiler.h
Outdated
There was a problem hiding this comment.
this is redundant and only spoils API. Profiling functions are cut during linking process because they are called conditionally. The only change i had to make to compile it was to add
.profile_fn = IS_ENABLED(CONFIG_PROFILER) ? profile_func : NULL, in _EVENT_INFO_DEFINE.
There was a problem hiding this comment.
Seems like CONFIG_PROFILER does not exist.
There was a problem hiding this comment.
it should be assigned only if profiler is enabled. Now event infos are created even when not used. That should also allow to clean up double defines for some macros, eg. MEM_ADDRESS_LABEL
There was a problem hiding this comment.
i don't like that event structure gets suffix and user must somehow know that after calling EVENT_INFO_DEFINE(control_event,..) it must use control_event_info. Imo, event info name should be explicit so that user does something like:
EVENT_INFO_DEFINE(control_event_info,
ENCODE(),
ENCODE(),
profile_control_event);
EVENT_TYPE_DEFINE(control_event,
true,
NULL,
&control_event_info);
babd721 to
be66328
Compare
|
Very useful, hope to be merged. :) |
JordanYates
left a comment
JordanYates
left a comment
There was a problem hiding this comment.
My thoughts on the general themes.
The inability to register new event subscribers at runtime is not great. #2746 has seen some renewed interest recently, and having dynamic modules unable to hook into events would be a big limitation.
There is no inbuilt integration with the existing Zephyr synchronization paradigm (poll). If I have a thread that just wants to run some operations on a new GPS sample that comes in, I not only need to raise the appropriate signals in the callback, but also manually copy out the event to some temporary storage, as the buffer doesn't exist after the subscribers have been notified. This is a larger downside if your event is a new buffer of 128 16bit 3-axis accelerometer samples (768 bytes).
Configuring CONFIG_HEAP_MEM_POOL_SIZE (via guess and check) for each application will never be fun. For a robust system, it will need to be at least as large as the sum of all the individual event sizes. This information is known at the locations where events are generated, but I can't see a way to make this any easier.
Less important, and not necessarily in scope for what the subsys is trying to achieve, but it is not possible to query the latest value of an event. For example, code may not necessarily care how often the battery voltage changes, or want to be notified each time a measurement is obtained, but simply query what the current state is.
|
Hi, Thanks for your thoughts
I see your point, but as you already mentioned option to dynamically add modules is generally unsupported yet in Zephyr. In my opinion first this option should be included in Zephyr and then it should be implemented also in Event Manager.
The changes you are suggesting are as you said "not necessarily in scope for what the subsys is trying to achieve" :) Its aim is to enable easy and safe communication between modules. If you have a module with an external sensor and you sample it every 2s you would probably store latest result in some variable. If you needed to have an update of measurement in this moment you would send request to make additional measurement. In case of EM it is similar. After receiving event you would need to store its value somewhere and in case you want additional measurement you just need to send event which will trigger measurement. |
Dynamic modules is one thing, but modules themselves can't explicitly state "I now care about these events" and "I no longer care about these events" via the API. It needs to be done via some global boolean flag in your callback (while making sure everything is thread-safe).
Right, but there are BIG caveats in terms of how much this subsys is helping you do that. All that this really gives you over a standard
Coming back to the accelerometer example, this would require (768 * (N+1)) bytes of I know that I am being pretty critical without offering any solutions. I am contrasting this with an internal subsys we have that performs a similar function without the limitations above. I need to have a conversation with my management around whether we want to at least propose our solution as an alternative. |
It can be done locally in the listener by simply checking if given event is enabled for me. I guess it could be moved to internals of event manager, use some bit mask and CLZ to quickly iterate over enabled listeners. It is only the optimization. Regarding collecting sensor samples to a database which could be shared between the modules, i don't think it is the scope of event manager. Event manager just distributes event to loosely coupled modules. One of those modules can be a database that collects the samples. Regarding use of heap, there are discussions to add an option to use dedicated buffer space instead of heap (e.g. mpsc_pbuf could be used which serves the event manager case - multiple producers, single consumer). It would be good to see what alternative could be used. |
zycz
force-pushed
the
event_manager
branch
2 times, most recently
from
8190228 to
1acde17
Compare
Add sample aplication of event manager with documentation to it. Aplication shows how events are submited and received . Signed-off-by: Jan Zyczkowski <[email protected]>
Add unit tests for event manager. Signed-off-by: Jan Zyczkowski <[email protected]>
Add first subscriber to event functionality to event manager. Jira: NCSDK-9067 Signed-off-by: Jan Zyczkowski <[email protected]>
jfischer-no
left a comment
jfischer-no
left a comment
There was a problem hiding this comment.
I have looked at this PR more closely, among other because I wanted something similar for USB subsystem. Event Manager as it is might be robust and fast, but that is about it. It is probably not extensible and not extensible at all to runtime subscription/unsubscription. Macro acrobatics hurt; need of EVENT_TYPE_DEFINE and EVENT_TYPE_DECLARE is awkward, especially since event define header must also be included by listener, and the latter could pull the information even without macro obfuscation; no chance to get rid of LINKER_ORPHAN_SECTION. I have tried to improve and change it, but went over to rewrite something similar from scratch. I will come soon with a proposal.
We can add enable/disable if there is a need for that. Runtime subscription costs I am not sure it is really needed in embedded we work on.
Depends what you are looking for. Performance was priority. Defining new event is not really magic. We have never got any question about it and we know people use it.
Please check again. It was removed without a problem. |
carlescufi
requested review from
anangl and
yperess
and removed request for
ycsin
| * @param ename Name of the event. | ||
| */ | ||
| #define EVENT_SUBSCRIBE_FIRST(lname, ename) \ | ||
| _EVENT_SUBSCRIBE(lname, ename, _EM_MARKER_FIRST_ELEMENT); \ |
There was a problem hiding this comment.
For EVENT_SUBSCRIBE and all other macros here, there is no reason at all to hide anything in enet_manager_priv.h, actually it makes no sense to have event_manager_priv.h, it just makes review and maintenance harder, exposed to application it is in anyway.
There was a problem hiding this comment.
There is perfect sense. These items should not be used directly - placing them in priv header makes it explicit.
| * | ||
| * @param ename Name of the event. | ||
| */ | ||
| #define EVENT_TYPE_DECLARE(ename) _EVENT_TYPE_DECLARE(ename) |
There was a problem hiding this comment.
The use of both EVENT_TYPE_DECLARE and EVENT_TYPE_DEFINE is awkward, specially as the user has to include header which "declare" the event anyway, there is no advantage over a simple union {uint32_t foo; void *bar}; type. The properties of the message can then be obtained from the event source header, without these awkward macros and without unnecessary complexity. Further instead of EVENT_TYPE_DYNDATA_DECLARE memory slabs or net_buf can be used, which again simplifies a lot. Therefore from my side strong NACK to EVENT_TYPE_DECLARE/EVENT_TYPE_DYNDATA_DECLARE.
There was a problem hiding this comment.
That seemed the simplest to do at the time when event manager was created. Can you elaborate on proposal?
Note that we migrate event manager from NCS - it's not a new code. There is quite a legacy behind. If there is no real advantage I would not change anything. The current API works fine.
| * @param ev_info_struct Data structure describing the event type. | ||
| * NULL can be used in case no data structure is needed. | ||
| */ | ||
| #define EVENT_TYPE_DEFINE(ename, init_log_en, log_fn, ev_info_struct) \ |
There was a problem hiding this comment.
I do not see any real reason to go that far with logging and with tracing anyway, but presumably it is result of EVENT_TYPE_DECLARE/EVENT_TYPE_DYNDATA_DECLARE complexity.
| * | ||
| * @param event Pointer to the event object. | ||
| */ | ||
| #define EVENT_SUBMIT(event) _event_submit(&event->header) |
There was a problem hiding this comment.
NACK for this macro, why not just to use event_submit(&event->header); ?
There was a problem hiding this comment.
simplification of usage - I recommend you to look at the code again
| /** Bool indicating if the event is logged by default. */ | ||
| bool init_log_enable; | ||
|
|
||
| /** Function to log data from this event. */ | ||
| log_event_data log_event_func; | ||
|
|
||
| /** Deprecated function to log data from this event. */ | ||
| log_event_data_dep log_event_func_dep; | ||
|
|
||
| /** Custom data related to tracking. */ | ||
| const void *trace_data; |
There was a problem hiding this comment.
What is unnecessary, takes more space in the structure than that which is productively necessary.
subsys/event_manager/Kconfig
Outdated
| Note that Event Manager uses orphan sections to handle its | ||
| data structures. |
There was a problem hiding this comment.
Does it use orphan sections or not?
There was a problem hiding this comment.
Not any more. This is a leftover after cleanup (@zycz )
subsys/event_manager/Kconfig
Outdated
| depends on LOG | ||
| default y | ||
| help | ||
| This option controls if events are printed to console. |
There was a problem hiding this comment.
This option controls if events are logged.
subsys/event_manager/event_manager.c
Outdated
| char log_buf[CONFIG_EVENT_MANAGER_EVENT_LOG_BUF_LEN]; | ||
|
|
||
| int pos = et->log_event_func_dep(eh, log_buf, sizeof(log_buf)); |
There was a problem hiding this comment.
Variables should be defined at the top of a block.
There was a problem hiding this comment.
I accept you have a different view on this but I don't agree. There is no rule for that and there are many well known advantages of defining variables as close to code as possible (i.e. in place).
subsys/event_manager/event_manager.c
Outdated
| k_spinlock_key_t key = k_spin_lock(&lock); | ||
|
|
||
| if (sys_slist_is_empty(&eventq)) { | ||
| k_spin_unlock(&lock, key); | ||
| return; | ||
| } | ||
|
|
||
| sys_slist_merge_slist(&events, &eventq); | ||
|
|
||
| k_spin_unlock(&lock, key); |
There was a problem hiding this comment.
The same can be implemented in one-line using struct k_fifo and k_fifo_get(). I also see no advantages not to do it this way, very unlikely eventq would contain more than a single event per k_work_submit(&event_processor);.
There was a problem hiding this comment.
Maybe. It could be a potential change for later.
Originally it was implemented to be as fast as possible so heavier api was avoided. To change it we would need to see if there would be no impact on performance.
subsys/event_manager/event_manager.c
Outdated
| while (NULL != (node = sys_slist_get(&events))) { | ||
| struct event_header *eh = CONTAINER_OF(node, | ||
| struct event_header, | ||
| node); | ||
|
|
There was a problem hiding this comment.
There is SYS_SLIST_FOR_EACH_CONTAINER macro, using it and moving variable definition to the top of a block, removing of few empty line below, would make code below looks much nicer.
Add changes cleaning event manager. Signed-off-by: Jan Zyczkowski <[email protected]>
Renamed event_manager files to app_event_manager. Signed-off-by: Jan Zyczkowski <[email protected]>
nordic-krch
left a comment
nordic-krch
left a comment
There was a problem hiding this comment.
Can you add test to hooks APP_EVENT_HOOK.... A lot of those is defined in the API but they are nowhere used.
| * @param et Pointer to the event type. | ||
| * @retval Boolean value of requested flag. | ||
| */ | ||
| static inline bool get_app_event_type_flag(const struct event_type *et, |
There was a problem hiding this comment.
it should be prefixed with module name -> app_event_get_type_flag
| * @param lname Listener name. | ||
| * @param cb_fn Pointer to the event handler function. | ||
| */ | ||
| #define APP_EVENT_LISTENER(lname, cb_fn) _APP_EVENT_LISTENER(lname, cb_fn) |
There was a problem hiding this comment.
Internal API shall be prefixed with Z_ so Z_APP_EVENT_LISTENER. Same with others.
| * | ||
| * @param id ID. | ||
| */ | ||
| #define ASSERT_APP_EVENT_ID(id) \ |
| */ | ||
| #define APP_EVENT_HOOK_POSTPROCESS_REGISTER(hook_fn) \ | ||
| _APP_EVENT_HOOK_POSTPROCESS_REGISTER(hook_fn, \ | ||
| _EM_SUBS_PRIO_ID(_EM_SUBS_PRIO_NORMAL)) |
There was a problem hiding this comment.
_EM should probably be changed to Z_APP_EVENT
There was a problem hiding this comment.
Changed to Z_APP_EM it is shorter :)
| * @param hook_fn Hook function. | ||
| */ | ||
| #define APP_EVENT_HOOK_POSTPROCESS_REGISTER_LAST(hook_fn) \ | ||
| const struct {} __event_hook_postprocess_last_sub_redefined = {}; \ |
There was a problem hiding this comment.
what is the purpose of this variable?
There was a problem hiding this comment.
It is a marker indicating that only one hook can be registered as Last.
Add changes cleaning event manager. Signed-off-by: Jan Zyczkowski <[email protected]>
|
App Event Manager right now won't be up streamed to Zephyr. I close PR. |
|
hi @rodrigobrochado , I have noticed your emoji. Any reason? The code is still available in Nordic Connect SDK :) |
|
Hi @pdunaj, yes, I saw it on NCS. I'm just excited about NCS features getting upstreamed to Zephyr. No big deal, but thanks for asking :) |
|
It's a pity that this PR was closed |
gmarull
left a comment
gmarull
left a comment
There was a problem hiding this comment.
some more comments. I have to say that except a few rough edges I like what is presented in this PR.
| 1. Include :file:`event_manager.h` in your :file:`main.c` file. | ||
| #. Call :c:func:`event_manager_init()`. |
There was a problem hiding this comment.
why does event manager need to be started manually? could it be done via SYS_INIT?
There was a problem hiding this comment.
The event manager was created to be an application skeleton (not a system one) and I believe it is better for application to have a control when event processing is started.
This said initialization function does almost nothing at the moment - it performs checks but was left in case we would like to extend the configuration in the future (like start a separate event processing thread, prepare an allocator or something similar).
| .. code-block:: c | ||
|
|
||
| /* Allocate event. */ | ||
| struct sample_event *event = new_sample_event(); |
There was a problem hiding this comment.
Yes - there are hooks (weak linkage) that allows user to pass own allocator functions. by default it's k_malloc.
| #define _EVENT_SUBSCRIBE(lname, ename, prio) \ | ||
| const struct event_subscriber _CONCAT(_CONCAT(__event_subscriber_, ename), lname) __used \ | ||
| __attribute__((__section__(_EVENT_SUBSCRIBERS_SECTION_NAME(ename, prio)))) = { \ | ||
| .listener = &_CONCAT(__event_listener_, lname), \ | ||
| } |
There was a problem hiding this comment.
any reason not to use https://docs.zephyrproject.org/latest/kernel/iterable_sections/index.html ?
There was a problem hiding this comment.
I think I had a look during the last rework if we can use more of standard macros. There must have been something that prevented it but I don't recall what it was.
| * ename type. | ||
| */ | ||
| #define _EVENT_ALLOCATOR_FN(ename) \ | ||
| static inline struct ename *_CONCAT(new_, ename)(void) \ |
There was a problem hiding this comment.
I think all event manager function calls should be namespaced, e.g. evm_, this just creates new_....
There was a problem hiding this comment.
you are right - this is leftover from times it was used in one app only that was not fixed during API rework, we should probably follow up on that in NCS.
There was a problem hiding this comment.
@zycz , can you create a ticket to follow this up?
| * ename type. | ||
| */ | ||
| #define _EVENT_ALLOCATOR_FN(ename) \ | ||
| static inline struct ename *_CONCAT(new_, ename)(void) \ |
There was a problem hiding this comment.
is dynamic allocation a mandatory thing?
There was a problem hiding this comment.
How else you would want to do it?
Events are dynamic so you have to do dynamic allocation on them. I expect having a preset of event objects create at build time would be quite limiting.
As for an actual allocator that is used - by default it is heap (k_malloc heap to be precise) but user can hook any other allocator (slab for example).
subsys/event_manager/event_manager.c
Outdated
| void __weak event_manager_trace_event_execution(const struct event_header *eh, | ||
| bool is_start) | ||
| { | ||
| } | ||
|
|
||
| void __weak event_manager_trace_event_submission(const struct event_header *eh, | ||
| const void *trace_info) | ||
| { | ||
| } | ||
|
|
||
| void * __weak event_manager_alloc(size_t size) |
There was a problem hiding this comment.
I think that in general we should avoid using weaks (it's a non-standard feature). Kconfig options to allow custom re-implementations would be a better choice IMO.
There was a problem hiding this comment.
Zephyr has multiple things based on weaks. I don't see a problem with using a feature that is actually requested by basic components of the OS.
This said you are right and we really could simply compile this allocators in by default and allow user to turn them off in preprocessor if other calls are provided. I would not change it at this point however as it does it's job.
| (et != NULL) && (et != __stop_event_types); et++) { | ||
|
|
||
| size_t ev_id = et - __start_event_types; |
There was a problem hiding this comment.
we should likely avoid using such variables directly and provide iterator functions instead, see for example device_required_foreach.
| * @{ | ||
| */ | ||
|
|
||
| #include "event_manager/event_manager.h" |
There was a problem hiding this comment.
let's use headers correctly: anything in the include path (non-local) must use <>.
| #ifdef __cplusplus | ||
| } | ||
| #endif |
There was a problem hiding this comment.
does this need to run on C++ code?
There was a problem hiding this comment.
I expect sample simply copied code from some previously defined event. Is there any harm coming out of it?
|
|
||
| static void log_config_event(const struct event_header *eh) | ||
| { | ||
| struct config_event *event = cast_config_event(eh); |
There was a problem hiding this comment.
maybe a macro would be more useful/readable (aligning with CONTAINER_OF usage), e.g.
struct config_event *event = EVM_CAST(eh, config);There was a problem hiding this comment.
good idea - we could follow this up along with new_event api @zycz
12 participants
Moves event manager with sample from sdk-nrf to zephyr.
Event Manager is a piece of software that supports development of consistent, modular, event-based application. In an event-based application, parts of the application functionality are separated into isolated modules that communicate with each other using events. Events are submitted by modules and other modules can subscribe and react to them.
This PR aims to:
-Initiate the discussion.
-Push the event manager as-is but once comments are taken we will move forward. This will be done in another task.
-Check if all applications already using event manager work fine with event manager placed in zephyr
-Introduce Event manager code into Zephyr