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

Plane Guided Alt Command: Use frame instead of bools for setting alt frame #28479

Merged

Conversation

Ryanf55
Copy link
Collaborator

@Ryanf55 Ryanf55 commented Oct 28, 2024

Purpose

When a plane receives an altitude command from mavlink SET_POSITION_TARGET_GLOBAL_INT, we were using the booleans in the Location field to set the frame. This switches to using the altitude setter with the frame which is more explicit.

We now convert the frames with mavlink_coordinate_frame_to_location_alt_frame.

Conveniently, deleting code saves 40b flash.

Tests performed

./Tools/autotest/autotest.py test.Plane.

It seems like this might be covered by SetpointGlobalPos, but that test is not run by Plane.

I also used the changealt and changealt_abs command in GUIDED mode while flying in SITL to verify we can change altitude. Those commands do not use this interface.

I do not have a great way to test this as is. Suggestions welcome!

Size Compare Durandal

Binary Name      Text [B]        Data [B]     BSS (B)      Total Flash Change [B] (%)      Flash Free After PR (B)
---------------  --------------  -----------  -----------  ----------------------------  -------------------------
arducopter-heli  0 (0.0000%)     0 (0.0000%)  0 (0.0000%)  0 (0.0000%)                                      14[28](https://github.com/ArduPilot/ardupilot/actions/runs/11563094404/job/32185663719?pr=28479#step:10:29)72
antennatracker   0 (0.0000%)     0 (0.0000%)  0 (0.0000%)  0 (0.0000%)                                      615404
arducopter       0 (0.0000%)     0 (0.0000%)  0 (0.0000%)  0 (0.0000%)                                      142344
ardurover        0 (0.0000%)     0 (0.0000%)  0 (0.0000%)  0 (0.0000%)                                      [29](https://github.com/ArduPilot/ardupilot/actions/runs/11563094404/job/32185663719?pr=28479#step:10:30)6528
arduplane        -40 (-0.0022%)  0 (0.0000%)  0 (0.0000%)  -40 (-0.0022%)                                   145824
blimp            0 (0.0000%)     0 (0.0000%)  0 (0.0000%)  0 (0.0000%)                                      593368
ardusub          0 (0.0000%)     0 (0.0000%)  0 (0.0000%)  0 (0.0000%)                                      352236

@Ryanf55 Ryanf55 requested a review from IamPete1 October 28, 2024 02:00
@Ryanf55
Copy link
Collaborator Author

Ryanf55 commented Oct 28, 2024

@IamPete1 Mind taking a look? This is starting to move towards treating altitude frame booleans as private.

It will make this PR smaller: #26328 (comment)

@Ryanf55 Ryanf55 force-pushed the plane-gcs-mavlink-alt-frame-constructor branch from d4bbf2f to 8b15739 Compare October 28, 2024 02:28
@Ryanf55
Copy link
Collaborator Author

Ryanf55 commented Oct 28, 2024

Switched to mavlink_coordinate_frame_to_location_alt_frame , now there's much less code. changealt still works.

Copy link
Contributor

@peterbarker peterbarker left a comment

Choose a reason for hiding this comment

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

Please outline what testing you have done on this PR.

ArduPlane/GCS_Mavlink.cpp Outdated Show resolved Hide resolved
@Ryanf55
Copy link
Collaborator Author

Ryanf55 commented Oct 28, 2024

Please outline what testing you have done on this PR.

I did in the description. Do you want me to upload logs, copy the exact terminal output, a video, or something more specific?

@Ryanf55
Copy link
Collaborator Author

Ryanf55 commented Oct 28, 2024

I'm working on adding an autotest in master to preserve existing behavior. Once that's in, we can revisit this PR.

The test methodology I used for this was not hitting the API I thought it was and I want this checked in CI before touching it.

See #28487

@Ryanf55 Ryanf55 marked this pull request as draft October 28, 2024 21:34
* And switch to mavlink_coordinate_frame_to_location_alt_frame

Signed-off-by: Ryan Friedman <[email protected]>
@Ryanf55 Ryanf55 force-pushed the plane-gcs-mavlink-alt-frame-constructor branch from 8b15739 to 02b497f Compare October 28, 2024 21:39
@peterbarker
Copy link
Contributor

So PH and I weren't going to scope creep you on that bit :-)

OTOH, now that you've started - why not early-return on the alt-mask check too? It's going to be a nice function after that.

@Ryanf55
Copy link
Collaborator Author

Ryanf55 commented Oct 29, 2024

So PH and I weren't going to scope creep you on that bit :-)

OTOH, now that you've started - why not early-return on the alt-mask check too? It's going to be a nice function after that.

If you have a way to easily test it and are comfortable, maybe we skip CI 🤣

The reason I don't early return on the alt mask is there will be other masks to test against coming up. The alt can be controlled in the upcoming path guidance mode at the same time as other fields in this function.

@Ryanf55 Ryanf55 marked this pull request as ready for review October 29, 2024 14:53
@Ryanf55 Ryanf55 changed the title Use frame instead of bools for setting alt frame Plane Guided Alt Command: Use frame instead of bools for setting alt frame Oct 29, 2024
Copy link
Member

@IamPete1 IamPete1 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 correct to me.

@tridge tridge merged commit 75af2d8 into ArduPilot:master Nov 5, 2024
68 checks passed
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.

6 participants