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.

Sign up for GitHub

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

Jump to bottom

Refactor ManualPositionSmoothVel and ManualAltitudeSmoothVel #12943

Merged
merged 7 commits into from
Sep 19, 2019
Merged

Refactor ManualPositionSmoothVel and ManualAltitudeSmoothVel #12943

merged 7 commits into from
Sep 19, 2019

Conversation

bresch
Copy link
Member

@bresch bresch commented Sep 11, 2019 •
edited
Loading

With #12909 , we can get rid of a lot of corner case handling logic and I decided to refactor completely the FlightTaskManualPositionSmoothVel and FlightTaskManualAltitudeSmoothVel to make them cleaner, easier to read and bug-free.
To do that, I'll encapsulate the usage of the jerk-limited trajectory generator with the position lock logic into an object that can be used by both flight tasks. Being in a new class, I'll also have unit test for it.

First step I made so far is to split the logic in small functions and decouple XY and Z axes. Next step is to move that logic into a class. There will be a base pure virtual interface class, a class for XY and one for Z, where XY is the vector version of Z.

  • Extract XY and Z classes
  • Update PositionSmoothVel
  • Update AltitudeSmoothVel
  • Write unit tests (well.. I started at least)

@bresch bresch changed the title Refactor ManualPositionSmoothVel and ManualAltitudeSmoothVel [WIP] Refactor ManualPositionSmoothVel and ManualAltitudeSmoothVel Sep 11, 2019
@bresch bresch force-pushed the dev-tasksmoothvel-refactor branch 2 times, most recently from 2ec4ca2 to 1b880a5 Compare September 12, 2019 13:44
@bresch bresch requested a review from MaEtUgR September 12, 2019 13:51
@bresch bresch self-assigned this Sep 12, 2019
@weekly-digest weekly-digest bot mentioned this pull request Sep 15, 2019
Closed
@bresch bresch force-pushed the dev-tasksmoothvel-refactor branch from 1b880a5 to 1d19082 Compare September 17, 2019 07:11
@bresch
Copy link
Member Author

bresch commented Sep 17, 2019

Rebased on #12909 .

@bresch bresch force-pushed the dev-tasksmoothvel-refactor branch 2 times, most recently from 2fa0ae2 to 92fbaac Compare September 17, 2019 08:49
@bresch
Copy link
Member Author

bresch commented Sep 17, 2019

Rebased on master after the merge of #12909

bresch
bresch commented Sep 17, 2019
View reviewed changes
src/systemcmds/tests/test_matrix.cpp Outdated
@@ -578,7 +578,7 @@ bool MatrixTest::sliceTests()
};

Matrix<float, 2, 2> C(data_2);
A.set(C, 1, 1);
Copy link
Member Author

Choose a reason for hiding this comment

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

FYI @jkflying
See also the other modifications in this file.

@bresch bresch changed the title [WIP] Refactor ManualPositionSmoothVel and ManualAltitudeSmoothVel Refactor ManualPositionSmoothVel and ManualAltitudeSmoothVel Sep 17, 2019
@bresch bresch force-pushed the dev-tasksmoothvel-refactor branch from 92fbaac to bdd2934 Compare September 17, 2019 13:30
@bresch
Copy link
Member Author

bresch commented Sep 17, 2019

@PX4/testflights Could you test this PR in -at least- attitude and position modes please?

  • Altitude mode:
    • fly around, change altitude
    • up and down motion, wait sometimes a few seconds between motions
  • Position control:
    • fly around
    • change altitude
    • start and stop
    • hold position for 2-3 minutes

Thanks!

@bresch
Copy link
Member Author

bresch commented Sep 17, 2019 •
edited
Loading

Status:

@dannyfpv
Copy link

dannyfpv commented Sep 17, 2019 •
edited
Loading

Tested on Pixhawk 4 v5 f-450

Altitude mode:
flew around & did the changing altitude, al smooth no issues noticed.
Up and down motion, waited sometimes a few seconds between motions.

