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

Position publish disarmed attitude #13210

Merged
merged 3 commits into from
Nov 24, 2019
Merged

Conversation

MaEtUgR
Copy link
Member

@MaEtUgR MaEtUgR commented Oct 16, 2019

Describe problem solved by this pull request
Smooth takeoff issues were only hotfixed but the root cause never solved.

Describe your solution
I start the flight task, run the position controller and publish the attitude setpoint as soon as the navigation state is set by the commander also when disarmed. In navigation states where no flight task is running the position controller is still not run and nothing published e.g. stabilized mode since #10805, offboard if the attitude setpoint is coming via MAVLink or when flying fixed wing.

Describe possible alternatives
What's still missing is that the Attitude controller can handle invalid setpoints e.g. timeouts, NAN values and so on.

Test data / coverage
SITL gazebo
Still open tests:

@dagar
Copy link
Member

dagar commented Oct 17, 2019

Side note - if you name your branches with a pr- prefix then the PR will also run on the hardware test rack. This can be a useful/lazy way to get a sense of cpu load before and after across a range of boards.

@MaEtUgR
Copy link
Member Author

MaEtUgR commented Oct 23, 2019

if you name your branches with a pr- prefix then the PR will also run on the hardware test

Very good hint, I should adapt my naming scheme then 👍

@MaEtUgR MaEtUgR force-pushed the position-publish-disarmed-attitude branch 2 times, most recently from 67f514e to 85a268d Compare October 29, 2019 13:39
@MaEtUgR
Copy link
Member Author

MaEtUgR commented Oct 29, 2019

I rebased since @sfuhrer is doing VTOL testing. I might be able to add this to the testing.

@sfuhrer
Copy link
Contributor

sfuhrer commented Oct 29, 2019

@MaEtUgR testflown on a standard VTOL: https://review.px4.io/plot_app?log=0122c386-8af6-4a24-8bb2-36a23a00938e

I had some airspeed measurement issues on the flight, can't image them to be related to this PR though. Beside that, everything functioned as it should (tested in STAB, ALT/POS, both MC and FW).

@sfuhrer
Copy link
Contributor

sfuhrer commented Oct 30, 2019

@MaEtUgR second testflight: https://review.px4.io/plot_app?log=4db9ffd5-1e1d-4e37-96ba-5217dcd9683f

@xdwgood
Copy link
Contributor

xdwgood commented Nov 9, 2019

I tested this PR on the tailsitter of the two motors, which solved the problem here:
https://logs.px4.io/plot_app?log=8c7b8d4e-42b7-4fa9-9ed7-5d364a133f19
(not arm but produced huge actuator output)
Screenshot from 2019-11-09 14-45-43

@LorenzMeier
Copy link
Member

@MaEtUgR @sfuhrer Can we get this in?

@sfuhrer
Copy link
Contributor

sfuhrer commented Nov 11, 2019

@MaEtUgR @sfuhrer Can we get this in?

From my side yes. I think @MaEtUgR wanted still to test in offboard though.

@MaEtUgR
Copy link
Member Author

MaEtUgR commented Nov 13, 2019

Yeah, I really want to get it in. I just didn't get through to do the offboard test. Passing the MAVSDK attitude offboard test should be enough and then it's ready to merge.

@MaEtUgR MaEtUgR force-pushed the position-publish-disarmed-attitude branch from 85a268d to 2858470 Compare November 13, 2019 20:36
@MaEtUgR
Copy link
Member Author

MaEtUgR commented Nov 13, 2019

I successfully ran the MAVSDK offboard test on SITL:
https://logs.px4.io/plot_app?log=643baa12-f9aa-4561-bec1-b4c4be310b66

I realized one thing though: The warnings likely reveal another race condition between the flight stack modules. When GPS fusion starts and it switches to Hold there are the messages

WARN  [mc_pos_control] Auto activation failed with error: Activation Failed
WARN  [mc_pos_control] Position-Ctrl activation failed with error: Activation Failed
WARN  [mc_pos_control] Altitude-Ctrl activation failed with error: Activation Failed

These do not show on master since to flight task is running and therefore noone cares if the data is not even available in the first loop iteration after switching the mode.

@MaEtUgR
Copy link
Member Author

MaEtUgR commented Nov 13, 2019

Indeed the commander switches to hold when global position is valid, the position controller starts the task, it expects valid triplets and doesn't find any so reports based on the check here: https://github.com/PX4/Firmware/blob/99a7491cdef655cefdd7aef66ea4a9ec97906db7/src/lib/flight_tasks/tasks/Auto/FlightTaskAuto.cpp#L139

The others are the fallback because there's no way to autonomously stay in air but there's also no RC connected in my example so it continues and ends up in the failsafe task for this one loop iteration where the data is missing.

It's nothing caused by this pr and it's also not unsafe it's just not nice and gets revealed by the warnings.

@MaEtUgR MaEtUgR marked this pull request as ready for review November 14, 2019 07:38
such that the controller always publishes attitude setpoints and there are no initialization issues.
since the position controller publishes them again already when disarmed.
@MaEtUgR MaEtUgR force-pushed the position-publish-disarmed-attitude branch from 2858470 to af5e8e5 Compare November 20, 2019 22:29
@MaEtUgR
Copy link
Member Author

MaEtUgR commented Nov 20, 2019

I rebased to be ready for merging again. The only conflict was the line https://github.com/PX4/Firmware/pull/12650/files#diff-81c4f50bdf837c4183b5d3cb81eb40eaL359-L360 close to my change was removed but it doesn't interfere.

@MaEtUgR
Copy link
Member Author

MaEtUgR commented Nov 24, 2019

Updated again. Any reviews?
From my side it's ready including real world, simulation and offboard tests.

@dagar dagar merged commit 28755d5 into master Nov 24, 2019
@dagar dagar deleted the position-publish-disarmed-attitude branch November 24, 2019 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants