Robot api 10.14 - #549
Draft
urfeex wants to merge 39 commits into
Draft
Conversation
also queryPolScopeVersion
They use the same endpoint, so same commit
These are not implemented
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #549 +/- ##
==========================================
- Coverage 80.40% 79.92% -0.49%
==========================================
Files 116 117 +1
Lines 6976 7098 +122
Branches 3083 3164 +81
==========================================
+ Hits 5609 5673 +64
- Misses 987 1034 +47
- Partials 380 391 +11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
* Updated documentation * Brought it up to the Dashboard Client API * Implemented NotImplemented errors on G5
urfeex
marked this pull request as ready for review
August 20, 2026 15:17
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Adds PolyScope X 10.14 Robot API support to the dashboard client.
Changes:
- Implements popup, shutdown, logging, robot-information, flight-report, and support-file endpoints.
- Centralizes Robot API version gating and accepts all successful 2xx responses.
- Adds compatibility documentation, tests, a 10.14 CI target, and an updated program fixture.
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/ci.yml |
Adds the URSim 10.14 preview test target. |
doc/architecture/dashboard_client.rst |
Documents X-only support-file downloads. |
doc/polyscope_compatibility.rst |
Lists Robot API feature availability by version. |
include/ur_client_library/ur/dashboard_client.h |
Exposes popup titles and support-file downloads. |
include/ur_client_library/ur/dashboard_client_implementation.h |
Extends the implementation interface. |
include/ur_client_library/ur/dashboard_client_implementation_g5.h |
Adds G5-compatible overrides. |
include/ur_client_library/ur/dashboard_client_implementation_x.h |
Defines Robot API command metadata and X endpoints. |
src/ur/dashboard_client.cpp |
Forwards the new public API calls. |
src/ur/dashboard_client_implementation_g5.cpp |
Handles the updated popup signature and X-only command. |
src/ur/dashboard_client_implementation_x.cpp |
Implements the new Robot API requests and downloads. |
tests/resources/upload_prog.urpx |
Refreshes the PolyScope X program fixture. |
tests/test_dashboard_client.cpp |
Updates facade mocks and forwarding tests. |
tests/test_dashboard_client_g5.cpp |
Verifies the X-only command throws on G5. |
tests/test_dashboard_client_x.cpp |
Adds PolyScope X 10.14 integration coverage. |
Suppressed comments (5)
src/ur/dashboard_client_implementation_x.cpp:389
- Directly embedding
log_textmakes the request body invalid JSON for normal log messages containing quotes, backslashes, newlines, or other control characters. Serialize this value with the JSON library before posting it.
const std::string message = R"({"message": ")" + log_text + R"("})";
src/ur/dashboard_client_implementation_x.cpp:390
- This unconditionally mirrors the caller's log entry to process stdout, bypassing the library's configured logger and potentially exposing sensitive text in service/CI logs. Sending the entry to the robot should not also print it locally.
std::cout << message << std::endl;
src/ur/dashboard_client_implementation_x.cpp:563
- The endpoint returns a ZIP archive, but the destination is opened in text mode. On Windows, newline translation can modify bytes and corrupt the archive; open it in binary mode.
std::ofstream save_file(save_path, std::ios_base::out);
src/ur/dashboard_client_implementation_x.cpp:574
- The write result is never checked, so a disk-full or short-write failure still returns the successful HTTP response and reports that the archive was downloaded. Check the stream after writing and closing, and return
ok = falseon failure.
save_file << response.message;
response.message = "Downloaded support files to " + save_path;
src/ur/dashboard_client_implementation_x.cpp:578
- This is the support-file download path, so reporting a failed program download is misleading during diagnosis.
URCL_LOG_ERROR("Failed to download program. Response message: %s", response.message.c_str());
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| } | ||
| else | ||
| { | ||
| auto response = dashboard_client_->commandDownloadSupportFiles("/tmp/support_files.zip"); |
Member
Author
|
This should be ready to review now. The failing test shows a model clash between the RobotAPI and the primary interface. For a ur7e, the RobotAPI reports a UR7, the primary interface a UR5. I guess this is expected behavior, since for the controller a UR7 is a UR5. We would need to update this. @urrsk what would be your preferred solution? In the helpers we have Edit: I just realized, there's a lot of AI review I want to look at first. Converting back to draft. |
urfeex
marked this pull request as draft
August 21, 2026 06:46
| #else // _WIN32 | ||
|
|
||
| std::string temp_save_path = (dest_dir / (std::filesystem::path(save_path).filename().string() + ".tmp")).string(); | ||
| int tmp_fd = _s_open_s(temp_save_path.c_str(), _O_WRONLY | _O_CREAT | _O_EXCL | _O_BINARY, _S_IREAD | _S_IWRITE); |
as this will not get reached otherwise
urfeex
force-pushed
the
robot_api_10.14
branch
from
August 21, 2026 15:02
34b2388 to
fde78fd
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 15 changed files in this pull request and generated 3 comments.
Suppressed comments (3)
include/ur_client_library/ur/dashboard_client_implementation.h:327
- Replacing this pure virtual signature breaks every downstream
DashboardClientImplsubclass that currently overrides the one-argument method. Preserve the existing virtual and add a title-aware overload with a default implementation that delegates to the legacy method, so external implementations remain source-compatible.
virtual DashboardResponse commandPopup(const std::string& popup_text, const std::string& popup_title = "") = 0;
src/ur/dashboard_client_implementation_x.cpp:450
- A non-JSON HTTP error is parsed before
response.okis checked, causing this query to throw instead of returning its failedDashboardResponse. Restrict JSON parsing to successful responses.
auto json_data = json::parse(response.message);
if (response.ok)
{
response.data["serial_number"] = std::string(json_data["serialNumber"]);
src/ur/dashboard_client_implementation_x.cpp:437
- This parses error responses before testing
response.ok; a plain-text or empty 4xx/5xx body therefore raises a JSON exception rather than returning the API failure. Move parsing inside the success branch.
auto json_data = json::parse(response.message);
if (response.ok)
{
response.data["robot_model"] = std::string(json_data["robotType"]);
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 15 changed files in this pull request and generated 1 comment.
Suppressed comments (4)
Previously missed (2) — in code that hasn't changed since the last review.
src/ur/dashboard_client_implementation_x.cpp:563
- The third parameter of
postisbool debug, not a content type, so this string literal is implicitly converted totrue. The helper already sendsapplication/json; remove the misleading argument.
response = post(endpoint, "", "application/json");
src/ur/dashboard_client_implementation_x.cpp:698
std::filesystem::renamedoes not replace an existing destination on Windows, so repeat downloads fail there despite this method and its test claiming overwrite support. Use a Windows replacement primitive such asMoveFileExW(..., MOVEFILE_REPLACE_EXISTING)while retaining atomicrenameon POSIX.
// std::filesystem::rename replaces an existing destination atomically on POSIX and
// uses MoveFileExW(MOVEFILE_REPLACE_EXISTING) on Windows, so repeat downloads to
// the same path work correctly on both platforms.
std::error_code ec;
std::filesystem::rename(temp_save_path, save_path, ec);
include/ur_client_library/ur/dashboard_client.h:408
- The default argument preserves source compatibility only; replacing the one-parameter overload removes its mangled symbol and breaks ABI for existing binaries. The corresponding virtual signature change also breaks external
DashboardClientImplsubclasses. Keep the one-parameter overload delegating to a new two-parameter overload, and preserve the old implementation virtual contract.
bool commandPopup(const std::string& popup_text, const std::string& popup_title = "");
tests/test_dashboard_client_x.cpp:727
responseis never used because all assertions below are commented out. The CI build enables-Wall -Wextrawith warnings as errors, so this produces an unused-variable build failure; either assert the response or avoid binding it.
auto response = dashboard_client_->commandDownloadSupportFiles("/tmp/support_files.zip");
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 15 changed files in this pull request and generated no new comments.
Suppressed comments (3)
Previously missed (2) — in code that hasn't changed since the last review.
src/ur/dashboard_client_implementation_x.cpp:698
std::filesystem::renamedoes not replace an existing destination on Windows, so the second download to the same path fails despite the stated overwrite behavior. Use a Windows replacement primitive such asMoveFileExW(..., MOVEFILE_REPLACE_EXISTING)(while retaining POSIXrename) rather than removing the destination first, which would lose atomicity.
// std::filesystem::rename replaces an existing destination atomically on POSIX and
// uses MoveFileExW(MOVEFILE_REPLACE_EXISTING) on Windows, so repeat downloads to
// the same path work correctly on both platforms.
std::error_code ec;
std::filesystem::rename(temp_save_path, save_path, ec);
src/ur/dashboard_client_implementation_x.cpp:639
- The fixed
<destination>.tmpname combined with_O_EXCLmeans a temp file left by a crash permanently blocks all later downloads to that destination; simultaneous downloads also collide. Generate a randomized unique name on Windows, as the POSIX branch does, and retry exclusive-create collisions.
std::string temp_save_path = (dest_dir / (std::filesystem::path(save_path).filename().string() + ".tmp")).string();
int tmp_fd = -1;
_sopen_s(&tmp_fd, temp_save_path.c_str(), _O_WRONLY | _O_CREAT | _O_EXCL | _O_BINARY, _SH_DENYRW,
_S_IREAD | _S_IWRITE);
include/ur_client_library/ur/dashboard_client_implementation.h:327
- Adding a default argument does not preserve the existing virtual signature: downstream
DashboardClientImplsubclasses overridingcommandPopup(const std::string&)will no longer override this pure virtual and will fail to compile. Preserve the one-argument virtual overload and add a two-argument overload with a default forwarding implementation (which can ignore the title), then override the latter for PolyScope X.
virtual DashboardResponse commandPopup(const std::string& popup_text, const std::string& popup_title = "") = 0;
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 9978847. Configure here.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 15 changed files in this pull request and generated 1 comment.
Suppressed comments (4)
Previously missed (1) — in code that hasn't changed since the last review.
src/ur/dashboard_client_implementation_x.cpp:674
- For a 204 response, the vendored httplib skips the response handler entirely (
httplib.h:14137-14144). Consequentlystatus_codeis never populated here andhttp_okmerely retains its initial value, unlike every response handled byhandleHttpResult. Read the final status from the validResultbefore deciding success so callers can distinguish the documented no-content case.
if (http_ok)
include/ur_client_library/ur/dashboard_client.h:408
- Changing the one-argument public method to a two-argument method with a default does not preserve ABI: existing binaries still reference the old mangled symbol and will fail to link/load against the updated shared library. Keep the one-argument overload (and the response overload below) and add explicit two-argument overloads that the old methods forward to; mirror this through the implementation hierarchy.
bool commandPopup(const std::string& popup_text, const std::string& popup_title = "");
include/ur_client_library/ur/dashboard_client_implementation.h:561
- Removing
constchanges the pure virtual contract in an installed header. Any downstreamDashboardClientImplsubclass implementing the previousconstoverride will no longer override this method and becomes abstract. Preserve the existing virtual signature and move the reconnecting/mutating work needed by the X implementation into a separate helper or otherwise keep it out of this interface.
virtual void assertHasCommand(const std::string& command) = 0;
tests/test_dashboard_client_x.cpp:663
- This branch executes a real shutdown whenever remote-control tests are enabled, but nothing implements the preceding comment's requirement to skip this on physical robots. A developer running the integration suite against a real 10.14 robot can therefore power it down and disrupt the rest of the run. Gate the destructive call on the URSim environment (or a dedicated explicit opt-in).
auto response = dashboard_client_->commandShutdown();
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 15 changed files in this pull request and generated no new comments.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/ur/dashboard_client_implementation_x.cpp:683
- On Windows,
std::filesystem::renamefails whensave_pathalready exists; it does not useMoveFileExW(MOVEFILE_REPLACE_EXISTING). Consequently the documented repeat-download behavior and the overwrite test only work on POSIX. Use an explicit Windows replacement operation (while retaining atomic POSIXrename) so an existing archive can be overwritten cross-platform.
std::filesystem::rename(temp_save_path, save_path, ec);
Member
Author
|
There are currently two issues:
As I'll be a week off, I'll unfortunately have to let it sit here until then. |
urfeex
marked this pull request as draft
August 21, 2026 19:21
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This adds the new RobotAPI endpoints coming up in PolyScope X 10.14.
This implements the following commands:
commandClosePopup()commandCloseSafetyPopup()commandShutdown()commandPopup(text, title)commandAddToLog(text)commandPolyscopeVersion()commandGetRobotModel()commandGetSerialNumber()commandGenerateFlightReport()commandDownloadSupportFiles(save_path)Note
Medium Risk
Touches the dashboard/Robot API client used for robot control, including shutdown and streamed file downloads. Version gating and HTTP handling changes can affect existing PolyScope X integrations.
Overview
Adds PolyScope X 10.14 Robot API coverage to the dashboard client so commands that previously threw
NotImplementedExceptionnow work when the robot API is new enough.Newly implemented on X: popup open/close (optional title), close safety popup, shutdown, add-to-log, PolyScope version / robot model / serial number, generate flight report, and
commandDownloadSupportFiles(X-only; streams a zip to disk via a temp file). Existing commands now go through a sharedg_command_list+assertHasCommandcheck against Robot API vs marketing versions.HTTP handling treats any 2xx as success, adds DELETE, and tracks connection state. CI gains a 10.14 preview URSim matrix entry; docs list the 10.14 feature groups.
Reviewed by Cursor Bugbot for commit 9411d89. Bugbot is set up for automated code reviews on this repo. Configure here.