PoC of streaming API for external_control#528
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| } | ||
| const auto stream_end_send_wall = std::chrono::steady_clock::now(); | ||
| g_my_robot->getUrDriver()->writeTrajectoryControlMessage( | ||
| urcl::control::TrajectoryControlMessage::TRAJECTORY_STREAM_END, k_num_points); |
There was a problem hiding this comment.
This is an important note: the client is obligated to provide the total number of points streamed when calling TRAJECTORY_STREAM_END, so that we can properly reset the counter so it counts down to zero and the trajectory ends naturally.
|
@urfeex - I see the bot has flagged a few things, I'll take a look at those and post an update. |
|
@urfeex - Also, an update: today we got this running on a physical UR5e and we were able to stream to the arm. So, there is at least some evidence that this is feasible in the real world. |
|
@urfeex - Pushed an update to address the deficiencies that the cursor bot tagged. I had claude write some tests, we confirmed they failed, made fixes, confirmed they passed. |
urfeex
left a comment
urfeex
left a comment
There was a problem hiding this comment.
It would be nice if we could move the IP-address refactoring to a separate PR, so this one gets a bit smaller in scope.
From a first glance, this looks quite clean. We would need some documentation updates, both on the trajectory point interface and one for the example (which could contain a lot of the text written in the example itself right now).
| * \param trajectory_action One of the values of TrajectoryControlMessage. The value selects which | ||
| * trajectory-control action the URScript dispatcher takes, and dictates how \p point_number is | ||
| * interpreted: | ||
| * - TRAJECTORY_CANCEL (-1): Cancels the currently executing trajectory. \p point_number is unused. |
There was a problem hiding this comment.
What happens to the remaining points if a trajectory is canceled? If I understand things correctly, the buffer cleanup will try to clean a very high number of points and then hit the socket read timeout as defined in clearTrajectoryPointsThread(). We could make this more effective (avoid the timeout) by passing the total number of points along as done with TRAJECTORY_STREAM_END. It should still work passing along 0 to not break behavior.
There was a problem hiding this comment.
I just tested this and adding trajectory_points_left = params_mult[3] - (STREAMING_SENTINEL - trajectory_points_left) in the script code section for elif params_mult[2] == TRAJECTORY_MODE_CANCEL: made the cleanup function clean the remaining number of points not timing out on reading from the socket.
There was a problem hiding this comment.
@urfeex - I just want to confirm before I make the change: you suggest that for writeTrajectoryControlMessage(TRAJECTORY_CANCEL, ..., if the trajectory was started with TRAJECTORY_STREAM_START, then point_number becomes meaningful, and should equal the total number of points sent, just like it would be for TRAJECTORY_STREAM_END, and then around line 1196 in external_control.urscript, if we are in streaming mode we adjust the point count as indicated above?
There was a problem hiding this comment.
Yes, exactly. Do you think, that makes sense?
There was a problem hiding this comment.
I think it makes total sense. I will get an update pushed.
| * calls the producer will issue on the trajectory socket. URScript reads exactly that many points | ||
| * and then completes. | ||
| * - TRAJECTORY_STREAM_START (2): Begins an open-ended (streaming) trajectory. The producer streams | ||
| * spline points and signals end-of-stream with TRAJECTORY_STREAM_END. \p point_number is unused. |
There was a problem hiding this comment.
Is this restricted to splines only? From what I can see the same mechanism also works for sending movej, movel, etc.
There was a problem hiding this comment.
I just tested this with the motion sequence from the instruction_executor example. This worked like a charm.
There was a problem hiding this comment.
That's great: it isn't a mechanism we happen to use right now, so not something I was thinking about testing. I could change that language from spline points to points or primitives?
There was a problem hiding this comment.
How about motion segments or motion primitives?
There was a problem hiding this comment.
Yes, I'll do motion primitives as that is what spline points actually reduce to, so it is I think the most accurate.
|
I've added the two issues that this PR will close when being merged. |
|
@urfeex - No problem about splitting out the testing changes, they were in their own commit anyway. I've made a new issue (#530) and a new PR (#531). I just made a new branch and cherry-picked out the relevant commit from this one. We will need to sort out how to drop out that commit from this review since it is the base commit. Probably, we just review what is here until satisfied, and then I'll do a final rebase above master and force-push to drop out the now vacuous testing changes commit, but lmk if you want to handle it differently. |
|
Yes, a rebase on master once the testing changes are merged should be the way to go. |
|
@urfeex - OK, rebased and force pushed. |
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## master #528 +/- ##
==========================================
- Coverage 79.06% 71.73% -7.33%
==========================================
Files 115 115
Lines 6767 6765 -2
Branches 2988 2988
==========================================
- Hits 5350 4853 -497
- Misses 1043 1570 +527
+ Partials 374 342 -32 Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
|
@urfeex - Pushed two commits: one with the pure comment change about splines, and another with the improvements around cancellation taking the number of points. |
|
@urfeex - Just checking in on this, as I'm not aware of anything more you had asked to change in this review. But if I've overlooked a previous comment or you have additional thoughts, please let me know and I'll be happy to iterate on them. |
|
No, I consider that to be in my field right now. This review is on my short list already, I will get to this before the end of the week! |
urfeex
left a comment
urfeex
left a comment
There was a problem hiding this comment.
I have two cosmetic suggestions, but we are still missing the updated sphinx documentation.
Specifically, the reverse interface documentation in https://github.com/UniversalRobots/Universal_Robots_Client_Library/blob/master/doc/architecture/reverse_interface.rst needs a major update and should outline the communication contract as you've done in the example.
Also, we have accompanying sphinx pages for each example in https://github.com/UniversalRobots/Universal_Robots_Client_Library/tree/master/doc/examples, so that should be added, as well. As I have written above, this should contain most of the comments from the cpp file, so the cpp example gets a bit more compact to read on its own.
Co-authored-by: Felix Exner <feex@universal-robots.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
Reviewed by Cursor Bugbot for commit 3708a9d. Configure here.
| # In streaming mode, a cancel should come with the total count, just like a STREAM_END. | ||
| trajectory_points_left = params_mult[3] - (STREAMING_SENTINEL - trajectory_points_left) | ||
| trajectory_streaming = False | ||
| end |
There was a problem hiding this comment.
Streaming cancel skips buffer drain
High Severity
For an active streaming trajectory, TRAJECTORY_CANCEL now recomputes trajectory_points_left from params_mult[3] like STREAM_END. When that total is missing, zero, or negative (e.g. default point_number or -1 from existing cancel callers), the result is zero or negative, so clearTrajectoryPointsThread never drains the trajectory socket, yet TRAJECTORY_RESULT_CANCELED is still sent. Leftover points can be consumed by the next trajectory.
Reviewed by Cursor Bugbot for commit 3708a9d. Configure here.
There was a problem hiding this comment.
@cursoragent - Isn't that a violation of the documented client obligations? They are required to pass a correct point count when cancelling a trajectory.
There was a problem hiding this comment.
Unable to authenticate your request. Please make sure to connect your GitHub account to Cursor. Go to Cursor
|
@urfeex - I had claude put together the desired documentation. It got a little overzealous since my ambient instructions for my local development include running clang-format, and it decided that was something to do at the same time. Unfortunately, I don't know the exact version of clang-format you use, so it is possible that this isn't going to work out well. I also asked claude to change around some of the existing language from the example, where it had gotten overly focused on sockets and buffers, and instead focus on the client obligation to keep the arm from starving for work. I think it reads better now, overall. As always, happy for any feedback you have on this! |
2 participants
@urfeex - Here is what I had Claude put together to demonstrate the possibility of keeping open trajectories, per #524.
It includes:
STREAM_STARTandSTREAM_ENDcontrol messages to theTrajectoryControlMesssageenumeration.external_contro.urscriptto support stream start and stream end.I'm using this right now and it seems to work for some basic cases. I haven't stress tested it or thought deeply about the various edge cases or error scenarios that this may need to cover.
The goal here is really just to put some code behind the ideas over in #524 so we can discuss further. I'm happy to rework any of this as necessary.
I will also leave some comments directly on the review for a few things I want to call out.
Note
Medium Risk
Changes real-time trajectory execution and reverse-interface protocol on the robot; mistakes could truncate motion or mis-report results, though finite trajectories are largely preserved and edge cases are tested.
Overview
Adds open-ended trajectory streaming alongside the existing finite
TRAJECTORY_STARTflow: producers open withTRAJECTORY_STREAM_START, send motion primitives without a fixed upfront count, then close withTRAJECTORY_STREAM_END(and the same total count onTRAJECTORY_CANCELin streaming mode).The C++ API documents the new
TrajectoryControlMessagevalues onReverseInterface/UrDriver.external_control.urscriptimplements streaming viaSTREAMING_SENTINEL, atrajectory_streamingflag, guardedSTREAM_ENDhandling, and tighter underrun / race logic intrajectoryThread.Also ships
examples/trajectory_streaming, architecture and example docs, and headless integration tests (success, underrun, cancel, stray/doubleSTREAM_END, over-count).Reviewed by Cursor Bugbot for commit 3708a9d. Bugbot is set up for automated code reviews on this repo. Configure here.