Lift Drag Bug Fix

[frede791]

🦟 Bug fix

Fixes #2188

Summary

This PR fixes a bug in the liftdrag plugin that prevented the moment coefficient (cm) from being correctly calculated. The reason for this bug is an mathematical error that calculated cos^2 rather than cos for the cosSweepAngle. Using this updated value, I added in missing calculations from the liftdrag plugin in gazebo-classic, so that the two are now identical in functionality. I also took the opportunity to update the header file to the current formatting and fixed a spelling mistake I found. I also added in the cm_delta parameter that was not present in the current version of the liftdrag plugin but is used by some drones (such as the tailsitter) and may thus find use in the future.

In addition, wind has also been added as a parameter to be considered in calculations, bringing in line with what is present in the gazebo-classic lift drag plugin.

Checklist

• Signed all commits for DCO
• Updated documentation (as needed)

[frede791]

Added in the wind velocity as a factor from the original liftdrag plugin.

[arjo129]

Run make codecheck .

See here: https://gazebosim.org/docs/harmonic/contributing

Remove trailing spaces as pointed out by the linter here: https://github.com/gazebosim/gz-sim/actions/runs/6396048528/job/17419401043?pr=2189

[frede791]

Removed long lines and fixed trailing white spaces.

[arjo129]

Currently this plugin causes a segfault in the tests - I think its because you access components without checking if they exist.
To test your plugin doesnt break current integration tests run the following steps:

• colcon build
• . install/setup.bash
• run build/gz-sim7/bin/INTEGRATION_lift_drag_system

[frede791]

Improved based on comments. Integration tests and codecheck both pass. Commented feedback needs a reply if possible.

[arjo129]

Thanks for iterating on this so quickly. LGTM!

[srmainwaring]

@ahcorde, I did not spot this change before it was merged into Harmonic, but have now tested with some of the models set up for ArduPilot and have some questions:

Sweep angle calculation

I think the calculation for the sweep angle adjustment should not include the square root. In the original Gazebo classic version of the plugin the variable was named cosSweepAngle2 (i.e. squared).

• https://github.com/gazebosim/gazebo-classic/blob/7af0e34ff724b3447f85eba039f596cf6895aa8e/plugins/LiftDragPlugin.cc#L150C10-L150C24

It was change to cosSweepAngle in:

• gazebosim/gazebo-classic@9b98cb3#diff-66d0f4a4af1ce5c7c5c7cffb63821f5a1aa944650b15be8e0ddcd44ee33e6603L167

I cannot find the PR corresponding to the change, so it's not clear why it was renamed.

The argument for using the square of the angle is that the sweep angle correction arises from using the velocity normal to the leading edge of the wing in the lift calculation, rather than the free stream velocity. As the velocity appears quadratically in the dynamic pressure the correction requires cos(sweep_angle)^2 not cos(sweep_angle). The adjustment is then applied as a correction to the lift coefficient rather than the velocity used in the dynamic pressure calculation which is possibly where the confusion arises, as it appears that it's taking a component of the resultant force, not the input velocity.

Unfortunately there is no reference provided to the source of the calculations used in the original derivation of the plugin, so it's hard to confirm the formula forming the basis for the calculations.

Moment calculation

This calculation is not enabled in the Gazebo classic version:

• https://github.com/gazebosim/gazebo-classic/blob/7ccef40e24831eb5cd974b489ee279fc064eacc4/plugins/LiftDragPlugin.cc#L346

There is not much explanation aside from it not being checked. I think this term is supposed to account for the movement of the centre of pressure with velocity, and allows the lift and drag coefficients to be calculated and applied at the quarter chord point rather than the unknown centre of pressure (so the cp parameter might be better named quarter chord?).

In any case there are a few models that will not work with this calculation enabled. The Zephyr is one of them: https://app.gazebosim.org/OpenRobotics/fuel/models/Zephyr%20Delta%20Wing. The solution to get this model working again is to zero <cma>0.0</cma> so the adjustment is zero. It should not affect any PX4 mdoels as AFAICT they all set the parameter to zero. The models used with ArduPilot are being updated to also zero this adjustment (which is backwards compatible with Garden).

An announcement that this change may break some models would be helpful.

[arjo129]

Hi Rhys, Thanks for pointing that out. It seems my review fell short of certain checks. I think it seems like this PR has introduced a regression. For moment calculations, the easiest thing to solve the incompatibilities is to default cma to zero. We can revert the change that added a regression to the cos term. On another note perhaps we should not allow any changes into existing plugins on the release branches. i,e A change like this should target `main` so that errors get caught during the tutorial party.

…

On Fri, Dec 22, 2023 at 12:16 AM Rhys Mainwaring ***@***.***> wrote: @ahcorde <https://github.com/ahcorde>, I did not spot this change before it was merged into Harmonic, but have now tested with some of the models set up for ArduPilot and have some questions: Sweep angle calculation I think the calculation for the sweep angle adjustment should not include the square root. In the original Gazebo classic version of the plugin the variable was named cosSweepAngle2 (i.e. squared). - https://github.com/gazebosim/gazebo-classic/blob/7af0e34ff724b3447f85eba039f596cf6895aa8e/plugins/LiftDragPlugin.cc#L150C10-L150C24 It was change to cosSweepAngle in: - ***@***.*** #diff-66d0f4a4af1ce5c7c5c7cffb63821f5a1aa944650b15be8e0ddcd44ee33e6603L167 <gazebosim/gazebo-classic@9b98cb3#diff-66d0f4a4af1ce5c7c5c7cffb63821f5a1aa944650b15be8e0ddcd44ee33e6603L167> I cannot find the PR corresponding to the change, so it's not clear why it was renamed. The argument for using the square of the angle is that the sweep angle correction arises from using the velocity normal to the leading edge of the wing in the lift calculation, rather than the free stream velocity. As the velocity appears quadratically in the dynamic pressure the correction requires cos(sweep_angle)^2 not cos(sweep_angle). The adjustment is then applied as a correction to the lift coefficient rather than the velocity used in the dynamic pressure calculation which is possibly where the confusion arises, as it appears that it's taking a component of the resultant force, not the input velocity. Unfortunately there is no reference provided to the source of the calculations used in the original derivation of the plugin, so it's hard to confirm the formula forming the basis for the calculations. Moment calculation This calculation is not enabled in the Gazebo classic version: - https://github.com/gazebosim/gazebo-classic/blob/7ccef40e24831eb5cd974b489ee279fc064eacc4/plugins/LiftDragPlugin.cc#L346 There is not much explanation aside from it not being checked. I think this term is supposed to account for the movement of the centre of pressure with velocity, and allows the lift and drag coefficients to be calculated and applied at the quarter chord point rather than the unknown centre of pressure (so the cp parameter might be better named quarter chord?). In any case there are a few models that will not work with this calculation enabled. The Zephyr is one of them: https://app.gazebosim.org/OpenRobotics/fuel/models/Zephyr%20Delta%20Wing. The solution to get this model working again is to zero <cma>0.0</cma> so the adjustment is zero. It should not affect any PX4 mdoels as AFAICT they all set the parameter to zero. The models used with ArduPilot are being updated to also zero this adjustment (which is backwards compatible with Garden). An announcement that this change may break some models would be helpful. — Reply to this email directly, view it on GitHub <#2189 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEEMQDC4ZX4XZ56HCGCKZDYKROFXAVCNFSM6AAAAAA5Q5WX4SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNRWGU4DSOJUGY> . You are receiving this because you commented.Message ID: ***@***.***>

[srmainwaring]

On another note perhaps we should not allow any changes into existing plugins on the release branches. i.e. A change like this should target main so that errors get caught during the tutorial party

Maybe, although in this case the changes looked reasonable because of the variable naming. Btw I'm still not 100% certain of the intended sweep angle calculation. If someone has a good reference that would settle it that would be great. From what I understand the original motivation for swept wings arose to reduce the effective stream velocity over the wings in transonic / supersonic flight (so argument for taking components of the velocity). Either way a comment above the calculation about the intention would make things clearer, and we could revert the variable name back to it's original cosSweepAngle2 if we decide that's the correct value. It does affect the lift and drag forces as well as the moment, so there should be a test to catch that.

<cma> does default to zero, the regression occurs for models where it is set to a non-zero value (overriding the default). In Garden the parameter was ignored, in Harmonic it isn't. I don't think it affects many models, as it appears in most cases to be set to zero anyway.

[arjo129]

No <cma> currently does not default to zero. And our documentation does not seem to have been updated for it.

I dont know about cos vs cos2 @frede791 is there a reason why you'd use cos over cos2 otherwise I'll revert that change.

[srmainwaring]

No <cma> currently does not default to zero.

You're right - I misread that. Thanks for the the PR defaulting <cma> to zero, that'll provide consistency between major versions.

[srmainwaring]

I dont know about cos vs cos2 @frede791 is there a reason why you'd use cos over cos2 otherwise I'll revert that change.

Reference for simple sweep theory: https://web.archive.org/web/20060727083150/http://www.desktopaero.com/appliedaero/potential3d/sweeptheory.html

Should relabel variable to cosSweepAngle2 and revert change.

[frede791]

@arjo129 Thanks for catching this.