Adapt RTDE output recipe based on robot response

[remi-siffert-ocado]

Not all RTDE outputs are available in all SW versions. When working with a fleet of robots with heterogeneous SW versions it's not practical to have multiple RTDE output recipes to account for this.
This PR aims at solving this problem by adapting the RTDE output recipe on-the-fly based on the robot response to the RTDE request. Outputs which are not available for this particular SW version are simply removed from the output recipe and a trimmed down output recipe is sent to the robot.

Potential issues / improvements:

• variables removed from the output recipe only cause a warning. When these variables are "optional" this is a fine behaviour but it could potentially be masking incorrect variable names. It could be better to make this behaviour configurable.
• an alternate implementation could be to hardcode the compatibility information for RTDE outputs in data_package.cpp and validate the output recipe beforehand. This seems less flexible though.
• the same logic could potentially be applied to the input recipe. Having optional inputs seems less likely though.

[fmauch]

variables removed from the output recipe only cause a warning. When these variables are "optional" this is a fine behaviour but it could potentially be masking incorrect variable names. It could be better to make this behaviour configurable.

This popped into my mind immediately. I think this should not be the default behavior, but some opt-in feature.

an alternate implementation could be to hardcode the compatibility information for RTDE outputs in data_package.cpp and validate the output recipe beforehand. This seems less flexible though

This reminds me of our discussion in #213 (comment).

the same logic could potentially be applied to the input recipe. Having optional inputs seems less likely though.

I agree with both.

[remi-siffert-ocado]

This popped into my mind immediately. I think this should not be the default behavior, but some opt-in feature.

That's fair. I'll add a parameter to the UrDriver constructor unless there's a better way.

This reminds me of our discussion in #213 (comment).

Yes it's definitely related. I feel that auto-negotiating based on the robot response is a bit more flexible than having a fixed list.

[urfeex]

That's fair. I'll add a parameter to the UrDriver constructor unless there's a better way.

I'd like to avoid adding more parameters to the constructor. In the past this has proven to a) blow up the constructor argument list quite significantly and b) easily introduce troubles in API compatibility which is why we have so many constructors at the moment.

With #218 we could create the driver with a minimal recipe and then reset the RTDEClient with the full recipe. In the reset method we could also add a flag for automatically drop incompatible fields. What would you think about that option?

[remi-siffert-ocado]

I'd like to avoid adding more parameters to the constructor. In the past this has proven to a) blow up the constructor argument list quite significantly and b) easily introduce troubles in API compatibility which is why we have so many constructors at the moment.
With #218 we could create the driver with a minimal recipe and then reset the RTDEClient with the full recipe. In the reset method we could also add a flag for automatically drop incompatible fields. What would you think about that option?

I agree that adding a parameter to the constructor is not ideal. It's probably a good idea to separate the construction from the configuration of the RTDE communication. I would argue that in this case the constructor should not contain anything related to RTDE anymore. It should be replaced with a distinct configureRTDECommunication / resetRTDEClient method that has to be called before startRTDECommunication.

The main problem is how do we handle the dependencies between these MRs. I see 3 options:

1. merge Adapt RTDE output recipe based on robot response #221 as-is with the caveat that the behaviour is not configurable
2. add a constructor parameter in Adapt RTDE output recipe based on robot response #221 that will be removed once Allow resetting the RTDE client #218 is merged
3. somehow combine Adapt RTDE output recipe based on robot response #221 and Allow resetting the RTDE client #218 into a bigger MR

What's your take on this @urfeex?

[urfeex]

3. would be my choice, though I would suggest finishing Allow resetting the RTDE client #218 first and then rebasing and adapting Adapt RTDE output recipe based on robot response #221 ontop of that. However, given my current workload I am not sure how quickly I will be able to implement the missing things on Allow resetting the RTDE client #218. Would it be ok with you letting this PR hang for one more week and then we use option 1. if I haven't come around finishing Allow resetting the RTDE client #218?

[urfeex]

I've finished #218. I just need a review from one of my colleagues to get that merged then we can modify this to build upon that. I would suggest adding a flag to the resetRTDEClient method.

[remi-siffert-ocado]

I've finished #218. I just need a review from one of my colleagues to get that merged then we can modify this to build upon that. I would suggest adding a flag to the resetRTDEClient method.

Thanks for the follow-up!

I am still trying to understand how we can use this resetRTDEClient method effectively in the context of the ROS2 driver. The current sequence of calls in the ROS2 driver is: instantiate the UrDriver and call startRTDECommunication once the first time we want to read RTDE data. With the current implementation where the RTDE output recipe is automatically adapted there's no additional call required and we're able to support different UR SW versions.
For this behaviour to be configurable the sequence has to change to: instantiate the UrDriver, call resetRTDEClient with a flag based on the hardware parameters and then call startRTDECommunication. Is that what you had in mind?

[urfeex]

For this behaviour to be configurable the sequence has to change to: instantiate the UrDriver, call resetRTDEClient with a flag based on the hardware parameters and then call startRTDECommunication. Is that what you had in mind?

I was not necessarily having the ROS 2 driver in mind, since you didn't mention that so far. Changing the recipe with the ROS 2 driver will only be useful if the driver is modified, as well, right? I think the recipe we have in there should be rather backwards-compatible.

But yes, adding the reset call after the UrDriver object has been created should work. That's also what I implemented #218 for initially.

[remi-siffert-ocado]

I was not necessarily having the ROS 2 driver in mind, since you didn't mention that so far.

That's my bad but yes the ROS 2 driver is the main use case I had in mind for this change.

Changing the recipe with the ROS 2 driver will only be useful if the driver is modified, as well, right? I think the recipe we have in there should be rather backwards-compatible.

Given that the recipe used in the ROS2 driver is configurable via the hardware parameters (https://github.com/UniversalRobots/Universal_Robots_ROS2_Driver/blob/main/ur_robot_driver/src/hardware_interface.cpp#L380) any output recipe is actually possible.
In our context we definitely have backwards-incompatible data in the output recipe. But another way of doing this would be to keep the default output recipe in UrDriver and instantiate another RTDEClient outside of UrDriver for additional data if required. I'll add a configuration parameter to the constructor of RTDEClient and resetRTDEClient I think this should cover all bases.

[remi-siffert-ocado]

Implemented the changes we discussed. Let me know what you think.

[urfeex]

This is looking good, thank you! However, I would prefer making the change in an API compatible manor if possible.

As you've noticed in the tests you had to update the client constructor usages.

If you merge in / rebase onto current master the example runs should also succeed which should give a succeeding pipeline.

[remi-siffert-ocado]

Thanks for the comments! Addressed everything in b9f8484.

[urfeex]

This is looking good now, thank you very much!