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

Added "found_wakeword" event to message bus #2367

Merged
merged 1 commit into from Nov 11, 2019
Merged

Conversation

speedfox-uk
Copy link
Contributor

@speedfox-uk speedfox-uk commented Oct 19, 2019

Description

Added an event to the message bus that will be sent when the wakeword
is found and before the notification sound is played

How to test

Connect to the message bus and output all of the messages. Say the wakeword. You should see the "found_wakeword" message,

Contributor license agreement signed?

Yes

@devs-mycroft
Copy link
Collaborator

Hello, @speedfox-uk, thank you for helping with the Mycroft project! We welcome everyone
into the community and greatly appreciate your help as we work to build an AI
for Everyone.

To protect yourself, the project, and users of Mycroft technologies we require
a Contributor Licensing Agreement (CLA) before accepting any code
contribution. This agreement makes it crystal clear that along with your
code you are offering a license to use it within the confines of this project.
You retain ownership of the code, this is just a license.

Please visit https://mycroft.ai/cla to initiate this one-time signing. Thank
you!

@devs-mycroft devs-mycroft added the CLA: Needed Need signed CLA from https://mycroft.ai/cla label Oct 19, 2019
@krisgesling krisgesling added CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) and removed CLA: Needed Need signed CLA from https://mycroft.ai/cla labels Oct 21, 2019
@forslund
Copy link
Collaborator

Hi This is working as expected...however it seems like the recognizer_loop:wakeword should mean the same thing. But that seem to be broken in some way. We should try to determine why the original message is late...

@speedfox-uk
Copy link
Contributor Author

It's broken because the message tone is played in the same method that does the wakeword detection (_wait_until_wake_word in mic.py). The other way to fix this would be to take playing the wakeword detected tone out of _wait_until_wake_word and put it somewhere after the wakeword message has been sent. Either that or remove the current "wakeword" message and just rename my new message to that. I don't mind either way.

@forslund
Copy link
Collaborator

It's a bit more than that, (at least to me) it seems like it's not sent until after the recording of the STT phrase is complete...

@forslund
Copy link
Collaborator

I do think it'd be best if your message was renamed but that would also require the session id to be included

SessionManager.touch()                                                  
        payload = {                                                             
            'utterance': self.wakeword_recognizer.key_phrase,                   
            'session': SessionManager.get().session_id,                         
        }                                                                       
        self.emitter.emit("recognizer_loop:wakeword", payload)

And the corresponding section in listener.py should be removed.

@speedfox-uk
Copy link
Contributor Author

OK, I'll make the change after I get home and update the PR.

@speedfox-uk
Copy link
Contributor Author

I've renamed it to "wakeword" and removed the old one. I'm just grabbing the utterance from "self.wake_word_name" rather than keeping a reference to the wakeword recogniser because it seemed like a simpler solution.

@forslund
Copy link
Collaborator

Thanks, will look it over and hopefully merge tomorrow.

@forslund
Copy link
Collaborator

forslund commented Nov 1, 2019

@davidwagnerkc had done some similar work so I've assigned him to do the review of these changes.

Copy link
Contributor

@davidwagnerkc davidwagnerkc left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. This makes much more sense. Thanks for taking the time to get a PR up.

@speedfox-uk
Copy link
Contributor Author

No problem. Just happy that this is making its way into master.

The message is now emitted directly after a wakeword is detected.
@forslund
Copy link
Collaborator

Did a rebase to clean it up but merging now. Many thanks for fixing this!

@forslund forslund merged commit 43e513b into MycroftAI:dev Nov 11, 2019
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)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants