Lib: SMF Modify HSM operation for UML-Style transitions #71729

glenn-andrews · 2024-04-21T03:45:50Z

A possible solution to #71675

Modify the SMF such that state transitions from parent states choose the correct Least Common Ancestor based on the transition source rather than the current state.

Also, SMF_CREATE_STATE() now always takes five arguments so all types of state machines (flat and hierarchical with and without initial transitions) can be used in the same project without errors. This is in response to #71252 The only subsystem using SMF, UCB-C, has been updated to take 5 arguments.

glenn-andrews · 2024-04-21T17:14:45Z

tests/lib/smf/src/test_lib_self_transition_smf.c

This is actually an entirely new file, not a rename.

doc/services/smf/index.rst

include/zephyr/smf.h

keith-zephyr · 2024-05-17T15:05:29Z

lib/smf/smf.c

 };

-static bool share_paren(const struct smf_state *test_state,
-			const struct smf_state *target_state)
+#if CONFIG_SMF_ANCESTOR_SUPPORT


Because you're using the __unused attribute, wrapping these functions in an #ifdef isn't required.

The #ifdef is required because we conditionally compile in parent in smf_state

Should I delete the __unused?

~~Yes.~~
Sorry - I responded before looking at the latest patchset. You need either __unused or #ifdef. I'm good with the __unused.

~~Yes.~~ Sorry - I responded before looking at the latest patchset. You need either __unused or #ifdef. I'm good with the __unused.

Unfortunately you need the #ifdefs, so I'll delete the __unused

doc/services/smf/index.rst

lib/smf/smf.c

tests/lib/smf/src/test_lib_self_transition_smf.c

glenn-andrews · 2024-05-18T17:41:41Z

Review comments fixed or otherwise addressed.

@keith-zephyr Please can you take another look?

keith-zephyr · 2024-05-20T14:55:29Z

doc/services/smf/index.rst

+.. note:: State machines can only call :c:func:`smf_set_state` from Run
+   functions, not Entry or Exit functions.


Delete this sentence. It is conflict with the previous note.

Suggested change

.. note:: State machines can only call :c:func:`smf_set_state` from Run

functions, not Entry or Exit functions.

keith-zephyr · 2024-05-20T15:05:53Z

lib/smf/smf.c

 static bool share_paren(const struct smf_state *test_state,
-			const struct smf_state *target_state)
+				 const struct smf_state *target_state)


Can you run clang-format on this?

keith-zephyr · 2024-05-20T15:07:05Z

lib/smf/smf.c

+		/*
+		 * The final target will be the deepest leaf state that
+		 * the target contains. Set that as the real target.
+		 */
+		while (new_state->initial) {
+			new_state = new_state->initial;
+		}


Fix the indentation.

Drat. Thought i caught them all. Clang-format run on the file.

keith-zephyr

Getting pretty close.
I'm also going to test this against my downstream uses of the SMF.

keith-zephyr · 2024-05-20T15:45:52Z

tests/lib/smf/src/test_lib_self_transition_smf.c

+	if (o->terminate == ROOT_ENTRY) {
+		smf_set_terminate(obj, -1);
+		return;
+	}


This condition never occurs and isn't verified by the test.

Non-blocking - prefer that you delete since it isn't used.

Suggested change

if (o->terminate == ROOT_ENTRY) {

smf_set_terminate(obj, -1);

return;

}

Would it be better to build a test that tests termination in root_entry? I think the other tests have that covered with PARENT_ENTRY not wrapped in a root state.

keith-zephyr · 2024-05-20T15:54:08Z

lib/smf/smf.c

@@ -35,8 +33,7 @@ static bool share_paren(const struct smf_state *test_state,
 	return false;
 }

-static bool last_state_share_paren(struct smf_ctx *const ctx,
-				   const struct smf_state *state)
+static bool last_state_share_paren(struct smf_ctx *const ctx, const struct smf_state *state)


This function no longer has any callers.

Yep, it's killing the tests. Let me delete it.

keith-zephyr · 2024-05-20T17:43:41Z

I tested this change against our downstream uses. Our unit tests pass and the code boots.

Downstream uses:
Power Delivery Subsystem
Power Delivery Driver

glenn-andrews · 2024-05-20T19:31:22Z

Looks like some bugs crept in. Let me update it when I'm off the clock.

…

