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

feat: added support for spacecraft thrusters #2431

Merged
merged 35 commits into from
Jul 19, 2024

Conversation

Pedro-Roque
Copy link
Contributor

@Pedro-Roque Pedro-Roque commented Jun 3, 2024

🎉 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

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

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
Copy link
Contributor Author

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 Pedro-Roque force-pushed the pr-spacecraft-thrusters branch from 4525a7e to 0ff5959 Compare June 4, 2024 15:49
@Pedro-Roque Pedro-Roque marked this pull request as ready for review June 4, 2024 18:57
@Pedro-Roque
Copy link
Contributor Author

Pedro-Roque commented Jun 4, 2024

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

@Pedro-Roque Pedro-Roque requested a review from ahcorde June 5, 2024 07:44
ahcorde
ahcorde previously requested changes Jun 5, 2024
tutorials/spacecraft_thrusters.md Outdated Show resolved Hide resolved
tutorials/spacecraft_thrusters.md Outdated Show resolved Hide resolved
Copy link
Contributor

@arjo129 arjo129 left a comment

Choose a reason for hiding this comment

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

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.

@Pedro-Roque
Copy link
Contributor Author

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?

@Pedro-Roque Pedro-Roque requested a review from arjo129 June 6, 2024 09:55
@arjo129
Copy link
Contributor

arjo129 commented Jun 6, 2024

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
Copy link
Contributor Author

@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
Copy link
Contributor

arjo129 commented Jun 7, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor Author

Pedro-Roque commented Jun 7, 2024

@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
Copy link
Contributor

arjo129 commented Jun 7, 2024

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

@Pedro-Roque
Copy link
Contributor Author

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
Copy link
Contributor

arjo129 commented Jun 10, 2024

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
Copy link
Contributor Author

Pedro-Roque commented Jun 10, 2024

Thanks @arjo129 , just fixed the above items!

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

Best,
Pedro

@arjo129
Copy link
Contributor

arjo129 commented Jun 11, 2024

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
Copy link
Contributor Author

Sounds good @arjo129 !

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

Copy link
Contributor

@arjo129 arjo129 left a comment

Choose a reason for hiding this comment

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

Its more or less looking good to me just handle the two changes I mentioned.

examples/worlds/spacecraft.sdf Outdated Show resolved Hide resolved
@arjo129 arjo129 changed the base branch from gz-sim8 to gz-sim7 June 11, 2024 07:19
@arjo129
Copy link
Contributor

arjo129 commented Jun 11, 2024

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.

Signed-off-by: Pedro Roque <[email protected]>
Signed-off-by: Pedro Roque <[email protected]>
Copy link
Contributor

@arjo129 arjo129 left a comment

Choose a reason for hiding this comment

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

LGTM. I think @bperseghetti, @azeey and @ahcorde should also take a look.

test/integration/spacecraft.cc Show resolved Hide resolved
Signed-off-by: Pedro Roque <[email protected]>
@Pedro-Roque Pedro-Roque requested review from azeey and arjo129 July 18, 2024 13:02
@arjo129
Copy link
Contributor

arjo129 commented Jul 18, 2024 via email

@Pedro-Roque
Copy link
Contributor Author

Done. Will wait for @ahcorde and @azeey for inputs

@Crola1702
Copy link
Contributor

I was trying to bring up and agent and I think gz_sim-ci-pr_any-focal-amd64 failure was my fault, sorry

@osrf-jenkins retest this please

@arjo129
Copy link
Contributor

arjo129 commented Jul 19, 2024

Can you merge with gz-sim7 to make the ABI checker happy

@Pedro-Roque
Copy link
Contributor Author

@arjo129 just merged it in, thanks for noting that!

@bperseghetti bperseghetti dismissed stale reviews from azeey and ahcorde July 19, 2024 10:10

Issues addressed, dismissing stale change request.

@bperseghetti bperseghetti merged commit d8eddd4 into gazebosim:gz-sim7 Jul 19, 2024
9 checks passed
@bperseghetti
Copy link
Member

bperseghetti commented Jul 19, 2024

@Pedro-Roque Thanks for your contribution, note this will not be in the binaries till a release is cut and run (including all other needed backports/forwardports that the team thinks is needed to allow for a release). I will see about the timing of that this coming Monday.

@Pedro-Roque
Copy link
Contributor Author

Thanks @bperseghetti - how can I get that info on Monday? Reach out via Discord?

@bperseghetti
Copy link
Member

Thanks @bperseghetti - how can I get that info on Monday? Reach out via Discord?

I'll ping you on here.

@Pedro-Roque
Copy link
Contributor Author

@bperseghetti just pinging for further info. A release with these additions would strongly simplify our setup

@bperseghetti
Copy link
Member

@bperseghetti just pinging for further info. A release with these additions would strongly simplify our setup

You can follow it here: #2491

@Pedro-Roque Pedro-Roque mentioned this pull request Jul 22, 2024
7 tasks
@Pedro-Roque
Copy link
Contributor Author

@bperseghetti just checking if there's any reason why the plugin was not propagated to newer gazebo versions - shall I do a PR for them? It is missing in harmonic, at least.

@iche033
Copy link
Contributor

iche033 commented Oct 29, 2024

forward ported to harmonic in #2504. I think there just needs to be a new gz-sim8 release.

@bperseghetti
Copy link
Member

forward ported to harmonic in #2504. I think there just needs to be a new gz-sim8 release.

@Pedro-Roque I can open a release request PR this evening.

@Pedro-Roque
Copy link
Contributor Author

Thanks @bperseghetti and @iche033 ! When are the binaries going to be released for Ubuntu 24?

@bperseghetti
Copy link
Member

Thanks @bperseghetti and @iche033 ! When are the binaries going to be released for Ubuntu 24?

Should be covered by this: #2671

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌱 garden Ignition Garden
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

7 participants