[mycroft] Initial contribution #11040

dalgwen · 2021-07-21T10:33:10Z

[mycroft] Initial Contribution

"Mycroft is the world’s leading open source voice assistant. It is private by default and completely customizable."
This binding will connect to Mycroft A.I. in order to control it or react to event by listening on the message bus.
Possibilies include :

Press a button in OpenHAB to wake Mycroft without using a wake word.
Simulate a voice command to launch a skill, as if you just spoke it
Send some text that Mycroft will say (Using its Text To Speach service)
Control the music player
Control the sound volume of Mycroft
React to all the aforementioned events ...
... And send/receive all other kind of messages on the message bus

Here a link to the forum thread I opened.
Here a link to the JAR builded against 3.2.0

fixes #11033

Signed-off-by: Gwendal ROULLEAU [email protected]

t2000

Thank you VERY much for contributing this binding.

I myself am still waiting for my Mark II...

However, its nice to see openHAB support for it. I just stumbled across your PR and since you had to wait for a while already, I thought I can help you in getting it merged.

Usually before I go deeper in the code, I have a look at the general idea on how it is implemented. Hence, I left my first comments all on the README file, so you can have a look at them and we can discuss them or you can directly address them if you do not have further questions.
So far the overall approach looks quite good which is why I only had some minor comments.

Once we agree how things should be implemented, I will go through the code and see if there are things which I can help you to improve as well.

bundles/org.openhab.binding.mycroft/README.md

kaikreuzer · 2021-11-10T20:24:54Z

Thanks for helping out on the review here @t2000! I'll happily be the second reviewer with the merge rights afterwards. 😄
Just as you, I am waiting since 3 years or so for my Mark II, still hoping it's gonna be shipped one day!
And of course many thanks to @dalgwen for the effort to implement this integration!

dalgwen · 2021-11-14T22:05:16Z

Many thanks to you, @t2000 and @kaikreuzer, for taking the time to review this binding. I'm happy to see it could be of some use to you, and I hope the situation at Mycroft will be resolved soon for the mark II to go to market and to your mailboxes !
I will commit some/all of the requested changes in a few minutes.

bundles/org.openhab.binding.mycroft/README.md

t2000

I found some time and went through your code.

The overall quality of the code looks quite good, so I only have some minor comments.

Please have a look and address them in separate commits or just reply to my comment if anything is unclear.

bundles/org.openhab.binding.mycroft/README.md

...enhab.binding.mycroft/src/main/java/org/openhab/binding/mycroft/internal/MycroftActions.java

...enhab.binding.mycroft/src/main/java/org/openhab/binding/mycroft/internal/MycroftHandler.java

bundles/org.openhab.binding.mycroft/src/main/resources/OH-INF/binding/binding.xml

bundles/org.openhab.binding.mycroft/src/main/resources/OH-INF/thing/thing-types.xml

t2000 · 2021-11-29T16:36:59Z

Apart from that last question: #11040 (comment) I am fine with that MR now. Thank you very much for your efforts so far!

t2000 · 2021-11-30T07:34:23Z

Thanks for your answers, could you please rebase your commit onto the latest main branch again to resolve the conflicts?

dalgwen · 2021-12-01T08:18:50Z

Rebase done :)

t2000 · 2021-12-02T07:19:05Z

@dalgwen Thank you very much for your contribution. Unfortunately I am not one of the maintainers, but I am sure that your code is now in a pretty good state to be merged ;)

@kaikreuzer Since you have already volunteered to have a look at this PR: The stage is yours. I am almost sure that you will find some more tiny glitches, because you always do. :-) So please have a look at this PR, thanks.

kaikreuzer

Thanks for the contribution @dalgwen, it looks really good, I only have minor remarks, but nothing serious.

I am almost sure that you will find some more tiny glitches, because you always do.

I am always trying not to be too picky, but I somehow cannot help it - I end up with quite some list of further comments. 😉 But thanks for your review, @t2000, much appreciated!

