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

Lighter Than Air Dynamics Systems #2241

Merged
merged 1 commit into from
May 14, 2024

Conversation

henrykotze
Copy link
Contributor

@henrykotze henrykotze commented Nov 11, 2023

🎉 New feature

Summary

This adds the dynamics to simulate a lighter-than-air vehicle such as blimps/airships. The modelling is based on [1] and [2]. Previously we added an airship-dynamics-plugin to Gazebo-Classic for PX4 Gazebo SITL , but we suffered simulation instability similar to that of the underwater-vehicles since Gazebo-Classic could not simulate the Added Mass dynamics.

This PR brings that airship-dynamics-plugin to Gazebo which supports the Added Mass dynamics.

Furthermore, I have extended, the Link class to provide access to the AddedMassMatrix, to allow the lighter-than-air system to easily access this matrix.

The code is somewhat based of the Hydrodynamics system.

I have an STL file which I can contribute, which is an envelope from Wind Reiter, and the coefficients in the integrations test is for this envelope model.

Citations

[1] Li, Y., & Nahon, M. (2007). Modeling and simulation of airship dynamics. Journal of Guidance, Control, and Dynamics, 30(6), 1691–1700.
[2] Li, Y., Nahon, M., & Sharf, I. (2011). Airship dynamics modeling: A literature review. Progress in Aerospace Sciences, 47(3), 217–239.

Test it

I have written a small integration test to check whether the lighter-than-air system is loaded correctly and stable.

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.

🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸

@github-actions github-actions bot added the 🌱 garden Ignition Garden label Nov 11, 2023
@henrykotze
Copy link
Contributor Author

Hi, I tried to get this PR as ready as possible for merging (reading the contribution docs and implementing the requirements), and have reached an unproductive state where I need some feedback to take this forward. So any comments, and or further requirements to implement would help alot.

I am still thinking of more tests to add to ensure any future additions will keep the core dynamics of LTA vehicles unchanged (except if some new modelling arises), since the system is unstable when having a velocity.

math::Matrix3d m11, math::Matrix3d m12,
math::Matrix3d m21, math::Matrix3d m22);

public: math::Vector3d added_mass_force(math::Vector3d lin_vel,
Copy link
Contributor

Choose a reason for hiding this comment

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

add docs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved

test/integration/lighter_than_air_dynamics.cc Outdated Show resolved Hide resolved
*/

#include <gtest/gtest.h>

Copy link
Contributor

Choose a reason for hiding this comment

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

include vector, string

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Apart from @ahcorde's style comments, there are some functionality related things which need minor changes that I've highlighted. Also a file in the examples/ folder would be nice as end users who install binaries don't get the unit tests!

test/worlds/CMakeLists.txt Outdated Show resolved Hide resolved
src/systems/lighter_than_air_dynamics/CMakeLists.txt Outdated Show resolved Hide resolved
}

this->dataPtr->airDensity = SdfParamDouble(_sdf, "airDensity",
SdfParamDouble(_sdf, "air_density", 998)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just keep air_density.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved

@arjo129
Copy link
Contributor

arjo129 commented Nov 20, 2023

Can you fix your commit sign-off. Instructions are here

Copy link

codecov bot commented Nov 21, 2023

Codecov Report

Attention: Patch coverage is 83.05085% with 30 lines in your changes are missing coverage. Please review.

Project coverage is 64.82%. Comparing base (922eea7) to head (bf74047).
Report is 3 commits behind head on gz-sim7.

❗ Current head bf74047 differs from pull request most recent head 3705ccd. Consider uploading reports for the commit 3705ccd to get more accurate results

Files Patch % Lines
...ighter_than_air_dynamics/LighterThanAirDynamics.cc 83.04% 29 Missing ⚠️
src/Link.cc 83.33% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           gz-sim7    #2241      +/-   ##
===========================================
- Coverage    64.88%   64.82%   -0.06%     
===========================================
  Files          360      358       -2     
  Lines        29448    29337     -111     
===========================================
- Hits         19108    19019      -89     
+ Misses       10340    10318      -22     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@henrykotze henrykotze force-pushed the lighter-than-air-dynamic branch 2 times, most recently from 4bfa7d1 to ea20b0d Compare November 21, 2023 18:23
@henrykotze
Copy link
Contributor Author

I hope its okay that I squashed and force pushed, I didnt intent to remove the co-author commits, but rather keep the history much more clearer.

@arjo129
Copy link
Contributor

arjo129 commented Nov 22, 2023

I hope its okay that I squashed and force pushed, I didnt intent to remove the co-author commits, but rather keep the history much more clearer.

No worries. You have to force push for DCO so 🤷

@henrykotze
Copy link
Contributor Author

Somehow missed all the other style suggestions.
Let me know if there is anything else to do here.

@henrykotze
Copy link
Contributor Author

Anything else todo here?

@henrykotze henrykotze force-pushed the lighter-than-air-dynamic branch 2 times, most recently from 4c468db to bfd3d26 Compare December 6, 2023 14:38
-visc_normal_y_);

