-
Notifications
You must be signed in to change notification settings - Fork 7.3k
Lib: SMF: Add return code to signal event propagation #83854
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Lib: SMF: Add return code to signal event propagation #83854
Conversation
18f011e
to
8cd0ac3
Compare
fa73a26
to
0a6b01b
Compare
0a6b01b
to
e438134
Compare
0d8b648
to
80edfef
Compare
@@ -1293,13 +1293,14 @@ static void s_probe(void *o) | |||
hawkbit_event_raise(HAWKBIT_EVENT_NO_UPDATE); | |||
smf_set_state(SMF_CTX(s), &hawkbit_states[S_HAWKBIT_TERMINATE]); | |||
} | |||
return SMF_EVENT_PROPAGATE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why SMF_EVENT_PROPAGATE
and not SMF_EVENT_HANDLED
, like in all other cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it's at the end of the function and anything at that point which is not handled should be propagated.
To be honest, since smf_set_state()
will block propagation, anywhere we do not explicitly use smf_set_handled()
can use SMF_EVENT_PROPAGATE
and still work the same.
include/zephyr/smf.h
Outdated
.run = _run, \ | ||
.exit = _exit, \ | ||
IF_ENABLED(CONFIG_SMF_ANCESTOR_SUPPORT, (.parent = _parent,)) \ | ||
IF_ENABLED(CONFIG_SMF_INITIAL_TRANSITION, (.initial = _initial,)) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this line should not be intended more than the one above, clang format should more be seen as a suggestion and not a a mandatory change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If everyone used clang-format I'd not have needed the first two commits, and every future commit would be clean of formatting changes.
I'd rather machine-assisted uniformity, than others making it 'pretty' and me breaking it every time I want to ensure my code meets compliance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-format is not perfect, that's why it is only described as a helper for formation in the docs. For the styling we have the Coding Style, which is enforced by CI and Reviewers. When making commits and editing files, it is normally not a good idea, to run clang-format on the whole file as it will change things that are fine to ugly.
How are things looking now? |
e59d05b
to
d5f41db
Compare
Done |
d5f41db
to
01e8550
Compare
Now Zephyr 4.1 is out, can we revisit this change? |
01e8550
to
df40a52
Compare
@carlescufi @maass-hamburg Are the requested changes complete to your satisfaction? |
@sambhurst can you take a look? |
@glenn-andrews rebase needed |
df40a52
to
9507f6c
Compare
I think I screwed up the rebase. Help! |
9507f6c
to
df40a52
Compare
Apply clang-format to USB-C in preparation for code changes No code is changed, only formatting. Signed-off-by: Glenn Andrews <[email protected]>
Apply clang-format in preparation for code changes. Signed-off-by: Glenn Andrews <[email protected]>
See Discussion zephyrproject-rtos#83659 for information about the purpose of this change. Modifies run actions of hierarchical state machines to return a value indicating if the event was handled by the run action or should be propagated up to the parent run action. Flat state machines are not affected, and their run action returns void. smf_set_handled() has been removed and replaced by this return value. smf_set_state() will not propagate events regardless of the return value as the transition is considered to have occurred. Documentation, tests, samples, has been updated. USB-C and hawkBit use SMF and have been updated to use the new return codes. Signed-off-by: Glenn Andrews <[email protected]>
df40a52
to
8eb077e
Compare
@glenn-andrews should be good now :) |
Thank you!!! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR updates the SMF and related subsystems to use an explicit return code in state run functions instead of relying on side‐effects via smf_set_handled. The key changes include modifying function signatures (changing from void to returning an enum smf_state_result) and updating all state machine run functions and their callers accordingly.
Reviewed Changes
Copilot reviewed 22 out of 24 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
subsys/usb/usb_c/usbc_tc_src_states_internal.h | Updated run function declarations to return enum smf_state_result instead of void |
lib/smf/smf.c | Modified the SMF execution paths to capture and act on the new return values |
include/zephyr/smf.h | Updated SMF API typedefs and comments to document the new return type in run functions |
samples/subsys/smf/smf_calculator_thread.c and hsm_psicc2_thread.c | Converted sample state implementations to return explicit SMF_EVENT_* values |
subsys/mgmt/hawkbit/hawkbit.c | Updated state functions to return enum values and adjusted several state transitions |
Files not reviewed (2)
- doc/releases/migration-guide-4.2.rst: Language not supported
- doc/services/smf/index.rst: Language not supported
Comments suppressed due to low confidence (4)
samples/subsys/smf/smf_calculator_thread.c:311
- Confirm that replacing the previous smf_set_handled() calls with explicit return values is consistent in all run methods and that each branch returns an appropriate SMF_EVENT_* value.
return SMF_EVENT_HANDLED;
subsys/usb/usb_c/usbc_tc_src_states_internal.h:14
- [nitpick] The run functions now return an enum instead of void; please ensure that the naming and documentation across the codebase clearly reflect this change.
enum smf_state_result tc_unattached_src_run(void *obj);
include/zephyr/smf.h:83
- [nitpick] Ensure that the comment and API documentation are updated to reflect that run functions now return an enum (SMF_EVENT_HANDLED or SMF_EVENT_PROPAGATE) instead of void.
typedef enum smf_state_result (*state_execution)(void *obj);
lib/smf/smf.c:161
- Verify that every branch in the updated run functions returns a valid SMF_EVENT_* value to avoid any undefined behavior during state execution.
enum smf_state_result rc = tmp_state->run(ctx);
See Discussion #83659 for information about the purpose of this change.
Modifies run actions of hierarchical state machines to return a value indicating if the event was handled by the run action or should be propagated up to the parent run action. Flat state machines are not affected, and their run action returns void.
smf_set_handled() has been removed and replaced by this return value. smf_set_state() will not propagate events regardless of the return value as the transition is considered to have occurred.
Documentation, tests and samples have been updated, as has the hawkBit and USB-C subsystems.