search

SICKAG / sick_safetyscanners2

ROS2 driver for SICK safety laser scanners
https://www.sick.com/de/en/opto-electronic-protective-devices/safety-laser-scanners/c/g187225
Apache License 2.0
28 stars 28 forks source link

Implement lifecycle node #11

Closed guilyx closed 3 years ago

guilyx commented 3 years ago

Solves #9

Side note: we had issues with the sick scanner node crashing with an error -11 and reported it on issue #8. This is solved when using a lifecycle node where we call configure then activate using ros2 lifecycle set /name_of_the_node configure && ros2 lifecycle set /name_of_the_node activate or using navigation2's lifecycle manager node.

puck-fzi commented 3 years ago

Hi, thanks for sharing this. It looks like there are some code duplication now between the classic node and lifecycle node. i will put a TODO, to refactor this in the future. however I will test and check the lifecycle node as is and merge it probably in two weeks, when I had access to a scanner. First brief glance at the PR looks promising

guilyx commented 3 years ago

I could work on the duplication, but it wouldn't be before early August. Agreed that this is something to do!

guilyx commented 3 years ago

@puck-fzi Any update on this ?

puck-fzi commented 3 years ago

Hi, sorry I didn't get around to Testing it before my Holidays. Will do that as soon as i get back. Sorry for the delay.

puck-fzi commented 3 years ago

Hi, I finally got around to testing it. Working as expected. There are two minor things I would like to be in the PR. Could you add a brief description to the readme on how to start and and activate the node with the lifecycle.

Second: when only doing the configure step, there will be a lot of warnings, that messages are tried to being published. Since I am not that familiar with lifecycle Nodes maybe you know how to best resolve this. [sick_safetyscanners2_lifecycle_node-1] [WARN] [1628500948.017306622] [LifecyclePublisher]: Trying to publish message on the topic '/scan', but the publisher is not activated

Otherwise I deem it ready to be merged, with the open future to do of refactoring it, that both lifecycle and non lifecycle node don't have that much code duplication.

guilyx commented 3 years ago

@puck-fzi Latest changes should prevent the warnings from happening, tell me if it does on your side.