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

Add Mozilla WebThings Gateway skill #1086

Merged
merged 1 commit into from Oct 14, 2019

Conversation

hobinjk
Copy link
Contributor

@hobinjk hobinjk commented Sep 4, 2019

Name of your skill: Mozilla WebThings Gateway

Description:

Enables control of a Mozilla WebThings Gateway. This includes simple IoT functions like turning on and off lights connected to the gateway.

Checklist:

+[submodule "NAME OF YOUR SKILL"]
+	path = name-of-your-skill
+	url = URL.FOR.YOUR.SKILL.git

There are currently no automated tests since meaningful automated tests would require mocking the OAuth process which would be very difficult

@krisgesling
Copy link
Contributor

Hi James,

Thanks for submitting your first Skill! 🎉

To be included in the Marketplace the Skill goes through three reviews by our Skill Testing Team. You can read more about these reviews and the overall Skills Acceptance Process here: https://mycroft.ai/documentation/skills/skills-acceptance-process/

Also I had a quick look at the failing integration test and it seems to be an unrelated issue. I'll re-run this later.

If you have push any updates in the meantime be sure to run mycroft-msk submit /opt/mycroft/skills/your-skill-dir to update this pull request. Also feel free to post here or in the Skills channel on Chat if you have any questions.

Looking forward to trying this out 🙂

@krisgesling krisgesling added needs validation Needs validatiion by another Skills Team Member or Community Member trying it new New Skill (not update to existing Skill) labels Sep 5, 2019
@forslund
Copy link
Collaborator

forslund commented Sep 6, 2019

Run test

@kgiori
Copy link

kgiori commented Sep 27, 2019

Any updates on testing and publication of this skill?

@krisgesling
Copy link
Contributor

Hi Kathy, we've been focused on the 19.08 release lately - which has now been released 🎉

So I'll give the Skill Testing Team a nudge and see if we can get this moving quickly 🙂

@krisgesling
Copy link
Contributor

Meta

  • Platform: Debian Linux / Mark 1
  • Mycroft-core version: 19.08.0
  • Who: @krisgesling / @gez-mycroft
  • Datestamp: 2019-09-30_14:09:27_UTC
  • Language and dialect of tester: English, Australian accent

0. Automated tests

Are all automated tests passing?

  • Skill tester - Jenkins
  • Continuous Integration - Travis-CI

1. Code Review - secure and stable

  • 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

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

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

  • 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.

  • Settings
    Is the settingsmeta 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". >

The top-level "name" attribute has been deprecated, this is now pulled from the README to ensure consistency between the Marketplace and Skill settings. It will show a deprecation warning in the logs for users but otherwise will just be ignored. This can be removed in a future update.

  • Integration Tests
    Does the skill include sufficient integration tests, included in the test/intent folder?

No integration tests.
It can be a challenge to test Skills relying on independent systems. A simple test to ensure the Skill loads, triggers an intent and correctly parses the utterance would be:

{
    "utterance": "turn kitchen on",
    "intent_type": "CommandIntent",
    "expected_data": {
      "Action": "turn",
      "Type": "on"
    }
}

Also worth noting that external calls can also be mocked however the documentation for this is still being integrated into our primary docs.
Action required: please include at least one simple integration test.

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

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)?

  • 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.

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

  • 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.

  • 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.

  • Tags
    Are listed tags appropriate for this Skill?

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

No license specified.

Action required: please add an appropriate license to your Skill's repository.

3. Functional Review - intuitive and expected

  • 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 install https://github.com/mozilla-iot/mozilla-webthings-gateway-skill
Output:

INFO - building SkillEntry objects for all skills
INFO - Downloading skill: https://github.com/mozilla-iot/mozilla-webthings-gateway-skill
INFO - Installing system requirements...
INFO - Successfully installed mozilla-webthings-gateway-skill

Checking that STT transcribes correctly
Utterance: ['install mozilla web things gateway']

 Best match (0.55): mozilla-webthings-gateway-skill.mozilla-iot by mozilla-iot
  • 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

  • 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.

Suggestions only - not required:
Currently the dialog will not be localizable for those who do not speak English. We recommend storing all vocabulary and dialog in their respective files rather than embedded in the code to enable our Community to provide translations back to your Skill. This can be considered for a future update.
Consider rewording needs.configure to direct users to their Skill settings at home.mycroft.ai.

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.

  • "turn the dining table on"

Okay, turning the dining table on

  • "turn the dining table light off"

Okay, turning the dining table off

  • "turn on the dining table light"

Okay, turning the dining table on

  • "dim the dining table light"

I don't understand

It seems that the intent currently requires a Type, but these utterances don't need any words from Type.voc. Changing the intent handler to be .optionally('Type') allows the Skill to trigger. Alternatively you could add extra vocab to Type.voc.