// Added Mass forces & Torques
auto force_added_mass = -1.0 * this->added_mass_force(linear_vel,
Copy link
Contributor

@arjo129 arjo129 Dec 27, 2023

Choose a reason for hiding this comment

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

IIUC, if you use the SDF <fluid_added_mass> tags, the added mass gets added by the physics engine. Theres no need to add the added mass terms here. I think @azeey, @mabelzhang or @quarkytale may be able to elaborate more on that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for this!

Referring to the paper, on which I have based the dynamics, the added mass effects should be incorporated in two places: (1) In the linear body acceleration and angular body acceleration, and then (2) in the coupling of the linear and angular velocities. I have added picture from the paper.

From my understanding, the physics engine incorporates it into the former (the acceleration terms). I have just added the forces and torques which is created by the added mass due to the coupling of the linear and angular velocities. I hope this make sense.

image
Equation 1
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would be awesome if someone could provide insight into the backend of the physics engine and how it incorporate the added mass effects. From my understanding, it would only compute the first term in equation (6) and not both terms.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep! You are right. The physics engine will only include added mass in the acceleration term. Any interaction due to velocities would have to be done outside of the engine.

<gravity>0 0 -9.81</gravity>

<plugin
filename="ignition-gazebo-physics-system"
Copy link
Contributor

Choose a reason for hiding this comment

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

For forward compatibility use gz-sim-physics-system instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved

auto pose = baseLink.WorldPose(_ecm);
// Since we are transforming angular and linear velocity we only care about
// rotation. Also this is where we apply the effects of current to the link
auto linear_vel = pose->Rot().Inverse() * (linearVelocity->Data());
Copy link
Contributor

Choose a reason for hiding this comment

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

please use camelCase for all variable names. See https://gazebosim.org/docs/harmonic/contributing#style-guides

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do!

I see there isnt really a style requirement for function parameters. Should I make my function parameters also camelCase?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All variable names and function names use camelCase

-visc_normal_y_);

// Added Mass forces & Torques
auto force_added_mass = -1.0 * this->added_mass_force(linear_vel,
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Sorry for the long feedback time. The plugin itself looks good to me. However the test does not seem to check for any velocity/positon values. This means we may not be able to catch regressions.

EXPECT_EQ(2000u, bodyVels.size());

EXPECT_NE(model.Entity(), kNullEntity);
EXPECT_NE(body.Entity(), kNullEntity);
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for the incredibly long feedback time. But this test does not seem to be checking anything about the behaviour of the dynamics. It'll be hard to spot regressions if we dont test that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that the tests are limited which I have implemented.
Will have a look at this more closely!
Thanks for the feedback.

Copy link
Contributor

Choose a reason for hiding this comment

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

What we do for hydrodynamics is push a model and then try to measure its terminal velocity. Perhaps this is an approach which you want to adopt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added tests to check the instability of pitch and yaw.

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 iterating on this PR. LGTM!

@henrykotze
Copy link
Contributor Author

Thanks for iterating on this PR. LGTM!

Thank you for your review and follow ups! Its much appreciated!

@arjo129 arjo129 requested a review from ahcorde May 14, 2024 07:24
@arjo129 arjo129 dismissed ahcorde’s stale review May 14, 2024 07:24

Review is stale

@arjo129
Copy link
Contributor

arjo129 commented May 14, 2024

Can you try merging the base branch once it seems the ABI checker is not happy.

@henrykotze
Copy link
Contributor Author

Rebased on top of gz-sim7?

@arjo129
Copy link
Contributor

arjo129 commented May 14, 2024

Yep.

copy of Hydrodynamics folder, and rename to lta

Renamed to LighterThanAirDynamics

- Added some initial private data
- Requires still alot of cleanup

some initial dynamics added

Building sucessfully with some warnings

Running test to determine succesfull Spawning

successfull ran tests, but still debugging alot

Added additional debug messages

Protection for zero division

- Test running sucessfully with adding forces and torques, but requires
more extensive testing and validation

Improved Documentation,some more work on tests

Improve documentation to include math

Silver bullet model withs its coefficients

small documentation update

Code style updates. wip

Style checks completed

Small correction to dynamics

Added Mass Force and Torques included

- Link class now provides the FluidAddedMassMatrix

Removed Munk from added mass & correct coeff names

Signed-off-by: henrykotze <henry@flycloudline.com>

update year

Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Signed-off-by: Henry Kotzé <henry@flycloudline.com>

update format suggestion

Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Signed-off-by: Henry Kotzé <henry@flycloudline.com>

update format suggestion in added_mass_force

Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Signed-off-by: Henry Kotzé <henry@flycloudline.com>

format fix: function header

Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Signed-off-by: Henry Kotzé <henry@flycloudline.com>

update formatting

Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Signed-off-by: Henry Kotzé <henry@flycloudline.com>

Update src/systems/lighter_than_air_dynamics/LighterThanAirDynamics.hh

Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Signed-off-by: Henry Kotzé <henry@flycloudline.com>

update year

Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Signed-off-by: Henry Kotzé <henry@flycloudline.com>

Functionality updates and style updates

Signed-off-by: henrykotze <henry@flycloudline.com>

Example added for Lighter-than-air envelope

Signed-off-by: henrykotze <henry@flycloudline.com>

Use numerical limits to query precision of types

Signed-off-by: henrykotze <henry@flycloudline.com>

fix integration path for sdf file

Signed-off-by: henrykotze <henry@flycloudline.com>

style format update

Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Signed-off-by: Henry Kotzé <henry@flycloudline.com>

update year

Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Signed-off-by: Henry Kotzé <henry@flycloudline.com>

style format update

Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Signed-off-by: Henry Kotzé <henry@flycloudline.com>

style format update

Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Signed-off-by: Henry Kotzé <henry@flycloudline.com>

style format update

Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Signed-off-by: Henry Kotzé <henry@flycloudline.com>

style format update

Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Signed-off-by: Henry Kotzé <henry@flycloudline.com>

style format update

Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Signed-off-by: Henry Kotzé <henry@flycloudline.com>

style format update

Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Signed-off-by: Henry Kotzé <henry@flycloudline.com>

style format update

Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Signed-off-by: Henry Kotzé <henry@flycloudline.com>

style format update

Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Signed-off-by: Henry Kotzé <henry@flycloudline.com>

style format update

Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Signed-off-by: Henry Kotzé <henry@flycloudline.com>

style format update

Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Signed-off-by: Henry Kotzé <henry@flycloudline.com>

style format update

Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Signed-off-by: Henry Kotzé <henry@flycloudline.com>

style format update

Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Signed-off-by: Henry Kotzé <henry@flycloudline.com>

style format update

Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Signed-off-by: Henry Kotzé <henry@flycloudline.com>

remove commented wind component

Signed-off-by: henrykotze <henry@flycloudline.com>

fix whitespace error from pre-commit

Signed-off-by: henrykotze <henry@flycloudline.com>

codecheck fix

conform to style guide requirements

wip: test with forces acting on it

working tests implementing, cleanup required

cleanup and formatting

revert back to original example

more revert to original
@arjo129 arjo129 enabled auto-merge (squash) May 14, 2024 09:10
@arjo129 arjo129 merged commit 4019e09 into gazebosim:gz-sim7 May 14, 2024
8 of 9 checks passed
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.

None yet

4 participants