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

Clean up Transition Flighttask #24126

Merged
merged 3 commits into from
Dec 19, 2024
Merged

Conversation

MaEtUgR
Copy link
Member

@MaEtUgR MaEtUgR commented Dec 18, 2024

Solved Problem

Follow-up to #23731

When looking for flash savings I realized flight task transition now uses 1.5+ kilobytes of flash which is completely useless on flash-constrained boards that do not support VTOLs so I thought about removing the task from flash constrained builds but we don't have a mechanism to enforce that no one will ever build a target with VTOL support but no transition flight task then.

After investigating this I was wondering why the transition task is so big now and cleaned up the logic while going through it.

Solution

First commit should be refactor as in:

  • the parameters do not update during a transition anymore
  • the fixed-wing parameters not being available gets handled slightly different by the param_get() function
  • parameter variables named after the parameter following the convention
  • there's no duplicate update of local position with separate evaluation
  • the activation is not using an invalid deltatime and applying the initialization values anymore
  • use the new AlphaFilter interface from AlphaFilter: set dt during update #24116
  • the transition task runs by definition only during a transition so the check for that flag is obsolete
  • clear separation of constants
  • simplify structure, reducing scope, naming, comments

The only intentional behavior change:

  • The maximum deceleration of 2 * VT_B_DEC_MSS is kept also when overshooting the transition target position

Changelog Entry

Bugfix: Keep high deceleration when overshooting transition target position

Test coverage

Honestly I cannot test my changes since I don't know how it should look like. @sfuhrer would you be able to verify it still works as intended?

@MaEtUgR MaEtUgR requested a review from sfuhrer December 18, 2024 15:14
@MaEtUgR MaEtUgR self-assigned this Dec 18, 2024
Copy link

github-actions bot commented Dec 18, 2024

🔎 FLASH Analysis

px4_fmu-v5x [Total VM Diff: -424 byte (-0.02 %)]
    FILE SIZE        VM SIZE    
--------------  -------------- 
+0.0%     +65  [ = ]       0    .debug_abbrev
   +11%     +56  [ = ]       0    ../../src/lib/version/version.c
  +0.3%      +9  [ = ]       0    ../../src/modules/flight_mode_manager/tasks/Transition/FlightTaskTransition.cpp
-0.0%     -72  [ = ]       0    .debug_aranges
  -5.0%      -8  [ = ]       0    ../../src/lib/version/version.c
 -20.5%     -64  [ = ]       0    ../../src/modules/flight_mode_manager/tasks/Transition/FlightTaskTransition.cpp
-0.0%    -216  [ = ]       0    .debug_frame
-0.0% -2.57Ki  [ = ]       0    .debug_info
  -0.2%      -4  [ = ]       0    ../../src/lib/version/version.c
  -3.3% -2.56Ki  [ = ]       0    ../../src/modules/flight_mode_manager/tasks/Transition/FlightTaskTransition.cpp
-0.0%   -1005  [ = ]       0    .debug_line
  -1.3%     -25  [ = ]       0    ../../src/lib/version/version.c
 -13.5%    -981  [ = ]       0    ../../src/modules/flight_mode_manager/tasks/Transition/FlightTaskTransition.cpp
  +0.1%      +1  [ = ]       0    task/task_cancelpt.c
-0.0% -2.26Ki  [ = ]       0    .debug_loc
  -0.2%     -15  [ = ]       0    ../../src/modules/flight_mode_manager/FlightModeManager.cpp
  -5.8%    -590  [ = ]       0    ../../src/modules/flight_mode_manager/tasks/Transition/FlightTaskTransition.cpp
  -0.5% -1.65Ki  [ = ]       0    [section .debug_loc]
  -0.1%     -15  [ = ]       0    src/modules/flight_mode_manager/FlightTasks_generated.cpp
-0.0%    -481  [ = ]       0    .debug_ranges
  -2.6%      -8  [ = ]       0    ../../src/lib/version/version.c
  -6.8%    -136  [ = ]       0    ../../src/modules/flight_mode_manager/tasks/Transition/FlightTaskTransition.cpp
  -0.7%    -336  [ = ]       0    [section .debug_ranges]
  -1.5%      -1  [ = ]       0    task/task_cancelpt.c
-0.0%    -590  [ = ]       0    .debug_str
  -0.7%      -8  [ = ]       0    ../../src/modules/commander/level_calibration.cpp
  -0.5%     -14  [ = ]       0    ../../src/modules/flight_mode_manager/tasks/Auto/FlightTaskAuto.cpp
 -35.1%    -568  [ = ]       0    ../../src/modules/flight_mode_manager/tasks/Transition/FlightTaskTransition.cpp
+0.5%      +1  [ = ]       0    .shstrtab
-0.0%    -329  [ = ]       0    .strtab
  -8.1%     -32  [ = ]       0    ../../src/lib/version/version.c
 -45.7%    -329  [ = ]       0    ../../src/modules/flight_mode_manager/tasks/Transition/FlightTaskTransition.cpp
  +0.0%     +32  [ = ]       0    [section .strtab]
-0.0%    -208  [ = ]       0    .symtab
  -7.0%     -64  [ = ]       0    ../../src/lib/version/version.c
 -36.1%    -208  [ = ]       0    ../../src/modules/flight_mode_manager/tasks/Transition/FlightTaskTransition.cpp
  +0.3%     +16  [ = ]       0    ../../src/modules/fw_pos_control/FixedwingPositionControl.cpp
  +0.1%     +48  [ = ]       0    [section .symtab]
