search

gazebosim / gz-sim

Open source robotics simulator. The latest version of Gazebo.
https://gazebosim.org
Apache License 2.0
618 stars 251 forks source link

feat: added support for spacecraft thrusters #2431

Open Pedro-Roque opened 4 weeks ago

Pedro-Roque commented 4 weeks ago

🎉 New feature

Summary

Adds support for Spacecraft thrusters controlled with a duty cycle signal (such as PWM). Method ported from PX4 Gazebo-Classic SITL implemented by @Jaeyoung-Lim

Test it

Checklist

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

Pedro-Roque commented 4 weeks ago

Thanks @ahcorde for the inputs! I had not yet started cleaning the branch as I had just finished the feature. I will address this in the coming days. Thanks!

Pedro-Roque commented 4 weeks ago

@ahcorde should be good for a review check. Let me know what else I should improve on it

Pedro-Roque commented 3 weeks ago

Thanks for this really awesome contribution. I've marked out some things that need changing. It would be nice if you had an example SDF and some unit tests also. These are useful as they allow us to make sure we don't introduce regressions when we make changes to the core simulation platform.

Thanks @arjo129 ! Unfortunately right now I'm a bit swamped, so might take a bit more time before I add an example SDF and unit tests. Would this be a hard requirement for now, or is it ok if we can merge this in and then I add them?

On another note: I need this plugin to be available on gz-sim7 (since we are using gazebo garden for now). Should I do a PR targetting that branch too?

arjo129 commented 3 weeks ago

I would say unit tests can go in later but we should have an example. At the very least thatll allow me to test it.

Pedro-Roque commented 3 weeks ago

