Feature/state machine #5
Conversation
Ian-Carroll
requested review from
andrewnash,
colton-smith,
Burke-Daniel,
paulina-alhassan and
DawsonMercer
|
|
||
| // Unit test asserting correct state transitions | ||
| // returns true if pass, false otherwise | ||
| void test_fsm() |
There was a problem hiding this comment.
Can we put tests in another file?
| // returns true if pass, false otherwise | ||
| void test_fsm() | ||
| { | ||
| char input; |
There was a problem hiding this comment.
Can you define somewhere what each char corresponds to, I figured it out after staring at the code for a while but a little key comment would be nice
|
|
||
| if (curr_state == (int)boot) | ||
| { | ||
| if (input == 'P' || input == 'p' || input == 'W' || input == 'w') |
There was a problem hiding this comment.
this if is the same for every state, IMO you should just pull it out to the beginning and break early if called
|
|
||
| int curr_state = (int)**state; | ||
|
|
||
| if (curr_state == (int)boot) |
There was a problem hiding this comment.
missing logic to deny state change if health not good
| { | ||
| State boot = BOOT; | ||
| State autonomous = AUTONOMOUS; | ||
| State manual = MANUAL; |
There was a problem hiding this comment.
Maybe I'm just not used to C code but isn't it redundant to recreate these globals?
| State autonomous = AUTONOMOUS; | ||
| State manual = MANUAL; | ||
|
|
||
| int curr_state = (int)**state; |
There was a problem hiding this comment.
is there a reason to reassign this variable that's passed in? Shouldn't you just name the original variable curr_state
There was a problem hiding this comment.
also shouldn't you be using camel case not snake case?
|
|
||
| else | ||
| { | ||
| *state = &autonomous; |
There was a problem hiding this comment.
am I reading this right as a state change from autonomous to autonomous?
Implemented a basic demo and unit testing for the Node state machine. Additional info is in fsm/README.md. This code needs to be super solid so any grilling is much appreciated!