Skip to content

Conversation

dakejahl
Copy link
Contributor

@dakejahl dakejahl commented Dec 25, 2024

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?

@dakejahl dakejahl mentioned this pull request Dec 25, 2024
31 tasks
@dakejahl
Copy link
Contributor Author

@Jaeyoung-Lim
Copy link
Member

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?

@dakejahl
Copy link
Contributor Author

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
https://github.com/gazebosim/gz-sim/tree/gz-sim9/src/systems
Sames goes for the proto messages. Ideally we upstream them rather than carry in PX4 as a custom message.

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.

@dagar
Copy link
Member

dagar commented Jan 22, 2025

@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.

@dakejahl
Copy link
Contributor Author

@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.

@Jaeyoung-Lim
Copy link
Member

@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 :)

@dakejahl
Copy link
Contributor Author

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.

@Jaeyoung-Lim
Copy link
Member

@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?

@dakejahl
Copy link
Contributor Author

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

      <!-- Optical flow sensor that inherits from CameraSensor -->
      <sensor name="optical_flow" type="custom" gz:type="optical_flow">
          <pose>0 0 0 0 1.5707 0</pose>
          <always_on>1</always_on>
          <update_rate>50</update_rate>
          <visualize>true</visualize>
          <topic>/optical_flow</topic>

          <gz:optical_flow>
              <!-- define settings for for camera/flow/range -->
          </gz:optical_flow>
      </sensor>

But I think the only way to achieve that is to create this in gz and contribute it.

such that gz becomes part of our development infrastructure, not work around it.

Another project to learn and keep up with 😅 but yeah that would be ideal.

@dakejahl
Copy link
Contributor Author

@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.

@dakejahl dakejahl marked this pull request as ready for review February 14, 2025 22:54
@dagar
Copy link
Member

dagar commented Feb 14, 2025

I'll take a look, can you resolve the conflicts?

Screenshot 2025-02-14 at 5 59 36 PM

@dakejahl
Copy link
Contributor Author

@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.

@dakejahl dakejahl force-pushed the dev/gz_plugins branch 2 times, most recently from 0fd3429 to 2a096ce Compare February 23, 2025 02:53
@dakejahl
Copy link
Contributor Author

I moved things into their own directories with Kconfig options

CONFIG_MODULES_SIMULATION_GZ_MSGS=y
CONFIG_MODULES_SIMULATION_GZ_BRIDGE=y
CONFIG_MODULES_SIMULATION_GZ_PLUGINS=y

The OpticalFlow library is now built in it's own cmake to that it doesn't get rebuilt all the time.

@AlexKlimaj AlexKlimaj enabled auto-merge (squash) February 25, 2025 22:11
@AlexKlimaj AlexKlimaj disabled auto-merge February 25, 2025 22:11
@AlexKlimaj AlexKlimaj enabled auto-merge (squash) February 25, 2025 22:18
@DronecodeBot
Copy link

This pull request has been mentioned on Discussion Forum for PX4, Pixhawk, QGroundControl, MAVSDK, MAVLink. There might be relevant details there:

https://discuss.px4.io/t/px4-sync-q-a-feb-26-2025/43951/1

@dakejahl
Copy link
Contributor Author

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
https://github.com/gazebosim/gz-sim/blob/gz-sim9/tutorials/server_config.md

@dakejahl dakejahl merged commit 6dc39d9 into PX4:main Mar 3, 2025
59 checks passed
@dakejahl dakejahl deleted the dev/gz_plugins branch March 3, 2025 21:21
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.

5 participants