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
Precise Wake Word Listener #1199
Conversation
612bd80
to
3fd5d37
Compare
3fd5d37
to
c0e438d
Compare
|
I'm having a bit of trouble getting precise running on my machine (Debian jessie: From speech client (after download is complete) I get the same issue if I run the precise-stream from the commandline as well. I'm on 64-Bit debian Stretch right now, will try on my Ubuntu desktop later tonight. Another thing, could this use the downloader in utils to consolidate the code a bit? Or is it too clunky? |
|
@forslund About the error, it's because I built the executable on too new of an OS. I'm working on running automation scripts in an Ubuntu 14.04 docker container which should fix this issue. Also, I didn't even realize we had a download util. However, things would get a little more ugly if I split the code up since it is non blocking. But, I'm fine modifying Downloader to add a blocking parameter even though that sort of defeats the purpose of the class since it inherits from the Thread class. |
|
Point well made. Will patiently wait for the updated executable :) |
767b38a
to
6e23396
Compare
|
@forslund OK, it should be fixed. I rebased so you will have to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works really nicely! I'm a little bit in love actually :)
The only big issue I've seen is that it downloads the executable each startup.
| Popen('echo "' + cmd + '" > /dev/ttyAMA0', shell=True) | ||
|
|
||
| arch = platform.machine() | ||
| exe_file = expanduser('~/.mycroft/precise/' + self.exe_name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This path is not one of the paths checked in resolve_resource_file() so the exe is downloaded each startup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be resolved now. Good catch!
| exe_file = expanduser('~/.mycroft/precise/' + self.exe_name) | ||
| url = self.url_base + 'dist/' + arch + '/' + self.exe_name | ||
|
|
||
| snd_msg('mouth.text=Updating Listener...') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be done using the enclosure api? Right now I get an error message saying it can't open the /dev/ttyAMA0 device. (also I think the /dev/ttyAMA0 should not be hardcoded, instead the enclosure->port value from the config might be better)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess, but that gets messy because then the websocket has to get passed from RecognizerLoop to the HotwordFactory to the HotwordEngine base class, with a handler being written to communicate enclosure text commands from the internal recognizerloop EventEmitter to the actual websocket. Do you think it'd better to just remove it from communicating to the arduino?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can leave it as is for the time being(maybe use the config value instead of hardcoded path), this was not a big issue, just something I thought of when looking at the output. Feedback to the user is good providing an explanation when the device won't respond.
I'm a bit surprised that it works though, I'd expect the enclosure process to block the serial device and hinder other processes to write to it.
| self.cooldown -= 1 | ||
| self.has_found = False | ||
| continue | ||
| if float(line) > 0.5: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Edit: Fixed! I just interpreted Codacy's comment wrong. It actually was complaining because of:
if condition:
val = True
else:
val = Falsevs
val = condition| import shutil | ||
| from urllib2 import urlopen | ||
| LOG.info('Downloading: ' + url) | ||
| req = urlopen(url) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the prefix is fixed to https://, I don't think Codacy's comment on this below matters.
|
Works! I'll test more tomorrow morning but if Michael tests it and wants to merge I trust his judgment. I've not yet tested broken downloads for example. |
|
is precise only trained for "hey mycroft" or can we change it? https://github.com/MycroftAI/mycroft-precise-python-experiments is this the code of precise? (so i can train new words) |
|
great :) was wondering if you would go the snowboy route and have them trainable in your website or something is it easy to train new words? how much wake word samples do you need to achieve good results? can you give some estimate on training time consideration aswel? is the precise repo up to date-ish? could i use that repo's code to train a new model and play around? |
|
@JarbasAI The repo isn't up to date yet. There's still some work left to ease the process of training. At the moment, it requires a fair amount of data to prevent false positives which is what I'm working on automating. But, for the time being, testing it as an experimental feature will serve to collect the data to prevent false positives. Also, for new wake words, since we still support pocketsphinx, those that choose to contribute their data are already building up a data for new wake words. |
|
I've tested some more and it seems to work really good. I'm still a bit worried about the direct write to the display but I've not tried it on an actual Mark-1 so there might be some magic I'm not aware of |
|
So holding I think we should hold off on pulling this in until we figure out the issues with the models |
|
Even if we don't make it default? Since it downloads new models every day,
we should be able to improve it after the merge where we can test on the
device, and it won't affect regular users. However, at the very least, I
have to make one more commit to this branch changing the source to s3.
…On Wed, Nov 8, 2017, 12:09 PM Michael Nguyen ***@***.***> wrote:
So holding I think we should hold off on pulling this in until we figure
out the issues with the models
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1199 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AFmlSwjf6CXellmETSa2IamkJtIj4UDVks5s0e5rgaJpZM4QRoNg>
.
|
|
(and make it not-default) |
|
Is it the fact that precise is default that makes Travis fail or is that something else? |
|
I didn't catch the non-default part! i agree with merging it on non-default. |
|
@forslund Well, it's failing because it's default and the tests don't use the new .update mechanism. |
|
Sigh, sorry github (or the universe) was acting up |
|
@MatthewScholefield Ok I'll make a not to revisit those tests |
bd1f241
to
5ffb425
Compare
|
@forslund @Mn0491 I've made it not default and it now reads from config for the URL so it can be overridden later. For a final test: git checkout feature/precise
git fetch
git reset --hard origin/feature/precise # or upstream/feature/precise
./start-mycroft.sh debug
# Shouldn't see anything about precise in logs and `hey mycroft` should work with pocketsphinx
! cat ~/.mycroft/mycroft.conf && # Make sure nothing in config
echo '{"hotwords": {"hey mycroft": {"module": "precise"}}}' > ~/.mycroft/mycroft.conf
./start-mycroft.sh debug
# Should see found precise executable in logs and wake word should still work (although perhaps with a lot of false positives) |
This allows changing the server in the future without an upgrade
26f0187
to
6107840
Compare
6107840
to
ec223dc
Compare
|
Is precise open sourced as well ? (cant find it) |
This adds precise as an optional wake word listener instead of Pocketsphinx. For testing, it has been made default by modifying the config with
{"hotwords": {"hey mycroft": {"module": "precise"}}}. Once this has been tested, I can remove theMake Precise defaultcommit before a merge.Notes on testing: It downloads a 70 MB file the first time it runs. On the unit, this will be a 30MB file and it will notify the user with text on the screen. During this initial download period, Mycroft will not respond to the wake word.