search

SICKAG / sick_visionary_ros

interface to sick visionary devices from ROS nodes
The Unlicense
8 stars 4 forks source link

add diagnostics #2

Closed fmessmer closed 10 months ago

fmessmer commented 11 months ago

this PR aims to propose changes made by @4am-robotics in order to get them included into the upcoming release

we have been working with a local copy based on the downloadable zip driver (downloaded a while back) mainly adding some basic diagnostics to the driver

in our downloaded driver version, we also faced runtime exceptions - which were reported to the SICK bug tracker (@stefansalzberger can you link the respective ticket, please?) in that driver version, sick_visionary_cpp_shared version 0.1.0 was used first and then later updated to 1.1.0 I will have to test the new version 2.1.0 on our hardware, but wanted to propose the diagnostics feature already

let me know if this is a desirable contribution, then we could also port this to sick_visionary_s`, too

StefanSalzberger commented 11 months ago

The Issue number in SICK support portal is ST-36754

zimmefaSICKAG commented 11 months ago

We have checked your changes and are grateful for your contributions. We are happy if you also commit your changes for Visionary-S. We would then integrate them as well.

Further, please also let us know if we have to expect any commits regarding your tests against the new sick_visionary_cpp_shared version version 2.1.0 that you could then adress in another pull request.

fmessmer commented 10 months ago

I added the diagnostics to visionary_s, too

I'm still waiting for a slot to test this myself on our visionary_t_mini Please note, that I do not have access to a visionary_s - thus I cannot test the visionary_s part of this PR

I dont think there will be any changes needed for sick_visionary_cpp_shared as the version used here is much newer than the version were we needed to add the patch....but I will report once I run this driver with our sensor

fmessmer commented 10 months ago

the tests on our hardware with this branch was successful :heavy_check_mark: i.e. no changes needed for sick_visionary_cpp_shared from our side

let me know in case you need anything else for getting this pr merged/released

peichtoSICKAG commented 10 months ago

Thank you for your contribution. We tested the changes against both devices successfully and thus will integrate your changes. We intend to move the parameters introduced with your change to the launch.xml to allow dynamic changes when launching the binary node before the next release. It's good to hear, that you don't have additional patches against the shared library. However since we have internal changes pending there, too, currently the release is scheduled for 15th december 2023. Since we have overlooked some CMake/clang-tidy warning in the pretest, the current 1.0.0 version is rejected by the ROS buildfarm. Thus expect the next release including your changes to be the first binary release then.