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
Issues 238 - Philips Hue Skill #240
Issues 238 - Philips Hue Skill #240
Conversation
| @@ -93,3 +93,10 @@ max_time = 600 # in seconds | |||
| notify_delay = 5 # in seconds | |||
| rate = 16000 | |||
| channels = 1 | |||
|
|
|||
| [PhillipsHueSkill] | |||
| verbose = False | |||
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.
A verbose option doesn't really seem necessary for this. I would recommend just having one bit of dialog feedback.
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'd like to keep this in, if you're not strongly against it. I live in a small studio apartment, so for me, the lights changing is all the feedback I need. Having Mycroft tell me what I can see him doing is just noise - it's how the Amazon Echo works, and I hate it (trying to turn off the lights before bed results in a jarring 'OK!' any time the volume is left too high).
Someone with a larger home, however, may appreciate the feedback, if, for example, they are trying to turn off the living room lights from the bedroom.
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'm not super strongly against it. I'll do some testing with other team members to see what we think.
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.
Ok, cool. If I do lose via the democratic process here though, can I ask if there's a practical reason to remove it? The overhead for keeping it seems pretty negligible (it's one extra line per intent type that makes use of it, and an essentially non-existent perf hit from the extra ifs) so I'm curious what your concerns are.
Just as a note, this functionality is important enough to me as a user, I would remove this if I had to to get it into core, but fork and add it back for my own Mycroft. I really, really hate the confirmations from the Echo, and wish I could disable them there.
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'm fine with keeping it, I think the only thing would be whether it would default to false or true.
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.
Sweet. I'm fine with either as the default. Let me know if you, or anyone else, has a preference.
9e6d3ce
to
b6033a7
Compare
|
The unit tests are going to fail with this latest commit. I didn't want to take the time to update them yet, because I'm having some trouble getting Mycroft to take the correct actions. @ethanaward, in the latest commit, I tried restructuring how I handle all of my keywords, as you'd mentioned in a few of your comments (using an IncreaseKeyword and a LightsKeyword to turn the brightness up, for example). Since doing this, all of the actions still work for certain phrasings, however, some phrases that I would expect would work do not. For example, if I say "turn up the lights,' the brightness increases as expected, however, if I say "increase the brightness of the lights," he fails to find a matching intent, and searches Wolfram Alpha. Similarly, if I say "turn up the color temperature of the lights," everything works, however, if I say "increase the color temperature of the lights," he gives me the weather report. "Turn down" and "decrease" for both of these intent types works out the same way, with "turn down" working correctly, and "decrease" giving me wolfram alpha or weather. With the way I have things structured, I would expect to have decent matches, so I'm confused why things are working. Do you have any ideas? Should I just go back to the longer, more well defined keywords? |
|
My first guess is some sort of vocab conflict. The weather intent requires only And could you post some logs of what the intent parser is interpreting? |
|
It's going to be a bit before I can go deep on this change. My recommendation would be to test the skill in isolation (via the skills container) and see if it works as you intend. It is possible to walk through the intent tests in a debugger, but it's not well documented and I haven't done it in a while. |
|
The links on this docs page for setting up the skills sdk, and installing dependencies both currently just say they are works in progress. Rather than screw around trying to set them up on my own, I just made a dummy branch, and stripped out all the other skills. Even without the other skills, it does not seem to be creating the intents properly. Logs for the run can be found here Let me know if there is anything else you need, or anything else I should try. |
This resolves a bug in the connect handler, which would cause it to fail if it was the first handler called.
There's currently no way (that I know of) to test these, because they depend on registering scene names, that are aquired from a bridge, as vocab.
867d04e
to
d8c13a3
Compare
|
Does anyone (@clusterfudge in particular) have any idea why this intent test would pass, but using the phrase would fail in practice: The regex is But everything with a '%' after the value fails to recognize the intent: Explicitly adding the '%' to the pattern made no difference. |
|
This is a case where Adapt is a little bit brittle. I hadn't expected regex entities to be used to parse out part of a token (i.e. the '50' from '50%'), and as such, there's no test cases for it, and totally expectedly it doesn't work :). I don't have an easy fix for adapt at this time, however I can recommend a change to your code. I was able to get the intent to fire by changing your regex slightly. from to The optional '%' allowed both 50 and 50% to parse correctly, and triggered the intent for me. It would then be on you to strip out the percent symbol in your intent handler. I don't have any good ideas on how to fix this in Adapt right now, but will reference this pull/comment in the event I come up with something. I also don't have a good explanation as to why your intent test is succeeding when it didn't work as part of mycroft-core; I've been out of the project for a while, and running unit tests in a debugger is currently broken for me. I don't have the time to invest into getting it working again, so I'm just going to leave that as a mystery for the time being. |
|
Thanks! I updated the regex to allow for the optional '%.' |
|
I've opened this pull request to the skills repo. Assuming that's where skills are going to be tracked going forward, perhaps we should close this out in favor of that one? |
|
@ChristopherRogers1991 |
|
Closing since this has been merged into the skills repo with MycroftAI/mycroft-skills/#3 |
…tAI#240) Add try/except around module shutdown
#238 - Phillips Hue Skill