Add tests demonstrating failure to recover from stream starvation - #552
Add tests demonstrating failure to recover from stream starvation#552acmorrow wants to merge 5 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 ? |
@urfeex that plan makes sense to me. For now, we will probably work against this PR for our stuff since it fixes a real use case for us. |
…-recovery-tests
| // 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) |
|
|
||
| It contains only breaking changes. | ||
|
|
||
| Migrating from 2.x.x to 3.x.x |
There was a problem hiding this comment.
@urfeex - Wanted to call this to your attention, since I note that you tagged this into the 2.15 version. This was speculative that the major version would bump with the wire, so if that isn't true, this should be reverted.
There was a problem hiding this comment.
We'll make a decision today when to include this. I've merely added it to the milestone so that I don't make a release without considering this beforehand. Most likely, this will indeed go to a 3.x release, though.
There was a problem hiding this comment.
I've moved it to a 2.15.1 milestone. We think that we can do this without an API breaking change (I will make a review about this later).
The wire-breaking changes are acceptable, since we consider the communication between the script code and the library an internal asset. We'll add a clear statement about this, separately. We have been doing wire-breaking changes in the past, as well.
urfeex
left a comment
urfeex
left a comment
There was a problem hiding this comment.
If we avoid doing API breakages here, we can merge this onto 2.x. Please also remove the migration text in that case.
| // every point written within them. It issues one identifier for each command that begins a move, | ||
| // and never reuses one for the life of a connection. A value of 0 belongs to no move, and is what | ||
| // the robot holds before it has been told about any. | ||
| std::atomic<int32_t> trajectory_move_id_ = { 0 }; |
There was a problem hiding this comment.
Could we move this to the ReverseInterface, instead? This way we would not need to expose it on the ReverseInterface's writeTrajectoryControlMessage() at all and remove the API break of this PR.
|
|
||
| - field 1: Trajectory control mode (1: TRAJECTORY_MODE_RECEIVE, 2: TRAJECTORY_MODE_STREAM_START, 3: TRAJECTORY_MODE_STREAM_END, -1: TRAJECTORY_MODE_CANCEL). See :ref:`streaming_trajectories` for the streaming modes. | ||
| - field 2: Trajectory point count. Its interpretation depends on the control mode in field 1. | ||
| - field 3: The identifier of the move that is being started, for the two control modes which start one. The robot adopts this identifier and then executes only those trajectory points which carry it, and that is what prevents points left over from a move that has already ended from running as part of the next one. The remaining control modes do not start a move, and the robot ignores the field for them. See :ref:`trajectory_point_interface` for the point record which carries the identifier back. |
There was a problem hiding this comment.
One way to avoid wire-breaking here, would be to re-use the ScriptReader::isVariableRegistered(const std::string& key) function introduced in 01292bd.
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
Breaking wire/API change (3.x) requires library and bundled URScript to upgrade together; changes real-time trajectory execution and recovery paths on the robot.
Overview
Introduces a per-move identifier on the trajectory/reverse-socket protocol so the robot can ignore stale trajectory points after a stream fails, is cancelled, or a producer keeps writing briefly. Trajectory records grow from 21 to 22 int32 fields; trajectory start messages on the reverse socket now carry
move_id(formerly padding).UrDriverincrements and assigns IDs onTRAJECTORY_START/TRAJECTORY_STREAM_START;TrajectoryPointInterface::setMoveId()stamps each outgoing point.writeTrajectoryControlMessage()gains a requiredmove_idargument (documented as a 3.x breaking change in migration notes).On the robot,
external_control.urscripttrackstrajectory_move_id, skips mismatched points intrajectoryThread, drains full records in cleanup, and on mid-stream underrun clearstrajectory_streamingandtrajectory_points_leftso the next move is not blocked bySTREAMING_SENTINELcleanup (issue #550).Adds unit tests for wire encoding of move IDs and broad integration tests for streaming gaps, underrun recovery (finite and stream), stale points after underrun, and cancel-with-understated count—with RTDE joint checks where motion must be verified.
Reviewed by Cursor Bugbot for commit 9196723. Bugbot is set up for automated code reviews on this repo. Configure here.