Skip to content

ENH: support fixed-time parachute deployment triggers (#437) - #1133

Merged
Gui-FernandesBR merged 3 commits into
RocketPy-Team:developfrom
thatrandomasiandev:enh/437-time-delay-parachute
Aug 15, 2026
Merged

ENH: support fixed-time parachute deployment triggers (#437)#1133
Gui-FernandesBR merged 3 commits into
RocketPy-Team:developfrom
thatrandomasiandev:enh/437-time-delay-parachute

Conversation

@thatrandomasiandev

Copy link
Copy Markdown

Summary

  • Adds trigger=("time", t_deploy) so parachutes can deploy at a fixed flight time from launch (motor delay-charge style), addressing ENH: Addition of Time Delay Parachute Deployment #437.
  • Flight sets parachute._eval_time before evaluating triggers; no change to the public (p, h, y, sensors, u_dot) triggerfunc signature.
  • Burnout-referenced delays are supported by composing with the motor: ("time", motor.burn_out_time + delay). A native ("burnout", delay) trigger is left as a follow-up to avoid coupling Parachute construction to motor state.

Test plan

  • Unit tests for ("time", t) True/False at boundary times, ascent-safe behavior, invalid forms, serialization
  • Flight._evaluate_parachute_trigger sets _eval_time and fires correctly
  • StochasticParachute accepts/rejects time triggers consistently with Parachute
  • Integration flight test: deploy within one sampling interval of t_deploy

Fixes #437

@thatrandomasiandev
thatrandomasiandev requested a review from a team as a code owner August 11, 2026 01:47
@Gui-FernandesBR Gui-FernandesBR linked an issue Aug 14, 2026 that may be closed by this pull request
@codecov

codecov Bot commented Aug 14, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.47%. Comparing base (df06506) to head (ffac553).
⚠️ Report is 1 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1133      +/-   ##
===========================================
+ Coverage    84.45%   84.47%   +0.01%     
===========================================
  Files          131      131              
  Lines        17495    17515      +20     
===========================================
+ Hits         14775    14795      +20     
  Misses        2720     2720              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Gui-FernandesBR

Copy link
Copy Markdown
Member

Conflicting with develop — please merge develop in and push so CI can run against the current base. I will review the substance after that.

Gui-FernandesBR and others added 2 commits August 15, 2026 13:03
Conflicts, both from RocketPy-Team#1121 ("evaluate parachute triggers once per time
node"), which landed after this branch was cut:

- rocketpy/simulation/flight.py: RocketPy-Team#1121 moved the parachute loop out of
  __simulate into __check_and_handle_parachute_triggers, and in doing so
  already changed the trigger evaluation time from self.t to node.t. This
  branch had made the same self.t -> node.t change by hand, so that part of
  it is now redundant upstream. Resolved to develop's extracted helper; the
  only thing this branch still changes in flight.py is the one line setting
  parachute._eval_time.

- tests/unit/test_parachute_triggers.py: both sides appended a test to the
  end of the file. Kept both.

Two follow-ups while here:

- Parachute.__init__ now declares self._eval_time = None next to
  _trigger_falling_only and _trigger_needs_height, so the attribute is part
  of the object's state rather than something Flight conjures onto it. The
  trigger closure reads self._eval_time directly; the getattr default was
  unreachable anyway, since triggerfunc only exists once __init__ has run.

- docs/user/parachute_triggers.rst gained a "Fixed-time trigger" section.
  That page documents the "apogee" and numeric-altitude forms and tells
  readers a burnout delay needs a custom trigger, so leaving the new
  built-in form out of it was the one real gap. Includes the
  ("time", motor.burn_out_time + delay) composition and a note that firing
  at speed produces large loads.

Verified: tests/unit 2131 passed, 17 skipped; the new integration test and
the three parachute unit modules pass; ruff check and format clean; pylint
10.00/10. Checked in a real flight that ("time", 3.0) and ("time", 8.0)
deploy at exactly those times while still ascending (vz +268 and
+192 m/s), and that a plain "apogee" chute still fires at apogee.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
RocketPy-Team#1116 widened the height check to numbers.Real and, to stop the two copies
of that check drifting apart, gave Parachute a single _is_a_height_trigger
that stochastic/ now calls. This branch had edited the same predicate and
the same two test modules, so it conflicted in three files. The two changes
compose better than either alone:

- _is_a_trigger keeps its ("time", t_deploy) branch, and the height case
  defers to _is_a_height_trigger as RocketPy-Team#1116 intended.

- The time delay now uses that same predicate instead of a float() coercion
  wrapped in try/except. That was a real disagreement waiting to happen:
  float("3.0") succeeds, so ("time", "3.0") was accepted by Parachute while
  the stochastic check refused it. A string delay is now refused on both
  sides rather than quietly coerced.

- RocketPy-Team#1116's drift guard grew the time forms, so it now covers this shape too.
  Reverting the Parachute side to the lenient coercion makes it fail with
  ('time', '3.0'): stochastic/ says False, Parachute says True.

- Both test modules had appended to the same lists and to the end of the
  same files; kept both sides throughout, and added ("time", "3.0") to the
  refused-trigger cases.

Verified: tests/unit 2154 passed, 17 skipped; the parachute unit modules and
the new integration test give 89 passed; ruff check and format clean; pylint
10.00/10.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@Gui-FernandesBR

Copy link
Copy Markdown
Member

Brought this up to date with develop and fixed what I found. All 9 checks are green and it merges clean. Two merge commits, e9518cd5 and ffac5533 — I merged rather than rebased so your commit history isn't rewritten under you.

Round one: #1121 ("evaluate parachute triggers once per time node"). It landed after your branch was cut and moved the parachute loop out of __simulate into __check_and_handle_parachute_triggers. In doing so it already changed the trigger evaluation time from self.t to node.t — the same change you had made by hand — so that part of your diff was redundant upstream. Resolved to develop's extracted helper. The only thing this PR still changes in flight.py is the one line setting parachute._eval_time, which is a much smaller footprint than before.

Round two: #1116 landed mid-review and widened the height check to numbers.Real, adding a single _is_a_height_trigger in Parachute that stochastic/ now calls, precisely so the two copies of that check can't drift apart. That collided with your _is_a_trigger edit and both test modules. The two changes compose better than either alone:

  • _is_a_trigger keeps your ("time", t_deploy) branch; the height case defers to _is_a_height_trigger.
  • The delay now uses that same predicate instead of float() in a try/except. This was a real disagreement waiting to happen: float("3.0") succeeds, so ("time", "3.0") was accepted by Parachute while the stochastic check refused it. A string delay is now refused on both sides rather than quietly coerced — which I think is the behaviour you want anyway, since a silently-working string delay is a typo that survives to flight time.
  • fix: accept numpy integer types as Parachute trigger #1116's drift guard grew the time forms, so it covers this shape now. Reverting the Parachute side to the lenient coercion makes it fail with ('time', '3.0'): stochastic/ says False, Parachute says True.

Two other things while I was in here:

  • Parachute.__init__ now declares self._eval_time = None next to _trigger_falling_only and _trigger_needs_height, so it's part of the object's state rather than something Flight conjures onto it from outside. The getattr default was unreachable anyway — triggerfunc only exists once __init__ has run.
  • Added a "Fixed-time trigger" section to docs/user/parachute_triggers.rst. That page documents the "apogee" and numeric-altitude forms and tells readers a burnout delay needs a custom trigger, so leaving the new built-in form out was the one real gap. It covers the ("time", motor.burn_out_time + delay) composition and notes that firing at speed produces large loads.

On the design, for the record: I checked the two things that could have made this quietly wrong and both are right. The overshoot fast path skips a parachute only when trigger_falling_only and is_ascending, and you set _trigger_falling_only = False, so a time trigger is not skipped during ascent. And to_dict builds an explicit dict, so _eval_time can't leak into saved .rpy files.

I also verified it in a real flight rather than only through the unit tests: ("time", 3.0) and ("time", 8.0) deploy at exactly those times while still ascending at +268 and +192 m/s, and a plain "apogee" chute on the same rocket still fires at apogee. tests/unit is 2154 passed / 17 skipped, the parachute modules plus your integration test give 89 passed, ruff is clean and pylint is 10.00/10.

One note for you: the ("time", t) list form (["time", 3]) is worth keeping even though it looks redundant — a tuple becomes a list through JSON, so it's what makes the trigger survive an .rpy round trip.

@Gui-FernandesBR
Gui-FernandesBR merged commit 7822ab1 into RocketPy-Team:develop Aug 15, 2026
9 checks passed
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.

ENH: Addition of Time Delay Parachute Deployment

2 participants