-
Notifications
You must be signed in to change notification settings - Fork 254
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
feat: added support for spacecraft thrusters #2431
Conversation
src/systems/spacecraft_thruster_model/SpacecraftThrusterModel.cc
Outdated
Show resolved
Hide resolved
src/systems/spacecraft_thruster_model/SpacecraftThrusterModel.cc
Outdated
Show resolved
Hide resolved
src/systems/spacecraft_thruster_model/SpacecraftThrusterModel.cc
Outdated
Show resolved
Hide resolved
src/systems/spacecraft_thruster_model/SpacecraftThrusterModel.cc
Outdated
Show resolved
Hide resolved
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! |
Signed-off-by: Pedro Roque <[email protected]>
4525a7e
to
0ff5959
Compare
@ahcorde should be good for a review check. Let me know what else I should improve on it |
There was a problem hiding this 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.
src/systems/spacecraft_thruster_model/SpacecraftThrusterModel.hh
Outdated
Show resolved
Hide resolved
src/systems/spacecraft_thruster_model/SpacecraftThrusterModel.cc
Outdated
Show resolved
Hide resolved
src/systems/spacecraft_thruster_model/SpacecraftThrusterModel.cc
Outdated
Show resolved
Hide resolved
src/systems/spacecraft_thruster_model/SpacecraftThrusterModel.cc
Outdated
Show resolved
Hide resolved
src/systems/spacecraft_thruster_model/SpacecraftThrusterModel.cc
Outdated
Show resolved
Hide resolved
src/systems/spacecraft_thruster_model/SpacecraftThrusterModel.cc
Outdated
Show resolved
Hide resolved
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? |
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. |
@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? |
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, |
@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? |
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. |
Regarding the Fuel model. I had a look. You just need to remove the 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 |
Thanks @arjo129 , just fixed the above items! Also, to target gz garden, should I make a PR to that branch? Best, |
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. |
Sounds good @arjo129 ! Let me know if I should do anything else for this to be merged. Thanks! |
There was a problem hiding this 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.
Your DCO needs fixing also are you basing your work on |
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. |
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. |
Signed-off-by: Pedro Roque <[email protected]>
@arjo129 @bperseghetti I've added a unit test now that checks for motion of the model upon setting a non-zero input. The test works as expected. Let me know if there is anything else needed before merging this in. @ahcorde @azeey just tagging to check if this is good to merge. |
Signed-off-by: Pedro Roque <[email protected]>
Signed-off-by: Pedro Roque <[email protected]>
Signed-off-by: Pedro Roque <[email protected]>
There was a problem hiding this 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.
Signed-off-by: Pedro Roque <[email protected]>
Yeah just revert and ignore windows for now. I'm not sure about it.
…On Thu, Jul 18, 2024, 9:03 PM Pedro Roque ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In test/integration/spacecraft.cc
<#2431 (comment)>:
> + serverConfig.SetSdfFile(sdfFile);
+
+ auto server = std::make_unique<Server>(serverConfig);
+ EXPECT_FALSE(server->Running());
+ EXPECT_FALSE(*server->Running(0));
+
+ using namespace std::chrono_literals;
+ server->SetUpdatePeriod(1ns);
+ return server;
+ }
+};
+
+/////////////////////////////////////////////////
+// Test that commanded motor speed is applied
+// See #1175
+TEST_F(SpacecraftTest, GZ_UTILS_TEST_DISABLED_ON_WIN32(InputTest))
@arjo129 <https://github.com/arjo129> test fails on Windows - seems like
the vehicle did not move at all there. Thoughts?
—
Reply to this email directly, view it on GitHub
<#2431 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAEEMQC3JXL4SF55RMVDFGLZM64LDAVCNFSM6AAAAABIXKU6BOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCOBVG42TOMZQGQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Signed-off-by: Pedro Roque <[email protected]>
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 |
Signed-off-by: Pedro Roque <[email protected]>
Can you merge with gz-sim7 to make the ABI checker happy |
@arjo129 just merged it in, thanks for noting that! |
Issues addressed, dismissing stale change request.
@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. |
Thanks @bperseghetti - how can I get that info on Monday? Reach out via Discord? |
I'll ping you on here. |
@bperseghetti just pinging for further info. A release with these additions would strongly simplify our setup |
You can follow it here: #2491 |
🎉 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
codecheck
passed (See contributing)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.