Skip to content
This repository was archived by the owner on Sep 8, 2024. It is now read-only.

Bugfix/audioservice issues #2804

Merged
merged 4 commits into from
Jan 29, 2021
Merged

Bugfix/audioservice issues #2804

merged 4 commits into from
Jan 29, 2021

Conversation

forslund
Copy link
Collaborator

@forslund forslund commented Jan 16, 2021 •
edited
Loading

Description

This is an attempt to make the audioservice behave better in the VK test. In particular it tries to handle exceptions seen in the audio.log after a failing VK run such as:

Traceback (most recent call last):
  File "/usr/lib/python3.6/concurrent/futures/_base.py", line 324, in _invoke_callbacks
    callback(self)
  File "/opt/mycroft/mycroft-core/.venv/lib/python3.6/site-packages/pyee/_executor.py", line 56, in _callback
    self.emit('error', exc)
  File "/opt/mycroft/mycroft-core/.venv/lib/python3.6/site-packages/pyee/_base.py", line 116, in emit
    self._emit_handle_potential_error(event, args[0] if args else None)
  File "/opt/mycroft/mycroft-core/.venv/lib/python3.6/site-packages/pyee/_base.py", line 86, in _emit_handle_potential_error
    raise error
  File "/usr/lib/python3.6/concurrent/futures/thread.py", line 56, in run
    result = self.fn(*self.args, **self.kwargs)
  File "/opt/mycroft/mycroft-core/mycroft/audio/services/simple/__init__.py", line 90, in _play
    if isinstance(self.tracks[self.index], list):
IndexError: list index out of range

This PR adds a lock and a check that the track in question still exists before starting playback.

It also handles an issue where the playback isn't restored as it should after speaking when using the dummy tts module

How to test

(Description of how to validate or test this PR)

Contributor license agreement signed?

CLA [ ] (Whether you have signed a CLA - Contributor Licensing Agreement

@devops-mycroft devops-mycroft added the CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) label Jan 16, 2021
@forslund forslund added the Status: Work in progress PR being actively worked on, not yet ready for review. label Jan 16, 2021
@devops-mycroft
Copy link

Voight Kampff Integration Test Failed (Results).
Mycroft logs are also available: skills.log, audio.log, voice.log, bus.log, enclosure.log

1 similar comment
@devops-mycroft
Copy link

Voight Kampff Integration Test Failed (Results).
Mycroft logs are also available: skills.log, audio.log, voice.log, bus.log, enclosure.log

@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

2 similar comments
@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

@forslund
Copy link
Collaborator Author

This commit contains fixes to the singing skill test steps to verify that with those in place. It after the singing skill has been updated that commit can be dropped along with the speed testing commit.

@krisgesling krisgesling mentioned this pull request Jan 18, 2021
Merged
1 task
@krisgesling
Copy link
Contributor

You're awesome, thanks Ake!!!

I've done PR's to add this VK method to the Singing Skill and mycroft-core - though renamed the method. I know you just grabbed this from News - name not your fault. Just thought wait_for_audio_service was more descriptive.

@forslund forslund force-pushed the bugfix/audioservice-issues branch from bd8aa13 to e7e9e0b Compare January 18, 2021 19:55
@forslund
Copy link
Collaborator Author

Still seem to find ways to fail :( Will continue investigation...

@devops-mycroft
Copy link

Voight Kampff Integration Test Failed (Results).
Mycroft logs are also available: skills.log, audio.log, voice.log, bus.log, enclosure.log

2 similar comments
@devops-mycroft
Copy link

Voight Kampff Integration Test Failed (Results).
Mycroft logs are also available: skills.log, audio.log, voice.log, bus.log, enclosure.log

@devops-mycroft
Copy link

Voight Kampff Integration Test Failed (Results).
Mycroft logs are also available: skills.log, audio.log, voice.log, bus.log, enclosure.log

@forslund forslund force-pushed the bugfix/audioservice-issues branch from 2edcfa5 to 026fb2b Compare January 19, 2021 05:47
@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

@devops-mycroft
Copy link

Voight Kampff Integration Test Failed (Results).
Mycroft logs are also available: skills.log, audio.log, voice.log, bus.log, enclosure.log

@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

3 similar comments
@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

@forslund forslund force-pushed the bugfix/audioservice-issues branch from 026fb2b to 10d86d9 Compare January 22, 2021 13:43
@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

@forslund forslund force-pushed the bugfix/audioservice-issues branch from 10d86d9 to d0ae65c Compare January 22, 2021 16:13
@devops-mycroft
Copy link

Voight Kampff Integration Test Failed (Results).
Mycroft logs are also available: skills.log, audio.log, voice.log, bus.log, enclosure.log

@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

@forslund
Use lock around play and stop
aace28d 
Moves the stopping logic to separate method to be able to call from
locked contexts
@forslund forslund force-pushed the bugfix/audioservice-issues branch from d0ae65c to aace28d Compare January 23, 2021 10:10
@devops-mycroft
Copy link

Voight Kampff Integration Test Succeeded (Results)

@forslund forslund removed the Status: Work in progress PR being actively worked on, not yet ready for review. label Jan 23, 2021
@forslund
Copy link
Collaborator Author

I've removed the Work in progress tag, The last 10 tests have completed without singing or news skills failures.

@forslund forslund added Type: Bug - quick Bug fixes that are quick to review and the implications of the change are clear and contained. Type: Tests Addition or improvement of automated tests labels Jan 23, 2021
krisgesling
krisgesling approved these changes Jan 29, 2021
View reviewed changes
Copy link
Contributor

@krisgesling krisgesling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All looks good and working locally too.

Here's hoping it really has put these tests to bed 🤞

@krisgesling krisgesling merged commit f30adb8 into MycroftAI:dev Jan 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
1 more reviewer

@krisgesling krisgesling krisgesling approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) Type: Bug - quick Bug fixes that are quick to review and the implications of the change are clear and contained. Type: Tests Addition or improvement of automated tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@forslund @devops-mycroft @krisgesling