PoC of streaming API for external_control#528
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 3 potential issues.
Reviewed by Cursor Bugbot for commit 6f44bdf. Configure here.
| # unread remainder still in the OS socket buffer is therefore | ||
| # the producer's total minus the consumer's consumed count. | ||
| trajectory_points_left = params_mult[3] - (STREAMING_SENTINEL - trajectory_points_left) | ||
| trajectory_streaming = False |
There was a problem hiding this comment.
STREAM_END lacks streaming guard
High Severity
TRAJECTORY_MODE_STREAM_END always recomputes trajectory_points_left using STREAMING_SENTINEL, without checking that an open stream is active. If STREAM_END arrives during a finite TRAJECTORY_START trajectory, after cancel, or twice, trajectory_points_left is no longer sentinel-based and the update can jump to a huge negative value and abort or truncate motion while still reporting success.
Reviewed by Cursor Bugbot for commit 6f44bdf. Configure here.
| # Producer signalled STREAM_END between the predicate check and | ||
| # the socket read. Drop any pending unread points and exit cleanly | ||
| # rather than spin one timeout per missing point. | ||
| trajectory_points_left = 0 |
There was a problem hiding this comment.
False success after stream end
Medium Severity
After STREAM_END clears trajectory_streaming, a failed trajectory-socket read with trajectory_points_left still positive forces trajectory_points_left to zero and leaves trajectory_result as success. That matches a post-STREAM_END producer underrun or an incorrect total in STREAM_END, so the client can get success while motion stops before the declared point count is executed.
Reviewed by Cursor Bugbot for commit 6f44bdf. Configure here.
| # unread remainder still in the OS socket buffer is therefore | ||
| # the producer's total minus the consumer's consumed count. | ||
| trajectory_points_left = params_mult[3] - (STREAMING_SENTINEL - trajectory_points_left) | ||
| trajectory_streaming = False |
There was a problem hiding this comment.
Streaming flag survives cancel
Low Severity
trajectory_streaming is set on TRAJECTORY_MODE_STREAM_START and cleared only on STREAM_END, not on TRAJECTORY_MODE_CANCEL or finite TRAJECTORY_MODE_RECEIVE. After cancel or switching to a declared-length trajectory without STREAM_END, the global stays true and can skew streaming-specific read-failure handling on later runs until another STREAM_END clears it.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 6f44bdf. Configure here.
| } | ||
| 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.
| option(INTEGRATION_TESTS "Build the integration tests that require a running robot / URSim" OFF) | ||
| option(POLYSCOPE_X_TESTS_WITH_REMOTE_CONTROL "Run Polyscope X tests that require remote control" OFF) | ||
| option(CHECK_RTDE_DOCS_RECIPE "Fetch the RTDE documentation to auto-generate a recipe containing all output fields and check that with the RTDE client. Requires python3 and pandas and an internet connection." OFF) | ||
| set(INTEGRATION_TESTS_ROBOT_IP "" CACHE STRING "If set, override the default robot IP (192.168.56.101) compiled into each integration test main() by passing --robot_ip <ip> to every ctest-registered integration test.") |
There was a problem hiding this comment.
I'm not sure why this is CACHE - it probably doesn't need to be. I'm also happy to find another mechanism to inject this state rather than a CMake variable. This was just an easy way to get it started. The really relevant change is computing INTEGRATION_TESTS_ROBOT_IP_ARG and then plumbing that through via EXTRA_ARGS on the test invocations
|
@urfeex - I see the bot has flagged a few things, I'll take a look at those and post an update. |
1 participant
@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
High Risk
Changes real-time robot motion control in
external_control.urscriptand the trajectory protocol; incorrect streaming semantics or edge cases could abort trajectories or leave motion in an unexpected state.Overview
Adds open-ended trajectory streaming for external control: producers can send spline points without declaring the total count up front, then finish with
TRAJECTORY_STREAM_ENDand the total point count.The C++ API gains
TrajectoryControlMessage::TRAJECTORY_STREAM_STARTandTRAJECTORY_STREAM_END, with expanded docs onReverseInterface/UrDriver::writeTrajectoryControlMessage.external_control.urscripthandles stream start (sentineltrajectory_points_left,trajectory_streamingflag), stream end (recompute remaining points from the producer’s total), and cleaner failure vs post-STREAM_ENDdrain intrajectoryThread.Ships
trajectory_streamingexample and headless integration tests (test_trajectory_streaming.cpp) for success, underrun failure, and mid-stream cancel. Integration test CMake gainsINTEGRATION_TESTS_ROBOT_IPand passes--robot_ipvia ctest; several testmain()parsers no longer stop after the first CLI flag.Reviewed by Cursor Bugbot for commit 6f44bdf. Bugbot is set up for automated code reviews on this repo. Configure here.