log:
https://review.px4.io/plot_app?log=80457d44-990b-4b4d-92d6-568fe7d11ea6

Position control:
flew around change altitude, start and stop hold the position for 2-3 minutes, no issues noted.

log:
https://review.px4.io/plot_app?log=993aa988-9843-4701-a6ac-2bb82478f970

pixhawk4 mini v5 f-450

Altitude mode:
flew around & did the changing altitude, al smooth no issues noticed.
Up and down motion, waited sometimes a few seconds between motions.

log:
https://review.px4.io/plot_app?log=3c22fc65-4975-42f4-a02b-2fd7b107475b

Position control:
flew around change altitude, start and stop hold the position for 2-3 minutes, no issues noted.

log:
https://review.px4.io/plot_app?log=a5963d76-6278-460c-9a41-7431946c8d24

@jorge789
Copy link

jorge789 commented Sep 17, 2019 •
edited by Tony3dr
Loading

Tested on CUAV+ V5:

Altitude mode:
fly around, change altitude
up and down motion, wait sometimes a few seconds between motions

log:
https://review.px4.io/plot_app?log=b6f62c4a-1260-468c-93fd-90e84d3b0dc3

Position control:
fly around
change altitude
start and stop
hold position for 2-3 minutes

log:
https://review.px4.io/plot_app?log=e8ffaa51-c4b6-4063-897c-662cf48f3382

Tested on PixRacer V4:

Altitude mode:
fly around, change altitude
up and down motion, wait sometimes a few seconds between motions

log:
https://review.px4.io/plot_app?log=60fed4c6-4d91-436c-b636-3280853bc97c

Position control:
fly around
change altitude
start and stop
hold position for 2-3 minutes

log:
https://review.px4.io/plot_app?log=85e9741a-8572-464e-81ac-c303e72275dc

@bresch
Copy link
Member Author

bresch commented Sep 18, 2019

@dannyfpv @jorge789 Thanks, I'll have a look at the logs.

@bresch
ManualPositionSmoothVel - Split large function into smaller ones, spl…
b3e9c06 
…it XY and Z axes

Next step is to move as much as possible to a library in order to
reuse the Z axis in the Altitude FlightTask
@bresch
Refactor FlightTaskManualPositionSmoothVel
8dee468
@bresch
ManualVelocitySmoothingXY - Add simple unit test
d5e5313
@bresch
ManualVelocitySmoothing - Cosmetic changes (renaming, indentation,..)
c6c4f71 
Set the classes as final and set destructor to non-virtual
@bresch
AltitudeSmoothVel - Use ManualVelocitySmoothingZ class instead of re-…
29b2ee9 
…implementing the logic
@bresch
Matrix - Explicitly cast array[3] to Vector3f
4dac21c 
Use .xy() for Vector2 -> Vector3 assignment
@bresch bresch force-pushed the dev-tasksmoothvel-refactor branch from bdd2934 to 754073b Compare September 19, 2019 08:30
@bresch
Copy link
Member Author

bresch commented Sep 19, 2019

@MaEtUgR It's all ready now and tested quite a bit. The logs looks fine and I just need a quick review and approval please.

MaEtUgR
MaEtUgR approved these changes Sep 19, 2019
View reviewed changes
Copy link
Member

@MaEtUgR MaEtUgR left a comment

Choose a reason for hiding this comment

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

Nice refactor! A lot more structured.

@bresch bresch merged commit 6f3868b into master Sep 19, 2019
@bresch bresch deleted the dev-tasksmoothvel-refactor branch September 19, 2019 15:31
@weekly-digest weekly-digest bot mentioned this pull request Sep 22, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MaEtUgR MaEtUgR MaEtUgR approved these changes

Assignees

@bresch bresch

Labels
Admin: Cleanup (housekeeping) 🧹 Admin: Enhancement (improvement) 💡 Multirotor 🛸
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bresch @dannyfpv @jorge789 @MaEtUgR