You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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
Apply negotiated types throughout RTDE packages, parsing and writing; expose stored field types through DataPackage::getDataType() and their protocol names through toString(DataType).
Add 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.
Preserve allocation-free send/receive paths for preallocated, matching recipes, including applying negotiated output types in place.
Harden failed handshakes, reconnect handling and fake-server lifecycle; fix output initialization in TCPServer::writeUnchecked().
Add robot-free protocol, type-validation, allocation and reconnect tests, controller-backed recipe checks, and a dedicated unit-coverage CI job.
Compatibility
Unknown fields raise RTDEInvalidKeyException during RTDEClient::init(), rather than construction. ignore_unavailable_outputs also filters unknown names.
Recipe-only packages begin untyped. Input types established by setData() are checked against the robot when sent; pre-typed input packages reject mismatches immediately.
getData() still throws std::bad_variant_access for a present field with the wrong or unset type.
std::string is no longer a DataPackage variant alternative, so using it with getData() or setData() is a compile error.
Direct parser users must configure negotiated layout/type information. Client reads into null pointers or packages with foreign recipes may allocate; reuse a package built from getOutputRecipe() for the allocation-free path.
❌ Patch coverage is 90.02123% with 47 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.94%. Comparing base (9a15f8d) to head (44804fd). ⚠️ Report is 1 commits behind head on master.
The reason will be displayed to describe this comment to others. Learn more.
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::string is not one of the RTDE protocol types, but retaining it here lets setData() change an untyped field from monostate to std::string. Such a package then passes isTyped() and serializePackage() emits a malformed variable-length payload, while getDataType() reports no type. Restrict untyped fields to the alternatives represented by DataType.
The reason will be displayed to describe this comment to others. Learn more.
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_allocate does not observe the thread that parses received packages, and sending_input_data_does_not_allocate does 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 as uint64_t skips initEmpty(recipe_types_) and parses the robot's DOUBLE bytes as an integer while reporting success. Validate both the recipe and every existing field type against recipe_/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.
The reason will be displayed to describe this comment to others. Learn more.
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 and RTDEWriter::run() performs serialization and socket writing on its own thread. Consequently this measured block cannot see allocations in the actual asynchronous send path, so sending_input_data_does_not_allocate can 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 with setData(), so it does not prove these are the negotiated types. A preallocated one-field timestamp package set as uint64_t, for example, skips initEmpty(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.cpp start after RTDEClient::init() and warmup, so an allocation added to initEmpty(types) would pass the suite even though no-allocation in-place typing is a central guarantee. Measure this call while allocation counting is active.
The reason will be displayed to describe this comment to others. Learn more.
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::allocator implementations call malloc directly; invoking ::operator new here 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 from monostate to the caller's type, so this function then returns that inferred type—even when RTDEWriter::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.
The reason will be displayed to describe this comment to others. Learn more.
🟡 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 verify requestedProtocolVersions() remains unchanged (or expose a completion signal) to cover exhaustion rather than an incidental state between attempts.
setProtocolVersion() leaves layout_hash_ encoding the old protocol whenever the package is only partially typed. This makes the public hash inconsistent with the package's actual wire layout; for example, emptyCopy() recomputes a different hash even though it copies this package's recipe, types, and protocol. Recompute the hash unconditionally—the update is allocation-free and also preserves the documented invariant for untyped packages.
Hash-only parser registration cannot recompute a protocol-specific hash, and untyped packages skipped the hash update, so parse could keep using the previous recipe-id layout.
Avoid unaligned uint16_t access when parsing package length
tests/fake_rtde_server.cpp:657
offset can be odd (the protocol-version request used by two_requests_in_one_write is 5 bytes), but PackageHeader::getPackageLength() dereferences a uint16_t* cast from this address. That is an unaligned access and undefined behavior on strict-alignment platforms. Parse the two bytes through BinParser/memcpy instead.
getPackageLength() cast the byte stream to the package size type, which is undefined
behavior whenever the pointer is not suitably aligned. The RTDE fake server hits this
when a single write coalesces packages: a 5-byte protocol version request leaves the
next header on an odd address. URStream::read() can reach it too, since a uint8_t
buffer only guarantees 1-byte alignment.
Copy the bytes out before byte-swapping, matching BinParser::peek(). GCC emits the
same single load as before at -O2.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🔵 Needs a closer look
The new protocol-v1 test is vulnerable to TCP fragmentation, and the unlimited round-trip example eventually invokes signed overflow.
Review effort: Balanced Findings: None
Previously missed (2)
In code that hasn't changed since last review
Prevent signed counter overflow during unlimited-duration runs
examples/rtde_roundtrip.cpp:186
The default run duration is unlimited, so this signed 32-bit increment eventually overflows (after about 50 days at 500 Hz), which is undefined behavior in C++. The robot-side + 1 and the later echoed_int - 1 also hit the register boundary. Define an explicit wrap/reset policy before reaching INT32_MAX and account for that transition in verification/frequency reporting.
Handle partial TCP frames before parsing RTDE responses
tests/test_rtde_writer.cpp:899
This callback treats one TCP recv() callback as one complete RTDE frame, even though TCP may split the seven-byte response. In that case it sets received for a partial payload and the assertion below fails nondeterministically. Buffer bytes until the length from the two-byte header is available, as the fixture callback above already does.
The round-trip example runs without a time limit by default, so its int32 cycle
counter reached INT32_MAX after about 50 days at 500 Hz and overflowed. It now wraps
at one million, which also keeps the robot's + 1 and the client's - 1 inside the
register. The lag is taken modulo the same period and the frequency report counts
cycles separately, so both survive a wrap.
The protocol-v1 writer test treated one recv() callback as a whole frame and could
observe a partial payload. It now buffers until the header's length has arrived, as
the fixture callback already did.
…t port
The robot-free primary tests had to bind the real primary port, because PrimaryClient
always connected to UR_PRIMARY_PORT. Any other process listening there made TCPServer
retry the bind forever, hanging the tests instead of failing them.
PrimaryClient now takes an optional port like RTDEClient does, and the two fake-server
fixtures use test ports.
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
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.