PickNikRobotics / ros2_robotiq_gripper

BSD 3-Clause "New" or "Revised" License
50 stars 40 forks source link

Fix for updateStatus segfault #14

Closed abake48 closed 1 year ago

abake48 commented 1 year ago

Related to #5

In the event that readResponse is empty, a call to updateStatus will still try to access the empty vector, leading to a segfault.

MarqRazz commented 1 year ago

Can we add a better way of tracking that the gripper failed to connect?

One suggestion would be to actually implement the on_init method and return CallbackReturn::ERROR; if we were not able to establish communication with the gripper (and print a useful error for the operator). As an example here is how the UR robot handles it.

abake48 commented 1 year ago

It looks like the on_activate method will return a CallbackReturn::ERROR when activateGripper returns False. I added a logging statement to the failure condition for better traceability of the error and an error status so it doesn't get stuck in activateGripper in the event an empty response is received while activating the gripper is in progress.

sea-bass commented 1 year ago

Can we add a better way of tracking that the gripper failed to connect?

One suggestion would be to actually implement the on_init method and return CallbackReturn::ERROR; if we were not able to establish communication with the gripper (and print a useful error for the operator). As an example here is how the UR robot handles it.

Tracing this through, the on_init() already is in charge of instantiating a RobotiqGripperInterface (and @abake48 now catches this exception that the constructor would otherwise throw). So I think we're good in that sense.

abake48 commented 1 year ago

Decided to add clang-format rules because this library didn't have any. The code should be formatted and not rough looking anymore.

abake48 commented 1 year ago

Anxiety over variables that cross threads. Probably isn't needed but since it's read and written to in both threads, it made me feel safe to just make that change. Idk if there's a best practice out there for that though.