After setting Type to be optional

  • "brighten the dining table light"

Sorry, I'm afraid I can't do that

This may be simply that the LIFX plugin doesn't yet support dimming/brightening?

  • Turn the dining table light red

Okay, turning the dining table light red

Summary

Thanks for this excellent Skill. I've been meaning to take some time to checkout the WebThings Gateway and was definitely not disappointed - so quick and easy to get up and running!

There are a couple of small things that we need to address before we can add this to the Marketplace. I've also noted a few things to consider for future updates but they will not prevent the Skill from being accepted.

Actions required:

  • Add at least one integration test
  • Add an appropriate license file to the Skill's repository
  • Check dim/brighten examples and requirement of type

Suggestions:

  • Currently the dialog will not be localizable for those who do not speak English. We recommend storing all vocabulary and dialog in their respective files rather than embedded in the code to enable our Community to provide translations back to your Skill. This can be considered for a future update.
  • Consider rewording needs.configure.dialog to direct users to their Skill settings at home.mycroft.ai.

Once you have completed any updates, you can update this pull request by re-running:
mycroft-msk submit /path/to/skill/directory

@krisgesling krisgesling added waiting Waiting on the Skill Author to respond and removed needs validation Needs validatiion by another Skills Team Member or Community Member trying it labels Sep 30, 2019
@hobinjk hobinjk force-pushed the add-mozilla-webthings-gateway branch 2 times, most recently from 03ac07a to 652c654 Compare October 3, 2019 04:13
@krisgesling
Copy link
Contributor

run test

@krisgesling
Copy link
Contributor

Hi James, thanks for making those updates.

I see now that dimming is meant to include a percentage to dim, wondering if we can add this to the example utterances in the README, as these get parsed and displayed as example utterances in the Marketplace.

I'll also look into what's happening with our integration tests but that's certainly not being caused by this Skill, so we can override that.

@hobinjk hobinjk force-pushed the add-mozilla-webthings-gateway branch from 652c654 to 4e29b1c Compare October 12, 2019 16:05
@hobinjk hobinjk force-pushed the add-mozilla-webthings-gateway branch from 4e29b1c to 871e364 Compare October 12, 2019 16:07
@hobinjk
Copy link
Contributor Author

hobinjk commented Oct 12, 2019

@krisgesling Thanks for the update! Sorry for the delay, I've updated the "dim" example to include a percentage.

@krisgesling krisgesling added approved Ready to be merged override autotester Skills Team decided to override the AutoTester (tester bugs) and removed waiting Waiting on the Skill Author to respond labels Oct 14, 2019
@krisgesling
Copy link
Contributor

Thanks James,

The failing test is unrelated to your Skill, so have overridden that for this PR.

@krisgesling krisgesling merged commit baf67ac into MycroftAI:19.08 Oct 14, 2019
@benfrancis
Copy link

Hi @krisgesling,

I have set up a Picroft but don't see this skill in the Skills Marketplace at https://market.mycroft.ai/skills

I was able to install it on the command line, but it didn't appear in the web interface to enable me to configure it.

Are there any more actions on our side needed to get this live and configurable?

Thanks

@krisgesling
Copy link
Contributor

Hi Ben,

Thanks for flagging it, we've resolved the issue and it's now displaying in the Marketplace.

You can also install the Skill by saying "Hey Mycroft, install Mozilla web things gateway".
Variations like "Mozilla gateway", "Mozilla web things" etc should also work.

@benfrancis
Copy link

Hi @krisgesling,

Thanks very much for fixing that. I am now able to install and configure the skill via the web interface. However, I'm finding that none of the voice commands work due to a bug WebThingsIO/gateway#1624 (comment)

We're trying to figure out what's causing this bug. Is it possible that the integer constants which are passed to DeviceApi().get_oauth_token() are somehow out of date and need updating?

@krisgesling
Copy link
Contributor

Hey Ben, these are set to not expire, so shouldn't change unless we receive a request to do so.

In the linked issue, I couldn't see the ValueError you mentioned in the logs you posted, do you still have those logs? Would be good to see the ouput.

@benfrancis
Copy link

@krisgesling wrote:

Hey Ben, these are set to not expire, so shouldn't change unless we receive a request to do so.

Good to know, thanks.

In the linked issue, I couldn't see the ValueError you mentioned in the logs you posted, do you still have those logs? Would be good to see the ouput.

The ValueError exception didn't appear in the logs, I only managed to figure out that it was being thrown by adding lots of print statements to the code.

Is there any more information I can get you that would help diagnose the problem?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Ready to be merged new New Skill (not update to existing Skill) override autotester Skills Team decided to override the AutoTester (tester bugs)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants