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

commander: prevent potential disarms in-air #12557

Merged
merged 2 commits into from
Jul 26, 2019
Merged

Conversation

julianoes
Copy link
Contributor

@julianoes julianoes commented Jul 25, 2019

This fixes the terrifying case where the drone disarms in-air just because it receives a MAVLink disarm command. We now check param2 for a magic number which enforces arming/disarming.

This is added to the mavlink protocol in:
mavlink/mavlink#1162

Testing done using MAVSDK mavlink/MAVSDK#817.

If you want to reproduce the test:

cmake -Bbuild -DCMAKE_BUILD_TYPE=Debug -H. && cmake --build build -j8 && build/src/integration_tests/integration_tests_runner --gtest_filter="SitlTest.ActionTakeoffAndKill"

@dagar
Copy link
Member

dagar commented Jul 25, 2019

Looks good, although I'm wondering about capturing more of these details in the state machine. I don't suppose the magic number (21196) is accessible through the mavlink spec?

@julianoes
Copy link
Contributor Author

Looks good, although I'm wondering about capturing more of these details in the state machine.

I wonder too but that's one we know.

I don't suppose the magic number (21196) is accessible through the mavlink spec?

It's just in the description of that message.

I'm glad you're back @dagar! :)

@julianoes
Copy link
Contributor Author

@ItsTimmy yay, the rover CI check prevented me from merging something that could break rover.

This fixes the terrifying case where the drone disarms in-air just
because it receives a MAVLink disarm command. We now check param2 for a
magic number which enforces arming/disarming.

This is added to the mavlink protocol in:
mavlink/mavlink#1162
We have to ignore the landed flag for rovers, it doesn't really apply
for them.
@julianoes julianoes force-pushed the fix-in-air-disarm branch from f36ba42 to a8d5413 Compare July 25, 2019 15:39
src/modules/commander/Commander.cpp Show resolved Hide resolved
@julianoes julianoes merged commit 98dfa30 into master Jul 26, 2019
@julianoes julianoes deleted the fix-in-air-disarm branch July 26, 2019 09:52
@MaEtUgR
Copy link
Member

MaEtUgR commented Jul 26, 2019

@julianoes Thanks for the hotfix! Just to be safe I tested the exact problem we had and it prevents disarming like expected.

What I still don't like and only makes this a hotfix for me is that RC and joystick are not handled by the same code path and checks and there arise other unexpected corner cases. One of them I found is that if you disarm in manual mode by RC it's allowed. By MAVLink command it's not.

@dagar
Copy link
Member

dagar commented Jul 26, 2019

What I still don't like and only makes this a hotfix for me is that RC and joystick are not handled by the same code path and checks and there arise other unexpected corner cases. One of them I found is that if you disarm in manual mode by RC it's allowed. By MAVLink command it's not.

That's the scenario I was thinking about above and why I'd like to pull most of this scattered logic together into a state machine.

@julianoes
Copy link
Contributor Author

I don't disagree.

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.

5 participants