Skip to content

smf: add compile check for ancestors.#102050

Closed
laughing-imp wants to merge 1 commit intozephyrproject-rtos:mainfrom
laughing-imp:smf_error
Closed

smf: add compile check for ancestors.#102050
laughing-imp wants to merge 1 commit intozephyrproject-rtos:mainfrom
laughing-imp:smf_error

Conversation

@laughing-imp
Copy link
Copy Markdown

@laughing-imp laughing-imp commented Jan 9, 2026

currently in SMF when CONFIG_SMF_ANCESTOR_SUPPORT or CONFIG_SMF_INITIAL_TRANSITION is disabled the value assigned to the member silently gets dropped when using the SMF_CREATE_STATE() macro. The goal of this PR is to add a clear compile time error which instructs the developer that this is happening when they are providing a non null value to the disabled member.

I spent time debugging what I assumed to be bad application level logic but it turned out that I forgot to enable ancestors; maybe this change could save someone else some time aswell.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jan 9, 2026

Hello @laughing-imp, and thank you very much for your first pull request to the Zephyr project!
Our Continuous Integration pipeline will execute a series of checks on your Pull Request commit messages and code, and you are expected to address any failures by updating the PR. Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request. If you haven't already, please make sure to review the project's Contributor Expectations and update (by amending and force-pushing the commits) your pull request if necessary.
If you are stuck or need help please join us on Discord and ask your question there. Additionally, you can escalate the review when applicable. 😊

@laughing-imp laughing-imp force-pushed the smf_error branch 2 times, most recently from 86619e8 to 406bf49 Compare January 9, 2026 22:17
Throw an error if ancestors are disabled but
a parent is given

Signed-off-by: Brandon Allen <allen@lunarmed.ca>
@glenn-andrews
Copy link
Copy Markdown
Contributor

Please add a description to the pull request.

Also, would it make sense to do the same thing for _initial?

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented Jan 9, 2026

@laughing-imp
Copy link
Copy Markdown
Author

laughing-imp commented Jan 10, 2026

I didnt think this through. Build asserts cant be used here because SMF_CREATE_STATE() is used within arrays (or atleast i cant figure out how). It does create a build time error in the correct scenarios but the error is syntax related and would not be clear to someone. This is a different solution I prompted up e796fca

from this diff (a test with ancestors disabled)

diff --git a/tests/lib/smf/src/test_lib_flat_smf.c b/tests/lib/smf/src/test_lib_flat_smf.c
index 72190cb6822..de924f2ed73 100644
--- a/tests/lib/smf/src/test_lib_flat_smf.c
+++ b/tests/lib/smf/src/test_lib_flat_smf.c
@@ -253,7 +253,7 @@ static void state_d_exit(void *obj)
 
 static const struct smf_state test_states[] = {
        [STATE_A] = SMF_CREATE_STATE(state_a_entry, state_a_run, state_a_exit, NULL, NULL),
-       [STATE_B] = SMF_CREATE_STATE(state_b_entry, state_b_run, state_b_exit, NULL, NULL),
+       [STATE_B] = SMF_CREATE_STATE(state_b_entry, state_b_run, state_b_exit, &test_states[STATE_A], NULL),
        [STATE_C] = SMF_CREATE_STATE(state_c_entry, state_c_run, state_c_exit, NULL, NULL),
        [STATE_D] = SMF_CREATE_STATE(state_d_entry, state_d_run, state_d_exit, NULL, NULL),
 };
(END)

it generates this error

In file included from /Users/brandonallen/work/tms-fw/zephyr/tests/lib/smf/src/test_lib_flat_smf.c:8:
/Users/brandonallen/work/tms-fw/zephyr/include/zephyr/smf.h:52:30: error: negative width in bit-field 'PARENT_SET_BUT_CONFIG_SMF_ANCESTOR_SUPPORT_DISABLED'
   52 |                     _parent, PARENT_SET_BUT_CONFIG_SMF_ANCESTOR_SUPPORT_DISABLED) +    \
      |                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/brandonallen/work/tms-fw/zephyr/include/zephyr/smf.h:28:30: note: in definition of macro 'SMF_VALIDATE_NON_NULL'
   28 |         (sizeof(struct { int _err_msg: (IS_ENABLED(_conf) || ((_val) == NULL) ? 1 : -1); }) * 0)
      |                              ^~~~~~~~
/Users/brandonallen/work/tms-fw/zephyr/tests/lib/smf/src/test_lib_flat_smf.c:256:21: note: in expansion of macro 'SMF_CREATE_STATE'
  256 |         [STATE_B] = SMF_CREATE_STATE(state_b_entry, state_b_run, state_b_exit, &test_states[STATE_A], NULL),
      |                     ^~~~~~~~~~~~~~~~
ninja: build stopped: subcommand failed.
FATAL ERROR: command exited with status 1: /opt/homebrew/bin/cmake --build /Users/brandonallen/work/tms-fw/zephyr/build

Would you accept something like this?

@glenn-andrews
Copy link
Copy Markdown
Contributor

The error message seems clear enough, but it makes the code pretty ugly and convoluted. I'd like to hear what the rest think about it.

@vlad-kulikov
Copy link
Copy Markdown
Contributor

Hi @laughing-imp, I agree with @glenn-andrews and it does look a bit messy. Also on Zephyr's documentation page, it is stated:

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 CONFIG_SMF_ANCESTOR_SUPPORT option must be enabled.

By default, the hierarchical state machines do not support initial transitions to child states on entering a superstate. To enable them the CONFIG_SMF_INITIAL_TRANSITION option must be enabled.

URL: https://docs.zephyrproject.org/latest/services/smf/index.html

If you can find a way to make it more aesthetic, it will be reviewed.

@vlad-kulikov
Copy link
Copy Markdown
Contributor

@laughing-imp if you want there is a suggestion by @glenn-andrews about how to improve the code.
Here is a link to the comment:
#98386 (review)

Copy link
Copy Markdown
Contributor

@vlad-kulikov vlad-kulikov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does create a build time error in the correct scenarios but the error is syntax related and would not be clear to someone.

As stated by @laughing-imp

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: State Machine Framework State Machine Framework

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants