Added the minimum time to wait before transitioning from BURNOUT back… #172
Conversation
rkontham24
left a comment
rkontham24
left a comment
There was a problem hiding this comment.
Good duration, though I think we should follow the naming convention of the thresholds. Something like sustainer_burnout_to_first_boost_time_threshold
|
Hey, I changed the variable name to sustainer_burnout_to_first_boost_time_threshold to fit the naming conventions better. |
|
Looks like you have to change it on line 171 as well, judging by our CI complaining. Full review incoming |
There was a problem hiding this comment.
Overall you're headed in the right direction, but 2 big things:
- There are actually two fsms in
fsm.cpp(one for booster; one for sustainer), it seems in this PR only one is edited but this logic should maintain parity on both (even if it only really matters for sustainer). - I think it would be helpful to look into the FSM functionality a bit, @rkontham24 and @tjmcmanamen38 are good resources for this
Other comments below
| case FSMState::STATE_FIRST_BOOST: | ||
| // if acceleration spike was too brief then go back to idle | ||
| if ((state_estimate.acceleration < sustainer_idle_to_first_boost_acceleration_threshold) && ((current_time - launch_time) < sustainer_idle_to_first_boost_time_threshold)) { | ||
| if ((state_estimate.acceleration < sustainer_idle_to_first_boost_acceleration_threshold) && ((current_time - launch_time) < sustainer_burnout_to_first_boost_time_threshold)) { | ||
| state = FSMState::STATE_IDLE; | ||
| break; | ||
| } |
There was a problem hiding this comment.
This logic is incorrect, this conditional is a guard against instantaneous (non-sustained) accelerations while the vehicle is on pad (or, in the IDLE state). I believe the logic here is correct as-is before, and this change should likely be reverted.
| // Minimum time to wait before deciding to go from BURNOUT to FIRST_BOOST. (ms) | ||
| #define sustainer_burnout_to_first_boost_time_threshold 250 |
There was a problem hiding this comment.
This comment could use some work. Nominally, the rocket should never go BURNOUT -> FIRST_BOOST, so it doesn't necessarily make sense to call it "minimum time to wait before x".
A better comment could be "Minimum duration of sustained acceleration before fallback to boost"
| case FSMState::STATE_BURNOUT: | ||
| // if low acceleration is too brief than go on to the previous state | ||
| if ((state_estimate.acceleration >= sustainer_coast_detection_acceleration_threshold) && ((current_time - burnout_time) < sustainer_coast_time)) { | ||
| if ((state_estimate.acceleration >= sustainer_coast_detection_acceleration_threshold) && ((current_time - burnout_time) < sustainer_coast_time && ((current_time - burnout_time) > minimum_time_for_burnout_to_first_boost))) { |
There was a problem hiding this comment.
I disagree with this logic, this will still trigger the back transition to FIRST_BOOST if the instantaneous acceleration occurs, the only thing this guards is it will prevent the FSM from doing so in the first 0.25s that the FSM is in the BURNOUT state.
What we're looking for instead is that
state_estimate.accelerationexceeds the threshold- The elapsed time is less than the coast time
AND
both of the above conditions are met for at least x seconds (in our case, 0.25s). Consider the following flight:
t-0: IDLE
t+0: FIRST_BOOST
t+3: BURNOUT
t+3.75: accel sensor issue causes it to read +16g accel
t+3.76: accel sensor issue clears and accel returns to normal
With the current code:
At 3.76s...
state_estimate.acceleration(16g) >= the threshold ✅(current_time - burnout_time)= 0.75s, <sustainer_coast_time✅(current_time - burnout_time)= 0.75s, >minimum_time_for_burnout_to_first_boost(0.25s) ✅
So the back transition is triggered even though it shouldn't be.
With the logic outlined above, in the same condition
state_estimate.acceleration(16g) >= the threshold ✅(current_time - burnout_time)= 0.76s, <sustainer_coast_time✅- Conditions above have been met for 0.01s, <
minimum_time_for_burnout_to_first_boost❌
So we don't back transition
… to FIRST_ BOOST.
I just added another condition that checks the amount of time that we are in BURNOUT before deciding to transition back into FIRST_BOOST. This makes sure that we don't instantaneously go back to FIRST_BOOST.