Contributing to Home Assistant

Archive - Mycroft Project Skills
1

Hi all,

I’d like to contribute to the Home Assistant skill, specifically adding the ability to say things like “Set the office lights to 50%” for dimmers.

However, I notice in the repo that the latest commits are pretty old, and there are a lot of open PRs. Then there also seems to be a Common IOT branch with some other features but not a lot of details.

If I plan to develop, I don’t want to go down a road and then have the direction change just because I wasn’t aware of it happening already :slight_smile:

So, that being said, what branch should I base my work off of? And what branch is the official marketplace skill running off of?

Thanks!

1 Like
2

Hi there,

Thanks for starting this thread! For some reason I haven’t been getting notifications on that repo so haven’t seen these open issues and PR’s until now.

The Common IoT work was done late last year but has some small snippets that require Python 3.6+. We are still supporting devices running 3.5 so it couldn’t be merged without modification. This Common IoT branch of this Skill was created as an example Skill for the IoT Framework, however as that Framework doesn’t yet exist in core, it can’t be used.

The current Marketplace Skill is based off the 20.02 branch. Though there are a couple of commits not yet pushed to the Marketplace there too.

@stratus has more recently started looking at this Skill and the Common IoT framework to see if we can polish off the final pieces and get it working. So he might want to weigh in here on whether it’s worth holding out for the Common IoT piece, or to continue building on the 20.02 branch.

3

Ahh, thank you for the insight. That all makes total sense. I will wait to hear from @stratus but begin getting familiar with the code base.

4

Of course, as I start to go through the code base, I see this:

It looks like the capability is there, it’s just not working for me. :wink:

5

So as I go through this further, could someone explain why some vocabs are handled via padatious? For instance:

set.light.brightness.intent:
set brightness of {{entity}} to {{brightnessvalue}} percent

While others work via:

LightDimVerb.voc:
dim|dimmer|dark|darken|lower|down
+
dimming.rx:

(set) (?:brightness of )?(?P<Entity>.*) (to) (?P<BrightnessValue>\d*)(?: percent)?
(dim|brighten|increase|decrease) (?:brightness of )?(?P<Entity>.*?) (?:by)? (?P<BrightnessValue>\d*)?(?: percent)?

I don’t yet understand why one would be preferred over another (or why both exist in concert at all). Thanks!

6

Went ahead and started with a bugfix PR before I dive in further. Let me know if anything needs to be adjusted in how I’m contributing. Thanks!

1 Like
7

Thanks Fmstrat!

I’ve let Stratus know about this thread in case he’s not getting notifications, but he might be snowed under with work so if we haven’t heard I’ll try take a look next week.

8

I’ll take a look at this today.

To answer the question about the intent parsers, without examining the code closely, you would use both in concert when you needed to be context aware such as with the Adapt parser while on the other hand not needing the complexity of regex in other situations. Regex in Mycroft are known to be… particularly sensitive and so are not always the best choice.

Shameless plug I just recently wrote about this in a series on Opensource.com. The intent parser article is here https://opensource.com/article/20/6/mycroft-intent-parsers

2 Likes
9

I left a comment on the commit. I don’t think its worthy of a request for change but lets discuss here what you think about my suggestion

10

It’s taken me a bit of time to get back to the PR from Christopher due to work things evolving underneath me. I hope to start tackling that today.

1 Like
11

Appologies, I totally missed the comment here since my emails from this forum are still dropped due to the spam listing on the sending mail server’s IP. In any event, as you probably saw in the PR, I thought the suggestion you had was spot on and made the alteration in the commit. Thanks!

1 Like
12

Hello Fmstrat, check also my PR #38. With others, we made lot of changes. Started with rewrite all intens to padatious and applied fixes to 4 issues from git. Your change is also already included. More people test it, more changes to get it approved. Thanks

1 Like