Fix: primary_configuration_timeout as configurable field in UrDriverConfiguration#489
Fix: primary_configuration_timeout as configurable field in UrDriverConfiguration#489litziacarla wants to merge 1 commit intoUniversalRobots:masterfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #489 +/- ##
==========================================
+ Coverage 75.92% 76.71% +0.78%
==========================================
Files 113 113
Lines 6019 6021 +2
Branches 2628 2628
==========================================
+ Hits 4570 4619 +49
+ Misses 1111 1064 -47
Partials 338 338
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
I don't see a reason not to add this, but I would be curious in what kind of network environment this is an issue. In my local tests I never had more than 100ms between calling |
|
In our setup we launch many nodes simultaneously (MoveIt, camera, RViz, mesh processing and others...), and the CPU load at startup is enough to delay the first primary client configuration packet beyond the hardcoded 1 second timeout. I measured a delay of ~1.3s in my case. |
urfeex
left a comment
There was a problem hiding this comment.
Looking back at #381 (where this wait block was added) it seems that we don't really need this right now. It was added to be able to safely get the robot model which was needed for selecting the right parameter set for the PD controller.
However, that feature has been removed from #381 and thus, this is no longer needed. However, it will be needed for #400 (which is basically the same feature, but factored out to its own PR).
@urrsk what do you think? Will we need that in the final version of #400? I guess yes, as we plan to have individual parameter sets for each robot type, right? Thus, it makes sense to wait for a Configuration data package in the beginning.
However, if the timeout can be hit in systems with heavy load, we might want to increase the default value. It is not really a timeout in terms of "Something is going too slow here", it's more a "Wait until you get this. We'll add a timeout here, just in case that we ever get to this point without a real primary interface being on the other side and we would like to avoid waiting forever on something that will never happen".
This PR adds primary_configuration_timeout as a configurable field in UrDriverConfiguration, replacing the previously hardcoded 1000 ms timeout used when waiting for primary client configuration data during driver initialization, since in some network environments, the hard coded value is insufficient and causes initialization failures.
This change is a prerequisite for [Universal_Robots_ROS2_Driver#primary_configuration_timeout as hardware interface parameter], which wires this parameter through the ROS 2 hardware interface and URDF/xacro description.
Note
Low Risk
Low risk: only replaces a hardcoded 1s initialization timeout with a config-driven value, changing startup behavior but not runtime control logic.
Overview
Makes the primary-client configuration wait timeout configurable via a new
UrDriverConfiguration::primary_configuration_timeoutfield (defaulting to 1s).Driver initialization now uses this value instead of a hardcoded 1000ms when waiting for
primary_client_->getConfigurationData(), improving robustness in slower network environments and producing the sameTimeoutExceptionwith the configured duration.Reviewed by Cursor Bugbot for commit 02f84af. Bugbot is set up for automated code reviews on this repo. Configure here.