bundles/org.openhab.binding.mycroft/README.md

kaikreuzer · 2021-12-03T22:09:01Z

bundles/org.openhab.binding.mycroft/README.md

+
+## Supported Things
+
+The only thing managed by this binding is a Mycroft instance.


Can you name directly the thing type id here?

Could you also elaborate what an "instance" exactly could be (hardware / software)?

I didn't find a more suited world to describe "a Mycroft" (as you guessed, the world should encompass the hardware version, and the picroft version, and every kind of Mycroft running on any computer ?)
Do you have something in mind, or should I just go for a brief description behind "instance" ? I think will put a description, tell me if you'd rather something else.
"The only thing managed by this binding is a Mycroft instance. (e.g. Mark I/II, Picroft, or any other variant)"

bundles/org.openhab.binding.mycroft/README.md

kaikreuzer · 2021-12-03T22:55:00Z

bundles/org.openhab.binding.mycroft/src/main/resources/OH-INF/thing/thing-types.xml

+			<channel id="speak" typeId="speak-channel"/>
+			<channel id="utterance" typeId="utterance-channel"/>
+			<channel id="player" typeId="player-channel"/>
+			<!-- <channel id="volume" typeId="volume-channel"/> -->


Suggested change

bundles/org.openhab.binding.mycroft/src/main/resources/OH-INF/thing/thing-types.xml

This binding will connect to Mycroft A.I. in order to control it or react to event by listening on the message bus. Signed-off-by: Gwendal ROULLEAU <[email protected]>

Signed-off-by: Gwendal ROULLEAU <[email protected]>

Signed-off-by: Gwendal Roulleau <[email protected]>

…el types Signed-off-by: Gwendal Roulleau <[email protected]>

Signed-off-by: Gwendal Roulleau <[email protected]>

Delete the duck channel Workaround for Mycroft bad management of volume. Signed-off-by: Gwendal Roulleau <[email protected]>

dalgwen · 2021-12-31T01:19:01Z

Unless I missed something or the build failed, I think I dealt with all the comments 🎉
(one question remains : should I reactivate the volume channel ?).
I hope I didn't make any more mistakes.

Signed-off-by: Gwendal Roulleau <[email protected]>

kaikreuzer · 2022-01-08T21:16:19Z

Thanks for your updates!

one question remains : should I reactivate the volume channel ?

I would say so, yes - thanks for your detailed explanation from which it seems as if this channel will be useful.

Signed-off-by: Gwendal Roulleau <[email protected]>

kaikreuzer

Thanks, looks all wonderful to me now. 😄
@t2000 Do you want to have a last look before merging?

t2000 · 2022-01-10T12:07:08Z

@kaikreuzer I had a brief look on the latest changes. LGTM.

@dalgwen Thank you for your efforts and implementing the volume hack.

I am waiting for my Mark II to arrive, so I can finally test it (even though it doesn't look as cute as when it was originally announced and thus my gf doesn't like the look of it anymore...).

t2000

LGTM

lolodomo · 2022-01-11T07:46:37Z

Please create a subsequent PR to add the default translation properties file.
You can produce it simply by running this command : mvn i18n:generate-default-translations

dalgwen · 2022-01-11T09:30:26Z

Please create a subsequent PR

I take this opportunity : Thank you for thinking about thing like that, and keeping the project clean !
Done in #12020.

lolodomo · 2022-01-11T12:22:14Z

And finally, you could add your binding's logo to the openHAB website. See https://www.openhab.org/docs/developer/bindings/#add-your-binding-s-logo-to-the-openhab-website

This binding will connect to Mycroft A.I. in order to control it or react to event by listening on the message bus. Signed-off-by: Gwendal Roulleau <[email protected]>

This binding will connect to Mycroft A.I. in order to control it or react to event by listening on the message bus. Signed-off-by: Gwendal Roulleau <[email protected]> Signed-off-by: Nick Waterton <[email protected]>

