Skip to content
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

Failure During Hardware Component Reconfiguration #1097

Closed
1 task done
sahand-ghaffari-ocado opened this issue Sep 9, 2024 · 3 comments · Fixed by #1098
Closed
1 task done

Failure During Hardware Component Reconfiguration #1097

sahand-ghaffari-ocado opened this issue Sep 9, 2024 · 3 comments · Fixed by #1098

Comments

@sahand-ghaffari-ocado
Copy link

sahand-ghaffari-ocado commented Sep 9, 2024

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

  • I agree to make this context public
@fmauch
Copy link
Collaborator

fmauch commented Sep 11, 2024

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.

@fmauch fmauch linked a pull request Sep 11, 2024 that will close this issue
@sahand-ghaffari-ocado
Copy link
Author

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.

@fmauch
Copy link
Collaborator

fmauch commented Sep 18, 2024

If you could help out writing tests for this, that would be great!

Basically, I think a short integration test that starts the robot, cycles the states down and up once more, making sure the available interfaces match the lifecycle state on the way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants