Fix no-progress timeout bug and apply code formatting#236
Conversation
- Fix duplicate ExecutionMonitor import in core/src/sim.rs - Apply clippy suggestions to collapse nested if statements - Fix mutable borrow issues in particle filter code - Apply rustfmt formatting throughout the workspace - Add execution_limits to run_particle_filter function Co-authored-by: jbrodovsky <57160841+jbrodovsky@users.noreply.github.com>
- Remove automatic last_progress update from check() method - Add mark_progress() method to explicitly mark progress - Update all callers to call mark_progress() after successful event processing - Fix test_execution_monitor_no_progress_timeout to properly test the timeout The no-progress timeout can now correctly detect when no progress has been made for the specified duration. Previously, check() was updating last_progress on every call, which prevented the timeout from ever triggering. Co-authored-by: jbrodovsky <57160841+jbrodovsky@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical bug in the ExecutionMonitor's no-progress timeout mechanism and applies code formatting improvements. The timeout was being reset on every call to check(), preventing it from ever triggering, which defeated its purpose as a watchdog timer.
Changes:
- Fixed ExecutionMonitor bug by separating timeout checking from progress marking
- Added explicit
mark_progress()calls in simulation loops after successful event processing - Applied code formatting improvements (collapsed nested if-statements, fixed import ordering, improved line wrapping)
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| core/src/sim.rs | Fixed ExecutionMonitor bug by removing automatic last_progress update from check() and adding new mark_progress() method; reorganized imports |
| sim/src/main.rs | Added mark_progress() calls after event processing in particle filter loops; reordered duplicate imports; marked unused _execution_limits variable |
| sim/src/common.rs | Collapsed nested if-statements into let-chain pattern for cleaner code |
| core/tests/integration_tests.rs | Improved line wrapping for better readability |
| core/src/rbpf.rs | Improved line wrapping in matrix operations |
| /// Mark that progress has been made in the simulation. | ||
| /// This should be called after successfully processing each event. | ||
| pub fn mark_progress(&mut self) { | ||
| self.last_progress = Instant::now(); | ||
| } |
There was a problem hiding this comment.
The new mark_progress() method lacks test coverage. The existing test test_execution_monitor_no_progress_timeout validates that the timeout triggers when no progress is made, but there is no test verifying that calling mark_progress() correctly resets the timer and prevents the timeout from triggering. Consider adding a test that calls mark_progress() between checks to ensure the timeout doesn't trigger when progress is being made.
| let csv_files = get_csv_files(&args.sim.input)?; | ||
| let is_multiple = csv_files.len() > 1; | ||
| let execution_limits = execution_limits_from_args(&args.sim); | ||
| let _execution_limits = execution_limits_from_args(&args.sim); |
There was a problem hiding this comment.
The _execution_limits variable is intentionally unused because geo_closed_loop_ukf and geo_closed_loop_ekf functions in the geonav crate don't accept ExecutionLimits parameters. This creates an inconsistency where geophysical navigation simulations lack execution timeout protection that other simulation modes have. Consider either updating the geonav functions to accept ExecutionLimits or removing this line until the functionality is ready to be integrated.
| let _execution_limits = execution_limits_from_args(&args.sim); |
The
ExecutionMonitor::check()method was updatinglast_progresson every call, preventing the no-progress timeout from ever triggering.Changes
last_progressupdate fromcheck(), added explicitmark_progress()method that callers must invoke after successfully processing each eventrun_closed_loopand particle filter loops to callmark_progress()after each eventExample
Known Limitation
The
_execution_limitsvariable inrun_geo_closed_loop_cliremains unused because geophysical navigation functions in thegeonavcrate don't yet acceptExecutionLimitsparameters.✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.