Add pause and resume capability to TTS playback thread #2940
Conversation
This is used during barge in
devops-mycroft
added
the
CLA: Yes
|
Voight Kampff Integration Test Succeeded (Results) |
Codecov Report
@@ Coverage Diff @@
## dev #2940 +/- ##
==========================================
- Coverage 52.64% 52.62% -0.03%
==========================================
Files 123 123
Lines 11021 11039 +18
==========================================
+ Hits 5802 5809 +7
- Misses 5219 5230 +11
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
I believe there is a possible lock scenario if the thread is stopped while in a pause waiting state (and also the speech client is shut down), unlikely but should is simple to put in safeguards against. See forslund@33a430e for a simple fix.
The other comments are mainly suggestions on improvements and can be ignored. If they are deemed useful you can cherry-pick them from forslund/mycroft-core/tree/feature/tts-pause-resume
| @@ -124,7 +124,7 @@ def mute_and_speak(utterance, ident, listen=False): | |||
| tts.init(bus) | |||
| tts_hash = hash(str(config.get('tts', ''))) | |||
|
|
|||
| LOG.info("Speak: " + utterance) | |||
| LOG.debug("Listen=%s, Speak:%s" % (listen, utterance)) | |||
There was a problem hiding this comment.
I think it's valid to keep this as an info level LOG, it's something that is generally good to be able to track even for a user. Also generally we use some sort of format string instead of the old % system. If % is used in logging it should use the logging lazy string evaluation i.e.
LOG.debug("Listen=%s, Speak:%s", listen, utterance)
There was a problem hiding this comment.
I agree this should be kept as an info level log. Knowing what the device thought it heard and what it said in response is quite useful.
| @@ -174,6 +174,14 @@ def handle_stop(event): | |||
| bus.emit(Message("mycroft.stop.handled", {"by": "TTS"})) | |||
|
|
|||
|
|
|||
| def handle_pause(event): | |||
There was a problem hiding this comment.
Missing docstrings for this and the handle_resume
| @@ -93,6 +94,8 @@ def run(self): | |||
| listening. | |||
| """ | |||
| while not self._terminated: | |||
| while self._paused: | |||
| sleep(0.2) | |||
There was a problem hiding this comment.
This while-sleep pattern can usually be replaced with an Event(). See forslund@8dc0fd6 for a suggestion
There was a problem hiding this comment.
I would go one step further. Why use threading.Event() when we have an event-driven architecture with a message bus? What is a good reason to use threading.Event() instead?
There was a problem hiding this comment.
Do you mean terminate the playback thread and restart it on the message?
The threading.Event() is generally a good way for cyclic code to sync with an external event like the case is here. Basically block until event.
| if self._paused: | ||
| self.p.terminate() | ||
| break | ||
| sleep(0.1) |
There was a problem hiding this comment.
This sort of busy waiting should be avoided if possible IMO. See forslund@2c050fb for a suggestion
Description
This adds pause and resume functionality to the TTS playback thread for use in barge in.
From Ken:
Replication of #2939 against the dev branch so it doesn't get lost.
How to test
Unit tests need to be added.
Contributor license agreement signed?