-
Notifications
You must be signed in to change notification settings - Fork 99
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added missing RTDE data packages and fixed incorrect names #213
Added missing RTDE data packages and fixed incorrect names #213
Conversation
2b09deb
to
07139fa
Compare
I am a bit confused with |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #213 +/- ##
==========================================
- Coverage 72.05% 71.68% -0.38%
==========================================
Files 71 71
Lines 2652 2649 -3
Branches 337 337
==========================================
- Hits 1911 1899 -12
- Misses 555 566 +11
+ Partials 186 184 -2 ☔ View full report in Codecov by Sentry. |
Thank you for providing this. Indeed I think we never touched this since we initially created it from the interface back then. I'll double-check things but it seems to look fine on a first glance.
I do understand your confusion, but it isn't only used inside the tests, we also use it inside the ROS drivers to set digital outputs. @urrsk we probably discussed this a couple of years ago, but I agree: Is that missing in the docs? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be in line with the upstream documentation. Thank you @remi-siffert-ocado
I think it would indeed be beneficial to have a test trying to claim all the possible RTDE fields to ensure they all actually exist. If you could find the time to provide such a test, that would be awesome! Note though, that this test will have to assemble the recipe based on the robot software version. We currently run the minimum required version in CI but it would be nice to add other entries, at least for the latest version, as well. It might be beneficial to actually put that into a separate test, I think. @remi-siffert-ocado please let me know whether you would like to implement such a test and if you would like to this as a part of this PR or a separate one. |
@fmauch Sure I can give a shot at writing the test you suggest for this PR. |
In the test itself it is possible to get the robot software version directly from the RTDEClient. You probably would have to setup the test with a small recipe to initialize the communication and then reset the client with the assembled version-sensitive recipe. To actually run the tests with the respective versions in CI, we would only need to modify the build matrix that I linked earlier. |
5284b88
to
04fb589
Compare
Added a test as suggested @fmauch. Let me know how this looks, thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking great! What I don't like is that we effectively have two lists of field names. Maybe we can merge the version information into the g_type_list
?
Please tell me, if you think, I am starting feature-creeping ;-) We can always move tasks to follow-up PRs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @remi-siffert-ocado for this great addition!
@remi-siffert-ocado Thanks for the help |
Updated the list of RTDE data packages for consistency with https://www.universal-robots.com/articles/ur/interface-communication/real-time-data-exchange-rtde-guide/.
Tested with URSim running Polyscope 5.17.2 with the following RTDE output recipe:
I have not updated the
test_ur_driver
to use this new output recipe but I'd be happy to if you think it's useful.