+2.6%    +424  [ = ]       0    [Unmapped]
-0.0%    -424  -0.0%    -424    .text
  +0.1%      +4  +0.1%      +4    ../../src/modules/flight_mode_manager/tasks/Auto/FlightTaskAuto.cpp
 -28.5%    -428 -28.5%    -428    ../../src/modules/flight_mode_manager/tasks/Transition/FlightTaskTransition.cpp
-0.0% -7.59Ki  -0.0%    -424    TOTAL

px4_fmu-v6x [Total VM Diff: -408 byte (-0.02 %)]
    FILE SIZE        VM SIZE    
--------------  -------------- 
+0.0%     +65  [ = ]       0    .debug_abbrev
   +11%     +56  [ = ]       0    ../../src/lib/version/version.c
  +0.3%      +9  [ = ]       0    ../../src/modules/flight_mode_manager/tasks/Transition/FlightTaskTransition.cpp
-0.0%     -72  [ = ]       0    .debug_aranges
  -5.0%      -8  [ = ]       0    ../../src/lib/version/version.c
 -20.5%     -64  [ = ]       0    ../../src/modules/flight_mode_manager/tasks/Transition/FlightTaskTransition.cpp
-0.0%    -216  [ = ]       0    .debug_frame
-0.0% -2.57Ki  [ = ]       0    .debug_info
  -0.2%      -4  [ = ]       0    ../../src/lib/version/version.c
  -3.4% -2.56Ki  [ = ]       0    ../../src/modules/flight_mode_manager/tasks/Transition/FlightTaskTransition.cpp
-0.0%   -1005  [ = ]       0    .debug_line
  -1.3%     -25  [ = ]       0    ../../src/lib/version/version.c
 -13.5%    -981  [ = ]       0    ../../src/modules/flight_mode_manager/tasks/Transition/FlightTaskTransition.cpp
  +0.1%      +1  [ = ]       0    task/task_cancelpt.c
-0.0% -2.26Ki  [ = ]       0    .debug_loc
  -0.2%     -15  [ = ]       0    ../../src/modules/flight_mode_manager/FlightModeManager.cpp
  -5.8%    -590  [ = ]       0    ../../src/modules/flight_mode_manager/tasks/Transition/FlightTaskTransition.cpp
  -0.5% -1.65Ki  [ = ]       0    [section .debug_loc]
  -0.1%     -15  [ = ]       0    src/modules/flight_mode_manager/FlightTasks_generated.cpp
-0.0%    -480  [ = ]       0    .debug_ranges
  -2.6%      -8  [ = ]       0    ../../src/lib/version/version.c
  -6.8%    -136  [ = ]       0    ../../src/modules/flight_mode_manager/tasks/Transition/FlightTaskTransition.cpp
  -0.7%    -336  [ = ]       0    [section .debug_ranges]
-0.0%    -607  [ = ]       0    .debug_str
  -0.7%      -8  [ = ]       0    ../../src/modules/commander/level_calibration.cpp
  -0.5%     -14  [ = ]       0    ../../src/modules/flight_mode_manager/tasks/Auto/FlightTaskAuto.cpp
 -35.8%    -585  [ = ]       0    ../../src/modules/flight_mode_manager/tasks/Transition/FlightTaskTransition.cpp
+0.5%      +1  [ = ]       0    .shstrtab
-0.0%    -329  [ = ]       0    .strtab
  -8.1%     -32  [ = ]       0    ../../src/lib/version/version.c
 -45.7%    -329  [ = ]       0    ../../src/modules/flight_mode_manager/tasks/Transition/FlightTaskTransition.cpp
  +0.0%     +32  [ = ]       0    [section .strtab]
-0.0%    -208  [ = ]       0    .symtab
 -14.3%     -16  [ = ]       0    ../../platforms/common/uORB/SubscriptionInterval.cpp
  -7.0%     -64  [ = ]       0    ../../src/lib/version/version.c
  +2.1%     +16  [ = ]       0    ../../src/modules/flight_mode_manager/tasks/Orbit/FlightTaskOrbit.cpp
 -35.1%    -208  [ = ]       0    ../../src/modules/flight_mode_manager/tasks/Transition/FlightTaskTransition.cpp
  +0.3%     +16  [ = ]       0    ../../src/modules/fw_pos_control/FixedwingPositionControl.cpp
  +0.1%     +48  [ = ]       0    [section .symtab]
+0.5%    +408  [ = ]       0    [Unmapped]
-0.0%    -408  -0.0%    -408    .text
  +0.0%     +13  +0.0%     +13    [section .text]
  +0.1%      +4  +0.1%      +4    ../../src/modules/flight_mode_manager/tasks/Auto/FlightTaskAuto.cpp
  +0.2%      +3  +0.2%      +3    ../../src/systemcmds/ver/ver.cpp
 -28.5%    -428 -28.5%    -428    ../../src/modules/flight_mode_manager/tasks/Transition/FlightTaskTransition.cpp
-0.0% -7.61Ki  -0.0%    -408    TOTAL

Updated: 2024-12-19T14:27:10

Copy link
Contributor

@sfuhrer sfuhrer left a comment

Choose a reason for hiding this comment

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

Nice cleanup, impressive that that saved 400 bytes.

I only have one comment, and otherwise looked at the code plus tried to break it in simulation without success.

@MaEtUgR MaEtUgR force-pushed the maetugr/flight-task-transition-cleanup branch from 54da80c to bb31767 Compare December 19, 2024 14:21
Copy link
Contributor

@sfuhrer sfuhrer 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 initiative and diligent work!

@sfuhrer sfuhrer merged commit 1239f0a into main Dec 19, 2024
59 of 60 checks passed
@sfuhrer sfuhrer deleted the maetugr/flight-task-transition-cleanup branch December 19, 2024 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants