Robot api 10.14 - #549
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.51% 80.68% +0.16%
==========================================
Files 116 117 +1
Lines 6976 7092 +116
Branches 3083 3161 +78
==========================================
+ Hits 5617 5722 +105
- Misses 980 982 +2
- Partials 379 388 +9
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
![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.
|
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. |
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);
|
There are currently two issues:
As I'll be a week off, I'll unfortunately have to let it sit here until then. |
|
Tick the box to add this pull request to the merge queue (same as
|
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 robot control and file-download paths over HTTP with version-dependent behavior; incorrect gating or download handling could affect remote operation or local filesystem writes, though downloads use hardened temp-file creation.
Overview
Extends the PolyScope X dashboard client (
DashboardClientImplX) with Robot API support aligned to PolyScope 10.14.0 (Robot API 5.0.107), replacing many priorNotImplementedExceptionstubs with real HTTP calls.New / wired commands include popups (
commandPopupwith optional title,commandClosePopup,commandCloseSafetyPopup), logging (commandAddToLog), robot info (commandPolyscopeVersion,commandGetRobotModel,commandGetSerialNumber), system control (commandShutdown), flight reports (commandGenerateFlightReportwith optionalreport_type), andcommandDownloadSupportFiles(PolyScope X only; G5 throws). Support-file download streams the zip to disk viamkstemp+ atomic rename (POSIX and Windows), with DELETE helper for popup endpoints.Version gating is centralized in
g_command_listandassertHasCommand, which can reconnect and comparerobot_api_version_to required API/marketing versions; docs now list feature groups by PolyScope version.commandPopupgains an optional title on the public API;handleHttpResulttreats any 2xx as success.CI adds URSim 10.14.0 (version-specific program/citadel paths), pre-commit skips formatting under
tests/resources/dockerursim, and integration tests/mock HTTP tests cover the new X client behavior.Reviewed by Cursor Bugbot for commit 5b29ea8. Bugbot is set up for automated code reviews on this repo. Configure here.