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

manual_contol_setpoint: fix mode slot numbering #12578

Merged
merged 1 commit into from
Aug 1, 2019
Merged

Conversation

MaEtUgR
Copy link
Member

@MaEtUgR MaEtUgR commented Jul 29, 2019

Describe problem solved by the proposed pull request
When you use your drone (or SITL) with a joystick attached to QGC and hence MANUAL_CONTROL MAVLink messages instead of a traditional RC link the mode changes to slot 1 as configured for the RC.

This happens because usually uORB structs get initialized with all zeros and then filled with the fields that are actually in use. The number 0 for the mode slot was already commanding to switch to the mode in slot one even though for example the joystick input via mavlink does not use this mechanism at all and doesn't set a value because it's unrelated. There might be other such cases we are not (yet) aware of and I suggest to have the number zero represent unsupported/unset for this reason.

Test data / coverage
I bench tested this PR on an fmu-v5 board to verify that the mode switch on and RC connected via SBUS still works as expected and at the same time original problem when using a joystick is solved.

Describe your preferred solution
I moved the mode_slot number which specifies what slot the mode switch currently is in to be again between 1 and 6 for valid mode configurations such that the number 0 represents no mode slot and hence no switching.

Additional context
This is reverting the cleanup from this commit f472ac5 which was probably done without some side effects in mind. Another such side effect would be if the signed mode_slot number was ever published to be -2 because then the commander would start overwriting random memory here: https://github.com/PX4/Firmware/blob/75531125a28bd57327345142b55b56ad6a68e41a/src/modules/commander/Commander.cpp#L2699-L2704

@MaEtUgR MaEtUgR requested review from julianoes and bkueng July 29, 2019 19:06
@MaEtUgR MaEtUgR self-assigned this Jul 29, 2019
@dagar
Copy link
Member

dagar commented Jul 29, 2019

Partially related this was one of the minor reasons I was going to split the switches out from manual control setpoint. #9712
Then Commander::set_main_state_rc() would only need to execute on switch updates, which wouldn't even be present in the mavlink case.

@dagar dagar changed the title manual_contol_ssetpoint: fix mode slot numbering manual_contol_setpoint: fix mode slot numbering Jul 29, 2019
@MaEtUgR
Copy link
Member Author

MaEtUgR commented Jul 30, 2019

@dagar Thanks for mentioning. It's indeed related. That would have been one of the next steps to futher clean this up. But the changes are not exclusive. It still makes sense to have this numbering even with the new message structure. Any plans to revive this?

@MaEtUgR
Copy link
Member Author

MaEtUgR commented Jul 30, 2019

I repeated all my bench tests with the fixes.

This fix is necssary because usually uORB structs get initialized with
all zeros and then et filled with the fields that are actually in use.
The number 0 for the mode slot was already commanding to switch to the
mode in slot one even though for example the joystick input via mavlink
does not use this mechanism at all.
@dagar
Copy link
Member

dagar commented Aug 1, 2019

@dagar Thanks for mentioning. It's indeed related. That would have been one of the next steps to futher clean this up. But the changes are not exclusive. It still makes sense to have this numbering even with the new message structure. Any plans to revive this?

I've rebased #9712. I'll rebase it again after this PR lands in master and we'll test it again.

@dagar dagar merged commit d0f1a55 into master Aug 1, 2019
@dagar dagar deleted the mode-slot-fix branch August 1, 2019 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants