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

create mc_rate_control (split out of mc_att_control) #12650

Merged
merged 3 commits into from
Nov 19, 2019

Conversation

dagar
Copy link
Member

@dagar dagar commented Aug 6, 2019

Testing

  • MC acro
  • MC stabilized/manual
  • MC rattitude
  • VTOL tailsitter
  • VTOL standard
  • MC landing gear

@dagar dagar requested review from bresch and MaEtUgR August 6, 2019 18:27
@dagar dagar added this to the Release v1.10.0 milestone Aug 6, 2019
@dagar dagar force-pushed the pr-mc_rate_control branch 2 times, most recently from 49f678b to 5056af4 Compare August 7, 2019 00:57
@dagar dagar marked this pull request as ready for review August 7, 2019 00:57
@dagar dagar force-pushed the pr-mc_rate_control branch 5 times, most recently from a2fc430 to 396712e Compare August 7, 2019 04:46
@dagar
Copy link
Member Author

dagar commented Aug 7, 2019

As a single diff this is probably rather hard to review, however the majority of it was simply duplicating mc_att_control and gutting code on each side. Besides that the only real change is slightly different logic in the now standalone mc_rate_control for when to not run.

@dagar dagar requested a review from a team August 7, 2019 14:44
MaEtUgR
MaEtUgR previously approved these changes Aug 8, 2019
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.

Everything I saw while scanning the code is in my opinion a huge leap in the right direction. I don't see a thing that breaks but I also couldn't study every single detail. It doesn't solve all problems but it doesn't have to but it makes it a lot easier and hence attractive for further improvements.

We should do some testing especially also of the acro, stabilized, rattitude modes.

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.

I started looking into details but it's too much to look through right now.

src/lib/drivers/accelerometer/CMakeLists.txt Outdated Show resolved Hide resolved
src/modules/mc_att_control/CMakeLists.txt Outdated Show resolved Hide resolved
@jorge789
Copy link

jorge789 commented Aug 8, 2019

Tested on PixRacer V4:
Modes Tested

Position Mode: Good.
Altitude Mode: Good.
Stabilized Mode: Vehicle did not respond to Yaw command.
Mission Plan Mode (Automated): Good.
RTL: Good.

- Procedure
Arm and Take off in position mode, after flying for approximately one minute, switched to altitude then stabilized mode proceed to switch to mission plan mode then make sure that vehicle follows all waypoints as shown in QGC, once completed all waypoin activate RTL and see landing behaviour.

Notes:
Vehicle failed to responde Yaw commands on stabilized mode (This happened only in PR12650 Firmware).

Log:
https://review.px4.io/plot_app?log=47a32527-fc4e-4297-a1d3-ae4bb3cec54e

MASTER COMPARISON:
https://review.px4.io/plot_app?log=3ac7de57-3964-4614-bbcb-e904e2e0251b

Tested on CUAV V5+:
Modes Tested

Position Mode: Good.
Altitude Mode: Good.
Stabilized Mode: Vehicle did not respond to Yaw command.
Mission Plan Mode (Automated): Good.
RTL: Good.

- Procedure
Arm and Take off in position mode, after flying for approximately one minute, switched to altitude then stabilized mode proceed to switch to mission plan mode then make sure that vehicle follows all waypoints as shown in QGC, once completed all waypoin activate RTL and see landing behaviour.

Notes:
Vehicle failed to responde Yaw commands on stabilized mode (This happened only in PR12650 Firmware).

Log:
https://review.px4.io/plot_app?log=1b449549-87f9-4dee-b4b1-ea58a0911c85

MASTER COMPARISON:
https://review.px4.io/plot_app?log=d4b59275-bca8-48c2-bfba-de89b38cf99e

@Junkim3DR
Copy link

Tested on NXP FMUK66 v3:

Modes Tested
Position Mode: Good.
Altitude Mode: Good.
Stabilized Mode: Good.
Mission Plan Mode (Automated): Good.
RTL: Good.

Procedure
Arm and Take off in position mode, after flying for approximately one minute, switched to altitude then stabilized mode proceed to switch to mission plan mode then make sure that vehicle follows all waypoints as shown in QGC, once completed all waypoint activate RTL and see landing behaviour.

Notes:
No issues noted, good flight in general.

Log:
https://review.px4.io/plot_app?log=d6a5d72d-41f7-415c-af3e-00a4fb773ea1

Tested on Pixhawk Pro v4Pro:

Modes Tested
Position Mode: Good.
Altitude Mode: Good.
Stabilized Mode: Good.
Mission Plan Mode (Automated): Good.
RTL: Good.

