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: Don't spam battery tune messages #11284

Closed
wants to merge 1 commit into from

Conversation

potaito
Copy link
Contributor

@potaito potaito commented Jan 23, 2019

When battery is low or critical, commander will publish a tune message. Not once though, but one message per thread cycle. The tunes library does take care of this and ignores messages with the same tune, but it's still unnecessary. Especially now when we have repeated tunes.

Solution:

Use flags to make sure the message is only sent once.

Adding more flags to the commander is not optimal, that I understand. But at least it will document the issue for future refactoring.

Implications

With this PR, the low and critical battery tunes can be interrupted by other tunes, since it's not continuously spammed.

@LorenzMeier
Copy link
Member

Given those tunes can be important, could we add a priority (maybe 1-2 levels in the tone driver) and it resumes the tune it had loaded before? Have you looked at how difficult this is?

@dagar
Copy link
Member

dagar commented Jan 23, 2019

I definitely think these tunes need to be repeated at some rate. We should also think about doing it as a basic state machine and stop carrying so much unnecessary state.

@dagar dagar added this to the Release v1.9.0 milestone Jan 23, 2019
@dagar dagar self-requested a review January 23, 2019 18:07
@potaito
Copy link
Contributor Author

potaito commented Jan 24, 2019

@LorenzMeier Tunes currently have no notion of priority. We did have some priorities hard-coded in the tunes library, but that was of course a dirty hack. I also experimented with what you suggest, which is to have non-repeated tunes interrupt repeated tunes only temporary. It worked technically, but in practice it would require some tuning to make it obvious to the user, that it's two distinct tunes being played. Like adding pauses in between for example.

@dagar I somewhat disagree with you for once. If it is necessary to repeatedly send a tune, that means that the tunes library needs to be reworked. I believe sending tunes repeatedly will make deterministic results in the future more difficult, especially when 3rd parties use custom tunes instead of px4 defaults. Some parties might not even want the battery warnings to be repeated at all, which should be respected. Thus I think all information should be encapsulated in the tune definition.

@potaito
Copy link
Contributor Author

potaito commented Feb 5, 2019

Maybe I'll pick this up again some other time

@potaito potaito closed this Feb 5, 2019
@potaito potaito deleted the fix/battery-tunes-spamming branch February 5, 2019 12:57
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.

3 participants