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

Reduce fsm mem #225

Merged
merged 7 commits into from
Nov 4, 2023
Merged

Reduce fsm mem #225

merged 7 commits into from
Nov 4, 2023

Conversation

ShiCheng-Lu
Copy link
Collaborator

@ShiCheng-Lu ShiCheng-Lu commented Oct 29, 2023

  • remove the need for transition list definition by defining the resulting transition table directly.
  • remove redundant information:
    • id in FsmState, since the index into the FsmState list is the id of the state
    • curr_state change from a state pointer to a StateId (integer), store the state list in fsm to allow this.
    • num_transitions from FsmSetting
main:
   text    data     bss     dec     hex filename
  28368    1408   13808   43584    aa40 build/arm/bin/projects/centre_console

reduce_fsm_mem:
   text    data     bss     dec     hex filename
  28296    1452   12952   42700    a6cc build/arm/bin/projects/centre_console

Copy link
Collaborator

@mitchellostler mitchellostler left a comment

Choose a reason for hiding this comment

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

Hey, I'm happy with these changes. Also like that we don't have a num_transitions to worry about anymore.

I'm a bit worried about invalid accesses in the truth table if the size isn't properly initialized to NUM_STATESxNUM_STATES.

if we took the name of the transition array as a parameter in fsm_init, instead of making it part of the settings arg, I think we could do

#define fsm_init(fsm, transitions, settings, context)
static_assert(sizeof(transitions) == (settings.num_states * settings.num_states))
...

since fsm_init will be called in the same file as the 2d array is declared.
Will leave this up to you.

Would also be nice to verify transitions are correct, but I can't think of a way to do this other than adding back the num_transitions value and doing a check for redundancy, but this overcomplicates things

@ShiCheng-Lu
Copy link
Collaborator Author

ShiCheng-Lu commented Nov 1, 2023

I think if we take transition out, might as well take states and initial state out as well, kinda weird storing only two values in the setting.
fsm_init(fsm, states, transitions, initial_state, context) (maybe a bit too long)
but I think adding an assert for transition and state length is definitely good to have.

(even jankier can be that we just not have an initial_state parameter, and just make sure that the first state is the one the fsm starts on. As of rn, all fsm starts with state 0)

as for redundancy for num_transitions, while I was making changes, I found the power had multiple duplicate transitions, so I don't think the num_transitions count is very indicative, best to just keep the transitions organized.

@ShiCheng-Lu ShiCheng-Lu merged commit ed931d9 into main Nov 4, 2023
1 check passed
@ShiCheng-Lu ShiCheng-Lu deleted the reduce_fsm_mem branch November 4, 2023 03:44
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.

2 participants