From 378b41af6a91cb5dfcabb24421f0a0c9e21a98e5 Mon Sep 17 00:00:00 2001 From: Alex Mikhalev Date: Mon, 30 Nov 2020 13:51:43 -0700 Subject: [PATCH] tunes: Improve logic for interrupting tunes src/lib/tunes already knows which tunes are interruptable, so add return values to expose than and use than in tone_alarm. This fixes the issue that repeating tunes cannot be interrupted without tune_override set to true. Signed-off-by: Alex Mikhalev --- src/drivers/tone_alarm/ToneAlarm.cpp | 116 +++++++++++-------- src/lib/tunes/tunes.cpp | 9 +- src/lib/tunes/tunes.h | 15 ++- src/systemcmds/tune_control/tune_control.cpp | 4 +- 4 files changed, 86 insertions(+), 58 deletions(-) diff --git a/src/drivers/tone_alarm/ToneAlarm.cpp b/src/drivers/tone_alarm/ToneAlarm.cpp index 5dc7ef81c3..03c98d940f 100644 --- a/src/drivers/tone_alarm/ToneAlarm.cpp +++ b/src/drivers/tone_alarm/ToneAlarm.cpp @@ -99,76 +99,92 @@ void ToneAlarm::Run() if (_tune_control_sub.copy(&tune_control)) { if (tune_control.timestamp > 0) { - if (!_play_tone || (_play_tone && tune_control.tune_override)) { + Tunes::ControlResult tune_result = _tunes.set_control(tune_control); + + switch (tune_result) { + case Tunes::ControlResult::Success: PX4_DEBUG("new tune %d", tune_control.tune_id); - if (_tunes.set_control(tune_control) == PX4_OK) { - if (tune_control.tune_override) { - // clear existing - ToneAlarmInterface::stop_note(); - _next_note_time = 0; - hrt_cancel(&_hrt_call); - } + if (tune_control.tune_override) { + // clear existing + ToneAlarmInterface::stop_note(); + _next_note_time = 0; + hrt_cancel(&_hrt_call); + } - _play_tone = true; + _play_tone = true; #if (!defined(TONE_ALARM_TIMER) && !defined(GPIO_TONE_ALARM_GPIO)) || defined(DEBUG_BUILD) - switch (tune_control.tune_id) { - case tune_control_s::TUNE_ID_STARTUP: - PX4_INFO("startup tune"); - break; + switch (tune_control.tune_id) { + case tune_control_s::TUNE_ID_STARTUP: + PX4_INFO("startup tune"); + break; - case tune_control_s::TUNE_ID_ERROR: - PX4_ERR("error tune"); - break; + case tune_control_s::TUNE_ID_ERROR: + PX4_ERR("error tune"); + break; - case tune_control_s::TUNE_ID_NOTIFY_POSITIVE: - PX4_INFO("notify positive"); - break; + case tune_control_s::TUNE_ID_NOTIFY_POSITIVE: + PX4_INFO("notify positive"); + break; - case tune_control_s::TUNE_ID_NOTIFY_NEUTRAL: - PX4_INFO("notify neutral"); - break; + case tune_control_s::TUNE_ID_NOTIFY_NEUTRAL: + PX4_INFO("notify neutral"); + break; - case tune_control_s::TUNE_ID_NOTIFY_NEGATIVE: - PX4_ERR("notify negative"); - break; + case tune_control_s::TUNE_ID_NOTIFY_NEGATIVE: + PX4_ERR("notify negative"); + break; - case tune_control_s::TUNE_ID_ARMING_WARNING: - PX4_WARN("arming warning"); - break; + case tune_control_s::TUNE_ID_ARMING_WARNING: + PX4_WARN("arming warning"); + break; - case tune_control_s::TUNE_ID_BATTERY_WARNING_SLOW: - PX4_WARN("battery warning (slow)"); - break; + case tune_control_s::TUNE_ID_BATTERY_WARNING_SLOW: + PX4_WARN("battery warning (slow)"); + break; - case tune_control_s::TUNE_ID_BATTERY_WARNING_FAST: - PX4_WARN("battery warning (fast)"); - break; + case tune_control_s::TUNE_ID_BATTERY_WARNING_FAST: + PX4_WARN("battery warning (fast)"); + break; - case tune_control_s::TUNE_ID_ARMING_FAILURE: - PX4_ERR("arming failure"); - break; + case tune_control_s::TUNE_ID_ARMING_FAILURE: + PX4_ERR("arming failure"); + break; - case tune_control_s::TUNE_ID_SINGLE_BEEP: - PX4_WARN("beep"); - break; + case tune_control_s::TUNE_ID_SINGLE_BEEP: + PX4_WARN("beep"); + break; - case tune_control_s::TUNE_ID_HOME_SET: - PX4_INFO("home set"); - break; - } - -#endif // (!TONE_ALARM_TIMER && !GPIO_TONE_ALARM_GPIO) || DEBUG_BUILD + case tune_control_s::TUNE_ID_HOME_SET: + PX4_INFO("home set"); + break; } - } else if (_play_tone && !tune_control.tune_override) { +#endif // (!TONE_ALARM_TIMER && !GPIO_TONE_ALARM_GPIO) || DEBUG_BUILD + + break; + + case Tunes::ControlResult::WouldInterrupt: // otherwise re-publish tune to process next PX4_DEBUG("tune already playing, requeing tune: %d", tune_control.tune_id); - uORB::Publication tune_control_pub{ORB_ID(tune_control)}; - tune_control.timestamp = hrt_absolute_time(); - tune_control_pub.publish(tune_control); + { + uORB::Publication tune_control_pub{ORB_ID(tune_control)}; + tune_control.timestamp = hrt_absolute_time(); + tune_control_pub.publish(tune_control); + } + + break; + + + case Tunes::ControlResult::InvalidTune: + PX4_WARN("Invalid tune: %d", tune_control.tune_id); + break; + + case Tunes::ControlResult::AlreadyPlaying: + // Do nothing + break; } } } diff --git a/src/lib/tunes/tunes.cpp b/src/lib/tunes/tunes.cpp index 07d8692648..a95b1b9567 100644 --- a/src/lib/tunes/tunes.cpp +++ b/src/lib/tunes/tunes.cpp @@ -78,11 +78,11 @@ void Tunes::reset(bool repeat_flag) _tempo = _default_tempo; } -int Tunes::set_control(const tune_control_s &tune_control) +Tunes::ControlResult Tunes::set_control(const tune_control_s &tune_control) { // Sanity check if (tune_control.tune_id >= _default_tunes_size) { - return -EINVAL; + return ControlResult::InvalidTune; } // Accept new tune or a stop? @@ -93,7 +93,7 @@ int Tunes::set_control(const tune_control_s &tune_control) // Check if this exact tune is already being played back if (tune_control.tune_id != static_cast(TuneID::CUSTOM) && _tune == _default_tunes[tune_control.tune_id]) { - return OK; // Nothing to do + return ControlResult::AlreadyPlaying; // Nothing to do } // Reset repeat flag. Can jump to true again while tune is being parsed later @@ -126,9 +126,10 @@ int Tunes::set_control(const tune_control_s &tune_control) } _current_tune_id = tune_control.tune_id; + return ControlResult::Success; } - return OK; + return ControlResult::WouldInterrupt; } void Tunes::set_string(const char *const string, uint8_t volume) diff --git a/src/lib/tunes/tunes.h b/src/lib/tunes/tunes.h index b2d807eaf0..195907c058 100644 --- a/src/lib/tunes/tunes.h +++ b/src/lib/tunes/tunes.h @@ -37,6 +37,7 @@ #pragma once +#include #include #include #include "tune_definition.h" @@ -64,6 +65,13 @@ public: Error = -1, }; + enum class ControlResult { + Success = 0, + AlreadyPlaying = 1, + InvalidTune = -EINVAL, + WouldInterrupt = -EBUSY, + }; + /** * Constructor with the default parameters set to: * default_tempo: TUNE_DEFAULT_TEMPO @@ -83,9 +91,12 @@ public: * the call to this function will be ignored, unless the override flag is set * or the tune being already played is a repeated tune. * @param tune_control struct containig the uORB message - * @return return -EINVAL if the default tune does not exist. + * @return return ControlResult::InvalidTune if the default tune does not exist, + * ControlResult::WouldInterrupt if tune was already playing and not interruptable, + * ControlResult::AlreadyPlaying if same tune was already playing, + * ControlResult::Success if new tune was set. */ - int set_control(const tune_control_s &tune_control); + ControlResult set_control(const tune_control_s &tune_control); /** * Set tune to be played using a string. diff --git a/src/systemcmds/tune_control/tune_control.cpp b/src/systemcmds/tune_control/tune_control.cpp index dc85e446f2..74e830372f 100644 --- a/src/systemcmds/tune_control/tune_control.cpp +++ b/src/systemcmds/tune_control/tune_control.cpp @@ -187,9 +187,9 @@ extern "C" __EXPORT int tune_control_main(int argc, char *argv[]) } } else if (!strcmp(argv[myoptind], "libtest")) { - int ret = tunes.set_control(tune_control); + Tunes::ControlResult ret = tunes.set_control(tune_control); - if (ret == -EINVAL) { + if (ret == Tunes::ControlResult::InvalidTune) { PX4_WARN("Tune ID not recognized."); }