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

Register bus handlers requiring loop after loop exists #2926

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

krisgesling
Copy link
Contributor

Description

AttributeError was being thrown by the Speech service.

File "/opt/mycroft/.venv/lib/python3.8/site-packages/pyee/_base.py", line 116, in emit
    self._emit_handle_potential_error(event, args[0] if args else None)
  File "/opt/mycroft/.venv/lib/python3.8/site-packages/pyee/_base.py", line 86, in _emit_handle_potential_error
    raise error
  File "/usr/lib/python3.8/concurrent/futures/thread.py", line 57, in run
    result = self.fn(*self.args, **self.kwargs)
  File "/opt/mycroft/mycroft/client/speech/__main__.py", line 140, in handle_mic_get_status
    data = {'muted': loop.is_muted()}
AttributeError: 'NoneType' object has no attribute 'is_muted'

Cause:
The main bus gets established and event handlers registered before the RecognizerLoop bus is established. Hence if something emits a mycroft.mic.get_status message in this short window, the handler will fail as loop is None.

Fix:
This change splits the bus events being registered into two groups:

  1. events required for setup - currently only open and based on the existing #TODO this might get removed as a breaking change.
  2. the rest of the events that interact with the RecognizerLoop or expect the speech service to be ready. These are now only registered after the loop has been initialized and run.

How to test

I haven't created a way to reliably reproduce the issue and a unit test for this is proving difficult. Any suggestions welcome.

Would need to emit a message like mycroft.mic.get_status as the service is being established and verify that the handler is not called. Then once the service is established, re-emit the message and assert that the handler is called.

Contributor license agreement signed?

  • CLA

@AIIX - the only thing calling this that I could see is mycroft-gui-mark-2. It's only going to not receive a response, and it's already doing that with the handler failing, but wanted to flag it in case this will impact that package in any way.

@devops-mycroft devops-mycroft added the CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) label Jun 24, 2021
@devops-mycroft
Copy link

devops-mycroft commented Jun 24, 2021

Voight Kampff Integration Test Succeeded (Results)

@AIIX
Copy link
Collaborator

AIIX commented Jun 24, 2021

It seems to affect the Mute button delegate in the drop down menu in the mycroft-gui-mark-2 package,I believe if mycroft.mic.get_status does not respond on boot we might have no way to know what the actual status is of the microphone in the GUI.

@ken-mycroft
Copy link
Contributor

My confusion arises from the fact that the response to the "mycroft.volume.get" message contains a muted flag and I was wondering why this is not being used?

@krisgesling
Copy link
Contributor Author

My confusion arises from the fact that the response to the "mycroft.volume.get" message contains a muted flag and I was wondering why this is not being used?

I think we're talking about different things. mycroft.volume.get relates to the audio output whilst mycroft.mic.get_status relates to the audio input.

@krisgesling
Copy link
Contributor Author

It seems to affect the Mute button delegate in the drop down menu in the mycroft-gui-mark-2 package,I believe if mycroft.mic.get_status does not respond on boot we might have no way to know what the actual status is of the microphone in the GUI.

@AIIX - presently the response wouldn't be emitted as it would just fail with the error above. So this PR shouldn't make that behavior worse.

I'm thinking we probably need to first wait for the Speech Service to be ready before checking the mic_status

@krisgesling krisgesling added this to Sprint 25: Core and Selene Bugfixes in Upcoming Sprints Aug 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) Type: Bug - complex
Projects
No open projects
Upcoming Sprints
Sprint 25: Core and Selene Bugfixes
Development

Successfully merging this pull request may close these issues.

None yet

4 participants