Add magic-mirror-voice-control

[dmwilsonkc]

Info

This PR adds the new skill, magic-mirror-voice-control, to the skills repo.

Description

This mycroft skill passes commands to an accessible MagicMirror installed anywhere on the same network as Mycroft. It requires a working install of MagicMirror and the MMM-Remote-Control module. It must be installed AND ACCESSIBLE ON THE SAME NETWORK AS MYCROFT.

This skill requires MMM-Remote-Control be installed and working properly on the MagicMirror.

You must configure the MagicMirror's config.js file to properly whitelist the ip address of your Mycroft.

In the MagicMirror's config.js:

Replace: address: "localhost", With: address: "0.0.0.0", and

Replace: ipWhitelist: ["127.0.0.1", "::ffff:127.0.0.1", "::1"], with ipWhitelist: ["127.0.0.1", "192.168.X.1/24"],

You can use this skill to hide or show modules, update the mirror or individual modules, refresh or restart the mirror, list installed modules, install modules by name (will still require you to configure the MagicMirror config.js by SSH or VNC for the particular skill you install), change pages of modules by either swipe commands or telling mycroft to "go to page [number]"(requires that MMM-pages be installed), restart or reboot the Raspberry Pi.

_{Created with mycroft-skills-kit v0.3.12}

[dmwilsonkc]

Just curious, what needs to happen next for this skill to make it into the marketplace?

[krisgesling]

Hey, the overall Skills Acceptance Process is outlined in our docs however we've been tweaking the specific tests conducted during the review and the latest review template can be found here.

We have a community team of skill testers as well as myself that conduct these reviews. Sometimes it is a little harder for skills like this as we need to find people who have a MagicMirror installation, or who is happy to set something up for the purposes of testing. I think real world experience is best so have put a call out to in the forum to those who are already using it. That should satisfy the functional review component.

[dmwilsonkc]

Anyone willing to test this skill to finish the submission??

[GPVM]

Meta

• Who: @GPVM / @gpvm
• Date and time: 15 Apr 2019 7:00pm CST
• Language and dialect of tester: English (Canada)

2. Information Review - accurate and understandable

This review checks the README for completeness - does it follow the README template and include all the relevant sections such as Intents, known issues, dependencies and so on?

• [ ] Icon Does the Skill have an appropriate icon that is either included in the repo or linked from an appropriate place (eg raw.githack.com not privateicons.com or rawgit.com)?

There is no icon included

• [ ] Description Are the title, short description, and long description (under 'About') clear and concise? Do they provide enough information to understand what the Skill does? Does the title have the word "skill" included? This is strongly discouraged.

The short description is very long (94 characters). We are trying to reduce this to around 50 characters because it will appear in the marketplace on the skill card and is the first thing users will read. Please consider shortening this short description to something like this: "Give voice commands to Mycroft to control a Magic Mirror.". The MMM module dependency is already detailed in your Readme Description section.

The heading Description needs to be changed to About to be parsed correctly

• [x] Examples Do the examples give you a clear understanding of how you can use the Skill? Is there a single example per dot-point?

yes

• [x] Supported Devices If relevant, are the supported devices listed? An example might be a Skill that requires the screen of the Mark II. If the section is not present, all devices are considered supported.

yes platform_picroft, or platform_mark1, or platform_plasmoid

• [ ] Categories Is there at least one category listed? At least one category must be selected for the Skill to be displayed correctly in the Mycroft Marketplace. Is the bolded category the most appropriate for this Skill? The bold category is considered the primary category and will be used for display in the Marketplace, all others are secondary and used for search results.

no categories listed

• [ ] Tags Are listed tags appropriate for this Skill?

no tags listed

• [ ] License Is an appropriate LICENSE file used in the repo - such as Apache-2.0 or GPL-3.0?

(( No License included. For instructions to add a license to a repo see https://help.github.com/en/articles/adding-a-license-to-a-repository))

[ricargoes]

hey, sorry to keep you waiting. I will start the code review now.

[ricargoes]

Meta

• Platform: Linux
• Mycroft-core version: 19.2.6
• Who: @ricargoes / @ricargoes
• Datestamp: 2019-04-26_18:38:23_CEST
• Language and dialect of tester: Spanish from Spain (review made in English)

0. Automated tests

Are all automated tests passing?

• [ ] Skill tester - Jenkins
• [x] Continuous Integration - Travis-CI

1. Code Review - secure and stable

• [x] Code Quality
  Can you understand what the code is doing? Is there inline documentation? Do you have any concerns about this code running on your machine? Are there any performance issues such as nested or infinite loops? Do you have significant concerns about the overall code quality?
  NOTE: We do not enforce PEP8 Checks on Skills

MINOR ISSUES

• Reading the code on the initialize function is hard. There are some operations which seem overcomplicated and there is no explanation.
• Attributes initialization should be done in __init__ function. There would be no difference in term of execution but it would standardize the code and improve its readability.
• In line 100 the variable installed_modules is not used
• The way to extract the ip address from the utterance seems too complicated. Mycroft core gives you tool to do it quite easily. Personally I would recommend a regex vocab to filter ip addresses and to retrieve them from it.
• In python comparison with None must be done with keywords is or is not. This happens in lines 352 and 371 at least
• In function handle_adjust_brightness_command there are a lot of levels of nesting. I understand this may be necessary but it makes the code very hard to read. It would be recommended to flatten it using auxiliary functions

• [x] Error Handling
  Are there any specific checks we make for error handling or graceful degradation?

Error handling is present in the code often to prevent the skill from crashing or behaving unexpectedly. It is specially present with the MMM api communication and with the handling of utterances which is nice. However, at some points the code uses exceptions to access conditional blocks: in function handle_Set_Ip_command a try..catch block is use to determine if the ip address was correctly given. Exceptions are meant to prevent unexpected errors and have an impact on performance. For an expected case (as the one mentioned before) an if block would be preferred.

• [x] Libraries
  Does the Skill include the correct libraries? Does it use too many libraries or dependencies?

Only python core modules are used. There are a lot of imports that are not being used, it would be recommended to remove them.

• [x] Required Dependencies
  Check requirements.txt and requirements.sh - are the required dependencies listed? If requirements.sh is used, is some form of conditional processing done to match against multiple distros? Often Skill Authors will add requirements.txt using only an “library=1.x.x” instead of “library >=1.x.x”. Check to make sure that there is an equal or greater than in the requirements to help future-proof the Skill, unless a specific version is needed.

No external dependencies are needed and there is no requirements file.

• [x] Settings
  Is the settingsmeta.json file well laid out? If settings are not used, has the default file been deleted? If it is the default file, the first setting section will be called "Options << Name of section". >

Mycroft settings are not used and no settingsmeta.json file is found so everything is ok.

However, I would recommend to integrate the ip address of the MM service in the standard mycroft settings. It would be easier for the coder to handle it and easier for the user to set it from the settings panel without having to resort to voice commands for that.

• [x] Integration Tests
  Does the skill include sufficient integration tests, included in the test folder?

There are several of them

• [x] Other Files
  Are there any other files included that are unnecessary or you are unsure of their function?

All included files are used and relevant.

Actions Required:

• None

Actions Suggested:

• Clean unused modules
• Fix minor issues
• Integrate ip.json into settingsmeta.json

Summary

Nice and complex skill that makes use of lots of mycroft possibilities. There are not major issues in the code though its readability could be better.

[dmwilsonkc]

@ricargoes Thanks for the code review! I have been busy traveling for work and unable to work on the skill for a while now. I will make some modifications and clean up the code. The ip address code will remain the same however. Personally I would rather tell Mycroft an ip address than require an entry in home.mycroft.ai, to me that kind of defeats the purpose of a voice assistant if you can't make settings with your voice, not to mention privacy.

[ricargoes]

Well, I certainly agree with you, you should be able to configure the skill by voice. However, it could be handy to be able to do it in the settings page as well. In addition, some more advanced configuration could be tricky to do it by voice (if you have to specify a port or a domain name for example). In regards of privacy, I understand most of the time the magic mirror will be accessed from the local network and exposing a local ip, while not desirable, is not a big security breach (and that only if someone manages to hack into mycroft's website).

In any case it was just a suggestion. You should use the solution you feel most comfortable with :)

[krisgesling]

Meta

• Who: @krisgesling / @gez-mycroft
• Device: Mark 1
• Mycroft-core: 19.2.11
• Datestamp: 2019-06-06_04:41:48_UTC
• Language and dialect of tester: Fluent English, Australian accent

3. Functional Review - intuitive and expected

• [x] Installation

Check that the Skill installs using voice commands. Mycroft will get the user to confirm which Skill should be installed if there is ambiguity in Skill names - such as duplicate names. If possible, name the Skill so that there is minimal duplication and/or conflict. You should also verify that the Skill name can be verbally pronounced by speaking the Skill name into the Mycroft command line several times, and reading the resulting transcriptions. Suggest alternative Skill names if it is difficult to verbally pronounce the Skill name. Please provide confirmation that the Skill was successfully installed and by what means (voice or mycroft-msm install), as well as what utterance was detected when invoking the install voice command.

Install method: mycroft-msm --latest install https://github.com/dmwilsonkc/magic-mirror-voice-control-skill Output:

 INFO - Best match (0.47): magic-mirror-voice-control-skill.dmwilsonkc by dmwilsonkc
 ERROR - MSM failed: AlreadyInstalled(magic-mirror-voice-control-skill.dmwilsonkc)

Checking that STT transcribes correctly: "install magic mirror"

 INFO - Best match (0.47): magic-mirror-voice-control-skill.dmwilsonkc by dmwilsonkc
 ERROR - MSM failed: AlreadyInstalled(magic-mirror-voice-control-skill.dmwilsonkc)

• [x] Settings
  If Skill includes a settingsmeta file - are the settings well laid out? Does the placeholder text make sense? This can also be checked on home.mycroft.ai/#/skill

NA

• [x] Dialog

Check the dialog directory to ensure that from a voice user interface perspective the dialogs read well. Alway play every dialog phrase on the command line by doing say so that you can check how the dialog is spoken. It is a good idea to run the dialog phrases through mimic.

Sometimes the dialog files will need a small tweak such as a space between words, or extra vowel sounds, to sound realistic. Sometimes words don't render as expected and alternative wording should be used. Some of the tricks you might need to use include separating words by a space - such as sub sonic instead of subsonic, or broad cast instead of broadcast.

A recommendation rather than requirement. I would consider modifying line 2 in not.connected.dialog as it is a very long statement. Perhaps something like: I was unable to connect to the magic mirror. You can say, Set I P address 192.168.X.X. Was going to ask if there was a reason not to have "set ip address to" in your SetIpKeywords.voc, but this is probably explained below.

Skill Functions

For each function of the Skill add a new checkbox with the utterance used to invoke the functionality. Confirm the output and behaviour of each. If any setup is required to perform these tests please indicate this directly before the test is described.

• [ ] "set ip address to one nine two dot one six eight dot one dot one hundred and thirty eight"

transcribed as: "set ip address to 192.168.1.1 138" I am setting the I P address to 192.168.1.1138

• [x] "set ip address one nine two dot one six eight dot one dot one hundred and thirty eight"

transcribed as: "set ip address to 192.168. 1. 138" I am setting the I P address to 192.168.1.138 I have successfully connected to the magic mirror.

• [ ] "set ip address one nine two dot one six eight dot one dot one three eight"

transcribed as: "set ip address to 192.168 o 1.1 3/8" I am setting the I P address to 192.168o1.13/8 I have successfully connected to the magic mirror.

I agree that it's great to be able to setup a skill with voice alone, however with long strings like IP addresses it does seem useful to also have an option to add it via your settings. Given this is a one time command, and then it should remain constant, I don't think this is a blocking problem however I can see some users getting pretty frustrated if they can't get it to transcribe their IP properly.

• [ ] "refresh mirror"

An error occurred while processing a request in Magic Mirror Voice Control Skill

Traceback (most recent call last):
  File "/opt/venvs/mycroft-core/lib/python3.4/site-packages/mycroft/skills/core.py", line 1065, in wrapper
    handler(message)
~~~~ft/skills/magic-mirror-voice-control-skill.dmwilsonkc/__init__.py", line 257, in handle_System_command
    response = status['status']
KeyError: 'status'

Looking at the response from remote I get either: {"success":true} or something like: {"success":false,"status":"error","reason":"unknown","info":{}} Should response = status['status'] be deleted and the following block be something like:

if 'success' in status.keys() and status['success']:
    self.speak_dialog('success')

The same error occurs on line 443 for show/hide intents.

Action Required: Resolve KeyError on status check.

• [x] "Hide clock"

As you wish Clock disappears

• [x] "Show clock"

Complete Clock appears

• [ ] "Update calendar"

An error occurred... Request: /remote?action=UPDATE&module=calendar Response:

Error: Cannot use simple-git on a directory that does not exist.
    at module.exports (/home/pi/MagicMirror/modules/MMM-Remote-Control/node_modules/simple-git/src/index.js:11:15)
    at Class.updateModule (/home/pi/MagicMirror/modules/MMM-Remote-Control/node_helper.js:836:23)
    at Class.executeQuery (/home/pi/MagicMirror/modules/MMM-Remote-Control/node_helper.js:719:22)
    at /home/pi/MagicMirror/modules/MMM-Remote-Control/node_helper.js:152:39
    at Layer.handle [as handle_request] (/home/pi/MagicMirror/node_modules/express/lib/router/layer.js:95:5)
    at next (/home/pi/MagicMirror/node_modules/express/lib/router/route.js:137:13)
    at Route.dispatch (/home/pi/MagicMirror/node_modules/express/lib/router/route.js:112:3)
    at Layer.handle [as handle_request] (/home/pi/MagicMirror/node_modules/express/lib/router/layer.js:95:5)
    at /home/pi/MagicMirror/node_modules/express/lib/router/index.js:281:22
    at Function.process_params (/home/pi/MagicMirror/node_modules/express/lib/router/index.js:335:12)

Action Required: I'm assuming this is because I haven't setup additional calendars, however it would be worth adding a proper error message that something went wrong with MagicMirror, rather than Mycroft.

• [ ] "Turn off weather" "hide weather" "turn on weather" "show weather" "show current weather" "show weather forecast"

Always get success message but nothing happens. Discovered I needed to add an API key, but still nothing happens. The params payload appears to get the correct action but returns blank for module name. Looks like a request to /remote?action=SHOW&module= returns {"success":true}. Blank calls returning true seems odd behaviour from MM but probably worth checking for. Any idea what's causing the weather based utterances not to return a module name?

Action Required: weather based utterances should match a module name. I only set MM up to test this Skill so potentially have missed some things so let me know if I've got this wrong.

Actions Required

First off, this is an excellent Skill and I've already seen a number of videos from different people using it. Thank you for all the work that has gone into it!

The items that require some action are:

• Resolve KeyError on status check at least on lines 257 and 443.
• Handle errors being returned as responses.
• Fix weather module utterances

Also see the IP address comment above. I did get it to work on the 2nd go, but in testing, the majority of my attempts failed.

Thanks again!

[krisgesling]

Have marked as test override since there's no way Jenkins will successfully run without a MagicMirror to connect to.