@arjo129 I Just added a spacecraft world with the DART spacecraft (mass and inertia were severely reduced for faster visualization. Also updated the tutorial.

Moreover, I think that the mesh file should not be there but hosted on fuel, but I couldn't find my way to upload it there. Do I need extra permissions?

arjo129 commented 3 weeks ago

You do need to sign in to upload files to fuel. Tbh you can just use a box if you're strapped for time.

Pedro-Roque commented 3 weeks ago

You do need to sign in to upload files to fuel. Tbh you can just use a box if you're strapped for time.

I'll try to do it now so that it's done properly at once. Standby,

Pedro-Roque commented 3 weeks ago

@arjo129 I've added my model in Fuel, but I'm getting this error even though I believe I have the file paths correctly set:

gz sim spacecraft.sdf
Warning: Ignoring XDG_SESSION_TYPE=wayland on Gnome. Use QT_QPA_PLATFORM=wayland to run on Wayland anyway.
[GUI] [Err] [SystemPaths.cc:425] Unable to find file with URI [file://dart/meshes/dart.dae]
[GUI] [Err] [SystemPaths.cc:525] Could not resolve file [file://dart/meshes/dart.dae]
[GUI] [Err] [MeshManager.cc:211] Unable to find file[file://dart/meshes/dart.dae]
[GUI] [Err] [SceneManager.cc:425] Failed to load geometry for visual: base_link_visual

Model is available here: https://app.gazebosim.org/proque/fuel/models/dart

Any idea of why this could be a problem?

arjo129 commented 3 weeks ago

I wont be able to test this till tomorrow. Ill let you know how to fix it.

Pedro-Roque commented 3 weeks ago

I wont be able to test this till tomorrow. Ill let you know how to fix it.

No worries, let me know and I'll fix it over the weekend.

arjo129 commented 3 weeks ago

Regarding the Fuel model. I had a look. You just need to remove the file:// prefix inthe mesh tag.

Also you need to re-sign off your commits: https://github.com/gazebosim/gz-sim/pull/2431/checks?check_run_id=25943650220

The automated linter also has a few complaints: https://github.com/gazebosim/gz-sim/actions/runs/9417757870/job/26011958069?pr=2431

Pedro-Roque commented 3 weeks ago

Thanks @arjo129 , just fixed the above items!

Also, to target gz garden, should I make a PR to that branch?

Best, Pedro

arjo129 commented 3 weeks ago

Also, to target gz garden, should I make a PR to that branch?

We try to avoid adding new features to older versions. If it really is needed we do do backports from time to time. Lets first merge it then it can be cherry picked later.

Pedro-Roque commented 3 weeks ago

Sounds good @arjo129 !

Let me know if I should do anything else for this to be merged. Thanks!

arjo129 commented 3 weeks ago

Your DCO needs fixing also are you basing your work on gz-sim7 so you should target that branch else the abi checker wont be happy.

Pedro-Roque commented 3 weeks ago

Your DCO needs fixing also are you basing your work on gz-sim7 so you should target that branch else the abi checker wont be happy.

Yes you are right, thanks for changing the target branch to gz-sim7! So later we cherry-pick this for main?

arjo129 commented 3 weeks ago

Yes. We will forward port later. First address the open issue a hand.

For DCO follow the instructions here: https://github.com/gazebosim/gz-sim/pull/2431/checks?check_run_id=26016983492

Pedro-Roque commented 2 weeks ago

@bperseghetti @azeey I've now updated everything with respect to the provided comments. Only thing that I'll have to do later is the unit tests. I'd be happy if this could be merged meanwhile, since I have another PR depending on this.

Pedro-Roque commented 2 weeks ago

@arjo129 are we ready to merge this in?

bperseghetti commented 2 weeks ago

Hey @Pedro-Roque I would suggest still adding a basic test to get some level of code coverage started. Also I'm assuming your dependent PR is the one in PX4, I can help merge that PX4 PR in for you but first a release of garden would need to be scheduled after this current PR gets in and that can take some time since it's more than just getting this PR merged.

With that being said I would suggest the fastest way to get this current PR merged in and start the "release timeline clock" is to create that super basic test based off what I shared for Ackermann (or really any other test in tree as well) with the goal of testing the functionality and also as many lines of code of this PR as possible (for code coverage).

Pedro-Roque commented 1 week ago

@bperseghetti I created a test and wanted to run it on my machine. However after following the instructions regarding code coverage, colcon only finds 7 tests in the whole workspace and no tests are found on the gz-sim7 package.

Is there something obvious I'm missing? I'll share here a log of the commands

arjo129 commented 1 week ago

How are you building your workspace? Generally I would run colcon build. Some times you may need to force cmake to reconfigure itself. Running colcon test will take very long. I suggest going inside the build/gz-sim7/bin directory. You should see the integration test binaries. You can run your test individually from there.

bperseghetti commented 1 week ago

@bperseghetti I created a test and wanted to run it on my machine. However after following the instructions regarding code coverage, colcon only finds 7 tests in the whole workspace and no tests are found on the gz-sim7 package.

Is there something obvious I'm missing? I'll share here a log of the commands

Want to just push up to this PR your test that you wrote and we can look at/suggest changes from there?

Pedro-Roque commented 1 day ago

How are you building your workspace? Generally I would run colcon build. Some times you may need to force cmake to reconfigure itself. Running colcon test will take very long. I suggest going inside the build/gz-sim7/bin directory. You should see the integration test binaries. You can run your test individually from there.

This is what is populated in the folder:

╭─  ~/gz_garden_ws/build/gz-sim7/bin ···························  07:38:29 
╰─❯ pwd              
/home/roque/gz_garden_ws/build/gz-sim7/bin

╭─  ~/gz_garden_ws/build/gz-sim7/bin ···························  07:38:30 
╰─❯ ls
runGui

@bperseghetti I'd like to have a working test before I push it, but I'd also like to try it out locally first.

arjo129 commented 23 hours ago

Seems like you are not building tests. Please use:

colcon build --merge-install

NOT:

colcon build --cmake-args -DBUILD_TESTING=OFF --merge-install

Consider deleteing build and install folders in your workspace for clean builds.

Pedro-Roque commented 18 hours ago

Seems like you are not building tests. Please use:

colcon build --merge-install

NOT:

colcon build --cmake-args -DBUILD_TESTING=OFF --merge-install

Consider deleteing build and install folders in your workspace for clean builds.

Hi @arjo129 the command I was running is

colcon build --cmake-args -DCMAKE_BUILD_TYPE=Coverage --merge-install --packages-select gz-sim7

I'll do a clean build and come back to you.

arjo129 commented 18 hours ago

Remove the coverage do a vanilla build.

Pedro-Roque commented 17 hours ago

Remove the coverage do a vanilla build.

Ok, the vanilla build seems to be building the tests. Maybe this is something we should update the docs with. I'll fix the test and push changes later in the week.