Add tests demonstrating failure to recover from stream starvation - #552
Add tests demonstrating failure to recover from stream starvation#552acmorrow wants to merge 4 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #552 +/- ##
==========================================
+ Coverage 78.85% 79.50% +0.65%
==========================================
Files 116 116
Lines 7632 6978 -654
Branches 3153 3084 -69
==========================================
- Hits 6018 5548 -470
+ Misses 1224 1050 -174
+ Partials 390 380 -10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
| // Stop writing to the trajectory socket for 500ms while the thread consumes | ||
| // what we already sent. We keep answering the reverse interface with NOOPs so | ||
| // that only the trajectory socket goes quiet. | ||
| for (int i = 0; i < 10; ++i) |
There was a problem hiding this comment.
Maybe sleep slightly shorter than 0.5 sec as this is exactly the timeout define on the robot, to avoid unstable test and the time what is used to compare here might run on different computers.
There was a problem hiding this comment.
Sure, that's reasonable. I'll cut it to 400ms.
|
@urrsk - I've pushed a possible solution to cleanup after starvation based on adding an id to points in a move. I followed up on your comment in #550 to discuss some of the ways my approach differs from your notes. Definitely interested to see what you think of this. It does seem to make the newly introduced tests go green, so that's a good start. |
…-recovery-tests
|
Hi @urrsk - Just checking in on this PR. Generally, does it seem like a viable approach? Can the wire protocol change be accepted? Please let me know if there is anything you need from me in order to help move a fix for this issue forward, whether under this mechanism or some other that you would prefer. I'm happy to rework it if needed. |
|
Without having dived into the details too much, it is obvious that this PR would require a major version bump. We do have a couple of things in mind that we want to change with the next major bump, so it might be reasonable to start preparing a 3.x on a new branch where this could potentially get merged to. Would you agree @urrsk ? |
3 participants
Hi @urrsk -
This PR, which I'm opening as a draft because some of the tests are currently failing, demonstrates the failure to correctly recover to a valid state after starving the arm of points during a streaming move.
There are also some new tests that currently pass, acting as controls/validation for existing behavior. They came into being as I was validating my understanding of the urscript behavior. But they seem like worthwhile tests on their own, and useful to prevent regressions. Easy enough to drop them if you just want the failing tests.
Note
High Risk
Changes the on-robot trajectory wire protocol and motion execution path (stale-point rejection and streaming failure recovery); breaking API and mandatory script pairing increase integration risk for custom external_control users.
Overview
Adds per-move identifiers across the trajectory socket and reverse-interface control messages so the robot can discard stale points after cancel, failure, or underrun instead of running them on the next move.
UrDriverincrementstrajectory_move_id_for eachTRAJECTORY_START/TRAJECTORY_STREAM_STARTand sets it onTrajectoryPointInterfacebefore writing points;external_control.urscriptsnapshots the announced id intrajectoryThreadand skips mismatched records (with periodicsyncwhen discarding long runs).Breaking: trajectory point records are 22 integers (was 21); control messages include
move_idin former padding (writeTrajectoryControlMessage(action, move_id, point_number, …)). Library and shippedexternal_control.urscriptmust be upgraded together; migration notes cover 2.x → 3.x.URScript fixes for streaming starvation / issue #550: on mid-stream underrun without
STREAM_END, cleartrajectory_streamingandtrajectory_points_leftso pre-move cleanup does not drain indefinitely; cleanup reads full point records including move id;is_first_pointonly clears after an executed point.Tests: unit coverage for move id on reverse and trajectory interfaces; robot streaming tests for gaps, no-lookahead producers, finite/stream recovery after underrun, stale points not executed, cancel with short count—with RTDE joint checks where motion matters.
Reviewed by Cursor Bugbot for commit 4aaffdf. Bugbot is set up for automated code reviews on this repo. Configure here.