libtunes: Various bugfixes and improvements #9117
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Custom msg tune can override any tune playing, regardless of the tune_override flag.
Bugfix: This fixes a bug where libtunes might indicate that there are more tones to play even after the last note of a tune.
Override flag must still be set to true!
Otherwise the only way of interrupting a repeated tune would be with the override flag, which should only be used rarely. Now repeating tunes have lowest priority, followed by one-shot tunes, followed by anything with an override flag.
Since the tune library also contains logic how tunes can be overriden, a user of the tunes library cannot know the strength of the current tune without replicating some logic. Easy solution is to add strength to the output of Tunes::get_next_tune().
potaito
force-pushed
the
pr-libtunes-various-fixes
branch
from
8ba9e9a to
0c09ff8
Compare
bkueng
reviewed
Mar 26, 2018
src/lib/tunes/tunes.cpp
Outdated
| bool reset_playing_tune = false; | ||
| // Sanity check | ||
| if (tune_control.tune_id >= _default_tunes_size) { | ||
| PX4_WARN("Tune ID not recognized."); |
There was a problem hiding this comment.
I'd remove the printf and just return the EINVAL. Reduces the amount of strings.
src/lib/tunes/tunes.h
Outdated
| @@ -62,42 +73,66 @@ class Tunes | |||
| */ | |||
| Tunes(unsigned default_tempo, unsigned default_octave, unsigned default_note_length, NoteMode default_mode); | |||
|
|
|||
| /** | |||
| * Default destructor | |||
There was a problem hiding this comment.
Yeah I figured that the documentation looks more complete if there is a comment above every function. But you are right, it is redundant.
|
@bkueng Thanks for the review. I made the changes in the last commit. Regarding the repeated tune interruption, I'll keep that in mind for a time in the future when I feel more inspired 😛 |
bkueng
approved these changes
Mar 26, 2018
MaEtUgR
pushed a commit
that referenced
this pull request
Jul 17, 2018
MaEtUgR
pushed a commit
that referenced
this pull request
Jul 17, 2018
These are additions for the upstream PR #9117 that are specific to Yuneec's tap_esc driver.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
@bkueng Could you take a look? This is part 2/2 of my libtunes additions, part 1/2 being #9096.
Bugfixes
overrideflag in the tune message is for.https://github.com/PX4/Firmware/blob/d87b7ac7f4f3a86b49b64a976e982e469d361bd0/src/lib/tunes/tunes.cpp#L101-L106
_repeatwas uninitialized and most compilers would set it to 0 then, but not all I guess).tune_control play -m "<melody_string>"for example)Improvements
In the current implementation this means, that repeated tunes have lowest priority, followed by normal tunes, followed by tunes with hard-coded high-priority, followed by tunes with the
tune_overrideflag set. In the future I assume that the hard-coded priorities will be removed and the override flag correctly used instead.int Tunes::get_next_tune(). This way the code playing back the tunes does not need to store the strength separately. Also, when tunes are interrupted or overridden, a player would not know that it might have to adjust the strength for a new tune. With this change, libtunes keeps track of the tune strength.tunes.cppand replaced it with an if-statement for better code readability.Tested on a pixracer (
px4fmu-v4_default) with a piezo buzzer and thetune_controlsystem command.