On Mon, 20 May 2024 at 10:44, Keith Short ***@***.***> wrote: I tested this change against our downstream uses. Our unit tests pass and the code boots. Downstream uses: Power Delivery Subsystem <https://source.chromium.org/chromiumos/chromiumos/codesearch/+/HEAD:src/platform/ec/zephyr/drivers/usbc/pdc_rts54xx.c;l=1398;drc=d68d5db24185c658633393dff2cf1b2b391f0b08> Power Delivery Driver <https://source.chromium.org/chromiumos/chromiumos/codesearch/+/HEAD:src/platform/ec/zephyr/drivers/usbc/pdc_rts54xx.c;l=1398;drc=d68d5db24185c658633393dff2cf1b2b391f0b08> — Reply to this email directly, view it on GitHub <#71729 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AH7LIR7DDUXKUZPK3KXJ7MTZDIY6JAVCNFSM6AAAAABGRAWSBCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMRQHEYTANRUGQ> . You are receiving this because you authored the thread.Message ID: ***@***.***>

keith-zephyr · 2024-05-20T23:02:42Z

tests/lib/smf/src/test_lib_self_transition_smf.c

+	if (o->terminate == ROOT_ENTRY) {
+		smf_set_terminate(obj, -1);
+		return;
+	}


Non-blocking - prefer that you delete since it isn't used.

Suggested change

if (o->terminate == ROOT_ENTRY) {

smf_set_terminate(obj, -1);

return;

}

Modify the SMF such that state transitions from parent states choose the correct Least Common Ancestor based on the transition source rather than the current state. SMF set as experimental. Signed-off-by: Glenn Andrews <[email protected]>

glenn-andrews · 2024-05-22T22:01:40Z

@sambhurst Do you have time to look at this?

sambhurst

Looks good

henrikbrixandersen · 2024-05-24T08:02:10Z

@glenn-andrews In the future, please avoid mixing whitespace changes with functional changes in the same commit. It makes it very difficult to isolate and review (and understand) the actual, functional changes. Thanks!

glenn-andrews · 2024-05-24T14:07:09Z

@glenn-andrews In the future, please avoid mixing whitespace changes with functional changes in the same commit. It makes it very difficult to isolate and review (and understand) the actual, functional changes. Thanks!

Very good point and I will attempt to do so.

That leads me to a question, though.

I tried using Format Selection in VSCode on places I touched, which works, but doing so manually means I may miss some (as I did above with indentation) I then ran clang-format on the entire file as requested, which would change formatting for the entire file.

If the file does not currently comply with clang-format, what is the appropriate procedure?

May I run clang-format in one commit, then make code changes in a second commit in the same PR, or do they have to be separate PRs?

henrikbrixandersen · 2024-05-24T20:06:06Z

I tried using Format Selection in VSCode on places I touched, which works, but doing so manually means I may miss some (as I did above with indentation) I then ran clang-format on the entire file as requested, which would change formatting for the entire file.

If the file does not currently comply with clang-format, what is the appropriate procedure?

May I run clang-format in one commit, then make code changes in a second commit in the same PR, or do they have to be separate PRs?

If you really feel the need to reformat an entire file like this, please do so in a separate commit. It is okay to include it in the same PR as a set of functional changes.

glenn-andrews · 2024-05-24T20:15:12Z

If you really feel the need to reformat an entire file like this, please do so in a separate commit. It is okay to include it in the same PR as a set of functional changes.

Thank you. I'll try to do so in future. It's not so much feeling the need to touch the entire file as making sure my part is formatted correctly, which is easier if I can just clang-format the document before committing rather than only formatting the parts I touched.

Maybe I should run clang-format on the entire source tree and submit that as a PR 😉

henrikbrixandersen · 2024-05-24T20:27:43Z

Maybe I should run clang-format on the entire source tree and submit that as a PR 😉

You wouldn't be the first ;)

zephyrbot added area: USB Universal Serial Bus area: State Machine Framework State Machine Framework area: USB-C labels Apr 21, 2024

zephyrbot requested review from jfischer-no, keith-zephyr, sambhurst and tmon-nordic April 21, 2024 03:46

zephyrbot assigned sambhurst Apr 21, 2024

glenn-andrews force-pushed the smf_uml_rules branch 4 times, most recently from 1f035e2 to 5416d51 Compare April 21, 2024 17:01

zephyrbot added the Release Notes To be mentioned in the release notes label Apr 21, 2024

zephyrbot requested review from aescolar, kartben and nashif April 21, 2024 17:01

