Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal to add Ouster ROS2 Drivers #263

Merged
merged 3 commits into from Dec 17, 2020

Conversation

SteveMacenski
Copy link
Contributor

This is a proposal to add the ROS2 Ouster driver to the REP 2005 common packages definition.

This package is analog to Velodyne drivers and is suitable for a large scope of navigation, autonomous vehicle and general perception tasks. They have been built against a design doc, making use of the modern tools that ROS2 enables including Lifecycle and Components, with serious consideration to performance and soft real-time requirements. It also has the official support of Ouster LIDAR.

It has a mere 22 stars at the moment, but I think the reuse is clear. Users include Box Robotics @tpanzarella and Rover Robotics @rotu with 2 very active maintainers: Tom and I.

We have CI setup and run the full suite of ROS2 linters with (what some may consider excessive) documentation on the API, explanation of features, networking setup assistance, and youtube videos explaining how to use and lidar features.

Extensive tests are currently lacking due to the HIL nature of testing something like this as a hardware driver. I'm chatting with Ouster right now to potentially get testing resources in the form of recommendations and potentially an automated build setup with a lidar in their office for complete HIL testing, far more than an existing sensor driver on this list or available in the ROS2 ecosystem.

@ros-discourse
Copy link

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-2020-05-21/14247/1

@gbiggs
Copy link
Contributor

gbiggs commented May 28, 2020

I would support this if the Ouster driver were moved into ros-drivers.

@SteveMacenski
Copy link
Contributor Author

SteveMacenski commented May 28, 2020

@gbiggs the long term home for this one is a little up in the air. I don't think it'll long term stay where it is. I think it'll either be moved into ros-drivers or an ouster org at some point. Its there kind of by "default" since its flexible rather than in another location that is more permanent and harder to change once a decision is made.

@kyrofa
Copy link
Contributor

kyrofa commented May 29, 2020

Ditto @gbiggs. I made a similar comment on #264, I thought we wanted to avoid this?

@SteveMacenski
Copy link
Contributor Author

SteveMacenski commented May 29, 2020

Ok, I'd also be OK rescinding this request to add to common packages until the home is resolved that would be reasonable. It would just be nice to be able to work on this as part of my TSC work since its analogous to the Velodyne drivers and the Ousters are popping up everywhere.

But I won't "die on the hill", as Matt Hansen says, about it. I'll keep working on it on nights/weekends.

@gbiggs
Copy link
Contributor

gbiggs commented Jun 1, 2020

What's blocking it from going into ros-drivers? If anything?

@SteveMacenski
Copy link
Contributor Author

SteveMacenski commented Jun 1, 2020

We don't know where this should end up in the long-term. It's not a sure thing that ros-drivers is that correct location. It is not entirely in my control to make a decision and its been pending for awhile. There hits a point where you just keep charging ahead and once that's been made it'll be ready.

@tpanzarella
Copy link

I think it'll either be moved into ros-drivers or an ouster org at some point

@SteveMacenski are you thinking it will move to Ouster?

@SteveMacenski
Copy link
Contributor Author

SteveMacenski commented Jun 2, 2020

That's the second other potential long-term home. Either way, if that's a requirement to get included on this list that its in some org, I'll pull this request back if that is a non-starter for @gbiggs and @kyrofa. I totally respect that viewpoint. If they'd be OK with the long-term statement that it could move to one of a number of places, then we can proceed on.

Like I said in the prior post - this package doesn't matter to me too much if its on this list, I thought it would be a nice gesture given its use in the industry.

@gbiggs
Copy link
Contributor

gbiggs commented Jun 2, 2020

It's not that I don't trust @SteveMacenski, but I don't want to set a precedent that could come back and bite us. I'd prefer to see it in an org first. The moment it's in an org, it will get a +1 from me.

@SteveMacenski
Copy link
Contributor Author

OK

@SteveMacenski
Copy link
Contributor Author

Reopening - got the A-OK to move it to ros-drivers, that move is pending. @tfoote is making that move soon.

@gbiggs @kyrofa does that change your vote?

@SteveMacenski SteveMacenski reopened this Aug 27, 2020
@gbiggs
Copy link
Contributor

gbiggs commented Aug 27, 2020

Yes. Once it's in ros-drivers I see no obstacles.

@SteveMacenski
Copy link
Contributor Author

Tis there now

@gbiggs
Copy link
Contributor

gbiggs commented Aug 27, 2020

👍

rep-2005.rst Outdated Show resolved Hide resolved
rep-2005.rst Outdated Show resolved Hide resolved
Co-authored-by: Jacob Perron <jacob@openrobotics.org>
@clalancette
Copy link
Contributor

From what I can tell, this one has a single thumbs-up from a TSC member. To be merged, it needs 2 more.

@SteveMacenski
Copy link
Contributor Author

@kyrofa does this satisfy your requirements? (@clalancette you can vote on regards for OR 😉 )

You're right though, for some reason I thought this already had the 3.

@clalancette
Copy link
Contributor

(@clalancette you can vote on regards for OR wink )

This was already approved by @gbiggs on behalf of OR, so we'll need votes from other TSC members.

Copy link
Contributor

@kyrofa kyrofa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for moving it, 👍 from me.

@SteveMacenski
Copy link
Contributor Author

I think that's 3 with Kyle, Geoff, and Aaron

@clalancette clalancette merged commit bb07d0f into ros-infrastructure:master Dec 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants