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

New vehicle type: Airship #490

Merged
merged 17 commits into from
Jul 19, 2020
Merged

New vehicle type: Airship #490

merged 17 commits into from
Jul 19, 2020

Conversation

dan-leo
Copy link
Contributor

@dan-leo dan-leo commented May 7, 2020

As mentioned in PX4/PX4-Autopilot#14792 and https://discuss.px4.io/t/new-vehicle-type-airship/16514, our team has developed a new vehicle type for the PX4 developer community, and we would like to submit a pull request for review.

antonerasm and others added 5 commits April 22, 2020 17:45
- aerodynamics
- bouyancy
- gazebo freezes upon entering
- direct use of -gen.sdf from sdf.jinja
@TSC21
Copy link
Member

TSC21 commented May 7, 2020

@dan-leo can you put here a screenshot of how it looks?

@dan-leo
Copy link
Contributor Author

dan-leo commented May 7, 2020

Absolutely!

image

Copy link
Member

@Jaeyoung-Lim Jaeyoung-Lim 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 the great contribution!

Small comments

  • It seems like there is some changes regarding the motor_model that is not compatible with multirotors. This seems to be coming from the fact that you want to support bidirectional thrust. Could we maybe make this compatible for also multirotors?
  • It is not clear why you need a buoyancy plugin and aerodynamics plugin separately unless it is expected other systems use the plugin as a module. Otherwise I think it should be a single plugin that simulates airship dynamics.
  • Maybe not for this PR, but to note: the airship doesn't have any effect with wind currently.
  • the .world. file is not used unless you want to have vehicle specific physics.

include/gazebo_lta_aerodynamics_plugin.h Outdated Show resolved Hide resolved
models/cloudship/cloudship.sdf Outdated Show resolved Hide resolved
models/cloudship/model.config Outdated Show resolved Hide resolved
src/gazebo_buoyancy_plugin.cpp Outdated Show resolved Hide resolved
src/gazebo_motor_model.cpp Outdated Show resolved Hide resolved
src/gazebo_motor_model.cpp Outdated Show resolved Hide resolved
worlds/cloudship.world Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
@antonerasm
Copy link
Contributor

@Jaeyoung-Lim thank you for the informative review, your feedback is much appreciated.

On your comments:

  • Yes, we would like to incorporate bidirectional thrust. I was thinking of having a third option for the motor directions 'cw', 'ccw' to support it. Can I open another PR for this feature?
  • It does make a lot more sense to include everything in a single plugin for the airship. We will make the necessary changes and update it.
  • We will add descriptive comments, the jinja file instead of the sdf, remove the world file, and fix the things you pointed out.
  • We would also like to add support for simulating wind, we are looking into that.

@antonerasm
Copy link
Contributor

@Jaeyoung-Lim I made the necessary changes as you suggested.
I look forward to your feedback.

@antonerasm
Copy link
Contributor

The following command does not work to start the simulation:
make px4_sitl gazebo_cloudship

This is because of the recent changes to split the models and worlds for sitl_gazebo. The models that are auto-generated with jinja have the suffix -gen.sdf to exclude them from git pushes.
However, in the PX4/Firmware file Tools/sitl_run.sh there is a clear reference to the SDF file Tools/sitl_gazebo/models/${model}/${model}.sdf. This fails for the auto-generated models as it does not include the -gen.sdf suffix.

Any ideas how to fix this?
My suggestion would be to extract the SDF file name in the model.config file. Do you agree? Can I submit a PR in PX4/Firmware to fix this?


double param_hull_volume_;
double param_air_density_;
double param_m11_;
Copy link
Member

Choose a reason for hiding this comment

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

Could you document what each parameter stands for?

Copy link
Contributor

Choose a reason for hiding this comment

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

On lines 56-83 I documented what each of the gazebo plugin parameters are. I thought that would be sufficient. Would you rather that I restate that here for each variable as well? Let me know what you think and I'll make the necessary changes

@antonerasm
Copy link
Contributor

@Jaeyoung-Lim please let me know if there is anything else you would recommend/suggest for the PR?

There is one CI check that is failing. It is related to the macOS build, any advice on this?

@dan-leo
Copy link
Contributor Author

dan-leo commented Jun 9, 2020

