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

Fix case-sensitive flight mode comparisons #12191

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rubenp02
Copy link
Contributor

@rubenp02 rubenp02 commented Dec 10, 2024

Fix case-sensitive flight mode comparisons

Description

Fixed a bug where the "Start mission" action failed on ArduPilot, displaying an error popup with the message "Unable to start mission: Vehicle failed to change to Auto mode." The issue was caused by case-sensitive comparisons between flight mode strings from the vehicle and the reference strings. Moving away from using strings entirely should be considered.

Replaced direct string comparisons with case-insensitive comparisons using QString::compare() with Qt::CaseInsensitive flag. This fix resolves the immediate issue and also corrects a few other incorrect mode comparisons unrelated to the "Start mission" problem.

Closes #12128

Checklist:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@HTRamsey
Copy link
Collaborator

Do you think this resolves #12128?

@rubenp02
Copy link
Contributor Author

Do you think this resolves #12128?

It probably does, the root cause is the same and I believe all direct mode string comparisons are fixed now.

@DonLakeFlyer
Copy link
Contributor

I don't quite see where these all cap flight mode names end up coming into QGC. How does that happen?

@DonLakeFlyer DonLakeFlyer requested a review from bkueng December 10, 2024 17:49
@DonLakeFlyer
Copy link
Contributor

If these mode names now come from the mavlink protocol instead of being hardwired there is more stuff broken then this. For example, here: https://github.com/mavlink/qgroundcontrol/blob/master/src/FirmwarePlugin/PX4/PX4FirmwarePlugin.cc#L35. The px4 flight mode names are translated. If the comparison flight mode string comes from the vehicle then it most likely is not translated. Or maybe it is but who knows. @bkueng?

@HTRamsey
Copy link
Collaborator

HTRamsey commented Dec 10, 2024

@DonLakeFlyer if you have a master branch up for ArduPilot SITL you should immediately notice the issue, at least that's how I found it a couple weeks ago.

@bkueng
Copy link
Member

bkueng commented Dec 11, 2024

PX4 doesn't use all caps for modes. The change is fine with me, but I think it's worth considering adjusting ardupilot.
It was recently added here: ArduPilot/ardupilot#28566

If these mode names now come from the mavlink protocol instead of being hardwired there is more stuff broken then this. For example, here: https://github.com/mavlink/qgroundcontrol/blob/master/src/FirmwarePlugin/PX4/PX4FirmwarePlugin.cc#L35. The px4 flight mode names are translated. If the comparison flight mode string comes from the vehicle then it most likely is not translated. Or maybe it is but who knows. @bkueng?

It should work as this list of modes isn't used outside of the firmware plugin when the modes come from mavlink. And the guided actions (e.g. takeoff) either directly use the mavlink command, or the mode object (_rtlFlightMode).
Translation isn't supported yet when using the standard modes protocol.

Fixed a bug where the "Start mission" action failed on ArduPilot,
displaying an error popup with the message "Unable to start mission:
Vehicle failed to change to Auto mode." The issue was caused by
case-sensitive comparisons between flight mode strings from the vehicle
and the reference strings. Moving away from using strings entirely
should be considered.

Replaced direct string comparisons with case-insensitive comparisons
using QString::compare() with Qt::CaseInsensitive flag. This fix
resolves the immediate issue and also corrects a few other incorrect
mode comparisons unrelated to the "Start mission" problem.
@rubenp02 rubenp02 force-pushed the bugfix/fix-start-mission-mode-change-failed branch from aea9df8 to 5c1d1a9 Compare December 11, 2024 11:47
@HTRamsey
Copy link
Collaborator

So where do we stand with this then? At a minimum it won't break anything

@DonLakeFlyer
Copy link
Contributor

I haven't had time to look at the new mavlink flight mode spec. I want to see if with that we can switch to not using strings internally any longer at all. This whole case sensitive problem, string compare stuff makes my skin crawl from a standpoint of being crappy.

@DonLakeFlyer
Copy link
Contributor

Ok, so this is fine for now. But @bytesByHarsh is going to help with doing a full review of this stuff. At a minimum we should talk with ArduPilot folks about why flight modes names are all upper cases. Given we transitioned to the 21st century century a while back. I think we can move away from a stuck caps lock key for users. I'm going to write up an Issue with things I am concerned about and investigation as to what we can do about it.

@rubenp02
Copy link
Contributor Author

I reckon the main issue here is using string comparisons for any sort of logic. It shouldn't matter if ArduPilot sends the mode names in whatever case

@DonLakeFlyer
Copy link
Contributor

It shouldn't matter if ArduPilot sends the mode names in whatever case

They are shown to the user so to me it does matter.

@rubenp02
Copy link
Contributor Author

I mean that they should get converted to enum values as soon as they reach QGC (if they must arrive as strings, ideally not even that) and then stay as enum values for all logic and up until its time to display them. Then you display them in any case or format you want, translate them, or whatever else that is required.

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.

Standard Mode issue with ArduCopter 4.6
4 participants