search

ros-drivers / ros2_ouster_drivers

ROS2 Drivers for the Ouster OS-0, OS-1, and OS-2 Lidars
https://ouster.com/
Apache License 2.0
134 stars 79 forks source link

Fix for firmware 2.4 #118

Closed vinnnyr closed 1 year ago

vinnnyr commented 1 year ago

As described in https://github.com/ros-drivers/ros2_ouster_drivers/issues/116

Firmware 2.4 removes auto_start_flag and udp_ip, and therefore the lidar communication is not established when the lidar is on the newer firmware.

v2.3 has been pulled from the Ouster website / will not be supporting it at all moving forwards from what I understand.

This PR also might break backwards compatibility for lidars on older firmware, I have only tested this on v2.4 and v2.3 devices.

SteveMacenski commented 1 year ago

Are these backward compatible with previously supported firmware versions? If there are a set of firmware versions this assumes, that has to be updated in the readme. Ex.

These are an implementation of ROS2 drivers for the Ouster lidar. This includes all models of the OS-x from 16 to 128 beams running the firmware 2.1.

vinnnyr commented 1 year ago

@SteveMacenski This will work for firmware 2.0 and above from what I understand, although I have only have / can test it with firmware 2.3 and 2.4. I would feel more comfortable setting the firmware limit at 2.3 and update it all over. Is that reasonable?

SteveMacenski commented 1 year ago

That sounds reasonable, please update the readme / any locations mentioning and we can merge. Since this is to the ros2 (bleeding edge) branch, changes like this are OK. When Iron turtle is forked though, that means that distro will only support 2.3/2.4

As an Ouster user, you should contact them requesting that their firmware download site contains all historical versions as well and feel free to link them here. Users need to be able to set a static version for their fleet of systems and to support binary releases, they need to be available -- unless they change their practices and make things backward compatible.

PhilippPolterauer commented 1 year ago

I tested this pull request for FW2.4. It worked immediately while the main branch does not. Can you confirm what is missing for this pull request to get included into the official repo?(maybe as a separate branch.)

vinnnyr commented 1 year ago

Sorry for the delay. this fell off my radar.

@SteveMacenski I updated the README to suggest firmware 2.4 is supported. I tried looking for other places to update, and came up short. Is this ready to be merged?

Valts-M commented 1 year ago

I have tested these changes on a OS0-32 running firmware version 2.4 and an OS0-64 running version 2.2. Worked fine on both.

SteveMacenski commented 1 year ago

https://github.com/ros-drivers/ros2_ouster_drivers/blob/cdd778696dbe560699c96345aead75c5282dd93c/ros2_ouster/src/ouster_driver.cpp#L111-L113

Update here and the suggested change above and I'll merge.

Sorry, last week was a US holiday.

SteveMacenski commented 1 year ago

Just need the code change too for the INFO log and we're good to go

vinnnyr commented 1 year ago

@SteveMacenski done. was that language ok (I just made it match your suggestion for the readme)

SteveMacenski commented 1 year ago

Yup!