RFC: SMF add return code to signal event has been handled

[glenn-andrews]

Overview

SMF implements a hierarchical state machine (HSM) if CONFIG_SMF_ANCESTOR_SUPPORT is enabled.

In an HSM, if the most-nested leaf state does not handle an event, parent state handlers are executed until one of them does handle it or the outermost state has been called. smf_set_handled() is called to indicate a state has handled the event and to stop propagation to parent states.

Problem

The problem with this is it is very easy to forget smf_set_handled() and accidentally propagate the event to the parents.

Most state machine frameworks use a return code from the handler function to indicate if the state handled the event, or not. Frameworks like Quantum Platforms go further and handle transitions as part of the return code.

SMF did not do this because the original version did not allow for events not to be propagated, and smf_set_handled() was an attempt at providing correct HSM behavior without breaking existing code.

Proposal:

Remove the smf_set_handled() function and modify the state handler functions to take a return code with two options:

enum smf_event_result {
        SMF_EVENT_HANDLED,
        SMF_EVENT_PROPAGATE,
};

By changing the return value from void to enum smf_event_result, the users will be forced to specify if the event has been handled or not, removing the error of omitting smf_set_handled().

if CONFIG_SMF_ANCESTOR_SUPPORT is not set, the state handler can return any return value, as it will be ignored.

Options

If preferred, we could also add an SMF_EVENT_TRANSITIONED to signal the event caused a transition and to execute the exit and entry actions associated with the transition. smf_set_state() would simply sets the destination function pointer but would not execute the entry/exit actions. My concern with that would be returning SMF_EVENT_TRANSITIONED without calling smf_set_state() is an error, and also breaks any code that does cleanup after calling smf_set_state().

An alternate idiom would be to have smf_set_state() return SMF_EVENT_HANDLED on success and use return smf_set_state() to transition. Again this breaks any code that performs actions after calling smf_set_state() that are currently allowed.

Ramifications:

All code currently using SMF with CONFIG_SMF_ANCESTOR_SUPPORT would break, including USB-C and hawkBit, as well as tests and samples. The PR would have to refactor these services, tests and samples as well.

[keith-zephyr]

There are known downstream users of the SMF. Those would also need updating as part of this change.
A few examples from the Chromium repository

[glenn-andrews]

This is a very good point, and it's good to know other people are using this. I'm aware the change will be painful to users, but the API is listed as Experimental/Unstable so it's better to do it sooner than later. I'd love to get this to being a Stable API, but the present API is not optimal for that.

To unlock the full power of hierarchical state machines you must be able to handle events in the child state and not propagate upwards to allow 'programming by difference'.

The current smf_set_handled() is a kluge job I wrote to make the minimum API change, but it's too easy to forget it every time you need it, leading to state machines that don't match the design and errors that may not be caught. Forcing a handled/propagate return code makes sure the developer considers if the event has been handled or not.

[glenn-andrews]

If this architecture change is accepted, then we may want to also create an SMF_EVENT_TRANSITION return code that triggers the exit/entry actions rather than doing so in the middle of a state's run action. See #79612 for someone surprised by what we're currently doing.

[henrikbrixandersen]

I think that's a good idea. We may also want to improve documentation on e.g. how to do automatic transition (transient states) from one state to another, a question that has popped up a few times as well.

[henrikbrixandersen]

@glenn-andrews Any update on this?

[glenn-andrews]

#83854 adds the return code

#93336 is the discussion for adding a TRANSITION return code to kick off the transition between states. It's currently stuck waiting for feedback.

[henrikbrixandersen]

Thanks. I already provided feedback on #93336, but not many Zephyr maintainers look into the discussions for RFCs. Better use the process described here: https://docs.zephyrproject.org/latest/develop/api/api_lifecycle.html#introducing-breaking-api-changes

[glenn-andrews]

It's not a stable API so I didn't think I should bother the Architecture meeting with it. I'd be happy to do so though.

[henrikbrixandersen]

It's not a stable API so I didn't think I should bother the Architecture meeting with it. I'd be happy to do so though.

You don't have to follow it all, but the RFC part (where you open an RFC as an issue, not a discussion) usually works better. If need be, that RFC can be brought up in the Arch WG meeting, stable API or not.

[glenn-andrews]

Gotcha, will do