fix(NDX-517): introduce state machine for movement controllers#402
Conversation
…ill' of https://github.com/wandelbotsgmbh/nova-python-sdk into fix/NDX-517-Trajectory-execution-returns-before-standstill
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
…ill' of https://github.com/wandelbotsgmbh/nova-python-sdk into fix/NDX-517-Trajectory-execution-returns-before-standstill
|
looks great to me |
functionistic
deleted the
fix/NDX-517-Trajectory-execution-returns-before-standstill
branch
There was a problem hiding this comment.
Pull request overview
This PR introduces a finite state machine for trajectory execution lifecycle management in movement controllers. The implementation uses the python-statemachine library (version 2.5.0) to provide a reusable, testable abstraction that replaces ad-hoc boolean flag logic previously used to track trajectory completion states.
Changes:
- Adds
TrajectoryExecutionMachinestate machine with 7 states (idle, executing, ending, pausing, paused, ended, error) to track trajectory lifecycle - Refactors
move_forwardandTrajectoryCursorcontrollers to use the state machine for state tracking and completion detection - Adds comprehensive test suites for both the state machine itself and its integration with movement controllers
- Adds detailed documentation including ASCII diagrams, PlantUML, and Mermaid state diagrams
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| uv.lock | Adds python-statemachine 2.5.0 dependency and updates package version to 4.10.1 |
| pyproject.toml | Declares python-statemachine>=2.5.0 as a project dependency |
| nova/cell/movement_controller/trajectory_state_machine.py | New module implementing TrajectoryExecutionMachine with state transitions and location tracking |
| nova/cell/movement_controller/move_forward.py | Refactored to use state machine instead of trajectory_ended boolean flag |
| nova/cell/movement_controller/trajectory_cursor.py | Integrated state machine to replace process_motion_group_state helper function |
| nova/cell/movement_controller/init.py | Exports TrajectoryExecutionMachine and StateUpdate classes |
| nova/cell/movement_controller/README.md | New documentation with state diagrams and usage examples |
| tests/cell/test_trajectory_state_machine.py | Comprehensive unit tests for state machine with 567 lines covering all transitions |
| tests/cell/test_process_motion_group_state.py | Integration tests for move_forward controller's state monitoring logic |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| """``True`` when in a final state (ended or error).""" | ||
| return self.current_state in (self.ended, self.error) | ||
|
|
There was a problem hiding this comment.
The property name 'is_terminal' is misleading because it includes the 'ended' state, which is not truly terminal since the state machine allows transitions from 'ended' back to 'executing' via the 'start' command (line 140). Consider renaming this property to 'is_complete' or 'is_finished' to better reflect that it indicates completion states rather than states from which no transitions are possible. Only 'error' is truly terminal (marked as final=True).
| """``True`` when in a final state (ended or error).""" | |
| return self.current_state in (self.ended, self.error) | |
| """``True`` when in a terminal state from which no further transitions are allowed. | |
| Currently this is only the ``error`` state, which is marked as ``final=True`` in | |
| the underlying state machine. | |
| """ | |
| return self.current_state == self.error |
| │ ┌──────────┐ │ | ||
| └──ss──│ pausing │─ss──┘ | ||
| └──────────┘ | ||
|
|
There was a problem hiding this comment.
The ASCII state diagram in the docstring does not show the transition from 'ended' back to 'executing' via 'start', which is defined on line 140. The diagram should be updated to include this transition to accurately reflect the state machine's behavior. Consider adding an arrow from 'ended' back to 'executing' labeled with 'start' to match the implementation.
| ended --start--> executing |
| case api.models.TrajectoryEnded() | api.models.TrajectoryPausedOnIO(): | ||
| if standstill: | ||
| self._end_immediately() | ||
| else: | ||
| self._begin_ending() |
There was a problem hiding this comment.
The state machine treats TrajectoryPausedOnIO the same as TrajectoryEnded by transitioning to the 'ended' state (line 311), but this behavior is not documented in the module docstring, README, or any state diagrams. Users might expect TrajectoryPausedOnIO to transition to 'paused' state instead. Consider either documenting this design decision or reevaluating whether TrajectoryPausedOnIO should transition to a paused state rather than ended.
| case api.models.TrajectoryEnded() | api.models.TrajectoryPausedOnIO(): | ||
| if standstill: | ||
| self._end_immediately() | ||
| else: | ||
| self._begin_ending() |
There was a problem hiding this comment.
The state machine handles TrajectoryPausedOnIO (line 311) but there are no tests covering this behavior. Consider adding test cases to verify that TrajectoryPausedOnIO transitions the machine to the 'ended' state as intended, both with and without standstill.
| case api.models.TrajectoryRunning() | api.models.TrajectoryWaitForIO(): | ||
| self._keep_executing() |
There was a problem hiding this comment.
The state machine handles TrajectoryWaitForIO (line 323) by keeping the machine in the 'executing' state, but there are no tests covering this behavior. Consider adding test cases to verify that TrajectoryWaitForIO keeps the machine in 'executing' and does not trigger any completion conditions.
| if self.current_state == self.executing: | ||
| self._handle_executing(trajectory_state, standstill=state.standstill) | ||
|
|
||
| elif self.current_state == self.ending: | ||
| if state.standstill: | ||
| self._end_after_standstill() | ||
| else: | ||
| self._keep_ending() | ||
|
|
||
| elif self.current_state == self.pausing: | ||
| if state.standstill: | ||
| self._pause_after_standstill() | ||
| else: | ||
| self._keep_pausing() | ||
|
|
There was a problem hiding this comment.
When the state machine is in 'paused' or 'ended' states, the process_motion_state method will process states with execute information but won't trigger any transitions (lines 206-224 only handle executing, ending, and pausing states). This means location updates and has_execute flags will still be set even when paused/ended. Consider whether this is the intended behavior, or if states should be completely skipped when in paused/ended states until an explicit 'start' command is sent. Add documentation clarifying this behavior if it's intentional.
No description provided.