I see the same macOS build error showing in latest commit: Fix typhoon h480 internals (#512) ... e2bf962

Copy link
Member

@Jaeyoung-Lim Jaeyoung-Lim left a comment

Choose a reason for hiding this comment

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

This is really cool and thanks for the contribution. The macOS failure is most likely unrelated to this PR

  • I believe the optical fow submodule was unintentionally updated
  • I am a bit concerned on how the reversible thrust is handled. I would prefer if we have a turning direction and a separate reversible(true/false) parameter which makes it more explicit. This is actually more inline on what the px4 mixer is doing.
  • I also think that you need to explicitly specify MAV_TYPE to airship as commented in New vehicle type: Airship PX4-Autopilot#14862 (comment) so that we are sure that the rotor velocity setpoints actually range from -1 ~ + 1 rather than 0 ~ 1
  • Since the MAV_TYPE is called airship in the mavlink standard, wouldn't it be more straight forward to call the dynamics plugin airship_dynamics_plugin rather than lta_dynamics plugin? LTA doesn't seem to be a term that is used in any of the mavlink standard or the px4 ecosystem.
  • Regarding New vehicle type: Airship #490 (comment) please do!

src/gazebo_wind_plugin.cpp Outdated Show resolved Hide resolved
force = std::abs(force);
}

int turning_dir = turning_direction_type_ / std::abs(turning_direction_type_);
Copy link
Member

Choose a reason for hiding this comment

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

Could we maybe not mix turning_direction_type_ and the turning_direction? maybe add another boolean sdf element for "reversible"? I think this matches the mixer definitions better and makes it more explicit.

@@ -233,7 +243,7 @@ void GazeboMotorModel::UpdateForcesAndMoments() {
#else
ignition::math::Pose3d pose_difference = ignitionFromGazeboMath(link_->GetWorldCoGPose() - parent_links.at(0)->GetWorldCoGPose());
#endif
ignition::math::Vector3d drag_torque(0, 0, -turning_direction_ * force * moment_constant_);
ignition::math::Vector3d drag_torque(0, 0, -turning_dir * force * moment_constant_);
Copy link
Member

Choose a reason for hiding this comment

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

My main concern with making the reversible parameter part of the turning direction, is that the reference rotor velocity range are not explicitly defined. (It can range from 01 or -1+1 depending on the MAV_TYPE( https://github.com/PX4/Firmware/blob/master/src/modules/simulator/simulator_mavlink.cpp#L133 ). This will add an additional layer of confusion when something is not working as intended.

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've set MAV_TYPE to 7 (Airship, controlled) only and by sticking to -1..1, that should certainly help.

@@ -42,6 +42,8 @@
namespace turning_direction {
const static int CCW = 1;
const static int CW = -1;
const static int CCW_REVERSABLE = 2;
const static int CW_REVERSABLE = -2;
Copy link
Member

Choose a reason for hiding this comment

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

As I commented in the other part of the code, I think reversible should be a separate state (possibly a boolean)

@Jaeyoung-Lim
Copy link
Member

@dan-leo @antonerasm Could we maybe try to conclude this and try to get it in?

@antonerasm
Copy link
Contributor

@Jaeyoung-Lim Certainly! My apologies for the delay. I will add the latest requests to get this done.

@antonerasm
Copy link
Contributor

@Jaeyoung-Lim I think I have covered everything. I also updated the PR in the Firmware branch to change the MAV_TYPE and the actuator command ranges. Please, let me know if there is anything else.

Copy link
Member

@Jaeyoung-Lim Jaeyoung-Lim left a comment

Choose a reason for hiding this comment

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

This looks good to me,

Could you maybe resolve the conflict so that we can merge? Thanks!

@Jaeyoung-Lim Jaeyoung-Lim merged commit ce1eca2 into PX4:master Jul 19, 2020
RyosukeMatsushima pushed a commit to RyosukeMatsushima/PX4-SITL_gazebo that referenced this pull request Jun 21, 2022
* added cloudship model

* detail edit

* added cloudship model from v1.10.1
- aerodynamics
- bouyancy
- gazebo freezes upon entering

* working cloudship in gazebo
- direct use of -gen.sdf from sdf.jinja

* Update airship hull profile

* Update airship dynamics, add reversable thrust to motor_model, and update airship model with the addition of wind

* Add parameter descriptions, fix Gazebo7 implementation and update aerodynamic forces

* reset pwm input offset

* Update reversible motor model

* Revert accidental submodule OpticalFlow updates

* Update GPS sensor

* Update airship model

Co-authored-by: Anton Erasmus <anton@flycloudline.com>
Co-authored-by: antonerasm <57213348+antonerasm@users.noreply.github.com>
arjo129 pushed a commit to gazebosim/gz-sim that referenced this pull request May 14, 2024
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](PX4/PX4-SITL_gazebo-classic#490) , 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](https://www.windreiter.com/shop/sb-324-300/), 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.


Signed-off-by: henrykotze <henry@flycloudline.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants