Support on_entry and on_exit for states #66

MartinBroers · 2023-11-03T11:48:15Z

Issue #60 describes what I needed. I may have sprinkled a few Debug statements here and there which may not have been required, but I am open to suggestions to remove them.

I am opening this MR since @pleepleus asked to have a go at it. I don't want him to start from scratch.

ryan-summers

Some general feedback is below. I don't know the state of this or if the intent is to merge it as-is or if it's just inteded to be a starting point.

If this is just a reference implementation for someone else to pick up, can we mark it as a draft? Otherwise, I'm not sure if I should be reviewing or not.

ryan-summers · 2023-11-13T10:24:55Z

macros/src/codegen.rs

@@ -356,6 +370,10 @@ pub fn generate_code(sm: &ParsedStateMachine) -> proc_macro2::TokenStream {
                            .zip(out_states.iter().zip(guard_action_parameters.iter().zip(guard_action_ref_parameters.iter()))),
                    )
                    .map(|(guard, (action, (out_state, (g_a_param, g_a_ref_param))))| {
+                        let out_state_string = &out_state.to_string()[0..out_state.to_string().find('(').unwrap_or_else(|| out_state.to_string().len())];


I think a split is more appropriate and canonical here

Fixed. I think it is now more readable than I had it first, thanks.

ryan-summers · 2023-11-13T10:25:54Z

macros/src/codegen.rs

+            #entry_list
+
+            #[allow(missing_docs)]
+            fn transition_callback(&self, new_state: &Option<#states_type_name>) {}


Why a transition callback when we already have an on_entry and on_exit for all states? Is this basically just intended to be a default function?

Yes, understandable. Idea was a generic callback to capture the transition, I use it for bookkeeping in other parts of my application, synchronizing states to my backend. Could your on_entry_[stateA,stateB] and on_exit[stateA,stateB] for this as well, but I'd be duplicating the code for all transitions. So I thought a generic one is better.
Will hide this behind a feature flag as well.

ryan-summers · 2023-11-13T10:26:20Z

macros/src/codegen.rs

                                self.state = Some(#states_type_name::#out_state);
                            }
                        } else {
                            quote! {
                                self.state = Some(#states_type_name::#out_state);
+                                    self.context_mut().#exit_ident();
+                                    self.context_mut().#entry_ident();


I don't want to force everyone to implement these if they're not needed. Can we guard these calls based on their existence in the FSM definition?

Yes, I have seen other functions to be behind these flags, will update this accordingly (and will put this MR in draft while I haven't done that).

When I have this flag and the other comments fixed, I think it is good to go.

MartinBroers · 2023-11-14T09:39:37Z

Some general feedback is below. I don't know the state of this or if the intent is to merge it as-is or if it's just inteded to be a starting point.

If this is just a reference implementation for someone else to pick up, can we mark it as a draft? Otherwise, I'm not sure if I should be reviewing or not.

Yes, wasn't totally clear on that. Will work on this to make it final and ping you when I'm happy. Also, open for edits by others if they have time before I do.

MartinBroers · 2023-11-24T14:23:08Z

I have added two new features, generate_entry_exit_states and generate_transition_callback. Code will not be generated when these flags are not present. Please let me know what you think of these additions. I am a bit unsure on the naming of the whole thing, but naming is important.

ryan-summers · 2023-11-27T10:01:38Z

macros/src/codegen.rs

@@ -19,6 +19,9 @@ pub fn generate_code(sm: &ParsedStateMachine) -> proc_macro2::TokenStream {
    let state_machine_context_type_name =
        format_ident!("{sm_name}StateMachineContext", span = sm_name_span);

+    let generate_entry_exit_states = sm.generate_entry_exit_states;


I'd much rather we generate the calls by the existence of the exit/entry function as specified in the state machine itself.

Like, maybe:

statemachine! { *Init < enter_init > exit_init + transition / guard_transition => Next, Next < enter_next > exit_next + transition / guard_transition2 => Init, }

What do you think?

Then, if the enter_* and exit_* functions don't exist in the UML, we don't generate the respective calls.

Or the notation you specified at the end of #60 (comment) would work as well. If we're following Boost SML, that would be preferable.

dgehriger · 2024-03-06T16:17:35Z

@ryan-summers : do you know what the state of this PR is? Has it gone stale?

ryan-summers · 2024-03-06T17:03:55Z

I haven't seen any response from the author to my review, so I would assume it's been abandoned, but maybe these comments may grab their attention as well :)

MartinBroers · 2024-03-07T11:08:43Z

@dgehriger I had it on my list to fix, but got other priorities to work on first. I spent some time to rework the comments, but I failed in properly understanding the macros. Then I got other things on my plate and this got pushed down on my todo list.

So, to re-open the issue for myself and start thinking about it again, I have the token parser to parse < and >, but I am failing to get those functions into the generated result. Its been a while since I've looked at it though.

So, in transition.rs I now have

if input.parse::<Token![<]>().is_ok()

which generates an entry for StateTransitions. Now, I need to get those fields back in the result, if they are not None, then generate a function for them. My code is very much in progress, but I'll push it anyway, and I am very open to remarks about how to get this issue done with.

andygikling · 2024-03-25T03:46:48Z

Bump. This appears to be one of the more feature-rich state machines in rust that anyone uses. on_enter and on_exit are table stakes for a state machine library. Can we get this feature in here please?

ryan-summers · 2024-03-25T10:53:02Z

I have nothing against the implementation @andygikling, but it is not yet done to my knowledge.

This, like many open-sourced projects, is maintained primarily through volunteer time. I'm happy to review any proposed changes you may have for the repo, but I don't have significant amounts of time to implement new features unless I need them for specific projects that I'm working on.

andygikling · 2024-03-25T11:38:29Z

Ok thanks I'll take a closer look and see what's missing!

MartinBroers · 2024-05-26T12:42:16Z

So, I've finally gotten around to update this PR. I implemented the notation with > and < to indicate entry- and exit functions.

@ryan-summers pinging for review.

ryan-summers · 2024-06-21T15:06:12Z

I see that I initially proposed it, but the > and < notation is extremely confusing to me.

Would it make more sense to provide generic on_enter_<snake-case statename> and on_exit_<snake-case statename> for all states and provide them in the trait definition? Then, if the user wants to implement the entry/exit functions, they just have to provide a custom implementation.

This keeps the notation entirely out of the DSL and the compiler can optimize out the empty enter/exit calls if the user doesn't actually need them.

It also should make the necessary code changes here pretty simple.

MartinBroers · 2024-06-23T12:28:55Z

@ryan-summers Thanks for the feedback. That was my original thought as well. When I read your comment I tried the flag and in rebasing the flag didn't work. So I updated it now and now both ways should work. I kinda like your suggestion with the '<' and '>' as well, so I left them in for now. Updated README and added an example.

ryan-summers · 2024-06-26T12:27:49Z

@ryan-summers Thanks for the feedback. That was my original thought as well. When I read your comment I tried the flag and in rebasing the flag didn't work. So I updated it now and now both ways should work.

I was actually advocating for removing the flag entirely and having the on_entry and on_exit functions defined by-default for all users. This means the DSL doesn't actually change at all and this change isn't breaking. The trait would provide the default implementation, so for example:

enum States {
    Start,
}

trait StateMachineContext {
    fn on_entry_start() {
        // Default, empty implementation that the user can override in their context definition.
    }
    fn on_exit_start() {
    }


}

MartinBroers · 2024-06-27T18:44:48Z

I have removed the flag and let all on_entry and on_exit functions be autogenerated from now. I think it is pretty neat now.

Thanks for your time. I will have a look at the rebase error and the pipeline failures.

Should I also add my example to the tests suite?

This patch adds functions which are executed when the statemachine transitions from a state A to a state B. First on_exit_state_a is called (which is defined as an empty trait implementation which can be overridden) followed by a call to on_entry_state_b. The on_entry and on_exit functions can be implemented in the Statemachine Context trait implementation or ignored altogether, since the standard implementation is empty. Signed-off-by: Martin Broers <[email protected]>

This patch adds a callback for succesfull transitions. This function can be used to implement a logging service of some sorts for the state machine. Signed-off-by: Martin Broers <[email protected]>

By using `>` and `<` we can now define entry functions and exit functions on states. These should be defined once and will apply to all transitions interacting with either entering or exiting the state. Signed-off-by: Martin Broers <[email protected]>

Signed-off-by: Martin Broers <[email protected]>

Also the parse will not parse ">" and "<" states anymore, since all on_entry and on_exit functions will be autogenerated. Signed-off-by: Martin Broers <[email protected]>

ryan-summers · 2024-06-30T11:54:39Z

@MartinBroers I removed the transition_callback function for now because I wanted to separate it from the on_entry and on_exit change. I'm not opposed to the idea, but I think it needs more refinement/discussion first, and the other callbacks are ready for now. Feel free to open up a secondary PR for the transition callback changes if you'd still like them!

I think I'd go for the route where it's defined as empty-by-default and the user doesn't need to specify any flags etc.

MartinBroers force-pushed the add-transition-callback branch from 8698944 to 803a5b1 Compare November 12, 2023 14:31

ryan-summers reviewed Nov 13, 2023

View reviewed changes

MartinBroers marked this pull request as draft November 14, 2023 09:37

MartinBroers force-pushed the add-transition-callback branch from 803a5b1 to eef74ae Compare November 24, 2023 14:08

MartinBroers marked this pull request as ready for review November 24, 2023 14:16

MartinBroers force-pushed the add-transition-callback branch from eef74ae to 39a0771 Compare November 24, 2023 14:21

ryan-summers reviewed Nov 27, 2023

View reviewed changes

ryan-summers mentioned this pull request May 23, 2024

Is there support for multiple guarded transitions for a state and a triggering event ? #71

Closed

MartinBroers force-pushed the add-transition-callback branch 2 times, most recently from 3a30100 to 8f3929a Compare May 26, 2024 12:41

MartinBroers force-pushed the add-transition-callback branch from 89950ab to dc26da7 Compare June 23, 2024 12:40

Martin Broers added 4 commits June 27, 2024 21:40

Added callback for succesfull transition

5a751a0

This patch adds a callback for succesfull transitions. This function can be used to implement a logging service of some sorts for the state machine. Signed-off-by: Martin Broers <[email protected]>

Added entry and exit functions

2785f10

By using `>` and `<` we can now define entry functions and exit functions on states. These should be defined once and will apply to all transitions interacting with either entering or exiting the state. Signed-off-by: Martin Broers <[email protected]>

Added generic on_entry_on_exit flag

a127650

Signed-off-by: Martin Broers <[email protected]>

MartinBroers force-pushed the add-transition-callback branch from 0df9de1 to ca4fe43 Compare June 27, 2024 19:56

Removed on_entry flag

82351af

Also the parse will not parse ">" and "<" states anymore, since all on_entry and on_exit functions will be autogenerated. Signed-off-by: Martin Broers <[email protected]>

MartinBroers force-pushed the add-transition-callback branch from ca4fe43 to 82351af Compare June 27, 2024 19:57

Removing transition_callback, adding documentation, updating CHANGELOG

fa4f609

ryan-summers approved these changes Jun 30, 2024

View reviewed changes

ryan-summers merged commit 2d44416 into korken89:master Jun 30, 2024
7 checks passed

ryan-summers mentioned this pull request Jun 30, 2024

Support on_entry and on_exit for states #60

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support on_entry and on_exit for states #66

Support on_entry and on_exit for states #66

MartinBroers commented Nov 3, 2023

ryan-summers left a comment

ryan-summers Nov 13, 2023

MartinBroers Nov 24, 2023

ryan-summers Nov 13, 2023

MartinBroers Nov 14, 2023

ryan-summers Nov 13, 2023

MartinBroers Nov 14, 2023

MartinBroers commented Nov 14, 2023

MartinBroers commented Nov 24, 2023

ryan-summers Nov 27, 2023

ryan-summers Nov 27, 2023

ryan-summers Nov 27, 2023

dgehriger commented Mar 6, 2024

ryan-summers commented Mar 6, 2024 •

edited

Loading

MartinBroers commented Mar 7, 2024 •

edited

Loading

andygikling commented Mar 25, 2024 •

edited

Loading

ryan-summers commented Mar 25, 2024 •

edited

Loading

andygikling commented Mar 25, 2024

MartinBroers commented May 26, 2024

ryan-summers commented Jun 21, 2024 •

edited

Loading

MartinBroers commented Jun 23, 2024

ryan-summers commented Jun 26, 2024 •

edited

Loading

MartinBroers commented Jun 27, 2024 •

edited

Loading

ryan-summers commented Jun 30, 2024

Support on_entry and on_exit for states #66

Support on_entry and on_exit for states #66

Conversation

MartinBroers commented Nov 3, 2023

ryan-summers 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

Choose a reason for hiding this comment

MartinBroers commented Nov 14, 2023

MartinBroers commented Nov 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgehriger commented Mar 6, 2024

ryan-summers commented Mar 6, 2024 • edited Loading

MartinBroers commented Mar 7, 2024 • edited Loading

andygikling commented Mar 25, 2024 • edited Loading

ryan-summers commented Mar 25, 2024 • edited Loading

andygikling commented Mar 25, 2024

MartinBroers commented May 26, 2024

ryan-summers commented Jun 21, 2024 • edited Loading

MartinBroers commented Jun 23, 2024

ryan-summers commented Jun 26, 2024 • edited Loading

MartinBroers commented Jun 27, 2024 • edited Loading

ryan-summers commented Jun 30, 2024

ryan-summers commented Mar 6, 2024 •

edited

Loading

MartinBroers commented Mar 7, 2024 •

edited

Loading

andygikling commented Mar 25, 2024 •

edited

Loading

ryan-summers commented Mar 25, 2024 •

edited

Loading

ryan-summers commented Jun 21, 2024 •

edited

Loading

ryan-summers commented Jun 26, 2024 •

edited

Loading

MartinBroers commented Jun 27, 2024 •

edited

Loading