Move communication setup to on_configure instead of on_activate

[fmauch]

Corrects point of initialization / connection to the robot by moving it to on_configure instead of on_activate.

@destogl we talked about this yesterday, I found my commit I had in mind :-)

[RobertWilbrandt]

This change seems reasonable, but if we start all communication in on_configure i would expect the corresponding cleanup to move to on_cleanup.

[firesurfer]

@fmauch Just saw this PR and thought I take a look at it as it might has the potential breaking multiarm support. Given that on_configure and on_activate run in different threads (configure in the non rt and activate in the rt one) - If I understood the documentation right - I would suggest changing:

Universal_Robots_ROS2_Driver/ur_robot_driver/include/ur_robot_driver/hardware_interface.hpp

Line 227 in a8a96dc

bool rtde_comm_has_been_started_ = false;

bool rtde_comm_has_been_started_ = false;

to

std::atomic_bool rtde_comm_has_been_started_ = false;

This goes btw. probably also for all other variables which are used from multiple threads (e.g. everything being set by URPositionHardwareInterface::checkAsyncIO())

[firesurfer]

@fmauch I agree with you. For passing information between the non-rt and rt thread and std::atomic_bool should be used in this case.

This goes btw. probably also for all other variables which are used from multiple threads (e.g. everything being set by URPositionHardwareInterface::checkAsyncIO())

In general you are right. Often one can get away with it if only one thread writes and one thread reads and it is okey to have partially updated data.
But in this case I would use the RTBuffer from the realtime_tools package.

[fmauch]

@firesurfer yes, thank you for your comments. Thread safety is definitively something we need to tackle at some point. For now, for this PR I would restrict things to the rtde_comm_has_been_started_ member.

[fmauch]

I just rebased and tested this. Especially with the deprecation and upcoming removal of passing the robot_description as a parameter and instead using a topic this becomes a required step.

[VinDp]

Looks good to me.

[fmauch]

@RobertWilbrandt a review from you would be required to merge this.

[RobertWilbrandt]

Sorry about that, i was not aware that i needed to be the person to re-review this. Looks good now.

[fmauch]

@Mergifyio backport iron

[mergify]

backport iron

✅ Backports have been created

• #937 Move communication setup to on_configure instead of on_activate (backport #732) has been created for branch iron