Lib: SMF: Add initial transitions to HSMs and add smf_set_handled() #66753
Conversation
|
Hello @glenn-andrews, and thank you very much for your first pull request to the Zephyr project! |
glenn-andrews
force-pushed
the
smf-initial-transitions
branch
3 times, most recently
from
9d3c157
to
3b8e28d
Compare
glenn-andrews
force-pushed
the
smf-initial-transitions
branch
from
3b8e28d
to
b51534f
Compare
doc/services/smf/index.rst
Outdated
| @@ -39,6 +39,10 @@ By default, a state can have no ancestor states, resulting in a flat state | |||
| machine. But to enable the creation of a hierarchical state machine, the | |||
| :kconfig:option:`CONFIG_SMF_ANCESTOR_SUPPORT` option must be enabled. | |||
|
|
|||
| By default, the hierarchical state machine does not support initial transitions | |||
| to child states on entering a superstate. To enable them the | |||
| :kconfig:option:`SMF_INITIAL_TRANSITIONS` option must be enabled. | |||
There was a problem hiding this comment.
Broken link
| :kconfig:option:`SMF_INITIAL_TRANSITIONS` option must be enabled. | |
| :kconfig:option:`CONFIG_SMF_INITIAL_TRANSITIONS` option must be enabled. |
lib/smf/Kconfig
Outdated
| @@ -13,4 +13,10 @@ config SMF_ANCESTOR_SUPPORT | |||
| help | |||
| If y, then the state machine framework includes ancestor state support | |||
|
|
|||
| config SMF_INITIAL_TRANSITIONS | |||
There was a problem hiding this comment.
Non-blocking suggestion:
| config SMF_INITIAL_TRANSITIONS | |
| config SMF_INITIAL_TRANSITION |
| * | ||
| * @param ctx State machine context | ||
| */ | ||
| void smf_set_handled(struct smf_ctx *ctx); |
There was a problem hiding this comment.
Can you update/create a sample to demonstrate the use of this?
There was a problem hiding this comment.
I have a rather large sample that implements the PSiCC2 Section 2.3.15 demo app with all levels of transitions. It's about 11K of source, given the large number of states the demo app encompasses.
There was a problem hiding this comment.
That's probably too large. I was thinking we had existing samples that use the SMF, but there are not.
However, there are some good unit tests for the SMF here.
Please extend one of the existing tests or create a new one that enables the CONFIG_SMF_INITIAL_TRANSITION option. The ensures that Zephyr's CI will validate that this new feature.
There was a problem hiding this comment.
Done. I've never used the unit tests before so feedback is appreciated.
There was a problem hiding this comment.
That's probably too large. I was thinking we had existing samples that use the SMF, but there are not.
Would it be worth me submitting my demo app as a separate sample for SMF? I can probably simplify it a bit.
There was a problem hiding this comment.
That's probably too large. I was thinking we had existing samples that use the SMF, but there are not.
Would it be worth me submitting my demo app as a separate sample for SMF? I can probably simplify it a bit.
Yes, please!
doc/services/smf/index.rst
Outdated
| Preventing Parent Run Actions | ||
| ============================= | ||
|
|
||
| Calling ``smf_set_handled`` will prevent calling the run action of parent |
There was a problem hiding this comment.
| Calling ``smf_set_handled`` will prevent calling the run action of parent | |
| Calling ``smf_set_handled`` prevents calling the run action of parent |
glenn-andrews
force-pushed
the
smf-initial-transitions
branch
3 times, most recently
from
2239f3a
to
a7aa0db
Compare
glenn-andrews
force-pushed
the
smf-initial-transitions
branch
from
a7aa0db
to
4e9d441
Compare
doc/services/smf/index.rst
Outdated
| - When a parent state intitiates a transition to self, the parents's exit | ||
| action is not called, e.g. instead of child_exit, parent_exit, parent_entry | ||
| it performs child_exit, parent_entry |
There was a problem hiding this comment.
I don't think this condition is tested. Can you update the test to include this?
There was a problem hiding this comment.
Do we want to be testing to confirm a bug exists? I don't want to see this codified as "the way things work" and add additional effort to fixing it by having to fix the test.
There was a problem hiding this comment.
The documentation here defines an API contract: A parent state initiating a transition to self does not call the parent_exit. I didn't consider this a bug.
So I was wondering if this condition could explicitly be tested.
There was a problem hiding this comment.
It's considered a bug here: #66341
There was a problem hiding this comment.
It seems odd to document behavior that isn't supported at the moment. Does it make more sense to explicitly state "calling smf_set_state() to transition to self is not supported".
glenn-andrews
force-pushed
the
smf-initial-transitions
branch
4 times, most recently
from
2cd4fe2
to
ff7521c
Compare
doc/services/smf/index.rst
Outdated
| - When a parent state intitiates a transition to self, the parents's exit | ||
| action is not called, e.g. instead of child_exit, parent_exit, parent_entry | ||
| it performs child_exit, parent_entry |
There was a problem hiding this comment.
The documentation here defines an API contract: A parent state initiating a transition to self does not call the parent_exit. I didn't consider this a bug.
So I was wondering if this condition could explicitly be tested.
doc/services/smf/index.rst
Outdated
| Calling ``smf_set_handled`` prevents calling the run action of parent states. | ||
| It is not required to call ``smf_set_handled`` if the state calls | ||
| ``smf_set_state``. |
There was a problem hiding this comment.
Please see suggested change re: how to have proper references for API functions. I realize that you probably just adopted the way things were done in the file so it would be nice (but not mandatory as I understand it might be a bit tedious) if you could maybe have a separate commit to update all references to use the :c:func: construct.
| Calling ``smf_set_handled`` prevents calling the run action of parent states. | |
| It is not required to call ``smf_set_handled`` if the state calls | |
| ``smf_set_state``. | |
| Calling :c:func:`smf_set_handled`` prevents calling the run action of parent states. | |
| It is not required to call :c:func:`smf_set_handled` if the state calls | |
| :c:func:`smf_set_state`. |
There was a problem hiding this comment.
Do you want me to refactor the entire document, or just the parts I've touched?
doc/services/smf/index.rst
Outdated
| without transitioning to another state, ie. calling smf_set_state, or if | ||
| ``smf_set_handled`` is called within the child_run function. | ||
| - When a parent state intitiates a transition to self, the parents's exit | ||
| action is not called, e.g. instead of child_exit, parent_exit, parent_entry | ||
| it performs child_exit, parent_entry |
There was a problem hiding this comment.
| without transitioning to another state, ie. calling smf_set_state, or if | |
| ``smf_set_handled`` is called within the child_run function. | |
| - When a parent state intitiates a transition to self, the parents's exit | |
| action is not called, e.g. instead of child_exit, parent_exit, parent_entry | |
| it performs child_exit, parent_entry | |
| without transitioning to another state, ie. calling :c:func:`smf_set_state`, or if | |
| :c:func:`smf_set_handled` is called within the ``child_run`` function. | |
| - When a parent state intitiates a transition to self, the parents's exit | |
| action is not called, e.g. instead of ``child_exit`` |srarr| ``parent_exit`` |srarr| ``parent_entry``, | |
| it performs ``child_exit`` |srarr| ``parent_entry``. |
doc/services/smf/index.rst
Outdated
| @@ -79,6 +117,13 @@ To transition from one state to another, the ``smf_set_state`` function is | |||
| used and it has the following prototype: | |||
| ``void smf_set_state(smf_ctx *ctx, smf_state *state)`` | |||
|
|
|||
| **NOTE:** If :kconfig:option:`CONFIG_SMF_INITIAL_TRANSITION` is not set, | |||
There was a problem hiding this comment.
There is a .. note:: Sphinx directive that would probably be appropriate here and in other places with **NOTE**s
glenn-andrews
force-pushed
the
smf-initial-transitions
branch
from
ff7521c
to
7b433b4
Compare
glenn-andrews
force-pushed
the
smf-initial-transitions
branch
from
7b433b4
to
f5b9370
Compare
|
Anything else I should do? |
There was a problem hiding this comment.
@glenn-andrews thanks much for your work on this! I've pushed a couple commits to your branch adding what was missing for the functions to properly render as links.
The one thing that I think needs to be changed is that you have all your changes into just one commit (which description now seems outdated/incomplete anyway?), which is not ideal. It would be great if you could split in several commits so that the additions of new Kconfigs and APIs are not mixed with what are "simple" documentation enhancements.
Thanks!
I'm not entirely sure how such a split would work. We've got about 8 instances where I replaced double-backticks with the appropriate sphinx directives, all the rest is directly relevant to the API being added. I could revert the changes to the double-backticks, then artificially add them again, but that seems silly. Can you give me a clearer idea of what is required? I feel I'm missing the intent of this request. |
glenn-andrews
force-pushed
the
smf-initial-transitions
branch
from
b23d649
to
5f631db
Compare
I'm terribly sorry, but I think I managed to nuke your changes by not pulling them before I amended my commit message. I believe I repaired the damage. Please confirm. |
Brings SMF framework closer into alignment with accepted Hierarchical State Machine operation by: 1. Allowing 'programming by difference' by having some child states handle events and prevent propagation up to the parent run actions while others propagate events up to a common handler in a parent state. 2. Optionally allow initial transitions within a parent state to determine the most nested child state to transition to. 3. Adding a test case for `CONFIG_SMF_INITIAL_TRANSITION` and `smf_set_handled()` 4. Updating documentation for the new API (and fixing some references) There was discussion in zephyrproject-rtos#55344 about not making the initial transition a Kconfig option, but I'm not sure of any way else of doing it without permanently adding a pointer to each `smf_state` entry, which is a problem for resource-constrained devices. This does not fix zephyrproject-rtos#66341 but documentation has been updated to warn users of the issue. Signed-off-by: Glenn Andrews <glenn.andrews.42@gmail.com>
Fixed where `smf_set_terminate` was written `smf_terminate` Signed-off-by: Glenn Andrews <glenn.andrews.42@gmail.com>
Added Doxygen markup. Signed-off-by: Glenn Andrews <glenn.andrews.42@gmail.com>
glenn-andrews
force-pushed
the
smf-initial-transitions
branch
from
2b4587a
to
1390bfe
Compare
|
How's this looking now? Any more action items for me? |
doc/services/smf/index.rst
Outdated
| functions doesn't make sense and will generate a warning. | ||
| .. note:: If :kconfig:option:`CONFIG_SMF_INITIAL_TRANSITION` is not set, | ||
| :c:func:`smf_set_initial` and :c:func:`smf_set_state` function should | ||
| not be passed a parent state as they will not know which child state |
There was a problem hiding this comment.
Non-blocking suggestion:
| not be passed a parent state as they will not know which child state | |
| not be passed a parent state as the parent state does not know which child state |
doc/services/smf/index.rst
Outdated
| - When a parent state intitiates a transition to self, the parents's exit | ||
| action is not called, e.g. instead of child_exit, parent_exit, parent_entry | ||
| it performs child_exit, parent_entry |
There was a problem hiding this comment.
It seems odd to document behavior that isn't supported at the moment. Does it make more sense to explicitly state "calling smf_set_state() to transition to self is not supported".
Incorporated changes requested by @keith-zephyr: 1. Grammar fix 2. Make explicit that transition to self in super-states is not supported Signed-off-by: Glenn Andrews <glenn.andrews.42@gmail.com>
|
Updated. Does the new wording seem okay? |
|
Thank you all so much for reviewing this! |
|
Hi @glenn-andrews! To celebrate this milestone and showcase your contribution, we'd love to award you the Zephyr Technical Contributor badge. If you're interested, please claim your badge by filling out this form: Claim Your Zephyr Badge. Thank you for your valuable input, and we look forward to seeing more of your contributions in the future! 🪁 |
| The statechart for this test is below. | ||
|
|
||
| .. graphviz:: | ||
| :caption: Test state machine for initial trnasitions and ``smf_set_handled`` |
Brings SMF framework closer into alignment with accepted Hierarchical State Machine operation by:
CONFIG_SMF_INITIAL_TRANSITIONandsmf_set_handled()There was discussion in #55344 about not making the initial transition a Kconfig option, but I'm not sure of any way else of doing it without permanently adding a pointer to each
smf_stateentry, which is a problem for resource-constrained devices.This does not fix #66341 but documentation has been updated to warn users of the issue.