Fix flaky spline interpolation test - #556
Conversation
Made new method: sendTrajectoryAndConfirmStart which launches an async thread to confirm that a new trajectory resets the spline_travel_time and then starts executing. This is more robust than the previous implementation. I have replaced all the calls to sendTrajectory where they were immediately followed by waitForTrajectoryStarted, with one call to sendTrajectoryAndConfirmStart. waitForTrajectoryStarted is no longer used, but I havent removed it yet.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #556 +/- ##
==========================================
- Coverage 79.48% 79.42% -0.06%
==========================================
Files 116 116
Lines 6969 6969
Branches 3083 3083
==========================================
- Hits 5539 5535 -4
- Misses 1049 1051 +2
- Partials 381 383 +2 Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
There was a problem hiding this comment.
Pull request overview
Improves spline interpolation integration-test reliability by monitoring trajectory startup concurrently with upload.
Changes:
- Adds asynchronous trajectory-start confirmation.
- Replaces separate send/wait calls with the combined helper.
- Adds synchronization state for the watcher.
Suppressed comments (1)
tests/test_spline_interpolation.cpp:332
- Correct the spelling of “Continously” to “Continuously”.
// Continously check spline travel time
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
tests/test_spline_interpolation.cpp:333
- Correct the spelling of “Continously” in this new comment.
// Continously check spline travel time
| // Wait for trajectory to start (should be quick) | ||
| if (confirm_future.wait_for(std::chrono::milliseconds(500)) != std::future_status::ready) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (2)
tests/test_spline_interpolation.cpp:217
- This 500 ms deadline preempts the helper's own 1-second start/reset timeouts, so a valid start that takes 500–1000 ms is forcibly stopped and reported as a failure. That shortens the previous test tolerance and can preserve the flakiness this change is intended to remove; make the outer deadline longer than the internal timeout.
if (confirm_future.wait_for(std::chrono::milliseconds(500)) != std::future_status::ready)
tests/test_spline_interpolation.cpp:336
- Correct the spelling of “Continously” to “Continuously.”
// Continously check spline travel time
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (2)
tests/test_spline_interpolation.cpp:336
- Correct the spelling of “Continuously.”
// Continously check spline travel time
tests/test_spline_interpolation.cpp:342
- This reset can be observed before
sendTrajectory()has sent the newTRAJECTORY_START. Moreover,splineTimerTraveledresets for every spline segment (resources/external_control.urscript:402-404), so an existing trajectory crossing a segment boundary can set this flag and later satisfy the> 0.002check even if the new trajectory was not accepted. Use a separate signal set immediately after sending the new START command, and only treat resets observed after that signal as confirmation;trajectory_sentis too late because it is set after all points are uploaded.
if (spline_travel_time < start_spline_travel_time || spline_travel_time == 0.0)
{
spline_travel_time_reset = true;
}
Also add debug messages when failing Also make reverse socket read blocking
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 e1aa95b. Configure here.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (1)
tests/test_spline_interpolation.cpp:350
- When the register is initially
0.0,spline_travel_time_resetbecomes true before the upload starts, so this worker may send a NOOP concurrently withsendTrajectory()sending its NOOP/START messages. The reverse socket write path does not serialize concurrent writes, which can reorder or interleave control frames and reintroduce flakiness. Gate this keepalive on the existingtrajectory_sentflag (or otherwise serialize all reverse-interface writes).
if (!g_my_robot->getUrDriver()->writeTrajectoryControlMessage(
urcl::control::TrajectoryControlMessage::TRAJECTORY_NOOP))
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (1)
tests/test_spline_interpolation.cpp:327
- When the pre-send travel time is already
0.0(notably on the first test after startup), the worker only considers the reset observed if it receives another zero-valued RTDE package after notifying the sender. If the sender starts the spline before that next package, the first value can already be positive, so the loop never sets this flag and falsely times out. Treat the zero baseline already sampled immediately before signaling readiness as the reset state, then wait for the timer to advance.
bool spline_travel_time_reset = false;
| } | ||
|
|
||
| // Confirm that the new trajectory is running | ||
| if (std::abs(spline_travel_time) > start_spline_travel_time) |
There was a problem hiding this comment.
Wouldn't this be problematic when there was a long trajectory running beforehand? I don't remember by hard when we reset the spline_travel_time, but it seems fragile to rely on it being small in the beginning. Once it has been reset, being larger than a reasonable threshold should be sufficient, right?
| waitForTrajectoryStarted(); | ||
| ASSERT_TRUE(sendTrajectoryAndConfirmStart(s_pos, s_vel, std::vector<urcl::vector6d_t>(), s_time)); | ||
|
|
||
| g_trajectory_running = true; |
There was a problem hiding this comment.
We could probably set this inside sendTrajectoryAndConfirmStart, right?
| async_cv.notify_one(); | ||
| } | ||
|
|
||
| bool spline_travel_time_reset = false; |
There was a problem hiding this comment.
| bool spline_travel_time_reset = false; | |
| bool spline_travel_time_reset = start_spline_travel_time == 0.0; |
Made new method: sendTrajectoryAndConfirmStart which launches an async thread to confirm that a new trajectory resets the spline_travel_time and then starts executing. This is more robust than the previous implementation.
I have replaced all the calls to sendTrajectory where they were immediately followed by waitForTrajectoryStarted, with one call to sendTrajectoryAndConfirmStart. waitForTrajectoryStarted is no longer used, but I havent removed it yet.
Note
Low Risk
Changes are confined to spline interpolation test code and timing/synchronization logic; no robot driver or production paths are modified.
Overview
Spline interpolation integration tests were flaky because trajectory upload and “has it started?” checks ran sequentially on the main thread, so spline travel time in RTDE could be read too late or miss the reset when overlapping trajectories.
sendTrajectoryAndConfirmStartstartsconfirmTrajectoryStartedon a background thread (std::async), waits until that thread is polling RTDE, then sends the spline on the main thread. The helper thread watchesoutput_double_register_1: it expects travel time to drop or hit zero when a new plan is accepted, then waits until travel time moves past ~2 ms to mean execution started, issuing trajectory NOOPs while sending. Shared state uses a mutex, condition variable, and atomics (async_ready,async_stop,trajectory_sent).Tests that previously called
sendTrajectorypluswaitForTrajectoryStartednowASSERT_TRUE(sendTrajectoryAndConfirmStart(...)), including cases that send a second trajectory without canceling or after cancel.waitForTrajectoryStartedremains in the file but is unused.Reviewed by Cursor Bugbot for commit d4cd181. Bugbot is set up for automated code reviews on this repo. Configure here.