Repository navigation
Adding new FSM config value for apogee timer (Issue #223) - #227
Conversation
|
FSM SILSIM: 6/6 passed ✓ — plots |
EDED2314
left a comment
EDED2314
left a comment
There was a problem hiding this comment.
Is any other component affected by this? CRC?
thoughts on another name for apog_detect_time_passed?
|
The CRC stuff is calculated based on the full sizeof the struct (because Michael is intelligent), so this shouldn't cause an issue. I agree on the naming thing; any thoughts, @mpkarpov-ui? |
mpkarpov-ui
left a comment
mpkarpov-ui
left a comment
There was a problem hiding this comment.
would be really nice to somehow exercise this in fsm silsim at one point but I can't come up with off the top of my head how.
otherwise lgtm, I'll sleep better with this in :)
| * Peter Giannetos | ||
| * Aaditya Voruganti | ||
| * Micheal Karpov | ||
| * Michael Karpov |
| // Note: This evaluates to TRUE (no lockout) if the lockout is disabled, OR if the condition is met | ||
|
|
||
| if (apog_detect_low_speed && apog_detect_cruise_lockout) { | ||
| // Condition 3: Enough time has passed since launched |
There was a problem hiding this comment.
super nit: maybe "time passed since launch"? instead of "launched"
|
|
||
| if (apog_detect_low_speed && apog_detect_cruise_lockout) { | ||
| // Condition 3: Enough time has passed since launched | ||
| bool apog_detect_time_passed = ((current_time - launch_time) >= config.thresholds.apog_launch_t_gt); |
There was a problem hiding this comment.
re the naming comments: the telemega calls this "mach lockout", if we want to be more accurate/verbose could call it "apog_minimum_flight_time_met", or to follow telemega could do "apog_mach_lockout_expired"
Don't have a super strong opinion on it
|
also rebase to pick up the cam ci changes <3 |
tjmcmanamen38
left a comment
tjmcmanamen38
left a comment
There was a problem hiding this comment.
Looks good overall, agreed with michaels comments on the naming though I think something different could be good.
Also whats the rationale behind the values in the test cases? really the only one I would personally see the need to set it is aether offnom boo. generally shouldn't be needed for single stages / sustainers
|
would like to see a hitl run testing this feature or some other verification but okay to merge prior to that (i.e. set the lockout greater than the expected time to apogee and see that it changes the behavior, etc.) |
bb258d0 to
c97f49f
Compare
I did try setting a timer past where drogue should happen for a couple of the tests, and it waited until that timer had passed to actually transition, so it seems to work as it should. |
de91ecf
You're not wrong; I've changed it to just be that one |
good enough for initial verification IMO. |
I've resolved the issue that came to @mpkarpov-ui in a dream, and added an additional user config FSM value for a timer that must pass before the FSM can transition from COAST to DROGUE, to eliminate the possibility for an early transition, even if not accompanied with a pyro at that same time (because that is a separately configurable setting).