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

Fix NAV_STATE enum values #3274

Merged
merged 3 commits into from May 31, 2018

Conversation

Projects
None yet
3 participants
@fiam
Copy link
Member

commented May 25, 2018

Since POSHOLD_2D entries were removed, we need to add 2 dummy
entries to keep the old values as they were, since they are logged
to blackbox.

Fix NAV_STATE enum values
Since POSHOLD_2D entries were removed, we need to add 2 dummy
entries to keep the old values as they were, since they are logged
to blackbox.

@fiam fiam added this to the 2.0 milestone May 25, 2018

@digitalentity
Copy link
Member

left a comment

Each navigation fsm state consumes flash since it's an array entry. However, we might reuse these entries for i.e. @giacomo892's CRUISE mode ;)

@fiam

This comment has been minimized.

Copy link
Member Author

commented May 25, 2018

@digitalentity Reusing them means that someone will need to update the blackbox related tools to deal with different meanings. At 80 bytes per entry, I think we can afford the wasted flash.

@stronnag

This comment has been minimized.

Copy link
Collaborator

commented May 25, 2018

Of historic note only; we've changed them in a non-backwards compatible way at least 3 times before without complaints. mwp's bb replay tool keeps a little list ...

@digitalentity

This comment has been minimized.

Copy link
Member

commented May 25, 2018

It would probably be better to add a field with permanent ID to fsm state entries and have this permanent ID logged to blackbox. This will be quite future-proof.

@fiam

This comment has been minimized.

Copy link
Member Author

commented May 25, 2018

Just noticed that we also have a seemingly unused value, NAV_STATE_LAUNCH_MOTOR_DELAY. I think we should just add additional field with the state permanent ID and log that instead.

Add a new field to each navigation state with its public ID
This ID will be permanent from now on, so we cahn freely change
navigationFSMState_t without having to adjust external logs
dealing with blackbox logs.

@fiam fiam force-pushed the agh_fix_nav_state_numbers branch from e83e1c2 to 48a0547 May 25, 2018

@@ -176,41 +176,82 @@ typedef enum {
NAV_FSM_EVENT_COUNT,
} navigationFSMEvent_t;

// This enum is used to keep values in blackbox logs stable, so we can
// freely change navigationFSMState_t.

This comment has been minimized.

Copy link
@digitalentity

digitalentity May 30, 2018

Member

I don't like the name PUBLIC_STATE, same naming as in box modes seems more reasonable - PERSISTENT_ID

This comment has been minimized.

Copy link
@fiam

fiam May 30, 2018

Author Member

Roger that

@@ -243,6 +284,7 @@ typedef enum {
} navigationFSMStateFlags_t;

typedef struct {
navigationPublicState_t publicState;

This comment has been minimized.

Copy link
@digitalentity

digitalentity May 30, 2018

Member

persistentId ?

Rename from "public state" to "persistent ID"
- NAV_PUBLIC_STATE_* -> NAV_PERSISTENT_ID_*
- .publicState -> .persistentId
- .navPublicState -> .navPersistentId

@digitalentity digitalentity merged commit e45cf00 into development May 31, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@digitalentity digitalentity deleted the agh_fix_nav_state_numbers branch May 31, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.