Procedure
Arm and Take off in position mode, after flying for approximately one minute, switched to altitude then stabilized mode proceed to switch to mission plan mode then make sure that vehicle follows all waypoints as shown in QGC, once completed all waypoint activate RTL and see landing behaviour.

Notes:
No issues noted, good flight in general.

Log:
https://review.px4.io/plot_app?log=33e25ec1-4c84-424b-b432-153faaa93b82

Tested on Pixhawk 4 mini v5:

Modes Tested
Position Mode: Good.
Altitude Mode: Good.
Stabilized Mode: Good.
Mission Plan Mode (Automated): Good.
RTL: Good.

Procedure
Arm and Take off in position mode, after flying for approximately one minute, switched to altitude then stabilized mode proceed to switch to mission plan mode then make sure that vehicle follows all waypoints as shown in QGC, once completed all waypoint activate RTL and see landing behaviour.

Notes:
No issues noted, good flight in general.

Log:
https://review.px4.io/plot_app?log=14ba9c4b-4022-4a23-9997-f7c171861a01

@jorge789
Copy link

jorge789 commented Aug 8, 2019

Tested on Pixhawk 2 Cube v3

Loaded el PR12650 and vehicle did not arm, and popup message said "Preflight Fail: Compass #2 uncalibrated. The vehicle has failed a pre-arm check. In order to arm the vehicle, resolve the failure."
Proceeded to calibrate all sensors but calibrations were rejected.

Loaded Stable firmware and vehicle calibrated and armed without issues.

**Log **
https://review.px4.io/plot_app?log=18094d44-f808-4b67-b7d6-8ef0df7f4f05

@dagar
Copy link
Member Author

dagar commented Aug 9, 2019

Tested on Pixhawk 2 Cube v3

Loaded el PR12650 and vehicle did not arm, and popup message said "Preflight Fail: Compass #2 uncalibrated. The vehicle has failed a pre-arm check. In order to arm the vehicle, resolve the failure."
Proceeded to calibrate all sensors but calibrations were rejected.

Loaded Stable firmware and vehicle calibrated and armed without issues.

**Log **
https://review.px4.io/plot_app?log=18094d44-f808-4b67-b7d6-8ef0df7f4f05

@jorge789 this is an unrelated problem caused by the buggy icm20948 driver (previously reported itself erroneously as another mpu9250). You should be seeing the same issue in master until you recalibrate.

@dagar dagar force-pushed the pr-mc_rate_control branch 2 times, most recently from 3c1f811 to b69f734 Compare August 9, 2019 01:30
@dagar dagar force-pushed the pr-mc_rate_control branch 8 times, most recently from de792e6 to 21f68bb Compare October 21, 2019 23:04
jlecoeur
jlecoeur previously approved these changes Oct 25, 2019
Copy link
Contributor

@jlecoeur jlecoeur left a comment

Choose a reason for hiding this comment

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

Looks good to me, apart from latency considerations.

@dagar
Copy link
Member Author

dagar commented Oct 25, 2019

Not ready to merge until all VTOL types have been carefully tested.

@dannyfpv
Copy link

dannyfpv commented Nov 5, 2019

Tested on Pixhawk 4 mini v5
Mission mode
Notes: Good flight
log:
https://review.px4.io/plot_app?log=18ab7d53-a401-4c12-805a-4b007a81806d

@dagar dagar force-pushed the pr-mc_rate_control branch from bf9c967 to 2b2d61b Compare November 7, 2019 15:06
@jlecoeur
Copy link
Contributor

What is the status of this? Was it tested on all VTOL types? Is anything else blocking?

@dagar
Copy link
Member Author

dagar commented Nov 19, 2019

What is the status of this? Was it tested on all VTOL types? Is anything else blocking?

These changes check out in SITL and have been tested to some extent on real hardware by the test team. I'm not absolutely confident all VTOL subtleties have been preserved, but to be honest that's a pretty good indicator we have a larger problem.

It's early in the v1.11 release cycle. Let's get this in, keep an eye out for possible regressions, and continue working towards a fundamentally better architecture that's maintainable.

Thanks for the help everyone!

@dagar dagar changed the title [WIP]: create mc_rate_control (split out of mc_att_control) create mc_rate_control (split out of mc_att_control) Nov 19, 2019
@dagar dagar merged commit 84fe64b into PX4:master Nov 19, 2019
@dagar dagar deleted the pr-mc_rate_control branch November 19, 2019 22:03
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.

7 participants