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

fix: recreate urcl log handler pointer if necessary #1154

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

domire8
Copy link

@domire8 domire8 commented Oct 14, 2024

We have discovered an issue with the UrclLogHandler that occurs when a resource manager is creating the UR hardware interface twice in a row without stopping ROS in between. I'm not sure if that can be recreated on your side easily but you could try:

  1. create controller manager for UR
  2. destroy controller manager while keeping ros2 alive
  3. recreate controller manager for UR, this will result in a segmentation fault

You will realize that the g_log_handler pointer has in fact been reset and not recreated properly, since it's initialization is to be found in the source file. My null pointer check should avoid that segfault.

Side question, what was the design intent of the UrclLogHandler? Were you going for a Singleton? That influences maybe how this problem could be fixed.

@fmauch
Copy link
Collaborator

fmauch commented Oct 16, 2024

Thank you for pointing that out. I am not sure if I understand everything, you are writing:

destroy controller manager while keeping ros2 alive

What exactly do you mean by "keeping ros2 alive"?

Anyway, this is a valid concern and in the back of my head that rang a bell, that we did touch this in the past: #1098

I don't quite understand why past-me came to the conclusion that it would be a good idea not to include the fix in order to write tests, though. Since #1098 also solves another issues about lifecycle state changing: Would you mind if we merged that instead?

@domire8
Copy link
Author

domire8 commented Oct 17, 2024

What exactly do you mean by "keeping ros2 alive"?

Yeah I'm sorry I got so used to our framework that I have a hard time describing issues in native ROS 2. We use dynamic composition a lot and that means that our supervisor process might load a controller manager with a UR and then unload when the user requests it and then load it again. When I said "keep ros2 alive" I meant that there are a bunch of other nodes still alive while the hardware interface is stopped and removed and then added again.

Would you mind if we merged that instead?

The issue is indeed well described in #1097 and #1098 should be a valid fix too, so yes we can close this PR without merge. I'll find another opportunity to contribute 😄

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 this pull request may close these issues.

2 participants