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

Nav rotorcraft rework #2964

Merged

Conversation

gautierhattenberger
Copy link
Member

This is a complete rework of the navigation for rotorcraft with the key features:

  • all nav functions and interface in float
  • nav function are registered (decoupling between nav API and implementation of standard pattern)
  • submodes to specify if setpoint is a position (legacy), speed or accel (better integration of algorithms like GVF)
  • guidance (H and V) is reorganized with default function to implement (run_pos, run_speed, run_accel)
  • guidance control (the old default PID) is separated from the guidance logic

Still some questions ahead:

  • hybrid and INDI are still tightly coupled with guidance (with compile flag) while it should be done with the new interface
  • the stabilization is also tightly coupled with guidance, it should be separated
  • the new nav API is inspired from rover nav, and some code could be merged in the future

@EwoudSmeur @fvantienen What is your opinion about this approach ? Do you see a proper way to integrate the indi/hybrid in a with this ?
I would like to split the stabilization from guidance, but maybe it is a bit to much for a single pull request ?

@OpenUAS
Copy link
Contributor

OpenUAS commented Jan 28, 2023

Thx, nice development. @gautierhattenberger @Fabien-B Small remark, It would be great if the testdrone color is not the same green as the expected track, can you make it Pink, Yellow or Reddish, or Cyan please.

Copy link
Member

@EwoudSmeur EwoudSmeur left a comment

Choose a reason for hiding this comment

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

Nice work Gautier! I think it was a great idea to bring a bit more structure to the guidance. I had a few questions looking through the code, but from what I could see it all made sense. It's hard to keep track of all the changes, as some things were just moved. I'll try to do some simulation tests soon!

@gautierhattenberger gautierhattenberger marked this pull request as ready for review February 25, 2023 17:42
EwoudSmeur
EwoudSmeur previously approved these changes Feb 27, 2023
@gautierhattenberger
Copy link
Member Author

@EwoudSmeur after testing again in simulation, I realized that the take_heading_control was not working as expected. So I found a fix that is also removing the HEADING_IS_FREE flag. Now the heading setpoint is always set by nav (in NAV mode), but the (hybrid) guidance may use it or not for the final stab setpoint (for which an internal variable is used for the "free" heading).


<dl_setting var="cruise_throttle" MIN="0" STEP="1" MAX="9600" shortname="cruis_throttle"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Cruise Throttle

Copy link
Member Author

Choose a reason for hiding this comment

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

I have removed the shortname instead of correcting the spelling since it was the same than name (so it makes no difference in the end)

@fvantienen
Copy link
Member

After maybe @EwoudSmeur has checked it we can merge it.
This PR should close #2938

@@ -9,7 +9,7 @@
<settings>
<dl_settings>
<dl_settings NAME="guidance_indi_hybrid">
<dl_setting var="gih_params.liftd_p50" min="0.1" step="0.1" max="10.0" shortname="liftd_p50" param="GUIDANCE_INDI_LIFTD_P50" persistent="true"/>
<dl_setting var="gih_params.liftd_p50" min="0.1" step="0.1" max="10.0" shortname="liftd_p50" param="GUIDANCE_INDI_LIFTD_P50" persistent="true" module="guidance/guidance_indi_hybrid"/>
Copy link
Member

Choose a reason for hiding this comment

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

Why is there a module added here?

Copy link
Member Author

Choose a reason for hiding this comment

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

The module attribute is a legacy prior to the "modules" we have now. It can be replaced by header and it was used to specified the header file for settings.
We will clean that someday...

conf/modules/guidance_pid_rotorcraft.xml Show resolved Hide resolved
&(stateGetPositionEnu_i()->x),
&(stateGetPositionEnu_i()->y),
&(stateGetPositionEnu_i()->z),
&pos->x,
Copy link
Member

Choose a reason for hiding this comment

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

If our navigation is in floats does it still make sense to send it down in integers? (Maybe something for later to update)

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, indeed, but it is more related to redefining most messages, so it is an other job

@gautierhattenberger gautierhattenberger merged commit 28ef30e into paparazzi:master Mar 2, 2023
@gautierhattenberger gautierhattenberger deleted the nav_rotorcraft_rework branch March 2, 2023 21:15
FlorianSan pushed a commit to enacuavlab/paparazzi that referenced this pull request May 30, 2023
This is a complete rework of the navigation for rotorcraft with the key features:

- all nav functions and interface in float
- nav function are registered (decoupling between nav API and implementation of standard pattern)
- submodes to specify if setpoint is a position (legacy), speed or accel (better integration of algorithms like GVF)
- guidance (H and V) is reorganized with default function to implement (run_pos, run_speed, run_accel) 
- guidance control (the old default PID) is separated from the guidance logic

---------

Co-authored-by: Freek van Tienen <freek.v.tienen@gmail.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