Support 128 beam models

[SteveMacenski]

I see in #160 closed from not being able to be used - so I'll rephrase for this ticket to be closed when 128 support is available in this codebase for users and maintainers (like me).

[dmitrig]

Fair enough. It's work in progress and we'll be pushing updates along with an upcoming firmware update for both gen1 and gen2 devices.

I think we're somewhat limited re: what we can usefully publish earlier without also making pre-release versions of the firmware publicly available, but I'd be curious to hear what would make maintaining the ROS2 client easier.

[SteveMacenski]

Awesome, that would be great. That's actually one of the last items before Tom and I can release the V1.0 of the ROS2 driver, making sure it supports all version of the OS-X series lidars. We've done a bunch of work to make it real time-"ish" and benchmarking the networking characteristics to be optimal and deterministic. Overall, its really good and I'm happy with it (a bunch of processing modes, all the types of time sync you could want, well documented, etc).

but I'd be curious to hear what would make maintaining the ROS2 client easier.

I think a couple things:

• The host-side code for 128 beam support, even if the firmware doesn't have it yet, that way the driver is ready to go on day 1 of the firmware release. I've poked Fisher about that as well.
• Structure or reference design for testing, this repo nor the ROS2 repo has any tests in it due to the nature of working with hardware. I imagine some element of the tests needs to be HIL, but testing the parts that don't as well in CI. I haven't built enough hardware drivers to know how to go about that. Most ROS drivers don't have tests in them, but I don't measure myself off of what "most people" do, so I want them either way. I have a build farm that runs to make sure it builds, but that doesn't really mean a whole lot.
• Per Update from new firmware updates ros-drivers/ros2_ouster_drivers#42 and 68ad03c, it would be great that as you make changes to the OS header files, you also submit a PR over on our repo for those same changes to keep them in sync

[chris-hagen]

In the latest beta version of the official ros ouster driver 1.14 (https://go.ouster.io/beta-program/beta-12/downloads/) all OS-X sensors with different layer counts have been harmonized under an unified new data format. In future any firmware update will migrate existing Gen 1 and 2 sensors to that data format, which will lead to errors with the current version of your ros2 driver. Maybe you can have a look for integrating the new format?

[dllu]

@SteveMacenski the repo was updated yesterday to support gen 2 sensors.

[dmitrig]

@SteveMacenski Re: testing, unfortunately the test harnesses are still tied up with some internal code. It's not likely that we'll ever make the HIL parts public but at least some of the regression tests for parsing data should be doable... no eta yet, though.

Re: PRs to the ROS2 repo: if I understand correctly, you mean submit PRs to keep include/ros2_ouster/OS1/ up to date with this repo? I think that the code has been changed enough in the ROS2 driver that porting changes is going to be non-trivial.

If the goal is to stay in sync, is there some way to work towards depending on the ouster_client code more directly? E.g. through a submodule or something? This repo started out as example code that was simple enough to copy back-and-forth, but IMO it's gotten a bit too complicated to use that way after adding new product lines, cross-platform support, etc.

[SteveMacenski]

No worries, I wasn't really expecting you to setup a HIL tester for us.

Yes, a PR on changes would be great. We didn't really change that much in the packet parsing stuff. The majority of my changes were in the batching functions to enable different sensor processors for IMU/laser scans/pointclouds/different types of images. I did not mess with the lower level stuff (both because my field isn't network-packet-parsing and because it seemed to work fine by itself). If the V2.0 firmware doesn't require any client side changes, then I suppose we don't need to update anything. Though we still lack 128 beam support. I tried to go over the changes, but there was ~5 commits, each with massive changes, that its not really possible for me to quickly parse what changes I need to make to migrate.

I think we could work something out like that, but we'd need to stay strictly ABI/API compatible for that to work. We'd also need you to release those clients as binaries so that we can install them in a build configuration for building and releasing the ROS2 drivers. E.g. if you want apt install ros2-ouster-drivers, I need the client code either in my repo or as an apt install ouster-client type thing and create rosdep keys for it.

I agree that we should find an easier solution. I would not like to maintain the basic client code, especially if its going to change often. Though I have a really good ROS2 ouster driver made, so it might be prudent for Ouster to help me in maintaining that with future upgrades like these. The hard part is already done in designing, testing, and creating it- We need now to splice in the new client code.

[dmitrig]

~5 commits, each with massive changes, that its not really possible for me to quickly parse ...

Yep, apologies. I realize that makes it hard to build on top of this code. We'll be pushing smaller / more frequent changes from now on.

.. we'd need to stay strictly ABI/API compatible for that to work ... [and] need you to release those clients as binaries

Unfortunately, there's going to be some more churn over the next few months due to some large-ish changes planned for the wire format (though they should be backwards compatible and only add some long-requested features). There's also some long-overdue cleanup of namespaces, naming consistency, etc, that'll have to happen first, but it is a goal to provide some API stability guarantees as soon as possible. Right now this repo is still somewhere between its initial goal as "sample code" and a library people would want to use as a dependency.

I agree that we should find an easier solution.

👍 -- what do you think of the submodule idea? I did see that you didn't change the packet parsing stuff much, but even just shuffling the contents around to different files means each commit here requires a hand-crafted patch to the ROS2 driver. Using a submodule, that could be replaced with just running git checkout in your repo to try out upstream changes. I think it could remove a good amount of the drudgery of staying in sync, and I imagine I would even try compiling your repo as a way to check that I'm not introducing (unexpected) API changes.

[SteveMacenski]

Unfortunately, there's going to be some more churn over the next few months due to some large-ish changes planned for the wire format (though they should be backwards compatible and only add some long-requested features)

Is any of this going to be backwards compatible? Would it be worth simply not bothering to update the ROS 1 or ROS 2 drivers until things stabilize if not backwards compatible? I don't think hour-for-hour there's alot of value in updating every couple of weeks for months if it keeps changing. We should stick to a static version in ROS 1 (v2.0) and ROS 2 (TBD) until things settle down ready for use.

In ROS-land, each distribution released package must be ABI compatible for the lifetime of the package in that distribution, so there cannot be non-backwards-compatible changes in the wire protocol or client merged into released distributions. Therefore, we should select a single firmware version (or a range of working ones) for each ROS distribution.

what do you think of the submodule idea?

Submodules don't get pulled to the build farm, the ouster client would have to be a debian released package we can install as a dependency or live in the same repo as the driver to get pulled in to clone -> build -> build debians -> release to for users to apt install ros-<distro>-ouster.

We only really made a few very small changes to the client, mostly to add different time modes (which maybe you also added to v2 so our changes are moot?) and I think a couple of lines in the batch processing lambda to help with the more modular "processor" architecture

Really the best option is to have another repo with just the client code that you release as debians (either via ROS build farm or your own setup) and then this repo with the ROS drivers that can build against that released debian client. That would let the ROS2 drivers use the same client library as well. The major issue here is because you have them side-by-side in the same repo and the client is changing in non-compatible ways.

[dmitrig]

Ok maybe I see the disconnect -- the parsing code is included in the public headers of the ROS2 driver. I was assuming it was just used internally and the package only provided a node / messages. If exposing the packet parsing code as a library is a requirement for the ROS2 driver, then waiting until this API is stable to pull in changes is the only reasonable option.

But even then, major versions of this code are probably not going to line up with ROS releases, and it would probably get out date pretty quickly. If I had to include a driver in the ROS distribution, I probably would hide as much of the underlying library as possible even after it stabilized, to make it possible to update it and add support for new firmware features without breaking ABI compatibility of the ROS package.

release as debians (either via ROS build farm or your own setup) ...

Do the ROS build farms support pulling in debs from PPAs? Or what's the preferred way to pull in 3rd party binaries?

[SteveMacenski]

If exposing the packet parsing code as a library is a requirement for the ROS2 driver, then waiting until this API is stable to pull in changes is the only reasonable option.

I can create a ros2 master branch that isn't tied to a given distribution that can have updates made to reflect the current state of things without any issue. The problem is when the next distribution comes down the pipe Galactic, then I'll branch off of ros2, using whatever state of the library (and matching firmware/client versions) that are there at the time. Rather than leaving it to random chance of timing, it makes more sense for us to specifically target certain ouster client / firmware versions for specific distributions based on the ROS2 release schedules and your internal timelines for firmware/client releases.

So if v2.0 is on the mind, then we could make ros2 master branch work with v2.0 and when Galactic comes around, we'd release it and add docs that Galactic supports v2 (or maybe v2.xx if other sub-versions maintain backward compatibility to the same client code). So the release of your clients don't need to line up with ROS2 distribution releases, but once a ROS2 release is done with version ABC, we're stuck with that until that distribution goes EOL, assuming no effort is placed into making client code work / designed to work with a range of firmware versions.

Out of date is fine if the ROS2 distribution itself is pretty out of date, but that's one of the really important reasons why open-source and client code usually puts deep care into making sure that firmware/clients are backward compatible to a reasonable degree. That way your customers can still upgrade their firmware to get the latest and greatest, but the client / drivers continue to work with a range of modern versions. Then if you do need to make ABI breaking changes to the client, you do so knowing full well that all currently released distributions and customers will likely not be able to use them until the new ROS distribution comes out in 12 month cycles.

Do the ROS build farms support pulling in debs from PPAs? Or what's the preferred way to pull in 3rd party binaries?

That's a good question that I do not know the answer to myself. Maybe is the answer I'd give 😉. Fun enough, the "thing" that the ROS build farm builds and releases debians for doesn't strictly speaking need to use ROS. You could create a release for your client code and have that pipeline take care of all of that for you if you like. The only requirement is that it has a package.xml manifest file that specifies name, version, build tool (catkin, cmake, etc), and dependencies it needs to install as rosdep keys before building. Hint hint nudge nudge.

---

tl;dr, you'd make your lives alot easier if when working on the client code, design it to be really flexible so that you can minimize the number of breaking changes to work with a large number of features expected to have in the firmware in the immediate future. The firmware can do whatever it wants between versions, but the client code is what we need to make sure is compatible with a broader range of firmware versions to really make this not be a total headache for yourselves and your customers.

If you know you're going to add XYZ to your firmware in the next 24 months, add it preemptively to the client or make sure that the client code is flexible to take in those new changes without breaking existing behaviors. The more firmware versions the same client code can handle, the better for everyone that owns one of your products and the open source tools that rely on it.

[dmitrig]

Thanks, we'll take that into consideration and we'll certainly let you know if we make the firmware roadmap public. Future firmware updates will be backwards compatible with old drivers -- v2.0 was necessary exception.

So, just to clarify -- I was only talking about "getting out of date" w.r.t support of new firmware features and updates / bug fixes in this code. And I was only bringing it up as a motivation to avoid exposing some of the (currently) unstable APIs in this code.

Like I said we'll be working on turning this "sample code" into something that's easier for downstream projects to depend on. Until we get to that point, it would still be great to find an easier way to keep the ROS2 driver updated. Let me know if you have any ideas!

[SteveMacenski]

Got it - so if we integrate the ROS2 version with this client code as its been updated with recently, that should last us awhile even with the changes to firmware over the next few months?

If so, then we could try a stopgap and getting those files into the ROS2 drivers - as long as that's going to last us the next year or more. I don't want to get into a habit of doing these manual conversions, they're going to be very time consuming and I'm not sure I can commit to doing them with any regularity. Really the best option is to release the client debians separately and create a proper version control policy on them that we can base on for the ROS/ROS2 drivers.

[dmitrig]

Got it - so if we integrate the ROS2 version with this client code as its been updated with recently, that should last us awhile even with the changes to firmware over the next few months?

Yes.

If so, then we could try a stopgap and getting those files into the ROS2 drivers - as long as that's going to last us the next year or more ... Really the best option is to release the client debians separately ...

If you do get a chance to update, I still think copying in the ouster_client subdir as-is, even if it's not submoduled, would be a step in that direction. That is, when we do have debs of the client library easily available, it would be a drop-in replacement. It would also make pulling any additional updates from this repo a bit easier (and it would be more likely that I could find time to contribute some patches).

(And yeah, I realize I'm kind of asking for more free work on an open-source project -- just a suggestion for how we could share some maintenance work going forward).

[SteveMacenski]

The problem is that we need a couple of changes to deal with adding the different time modes, unless your new V2 client handles that already - code diff ros-drivers/ros2_ouster_drivers#36. If that's the case (or you can add it to the client here) then I agree, we can just copy it over and be done as a stop gap as long as the rest is the same. If these changes can be integrated into this repo or eq. exist, then our lives get much easier.

Though the packet size is now variable, so this client code needs some function to tell me what packet size to use for the connected sensor. I want to statically allocate these packets at bringup so that there's no dynamic memory allocation at runtime which is important for the safety minded people using this driver I designed for.

[dmitrig]

Yeah, we added timestamp mode a while back. Not against adding TIME_FROM_ROS_RECEPTION, either. We've also expanded the sensor metadata / parsing functions to deal with the different packet sizes.

Sounds like we agree on trying to minimize the changes that need to be maintained for the ROS2 drivers and working towards depending on some ABI-stable binaries we provide longer-term. Happy to budget some time for adding anything you guys need ahead of stabilizing the APIs here -- maybe it'd be easier to break those requests out into individual issues though.

[GuillaumeHauss]

Good morning @dgarbuzov and @SteveMacenski ,
I used some of the confined days of my holidays to start working on a ROS2/Firm V2.0 compatible nodes, based on this repository but also on the package from Steve here.
I got to the point where I can successfully visualized the point cloud of either a 32-layers or 64-layers OS1 lidars, in real-time, without bugs.
However, the code is still super messy and not optimized as I'm not a C++ expert.
I'll be happy to provide this branch as future base for the official ROS2/Firm V2.0 release.
Cheers

[SteveMacenski]

Is that based on this repo or mine? If mine, then a PR against the ROS2 branch would be appreciated and we can take a look there, but really you shouldn't need to mess with very much more than the ouster client headers copied over there.

@dgarbuzov maybe worth a call at some point soon to look at these points and come to a resolution about what you should add to the client so we can see how close we can get them so that it is just a trivial copy-paste every new firmware version. If it gets to that point, there may be other automation options available to us.

[GuillaumeHauss]

It is based on this repo. The integration of the new types/parsing/client init logic was too much a trouble to start with your Repo, Steve. However, it was my first intention to go that way, but maybe I missed the easy way to do so.

[SteveMacenski]

Yeah, you didn't need to touch any of that really, but that's OK.

[dmitrig]

Gonna close this for now -- please open issues / PRs for anything that we can upstream to make updating the ROS2 driver easier!