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

Reorganize states and enter actions with enum-map #48

Merged
merged 2 commits into from
May 13, 2024
Merged

Conversation

taesungh
Copy link
Member

Following from #39 and as part of #21, we can rename some of the states to identify what the pod is doing/has done rather than naming them after the action message. We can also simplify the correspondence for enter actions by using enum-map rather than HashMap. With EnumMap, all enum values must have an associated map value which eliminates the need for checking if the key exists. Additionally, EnumMap stores the mapping as an array which should be more performant than hashing.

The enter actions were previously just associated functions, but they'll need to manipulate pod components (e.g. the signal light, brakes, and others), so they were changed to actual methods which mutably borrow. The enter actions cannot be tested yet as the actual state transition logic from #39 needs to be fixed while completing #21.

Changes

  • Rename states to better reflect a sense of being rather than an action
  • Use enum-map instead of HashMap for complete mapping of enter actions
  • Make enter actions actual methods instead of associated functions
    • Take mutable references for eventual component usage

- Rename states to better reflect a sense of being rather than an action
- Use enum-map instead of HashMap for complete mapping of enter actions
- Make enter actions actual methods instead of associated functions
  - Take mutable references for eventual component usage
@taesungh taesungh requested a review from a team May 13, 2024 03:11
Copy link
Contributor

@samderanova samderanova left a comment

Choose a reason for hiding this comment

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

Thanks for working to make the code simpler. A few questions:

@taesungh taesungh force-pushed the update/fsm-states branch from 61c5dc8 to 9950142 Compare May 13, 2024 05:20
- Use corresponding verbs of the new state names on the control panel
  action buttons and for the socket event name
@taesungh taesungh force-pushed the update/fsm-states branch from 9950142 to a0e2864 Compare May 13, 2024 05:22
@taesungh taesungh requested a review from ryescholin May 13, 2024 06:32
Copy link
Member

@ryescholin ryescholin left a comment

Choose a reason for hiding this comment

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

Looks great

@taesungh taesungh merged commit 6f4e678 into main May 13, 2024
2 checks passed
@taesungh taesungh deleted the update/fsm-states branch May 29, 2024 03:49
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.

4 participants