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

Restored ability to store wakewords locally #2141

Merged
merged 5 commits into from
May 31, 2019

Conversation

penrods
Copy link
Contributor

@penrods penrods commented May 29, 2019

The code that handles the local save of wake words configuration -
"record_wake_words" - was removed some time ago. This restores that
capability.

How to test

Add the mycroft.conf

{
     "record_wake_words": true
}

Then restart Mycroft. It should begin saving .wav files under the
/tmp/mycroft_wake_words directory.

The code that handles the local save of wake words configuration -
"record_wake_words" - was removed some time ago.  This restores that
capability.

== TESTING ==

Add the mycroft.conf
  {
     "record_wake_words": true
  }
Then restart Mycroft.  It should begin saving .wav files under the
/tmp/mycroft_wake_words directory.
Add documentation for 'record_wake_words' and improve the doc for
'save_utterances'.
@penrods penrods added the CLA: Yes Contributor License Agreement exists (see https://github.com/MycroftAI/contributors) label May 29, 2019
@penrods penrods requested a review from forslund May 29, 2019 20:38
Copy link
Collaborator

@forslund forslund left a comment

Choose a reason for hiding this comment

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

Apart from that this seems to be doing what it should.

@@ -356,14 +362,16 @@ def _upload_wake_word(self, audio):
else:
model_hash = '0'

metadata = {
return {
'name': self.wake_word_name.replace(' ', '-'),
'engine': md5(ww_module.encode('utf-8')).hexdigest(),
'time': str(int(1000 * get_time())),
'sessionId': SessionManager.get().session_id,
'accountId': self.account_id,
'model': str(model_hash)
}
Copy link
Collaborator

@forslund forslund May 29, 2019

Choose a reason for hiding this comment

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

I wonder if you want a simple list here. This currently results in the literal filename name.engine.time.sessionId.accountId.model.wav (join of a dict seem to be joining the keys).

Alternatively you could use the .values() but I'm not sure if that will always preserve the order so maybe make it an ordered dict?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Works better if I hit "Save" in my editor...

I'd already changed that to use values(). I don't honestly care that much about order when reviewing a local copy, so I don't think it matters much

Copy link
Collaborator

Choose a reason for hiding this comment

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

I personally care (otherwise I wouldn't have mentioned it). It's good style to guarantee consistency and if anyone makes a script for doing any type of analysis on the recorded wakewords or something it'd be good if it can easily parse out the name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough!

The wakeword file was built from a dict with no guaranteed consistency.  Now it sorts the dictionary when building the,
filename, and used underscore separators resulting in a filename like:
   <accountId>_<engine>_<model>_<name>_<sessionId>_<time>.wav
@forslund
Copy link
Collaborator

The grumpy swede is pleased with the consistency. Merging

@forslund forslund merged commit 115bf77 into dev May 31, 2019
@penrods
Copy link
Contributor Author

penrods commented Jun 3, 2019

"Bork a bork a bork bork a...."
:)

@penrods penrods deleted the feature/restore-local-wakeword-debug branch June 3, 2019 18:42
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

2 participants