Compile time State Machine#549
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the state machine implementation to enable compile-time type checking, moving from a runtime dynamic map-based approach to a compile-time template-based design using C++20 features.
Changes:
- Replaced
std::functioncallbacks with function pointers and dynamic containers with fixed-sizeStaticVector - Introduced templated
StateandStateMachineclasses that enforce type safety via concepts - Added
IStateMachineinterface to allow polymorphic access while maintaining type safety - Migrated implementation from
.cppto header-only templates
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| Inc/ST-LIB_LOW/StateMachine/StateMachine.hpp | Complete refactor introducing templated State and StateMachine classes with compile-time checking |
| Src/ST-LIB_LOW/StateMachine/StateMachine.cpp | Removed - implementation moved to header as templates |
| Inc/ST-LIB_LOW/StateMachine/StateOrder.hpp | Changed from vector to std::span for state order IDs |
| Src/ST-LIB_LOW/StateMachine/StateOrder.cpp | Updated StackOrder initialization to use span with nullptr |
| Inc/ST-LIB_LOW/StateMachine/StackStateOrder.hpp | Templated on StateMachineType for type safety |
| Inc/ST-LIB_LOW/StateMachine/HeapStateOrder.hpp | Templated on StateMachineType with deduction guide |
| Inc/ST-LIB_HIGH/Protections/ProtectionManager.hpp | Updated to use IStateMachine interface |
| Src/ST-LIB_HIGH/Protections/ProtectionManager.cpp | Updated to use IStateMachine interface methods |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
FoniksFox
left a comment
FoniksFox
left a comment
There was a problem hiding this comment.
Transitions should have their predicate passed in compile time to allow for better compiler optimization.
The Time module is deprecated, should use Scheduler now and get rid of all the High precission, Mid precission, Low precission stuff in favor of the unified scheduler api.
Would like to have some more concepts in things overall, I haven't tested it yet, but it appears fragile, I fear that I might get huge error messages from this class.
Won't comment on the API yet sicne I want to see the documentation with some use examples, but I like the helper functions.
This file is quite heavy for a reader, maybe consider partinioning it into smaller pieces and expose the main API foremost in a file without all the bloat of the rest of the code. That's just my preference though.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 17 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
I'm not sure about this (I've not checked it up completely), but are states reserving space for EVERY transition defined in any of the states of the state machine? That is, if you have 3 states, each with 3 transitions, I think that, right now, every states would reserve space for 9 transitions, is that right? Because if it is, it seems quite a waste. |
|
Nested State Machines are looked up in O(n), Can't you make them looked up in O(1)? To avoid unnecesary looping |
|
Right now, even if transitions are made in compile time, they add inderection when calling the predicate, since it could change. Declare the transitions array as a const array of transitions to allow the compiler to make a more aggresive optimization |
Refactor of the state machine to make compile time checks. Now the state machine can only accept states that are part of the same state enum and checks it in compile time.
To create a state machine, first create the states and its transitions with make_state and then create the state machine using this states. Then call the classical functions like add_enter_action, or add_low_precision_cyclic_action like always. Now only func pointers are allowed, no std::function
Wiki entry: https://wiki.hyperloopupv.com/es/firmware/st-lib/StateMachine
State Orders and that stuff i've adapted them to make the code compile, I cant ensure they will work. We dont lose much because previously they didnt work :p