search

UniversalRobots / Universal_Robots_ROS2_Driver

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

fix: recreate urcl log handler pointer if necessary #1154

Closed domire8 closed 1 month ago

domire8 commented 1 month ago

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 commented 1 month ago

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: https://github.com/UniversalRobots/Universal_Robots_ROS2_Driver/pull/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 commented 1 month ago

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 :smile:

fmauch commented 1 month ago

Closing, now that #1098 is merged. Thank you @domire8 for bringing that up again!