glenn-andrews commented Apr 21, 2024

View reviewed changes

tests/lib/smf/src/test_lib_self_transition_smf.c

Copy link

Collaborator Author

glenn-andrews Apr 21, 2024

Choose a reason for hiding this comment

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

This is actually an entirely new file, not a rename.

glenn-andrews force-pushed the smf_uml_rules branch 5 times, most recently from 4348b81 to eb05134 Compare April 25, 2024 18:07

glenn-andrews mentioned this pull request Apr 25, 2024

RFC: State Machine Framework: Breaking behavior change to align closer to the UML specification for State Machines #71675

Closed

glenn-andrews force-pushed the smf_uml_rules branch 4 times, most recently from a76ceb3 to 4350114 Compare May 11, 2024 02:17

keith-zephyr reviewed May 17, 2024

View reviewed changes

glenn-andrews force-pushed the smf_uml_rules branch from 4350114 to daa0a6d Compare May 18, 2024 17:40

glenn-andrews force-pushed the smf_uml_rules branch 2 times, most recently from a48b474 to 1460d7d Compare May 20, 2024 15:02

keith-zephyr reviewed May 20, 2024

View reviewed changes

glenn-andrews force-pushed the smf_uml_rules branch from 1460d7d to d091517 Compare May 20, 2024 15:19

keith-zephyr reviewed May 20, 2024

View reviewed changes

glenn-andrews force-pushed the smf_uml_rules branch from d091517 to 51ec1ba Compare May 20, 2024 19:33

keith-zephyr previously approved these changes May 20, 2024

View reviewed changes

glenn-andrews dismissed keith-zephyr’s stale review via ab678da May 21, 2024 00:30

glenn-andrews force-pushed the smf_uml_rules branch from 51ec1ba to ab678da Compare May 21, 2024 00:30

keith-zephyr approved these changes May 21, 2024

View reviewed changes

sambhurst approved these changes May 22, 2024

View reviewed changes

nashif merged commit 531c457 into zephyrproject-rtos:main May 23, 2024
24 checks passed

glenn-andrews mentioned this pull request May 24, 2024

Lib: SMF: Add UML-Style transition logic #70914

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lib: SMF Modify HSM operation for UML-Style transitions #71729

Lib: SMF Modify HSM operation for UML-Style transitions #71729

glenn-andrews commented Apr 21, 2024 •

edited

Loading

glenn-andrews Apr 21, 2024

keith-zephyr May 17, 2024

glenn-andrews May 18, 2024

keith-zephyr May 20, 2024 •

edited

Loading

glenn-andrews May 20, 2024

glenn-andrews May 20, 2024

glenn-andrews commented May 18, 2024

keith-zephyr May 20, 2024

keith-zephyr May 20, 2024

keith-zephyr May 20, 2024

glenn-andrews May 20, 2024

keith-zephyr left a comment

keith-zephyr May 20, 2024

keith-zephyr May 20, 2024

glenn-andrews May 21, 2024

keith-zephyr May 20, 2024

glenn-andrews May 20, 2024

keith-zephyr commented May 20, 2024

glenn-andrews commented May 20, 2024 via email

keith-zephyr May 20, 2024

glenn-andrews commented May 22, 2024

sambhurst left a comment

henrikbrixandersen commented May 24, 2024

glenn-andrews commented May 24, 2024

henrikbrixandersen commented May 24, 2024

glenn-andrews commented May 24, 2024

henrikbrixandersen commented May 24, 2024

		.. note:: State machines can only call :c:func:`smf_set_state` from Run
		functions, not Entry or Exit functions.

Lib: SMF Modify HSM operation for UML-Style transitions #71729

Lib: SMF Modify HSM operation for UML-Style transitions #71729

Conversation

glenn-andrews commented Apr 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keith-zephyr May 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glenn-andrews commented May 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keith-zephyr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keith-zephyr commented May 20, 2024

glenn-andrews commented May 20, 2024 via email

Choose a reason for hiding this comment

glenn-andrews commented May 22, 2024

sambhurst left a comment

Choose a reason for hiding this comment

henrikbrixandersen commented May 24, 2024

glenn-andrews commented May 24, 2024

henrikbrixandersen commented May 24, 2024

glenn-andrews commented May 24, 2024

henrikbrixandersen commented May 24, 2024

glenn-andrews commented Apr 21, 2024 •

edited

Loading

keith-zephyr May 20, 2024 •

edited

Loading