-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Speech: fix "Only when Armed" #3641
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
Conversation
311bb03 to
ce3f823
Compare
I don't think that first part is correct. It's definitely tangled and hard to trace, but it isn't completely ignored. That's not to say that I disagree with your approach though. Having |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The addition of the MainV2.speechEnabled() bit is the key here. Every caller should have really been checking that function from the beginning, instead of what they've been checking, but not every caller actually has access to MainV2.
I think this approach is fine, I'd just drop the change on line 15. Don't worry about what I said earlier about cleaning up the callers
ce3f823 to
eaf57f5
Compare
|
@robertlong13 - changes applied as requested. There is a remaining bug we should probably try and track down before merge: I was mistaken about messages leaking past during initialization or connection - it's actually that they are announced until you view the Config/Planner screen, at which point the user preference of "Only when Armed" is applied correctly. Note, this fix is required to even get that far, but I'm afraid the fix is only a half-measure until we track this down. |
|
@yuri-rage I think the init is because the startup check is missing . The other speech options do this check |
|
@mhornsby - I think your thought process is correct, but your suggested fix probably isn't quite the ticket. Fixed in the second commit and now merged. |

Returns out of
SpeakAsync()when the user has disabled speech, fixing a bug where spoken MavLink messages meeting the severity threshold slipped through despite "Only when Armed" being selected.There are some inconsistencies throughout the codebase in how speech is enabled/disabled. Frankly, I don't have the time to track them all down, but this fix should help homogenize behavior a bit.
The
speechEnableproperty in this class was previously completely ignored and should probably be leveraged better overall.At initial startup/connection there is a possibility that a couple of messages still slip through while disarmed. I'm not sure exactly what causes this - perhaps it takes a moment for the armed state to propagate across the application. This fix at least addresses the lion's share of nuisance messages reported.
Attached is an ArduPilot Lua script that can be used for testing.
Fixes #3509
Should also close #3611
speech.zip