-
Notifications
You must be signed in to change notification settings - Fork 14.6k
[wip] gz plugins #24153
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
[wip] gz plugins #24153
Conversation
IMO, we should bundle gz plugins with the gz models, since this is how gz is designed. IMHO, gz bridge should support generic messages, not specific to plugins. Given that said, I quite disliked how gz classic models and plugins were a submodule, but cannot think of a better alternative @dagar Any opinions? |
I talked with @dagar about this. The goal will be to have the plugins developed in PX4 designed in such a way that they could be upstreamed as a system plugin The plugins will be kept in PX4-Autopilot source under the gz_bridge/ module in a plugin/ directory for convenience. This will basically be a staging area for plugins until (if/when) they are upstreamed. |
@Jaeyoung-Lim I think I'd agree keeping models + plugins together (perhaps someplace like https://github.com/PX4/PX4-gazebo-models, but renamed) makes sense for any long term plugins that aren't headed for upstream Gazebo. Other than that I was hoping to make it as convenient as possible. |
So it sounds like we might want to rename PX4-gazebo-models to PX4-SITL_gazebo_gz? Considering the repo is for gz models. This would also at least make things consistent between classic and gz. |
@dakejahl @dagar I agree, we should try to make custom plugins as little as possible, and try to upstream as much as possible with the new gz. Plugins would not "always" talk to gz bridge, since we might need certain plugins to simulate failure, devices that may indirectly interact with px4. I am not opposed to renaming the repository, since I couldn't care less whether the name of the repository is accurate :) |
After a lot of work yesterday I am starting to come to the conclusion that it may not be possible to design custom sensor plugins that are upstream-able. All of the sensors in gz are strictly defined https://github.com/gazebosim/sdformat/blob/sdf15/include/sdf/Sensor.hh#L54-L137 If we wanted to create a new sensor type and upstream it, we'd have to add it to every repo. It's not simply a matter of moving our source files into the right spot in gz-sensors unfortunately. IMO does not seem worth the effort, I don't think we want to be in the business of gz development at this level. |
@dakejahl You are right that it is a bit cumbersome to add new sensor "types" since it propagates upto sdf definitions. But we have already been upstreaming a few plugins, and I would not want to repeat us doing the same mistakes as gazebo classic. Basically Gazebo plugins suck for drones, and it is because we didnt contribute to it :) Given that said I think custom plugins would be a nice compromise where we have something that doesnt make these gazebo details block or slow us down, but once it is properly integrated, we should upstream it, such that gz becomes part of our development infrastructure, not work around it. Anyway, this wouldnt change anything for what needs to be done in this PR, right? |
For this PR as it is -- no. I was exploring the idea of creating a sensor plugin that inherits from the CameraSensor so that the plugin itself can own the sensor, rather than needing to be paired with sdf that provides the camera sensor. Same with the rangefinder. I wanted to be able to do this
But I think the only way to achieve that is to create this in gz and contribute it.
Another project to learn and keep up with 😅 but yeah that would be ideal. |
@dagar can we merge this as is for now? I need to test optical flow and range finger altitude hold against main. I want to refine this implementation further (bring in opencv directly) but I'd like to do that in subsequent PRs. As it stands it is functional and the issues I am trying to solve are reproducible with this PR. |
e3bcdd6
to
1423845
Compare
@dagar this is ready for review. All of the checks are passing except for the known-unrelated ones. I will follow up on this PR with another PR for a gstreamer plugin. |
0fd3429
to
2a096ce
Compare
I moved things into their own directories with Kconfig options
The OpticalFlow library is now built in it's own cmake to that it doesn't get rebuilt all the time. |
This pull request has been mentioned on Discussion Forum for PX4, Pixhawk, QGroundControl, MAVSDK, MAVLink. There might be relevant details there: |
This PR is not ready until #24421 is merged. Then I will refactor. I will also make use of GZ_SIM_SERVER_CONFIG_PATH to load plugins so that we don't have to pollute every world file |
Adds build infrastructure for gz plugins. I've implemented an optical flow sensor plugin and created a model for it.
In my opinion we should tightly couple the gz_bridge with gz_plugins, since the gz_bridge needs to subscribe to our custom proto msgs and publish to uorb.
Looking for feedback and help with restructuring the build.
Should we follow the same design as gazebo classic and bundle this all up into a submodule?