This binding will connect to Mycroft A.I. in order to control it or react to event by listening on the message bus. Signed-off-by: Gwendal Roulleau <[email protected]>

This binding will connect to Mycroft A.I. in order to control it or react to event by listening on the message bus. Signed-off-by: Gwendal Roulleau <[email protected]> Signed-off-by: Andras Uhrin <[email protected]>

dalgwen requested a review from a team as a code owner July 21, 2021 10:33

dalgwen changed the title ~~[mycroft] Mycroft binding - Initial contrib (#11033)~~ [mycroft] Mycroft binding - Initial contrib Jul 21, 2021

dalgwen force-pushed the 11033-mycroft_binding_initial_contribution branch 2 times, most recently from d8c1dd7 to 4691317 Compare July 21, 2021 10:53

dalgwen changed the title ~~[mycroft] Mycroft binding - Initial contrib~~ [mycroft] Initial contribution Jul 21, 2021

fwolter added the new binding If someone has started to work on a binding. For a new binding PR. label Jul 25, 2021

t2000 requested changes Nov 10, 2021

View reviewed changes

dalgwen force-pushed the 11033-mycroft_binding_initial_contribution branch 2 times, most recently from 706cfc8 to fefa134 Compare November 14, 2021 22:28

kaikreuzer reviewed Nov 15, 2021

View reviewed changes

bundles/org.openhab.binding.mycroft/README.md Outdated Show resolved Hide resolved

t2000 requested changes Nov 20, 2021

View reviewed changes

dalgwen force-pushed the 11033-mycroft_binding_initial_contribution branch from 44df6b5 to c8ff528 Compare November 27, 2021 12:23

dalgwen force-pushed the 11033-mycroft_binding_initial_contribution branch from c8ff528 to 0bca9f4 Compare December 1, 2021 08:12

kaikreuzer requested changes Dec 3, 2021

View reviewed changes

dalgwen added 10 commits December 27, 2021 08:54

[mycroft] Mycroft binding - Initial contrib (openhab#11033)

25982dc

This binding will connect to Mycroft A.I. in order to control it or react to event by listening on the message bus. Signed-off-by: Gwendal ROULLEAU <[email protected]>

Adding some actions

b67b7d0

Signed-off-by: Gwendal ROULLEAU <[email protected]>

Some corrections after review.

7a784bc

Signed-off-by: Gwendal ROULLEAU <[email protected]>

Remove volume channel information

a59a7c0

Signed-off-by: Gwendal Roulleau <[email protected]>

Removing Actions

223601b

Signed-off-by: Gwendal Roulleau <[email protected]>

Fix misuse of initialization boolean to prevent reconnection issue

59e086d

Signed-off-by: Gwendal Roulleau <[email protected]>

Length restrictions on websocket name

f798010

Signed-off-by: Gwendal Roulleau <[email protected]>

Rewrite the scheduled reconnection

1c31133

Signed-off-by: Gwendal Roulleau <[email protected]>

Remove two empty lines

423ccda

Signed-off-by: Gwendal Roulleau <[email protected]>

Rewriting a comment

b2e10a8

Signed-off-by: Gwendal Roulleau <[email protected]>

dalgwen force-pushed the 11033-mycroft_binding_initial_contribution branch from 2c5c865 to c00f286 Compare December 31, 2021 01:06

dalgwen added 5 commits December 31, 2021 02:12

Apply suggestion from code review : javadoc on messages classes

a829e11

Signed-off-by: Gwendal Roulleau <[email protected]>

Apply suggestion from code review : javadoc

4dd61fa

Signed-off-by: Gwendal Roulleau <[email protected]>

Apply suggestion from code review : description, and use system chann…

b456248

…el types Signed-off-by: Gwendal Roulleau <[email protected]>

Refactor message parsing to be easier to read

57d023a

Signed-off-by: Gwendal Roulleau <[email protected]>

Refactor volume management

684149f

Delete the duck channel Workaround for Mycroft bad management of volume. Signed-off-by: Gwendal Roulleau <[email protected]>

dalgwen force-pushed the 11033-mycroft_binding_initial_contribution branch from c00f286 to 684149f Compare December 31, 2021 01:13

dalgwen added 2 commits January 6, 2022 19:20

Add french i18n

b11d9da

Signed-off-by: Gwendal Roulleau <[email protected]>

Code analysis tools fix --> 2010-2022

3451234

Signed-off-by: Gwendal Roulleau <[email protected]>

Reactivation of the Mycroft volume channel

facd7be

Signed-off-by: Gwendal Roulleau <[email protected]>

kaikreuzer added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Jan 9, 2022

kaikreuzer approved these changes Jan 9, 2022

View reviewed changes

t2000 approved these changes Jan 10, 2022

View reviewed changes

kaikreuzer merged commit 40b5932 into openhab:main Jan 10, 2022

kaikreuzer added this to the 3.3 milestone Jan 10, 2022

dalgwen mentioned this pull request Jan 11, 2022

[mycroft] Init i18n #12020

Merged

dalgwen mentioned this pull request Jan 11, 2022

[mycroft] Add binding logo openhab/openhab-docs#1732

Merged

dalgwen deleted the 11033-mycroft_binding_initial_contribution branch December 29, 2022 22:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[mycroft] Initial contribution #11040

[mycroft] Initial contribution #11040

dalgwen commented Jul 21, 2021 •

edited

Loading

t2000 left a comment

kaikreuzer commented Nov 10, 2021

dalgwen commented Nov 14, 2021 •

edited

Loading

t2000 left a comment

t2000 commented Nov 29, 2021

t2000 commented Nov 30, 2021

dalgwen commented Dec 1, 2021

t2000 commented Dec 2, 2021

kaikreuzer left a comment

kaikreuzer Dec 3, 2021

kaikreuzer Dec 3, 2021

dalgwen Dec 20, 2021

kaikreuzer Dec 3, 2021

dalgwen Dec 31, 2021

dalgwen commented Dec 31, 2021

kaikreuzer commented Jan 8, 2022

kaikreuzer left a comment

t2000 commented Jan 10, 2022

t2000 left a comment

lolodomo commented Jan 11, 2022

dalgwen commented Jan 11, 2022

lolodomo commented Jan 11, 2022


		## Supported Things

		The only thing managed by this binding is a Mycroft instance.

	<!-- <channel id="volume" typeId="volume-channel"/> -->
	<!-- <channel id="volume" typeId="system.volume"/> -->

[mycroft] Initial contribution #11040

[mycroft] Initial contribution #11040

Conversation

dalgwen commented Jul 21, 2021 • edited Loading

t2000 left a comment

Choose a reason for hiding this comment

kaikreuzer commented Nov 10, 2021

dalgwen commented Nov 14, 2021 • edited Loading

t2000 left a comment

Choose a reason for hiding this comment

t2000 commented Nov 29, 2021

t2000 commented Nov 30, 2021

dalgwen commented Dec 1, 2021

t2000 commented Dec 2, 2021

kaikreuzer left a comment

Choose a reason for hiding this comment

kaikreuzer Dec 3, 2021

Choose a reason for hiding this comment

kaikreuzer Dec 3, 2021

Choose a reason for hiding this comment

dalgwen Dec 20, 2021

Choose a reason for hiding this comment

kaikreuzer Dec 3, 2021

Choose a reason for hiding this comment

dalgwen Dec 31, 2021

Choose a reason for hiding this comment

dalgwen commented Dec 31, 2021

kaikreuzer commented Jan 8, 2022

kaikreuzer left a comment

Choose a reason for hiding this comment

t2000 commented Jan 10, 2022

t2000 left a comment

Choose a reason for hiding this comment

lolodomo commented Jan 11, 2022

dalgwen commented Jan 11, 2022

lolodomo commented Jan 11, 2022

dalgwen commented Jul 21, 2021 •

edited

Loading

dalgwen commented Nov 14, 2021 •

edited

Loading