UniversalRobots / Universal_Robots_ROS2_Driver

Universal Robots ROS2 driver supporting CB3 and e-Series
BSD 3-Clause "New" or "Revised" License
397 stars 209 forks source link

Failure During Hardware Component Reconfiguration #1097

Open sahand-ghaffari-ocado opened 1 week ago

sahand-ghaffari-ocado commented 1 week ago

Affected ROS2 Driver version(s)

2.2.14

Used ROS distribution.

Humble

Which combination of platform is the ROS driver running on.

Ubuntu Linux with standard kernel

How is the UR ROS2 Driver installed.

Build both the ROS driver and UR Client Library from source

Which robot platform is the driver connected to.

UR E-series robot

Robot SW / URSim version(s)

5.17.0

How is the ROS driver used.

Headless without using the teach pendant

Problem Description

Reconfiguring the UR hardware component (which can be done using the ~/set_hardware_component_state service call to unconfigure and then activate the UR hardware component) results in a code failure due to following issues

  1. The registerUrclLogHandler() function which is called in the on_configure() function uses std::move to transfer ownership of g_log_handler. As a result, calling the on_configure()function again causes a failure. This is because the g_log_handler pointer has already been moved, and it no longer has access to the setTFPrefix() function.
  2. The checkAsyncIO() function cannot be called in theasyncThread()function after reconfiguring the hardware component. This is because the async_thread_shutdown_ variable is set to true in the on_cleanup() function and is not reset to false in the on_configure() function.

Workaround Suggestion

To avoid such crashes, following changes are suggested

  1. It is suggested to declare and assign the pointer within the registerUrclLogHandler() function instead of doing so globally. Therefore, it is suggested to modify the function as follows:
void registerUrclLogHandler(const std::string& tf_prefix)
{
  if (g_registered == false) {
    std::unique_ptr<UrclLogHandler> g_log_handler(new UrclLogHandler);
    g_log_handler->setTFPrefix(tf_prefix);
    // Log level is decided by ROS2 log level
    urcl::setLogLevel(urcl::LogLevel::DEBUG);
    urcl::registerLogHandler(std::move(g_log_handler));
    g_registered = true;
  }
}
  1. It is suggested to set async_thread_shutdown_ to false in on_configure()function

Accept Public visibility

fmauch commented 6 days ago

Thank you for reporting this and already suggesting a way of resolving the issue.

To implement this it would be nice to have a test case actually covering this, though. I'll add a draft PR with the changes you suggested but before merging I would like to have tests for changing the lifecycle state. Unfortunately, I don't know how much time I will have available to spend on this.

sahand-ghaffari-ocado commented 1 day ago

Thank you for your quick response and for moving forward with the draft PR! I agree that having a test case to cover the changes would be ideal before merging. What kind of tests are you thinking of adding to cover the lifecycle state change? I'd be happy to assist with writing or reviewing the tests if needed, especially if you're short on time. By the way, I also opened a PR in ros2_control to fix the bug related to hardware component unconfiguration.