Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

State Machines Phase 2 #534

Merged
merged 568 commits into from
Dec 4, 2024
Merged

State Machines Phase 2 #534

merged 568 commits into from
Dec 4, 2024

Conversation

bocchino
Copy link
Collaborator

This pull request integrates Phase 2 state machines from the feature branch to main.

There are a lot of changes here, so I'll schedule one or more meetings to go over them.

Make event logging function const when there is no throttling
Make functions const where possible
@bocchino
Copy link
Collaborator Author

bocchino commented Nov 19, 2024

Notes from Review on 11/19/24

User's Guide

Comment Status
Add qualifiers to state machine sections to say “internal only” Done
Add diagrams for FPP user guide concepts (not blocking for PR) Done
Is “guard” sufficiently distinct from “guarded”? Perhaps "condition"? "Guard" is well-established in state machine terminology. Using a different term like "condition" could be confusing to users familiar with UML state charts. There seems to be little danger of confusion because guarded ports and state machine guards appear in different contexts.
Confirm cross reference to fprime/docs/Design on state machines Added cross references

@bocchino bocchino requested a review from Kronos3 November 19, 2024 19:24
@LeStarch
Copy link
Collaborator

LeStarch commented Nov 20, 2024

Notes from Review on 11/20/24

Notes on this PR

Comment Status
Get one more set of eyes on the Scala code Asked @Kronos3 to take a look
Remove slashes from state machine triggers when there is no action Done
Review state machine diagrams with Garth Done
Add comments to break up flattened transition codes (before doing each thing) Done, but note that we flatten exit actions and transition actions into a single list before generating code, so we don't have a way to separate those in the comments. The comments now separate exit/transition actions, entry actions, and target entry.
Review change to generated code for message buffers To be reviewed in PR diff. See, e.g., https://github.com/nasa/fpp/pull/534/files#diff-7451332676ad2ac4845778fd06af177449c06f0db8cdecce9216d3aee4d52b0c
Decide whether to move state machine init to a "state machine start" phase Tracked in #537. Not blocking for this PR.

Notes on F Prime Branch

See #508 (comment)

@bocchino
Copy link
Collaborator Author

All requested action items from 11/19/24 and 11/20/24 are complete.

Copy link
Collaborator

@Kronos3 Kronos3 left a comment

Choose a reason for hiding this comment

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

I skimmed through the generated code and it looks pretty good to me. One minor note is that it may be better in the future to build in the implementation of the state machine into the component class itself (i.e. dump the state machine logic into the owner class)

At the moment we are calling into the state machine from the component which in turns calls out actions back to the component. Another option might be to put the state machine action implementations in the hpp so that they may be inlined.

@bocchino
Copy link
Collaborator Author

Thanks for your comments.

it may be better in the future to build in the implementation of the state machine into the component class itself

Do you mean replicate the state machine base class in every component where the state machine is instantiated? Wouldn't that cause code duplication if the same state machine is used in more than one component? Or do you mean eliminate the inner class that implements the state machine? We know a way to do that, but it uses multiple inheritance and to me seems less clean than the solution we have adopted. It also has slightly worse type safety.

Another option might be to put the state machine action implementations in the hpp so that they may be inlined.

I think the action implementations can be inlined. There are two levels of action implementation (in the inner class and in the component class) but they are both defined in the same auto-generated cpp file, so the compiler can see them both.

@bocchino bocchino merged commit 80f3c4a into main Dec 4, 2024
11 checks passed
@bocchino bocchino deleted the feature/state-machine-phase-2 branch December 4, 2024 00:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants