Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #560 +/- ##
==========================================
+ Coverage 80.86% 82.18% +1.32%
==========================================
Files 119 121 +2
Lines 7231 7606 +375
Branches 3220 3518 +298
==========================================
+ Hits 5847 6251 +404
+ Misses 993 971 -22
+ Partials 391 384 -7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
Contributor
There was a problem hiding this comment.
Pull request overview
Moves RTDE field typing from a static table to robot setup acknowledgements while preserving preallocated data paths.
Changes:
- Adds negotiated
DataTypehandling across packages, parsers, clients, and writers. - Adds fake-server, behavior, and allocation tests.
- Fixes
TCPServer::writeUnchecked()initialization.
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
tests/test_rtde_writer.cpp |
Tests negotiated writer types and validation. |
tests/test_rtde_parser.cpp |
Tests typed parser behavior. |
tests/test_rtde_data_package.cpp |
Tests dynamic field typing. |
tests/test_rtde_client.cpp |
Updates invalid-recipe expectations. |
tests/test_rtde_client_fake_server.cpp |
Adds robot-free client tests. |
tests/test_rtde_allocations.cpp |
Adds real-time allocation checks. |
tests/test_producer.cpp |
Supplies negotiated parser types. |
tests/test_pipeline.cpp |
Supplies negotiated pipeline types. |
tests/rtde_test_helpers.h |
Adds test access to internal typing APIs. |
tests/resources/generate_rtde_outputs.py |
Sources fields from the fake server. |
tests/fake_rtde_server.cpp |
Emulates recipe type acknowledgements. |
tests/CMakeLists.txt |
Registers new test executables. |
src/rtde/rtde_writer.cpp |
Validates and merges typed input packages. |
src/rtde/rtde_parser.cpp |
Parses using negotiated output types. |
src/rtde/rtde_client.cpp |
Propagates acknowledged recipe types. |
src/rtde/data_package.cpp |
Implements dynamic RTDE typing. |
src/comm/tcp_server.cpp |
Initializes the write counter. |
include/ur_client_library/rtde/rtde_writer.h |
Declares writer type setup. |
include/ur_client_library/rtde/rtde_parser.h |
Declares parser type setup. |
include/ur_client_library/rtde/data_package.h |
Adds DataType and untyped storage. |
examples/rtde_client.cpp |
Removes obsolete string-allocation guidance. |
doc/migration_notes.rst |
Documents behavioral changes. |
doc/examples/rtde_client.rst |
Updates real-time usage guidance. |
doc/architecture/rtde_client.rst |
Documents negotiated typing architecture. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 24 changed files in this pull request and generated 2 comments.
Suppressed comments (1)
include/ur_client_library/rtde/data_package.h:124
std::stringis not one of the RTDE protocol types, but retaining it here letssetData()change an untyped field frommonostatetostd::string. Such a package then passesisTyped()andserializePackage()emits a malformed variable-length payload, whilegetDataType()reports no type. Restrict untyped fields to the alternatives represented byDataType.
using _rtde_type_variant = std::variant<std::monostate, bool, uint8_t, uint32_t, uint64_t, int32_t, double,
vector3d_t, vector6d_t, vector6int32_t, vector6uint32_t, std::string>;
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated 2 comments.
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
tests/test_rtde_allocations.cpp:52
- This thread-local design excludes not only the fake server but also the client's background-reader and writer threads. Therefore
background_receive_does_not_allocatedoes not observe the thread that parses received packages, andsending_input_data_does_not_allocatedoes not observe the thread that serializes and writes them; both tests can pass with allocations in the paths their names claim to cover. Instrument all client-owned threads while excluding only the server thread.
// Counting is per-thread: the fake server and, in the background-read case, the client's read
// thread run in the same process, and their allocations are none of this test's business.
thread_local std::size_t g_allocation_count = 0;
thread_local bool g_count_allocations = false;
src/rtde/rtde_parser.cpp:161
- A caller can make a preallocated package “typed” with
setData()before the first blocking receive, so this check does not prove that its types came from the robot. For example, a timestamp-only package set asuint64_tskipsinitEmpty(recipe_types_)and parses the robot's DOUBLE bytes as an integer while reporting success. Validate both the recipe and every existing field type againstrecipe_/recipe_types_, retyping or replacing packages that do not match.
if (!data_package->isTyped())
{
// A package built from a recipe alone doesn't know its field types yet. Applying the ones
// the robot reported doesn't allocate, so this happens right here rather than by handing
// the caller a replacement package.
tests/test_rtde_data_package.cpp:460
- This test does not measure allocations, so an allocation introduced inside
initEmpty(types)would still pass despite the test name and the PR's real-time guarantee. Surround this call with the allocation counter (or move this case into the allocation-test binary) and assert that its count remains zero.
package.initEmpty({ "DOUBLE", "VECTOR6D" });
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated no new comments.
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
tests/test_rtde_allocations.cpp:277
- The counter is thread-local, while
sendPackage()only queues data andRTDEWriter::run()performs serialization and socket writing on its own thread. Consequently this measured block cannot see allocations in the actual asynchronous send path, sosending_input_data_does_not_allocatecan pass despite a send-thread regression. Add writer-thread instrumentation or a same-thread serialization allocation test.
{
AllocationCounter counter;
for (int i = 0; i < g_MEASURED_CYCLES; ++i)
{
all_sent &= input_pkg.setData("speed_slider_fraction", 0.5);
all_sent &= client_->getWriter().sendPackage(input_pkg);
}
src/rtde/rtde_parser.cpp:157
isTyped()also becomes true when the caller has populated every field withsetData(), so it does not prove these are the negotiated types. A preallocated one-fieldtimestamppackage set asuint64_t, for example, skipsinitEmpty(recipe_types_)and parses the robot's DOUBLE bytes as UINT64. This path also never verifies field names, so an untyped same-length package with a different recipe is typed by position. Validate the recipe and always reapply the acknowledged types before parsing; replace the package only when its recipe differs.
DataPackage* data_package = dynamic_cast<DataPackage*>(result.get());
data_package->setProtocolVersion(protocol_version_);
if (!data_package->isTyped())
tests/test_rtde_data_package.cpp:460
- This test never observes allocations: it only verifies that the package remains usable. The counters in
test_rtde_allocations.cppstart afterRTDEClient::init()and warmup, so an allocation added toinitEmpty(types)would pass the suite even though no-allocation in-place typing is a central guarantee. Measure this call while allocation counting is active.
TEST(rtde_data_package, applying_types_does_not_reallocate)
{
std::vector<std::string> recipe{ "timestamp", "actual_q" };
test::TestableDataPackage package(recipe);
double timestamp = 0.0;
ASSERT_FALSE(package.getData("timestamp", timestamp));
package.initEmpty({ "DOUBLE", "VECTOR6D" });
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated no new comments.
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
tests/test_rtde_allocations.cpp:184
- This guard can pass while the allocation tests miss the allocations they are intended to detect. As the comment above notes, some libstdc++/musl
std::allocatorimplementations callmallocdirectly; invoking::operator newhere proves only that this replacement works, while vector/string growth in the measured RTDE paths can bypass it and leave the count at zero. Validate the counter with a representative standard-container allocation and either intercept that platform's allocation path or fail/skip when it cannot be observed.
// Guards the tests below: if the counter stopped seeing allocations, they would pass vacuously.
// Call operator new directly rather than writing `new int`: a new-expression may be omitted even
// when the pointer escapes, which is what Alpine's gcc 15 does at -O2. Allocate with operator new
// rather than a container: on some libstdc++ / musl builds std::allocator uses malloc and would
// never hit the replaced operator new that the RTDE tests count.
TEST(AllocationCounterTest, counts_allocations)
{
std::size_t allocations = 0;
{
AllocationCounter counter;
g_allocation_sink = ::operator new(sizeof(int));
src/rtde/data_package.cpp:212
getDataType()does not necessarily report a robot-acknowledged type as the new API promises. On an application-created input package,setData()changes the variant frommonostateto the caller's type, so this function then returns that inferred type—even whenRTDEWriter::sendPackage()later rejects it because the robot reported a different type. Consumers therefore cannot tell whether this result is authoritative. Track the acknowledged type separately from the value/inferred type, or explicitly expose this as the stored value type and provide the robot-reported type through the client/writer API.
std::optional<rtde_interface::DataType> rtde_interface::DataPackage::getDataType(const std::string_view name) const
{
const auto it =
std::find_if(data_.begin(), data_.end(), [&name](const std::pair<std::string, _rtde_type_variant>& element) {
return element.first == name;
});
if (it == data_.end())
{
return std::nullopt;
}
return typeOf(it->second);
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Exception safety, real-time allocation guarantees, and direct protocol-v2 parsing compatibility remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 32/32 changed files
- Comments generated: 3
- Review effort level: Balanced
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new type-reporting contract is inconsistent with caller-established types, and reconnect tests contain synchronization and coverage defects.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
examples/rtde_writer.cpp:93
- Correct the typo in this user-facing example comment: “may” should be “many.”
tests/test_rtde_client_reconnect.cpp:320 - These two point-in-time state checks do not establish that reconnecting stopped. During each retry the client repeatedly returns to
UNINITIALIZED, so an implementation that retries forever can satisfy both assertions. Wait for the expected request count and then verifyrequestedProtocolVersions()remains unchanged (or expose a completion signal) to cover exhaustion rather than an incidental state between attempts.
- Files reviewed: 32/32 changed files
- Comments generated: 3
- Review effort level: Balanced
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Public setup mutators can race with serialization, and additional undefined-behavior and test-lifetime issues remain.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 32/32 changed files
- Comments generated: 4
- Review effort level: Balanced
copyFrom onto the same package memcpy-overlapped, the fake server only acknowledged protocol v2, and Codecov never saw the unit binaries. Guard the self-copy, lock the remaining contract with tests, teach the fake server v1, and add a dedicated unit coverage job.
Match master: unknown names still return false, but a present field with the wrong type (including untyped monostate) throws std::bad_variant_access again instead of logging and returning false.
Add a step for the getDataPackage to ensure or update the recipe types
- Repair foreign output recipes in RTDEClient read paths and warn instead of throwing. - Allocate a typed DataPackage for null pointers with a warning. - Accept partially typed same-recipe input packages in RTDEWriter and fill unset fields with zeros. - Return false instead of throwing on RTDEParser layout mismatch. - Add typed template registration to RTDEParser for null/non-data pointer allocation and deprecated vector overload. - Update migration documentation and regression test suites.
- Document partial input package handling and atomic validation in DataPackage::copyFrom. - Document typed template registration, hash reset, and vector allocation in RTDEParser. - Document silent false on layout mismatch for zero-allocation guarantees. - Document RTDEClient output layout repair, null allocation, and thread safety invariants.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Package ownership can be lost or unexpectedly published on failed reads, and parser templates are not validated against the configured recipe.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 33/33 changed files
- Comments generated: 3
- Review effort level: Balanced
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Core wire serialization, ownership, and reconnect concurrency changes warrant final human validation despite extensive coverage.
Review details
- Files reviewed: 34/34 changed files
- Comments generated: 0 new
- Review effort level: Balanced
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.
Summary
Replace the hardcoded RTDE field-name/type table with types reported by the robot during recipe setup. This allows applications to use additional controller fields without updating the library's field table, provided their protocol types are supported.
Changes
DataPackage::getDataType()and their protocol names throughtoString(DataType).RTDEClient::createInputDataPackage()for pre-typed input packages and validate outgoing packages against the negotiated recipe. Unset input fields in matching recipes are sent as typed zeros.TCPServer::writeUnchecked().Compatibility
RTDEInvalidKeyExceptionduringRTDEClient::init(), rather than construction.ignore_unavailable_outputsalso filters unknown names.setData()are checked against the robot when sent; pre-typed input packages reject mismatches immediately.getData()still throwsstd::bad_variant_accessfor a present field with the wrong or unset type.std::stringis no longer aDataPackagevariant alternative, so using it withgetData()orsetData()is a compile error.getOutputRecipe()for the allocation-free path.See doc/migration_notes.rst for migration details.