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

[mielecloud] Initial contribution of the Miele Cloud binding #9146

Conversation

BjoernLange
Copy link
Contributor

This is the initial contribution of the Miele Cloud binding as announced in #9106.

Further resources associated with the contribution:

The binding provides openHAB with access to Miele@home appliances connected via the Miele cloud. These devices communicate over WiFi (as compared to the devices communicating via the XGW3000 gateway which is using ZigBee). A detailed description of the binding's capabilities is given in the README.

All contributors will sign-off the squashed commit once necessary changes have been made and the PR has been approved.

Please note: We tested the binding with the most recent openHAB 3.0 milestone build (3.0.0.M3) and discovered a binary incompatibility with Inbox.approve because of which we were unable to complete our tests. We still consider the binding "ready for review" and will do extensive testing once the problem is solved.

@cpmeister cpmeister added the new binding If someone has started to work on a binding. For a new binding PR. label Nov 28, 2020
@kaikreuzer
Copy link
Member

Thanks @BjoernLange! Could you please also make sure to sign-off your commits, so that the DCO check passes? See https://www.openhab.org/docs/developer/contributing.html#sign-your-work

@BjoernLange BjoernLange force-pushed the 9106-miele-cloud-binding-initial-contribution branch from 1b3932d to 6582025 Compare November 30, 2020 07:59
@BjoernLange
Copy link
Contributor Author

Thank you for the hint @kaikreuzer! We didn't expect sign-off to be a prerequisite for review. The commit is signed now.

Copy link
Member

@fwolter fwolter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed the readme so far. The Channel descriptions don't state any units. Do you use Units of Measure for temperatures and so on?

For a detailed walk through the account configuration, see [Account Configuration Example](#account-configuration-example).

Once a Miele account is paired, all supported appliances are automatically discovered as individual things and placed in the inbox.
They can then be paired with your favorite management UI, e.g. the PaperUI.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you remove the reference, as PaperUI will be removed in OH3?


![Pairing Successful](doc/pairing-success.png)

Once the bridge instance is `ONLINE`, you can either pair things for all appliances via your favorite management UI, e.g. the Paper UI, or use a things-file.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PaperUI

Comment on lines +111 to +99
| remote_control_can_be_started | Switch | Indicates if this device can be started remotely. | Yes |
| remote_control_can_be_stopped | Switch | Indicates if this device can be stopped remotely. | Yes |
| remote_control_can_be_paused | Switch | Indicates if this device can be paused remotely. | Yes |
| remote_control_can_be_switched_on | Switch | Indicates if the device can be switched on remotely. | Yes |
| remote_control_can_be_switched_off | Switch | Indicates if the device can be switched off remotely. | Yes |
| remote_control_can_set_program_active | Switch | Indicates if the active program of the device can be set remotely. | Yes |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of the Channels seems to describe properties rather than changing values. You could make those properties. Same might apply for the plate present Channels.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We decided to use channels for the remote_control_* values because these are dynamically updated by the cloud based on the device state. Because of this we assumed them to be part of the "state" rather than a fixed "property". Do you think a property would be more suitable even if it is updated frequently? Our intend was to make scripting more comfortable and allow for generic rules. As far as I know properties cannot be accessed from within rules, is that still correct?

The plate_is_present channels are a different story, though. Making these properties is reasonable. What would be the preferred way in that case? Having one property plate count vs. having six boolean properties?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Channel is just right for remote_control_*, then.

If the plate count is only an information, I'd go for plate_count.

Comment on lines +117 to +101
| spinning_speed | String | The spinning speed of the active program. | Yes |
| spinning_speed_raw | Number | The raw spinning speed of the active program. | Yes |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you take a look at Channel state descriptions? That might make the _raw Channels unnecessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume you are referring to the <options> tag specifically, but using this is in our opinion not reasonably possible at the moment. In order to provide a full description of the possible options the list would have to be either static or accessible via the cloud. Unfortunately, both is not the case. I will use the active program / program ID as an example but the following outline applies to all channels that we defined raw channels for.

The list of possible options for the active program depends on the kind of device (oven, washing machine, ...) and on the specific model. Additionally, for some devices custom programs can be defined. Thus, we cannot statically list all possible values in the binding.

Also, there is no endpoint in the Miele 3rd Party API that supplies a list of possible values including translations. The only supplied information is the localized value of the currently selected / active program. We considered to dynamically alter the channel with these values once they are received but found it to be rather confusing than helpful from a user's perspective.

Have other bindings had similar problems? How did they solve them? Or is our solution fine given these circumstances? If you have a good advice we will happily apply it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you don't have a mapping of the options, it's barely possible to use state descriptions. I don't have any better solution.

Comment on lines 171 to 175
| Channel ID | Channel Type ID |
| ---------- | --------------- |
| remote_control_can_be_started | remote_control_can_be_started |
| remote_control_can_be_stopped | remote_control_can_be_stopped |
| remote_control_can_be_switched_on | remote_control_can_be_switched_on |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Channel Type ID column seems to be redundant. Would a list be sufficient?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, it is redundant in almost all cases. The sole exception is the hob which uses multiple channels of the same type for the plate_power_step channels. So, better make a list for all other devices and leave the table for the hob?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that'd be better. I'm wondering why the Channel Type ID is important to the user at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to define the correct type of item in .items files the user needs to know the type of the channel which is defined by the channel type. Therefore, we included it in the documentation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood. You could add the Item type to the Channels in the lists which differ from above table.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed everything to lists, the only special case is the hob where I used some additional text to explain.

@BjoernLange
Copy link
Contributor Author

I reviewed the readme so far. The Channel descriptions don't state any units. Do you use Units of Measure for temperatures and so on?

No, we do not use units of measure at the moment.

@fwolter
Copy link
Member

fwolter commented Nov 30, 2020

Can you at least make the temperature Channels make use of QuantityType?

@fwolter fwolter self-assigned this Dec 1, 2020
<features name="org.openhab.binding.mielecloud-${project.version}" xmlns="http://karaf.apache.org/xmlns/features/v1.4.0">
<repository>mvn:org.openhab.core.features.karaf/org.openhab.core.features.karaf.openhab-core/${ohc.version}/xml/features</repository>

<feature name="openhab-binding-mielecloud" description="mielecloud Binding" version="${project.version}">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the capitalization correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, that one slipped us.

/**
* Constants for all channels.
*/
@NonNullByDefault({})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this annotation has any effect? Same for below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The build for openHAB 2.5.x used to warn on internal classes that they should be annotated with @NonNullByDefault although the containing class was annotated. This was our workaround. I checked and it seems it isn't necessary any more so I will remove these annotations.

* @author Roland Edelhoff - Initial contribution
*/
@NonNullByDefault
public class OAuthException extends RuntimeException {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you make this an unchecked exception by intention?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we actually prefer unchecked exceptions. Shall we change it to a checked one?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would fit better to the other code in this repo, but I won't insist on it. As you're using many lambdas converting could be painful.

Comment on lines 42 to 43
@Nullable
private OAuthFactory oauthFactory;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could inject this via the constructor to avoid the Nullable annotation. The framework takes care, that this is not null. Same for MieleCloudConfigService.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for pointing this out! It saves a lot of trouble. Is this also possible for MieleHandlerFactory?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

} else {
return Optional.of(tokenResponse.getAccessToken());
}
} catch (Exception e) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What type of exception do you expect here? Can you specify the concrete type?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We expect multiple exceptions here:

  • org.openhab.binding.mielecloud.internal.auth.OAuthException: If the OAuthClientService is not available
  • IOException: If a network/IO issue occurs while getting the access token response
  • OAuthErrorException / OAuthResponseException: If an OAUTH error response was returned by the cloud while getting the access token response
  • OAuthException: For other exceptions that occurred while getting the access token response

The latter three come from OAuthClientService.getAccessTokenResponse() (btw. the documentation seems to be outdated there). We want to handle all these exceptions the same way and their common parent is, well, Exception.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general you want to avoid catching Exception unless you are handling all cases of Exception. If you have multiple exception types and want to handle them the same way you use multi-catch:
catch (OAuthException | IOException | OAuthErrorException | OAuthResponseException( {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree to @boc-tothefuture . Catching Exception catches also all RuntimeExceptions

return Optional.of(tokenResponse.getAccessToken());
}
} catch (Exception e) {
logger.info("Cannot obtain access token from persistent storage.");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logging to info should be used sparsely e.g. a newly started component or a user file that has been loaded. This could be debug or warn.

Comment on lines 80 to 77
logger.warn("Failed to remove refresh listener: OAuth client service is unavailable.");
logger.debug("Exception details:", e);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logging pattern is uncommon to OH bindings. If you have a recoverable error like a network fail, you would log a text with the exception's message to debug. If you have a persistent error you would log to warn. These are only examples. See the definitions: https://www.openhab.org/docs/developer/guidelines.html#f-logging

The stack trace should only be logged if the message on its own is not sufficient and the stack trace is crucial for debugging.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I replaced this pattern in all locations it occurred. Please check again.

Copy link
Member

@fwolter fwolter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is pretty good code (thank god 😆)

Comment on lines 189 to 212
try {
TimeUnit.SECONDS.sleep(1);
} catch (InterruptedException e) {
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the purpose of this sleep? If you want to ensure that there's some delay, you should choose a value of at least 100ms, otherwise the JVM may skip the sleep entirely.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The delay is 1s which is larger than 100ms 😉

You are right, the purpose is not that obvious here: After our bridge changed to ONLINE (see the sleep above) it might take a moment for our discovery service to initialize and discover all devices. We wait for this because the overview page which the user is redirected to shows a template for a .things file which is populated with the discovery results. If these are missing then the template is incomplete. We want to avoid confusing the user here. Obviously that is a hacky workaround and we should rather wait until a maximum delay expired or the number of discovered things in the inbox doesn't change any more. Or is there a more common / easier solution?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems it was a bit too late that day...

It'd be good if you solve this in a more elegant way. Unfortunately I can't come up with a solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We now wait until the number of discovery results from this binding doesn't change any more.

@BjoernLange BjoernLange requested a review from a team as a code owner December 3, 2020 11:38
Copy link
Member

@fwolter fwolter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some formatting issues. You can fix them with mvn spotless:apply.

Are the JavaScript and CSS files redundant (formatted and stripped)?

There are some checkstyle warnings left. You could take a look at target/code-analysis/report.html.


<name>@text/binding.mielecloud.name</name>
<description>@text/binding.mielecloud.description</description>
<author>Miele &amp; Cie. KG</author>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The author tag is deprecated and should therefore be removed. See openhab/openhab-core#1844.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We removed it. Is there any other (openHAB official) possibility for Miele to state that they kicked-off development of this binding?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could include Miele in the author tag in each file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We discussed this with Miele and came to the conclusion that including them as author in every file isn't reasonable. Miele would rather like to include a small section at the end of the README. Something like

Development of this binding was initiated by Miele & Cie. KG.

Can we do this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We added a paragraph with an acknowledgement.

Comment on lines 1 to 6
<config-description:config-descriptions
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xmlns:config-description="https://openhab.org/schemas/config-description/v1.0.0"
xsi:schemaLocation="https://openhab.org/schemas/config-description/v1.0.0 http://openhab.org/schemas/config-description-1.0.0.xsd">
<config-description uri="thing-type:mielecloud:device" />
</config-description:config-descriptions>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the purpose of this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We used to have something in there, but it is not required any more. Removed it.

thing-type.mielecloud.wine_storage.description=The generic thing type for all Miele wine storage devices.

# Channel related texts
channel-type.mielecloud.remote_control_can_be_started.label=Remote control can be started
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Words in labels should be capitalized (except prepositions and so on). See https://www.openhab.org/docs/developer/bindings/thing-xml.html#formatting-labels-and-descriptions

Labels are expected to be as short as possible. Guideline is 2-3 words with up to 25 chars.

@@ -0,0 +1,15023 @@
/*! normalize.css v5.0.0 | MIT License | github.com/necolas/normalize.css */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really needed today?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is still needed to have a common initial state across all browsers.

bundles/pom.xml Outdated Show resolved Hide resolved
@BjoernLange BjoernLange force-pushed the 9106-miele-cloud-binding-initial-contribution branch from 1e00ed6 to a36c6fb Compare December 9, 2020 09:32
@BjoernLange
Copy link
Contributor Author

Are the JavaScript and CSS files redundant (formatted and stripped)?

Yes. We thought it would be reasonable to contribute both versions as otherwise it will be hard to impossible to change them in the future.

@BjoernLange
Copy link
Contributor Author

Hi @fwolter! I worked through your review comments and made the appropriate changes or commented on the discussions. Hope I didn't miss anything. Will you close the discussions once you consider them "done"?

@fwolter
Copy link
Member

fwolter commented Dec 16, 2020

Are the JavaScript and CSS files redundant (formatted and stripped)?

Yes. We thought it would be reasonable to contribute both versions as otherwise it will be hard to impossible to change them in the future.

I'd suggest to keep only the formatted version and use that. I don't think performance and data volume matters in this case. That would avoid at least the redundancy of this huge files.

@BjoernLange
Copy link
Contributor Author

We catched up with the recent comments.

@@ -98,8 +98,7 @@ protected String getRedirectionDestination(HttpServletRequest request) {
try {
accessToken = authorizationHandler.getAccessToken(bridgeUid);
} catch (OAuthException e) {
logger.warn("Failed to obtain access token");
logger.debug("Exception details:", e);
logger.warn("Failed to obtain access token.", e);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems a bit unreasonable to log the stack trace when the authentication failed. You could log the exception's message. The stack trace should only be logged in rare cases e.g. a bug in your code has been detected.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the inconvenience, this part of the code should have been removed. The access token is no longer part of the bridge configuration, so accessing it here is unnecessary. I removed the code.

The following table lists all available channels.
See the following chapters for detailed information about which appliance supports which channels.
Depending on the exact appliance configuration not all channels might be supported, e.g. a hob with four plates will only fill the channels for plates 1-4.
Channe ID and channel type ID match unless noted.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Channe ID and channel type ID match unless noted.
Channel ID and channel type ID match unless noted.

Copy link
Member

@fwolter fwolter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Since this is a new binding, another maintainer needs to review your code before it can be merged.

@fwolter fwolter added the cre Coordinated Review Effort label Dec 21, 2020
Copy link
Contributor

@mhilbush mhilbush left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution. Finally now I have a reason to enable networking on my dishwasher!

I left a few review comments relating to logging at info level that should be changed to debug level in order to avoid repeatedly spamming the openhab log file.

In general, there are quite a lot of info level logging elsewhere throughout the binding that should be changed to debug level.

return;
}

logger.info("Opening SSE connection...");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be debug level

}

silentlyCloseReader();
logger.info("SSE stream ended. Closing stream.");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be debug level

onStreamClosedCallback.accept(exception.getCause());
}
}
logger.info("SSE stream closed.");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be debug level

@BjoernLange
Copy link
Contributor Author

Thanks for your contribution. Finally now I have a reason to enable networking on my dishwasher!

That's great @mhilbush! I also saw your reply on the forum, thank you for trying it out!

I left a few review comments relating to logging at info level that should be changed to debug level in order to avoid repeatedly spamming the openhab log file.

In general, there are quite a lot of info level logging elsewhere throughout the binding that should be changed to debug level.

I went through the code and changed all info logs to debug or warn.

@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/miele-cloud-binding/93266/79

@mhilbush
Copy link
Contributor

@BjoernLange Sorry for the delayed response. Thanks for making the changes to the log levels. I downloaded and built the latest PR. The binding is super quiet now!

I had been running the Beta 4 release. As that release is pretty old, and not everyone is comfortable building PRs, it might be a good idea to get a new beta version out there. 😉

@BjoernLange BjoernLange force-pushed the 9106-miele-cloud-binding-initial-contribution branch from 8365026 to ff76f65 Compare February 3, 2021 07:07
Copy link
Contributor

@cpmeister cpmeister left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still have a bit more to go through but these are what I found so far.

@cpmeister cpmeister self-assigned this Feb 9, 2021
Björn Lange added 2 commits May 21, 2021 10:09
Also-by: Bert Plonus <[email protected]>
Also-by: Martin Lepsy <[email protected]>
Also-by: Benjamin Bolte <[email protected]>
Signed-off-by: Björn Lange <[email protected]>
Also-by: Bert Plonus <[email protected]>
Also-by: Martin Lepsy <[email protected]>
Also-by: Benjamin Bolte <[email protected]>
Signed-off-by: Björn Lange <[email protected]>
@BjoernLange BjoernLange force-pushed the 9106-miele-cloud-binding-initial-contribution branch from d87f2b8 to 7389bf9 Compare May 21, 2021 11:13
Björn Lange added 3 commits May 21, 2021 13:14
Also-by: Bert Plonus <[email protected]>
Also-by: Martin Lepsy <[email protected]>
Also-by: Benjamin Bolte <[email protected]>
Signed-off-by: Björn Lange <[email protected]>
Also-by: Bert Plonus <[email protected]>
Also-by: Martin Lepsy <[email protected]>
Also-by: Benjamin Bolte <[email protected]>
Signed-off-by: Björn Lange <[email protected]>
Also-by: Bert Plonus <[email protected]>
Also-by: Martin Lepsy <[email protected]>
Also-by: Benjamin Bolte <[email protected]>
Signed-off-by: Björn Lange <[email protected]>
@BjoernLange
Copy link
Contributor Author

Hi @kaikreuzer, I implemented all requested changes. Please check whether you are satisfied with the thing modeling now. If you are, I will update the documentation to reflect the new configuration parameters and do manual testing to ensure that nothing broke through the changes.

@kaikreuzer
Copy link
Member

Million thanks @BjoernLange, this looks pretty good to me!
Go ahead and update the docs and we should be ready to merge!

Also-by: Bert Plonus <[email protected]>
Also-by: Martin Lepsy <[email protected]>
Also-by: Benjamin Bolte <[email protected]>
Signed-off-by: Björn Lange <[email protected]>
@BjoernLange BjoernLange force-pushed the 9106-miele-cloud-binding-initial-contribution branch from 689109c to 2f3d31b Compare May 25, 2021 07:54
@BjoernLange
Copy link
Contributor Author

Good morning @kaikreuzer! Everything looks good and I updated the documentation to reflect the recent changes. From my perspective it can be merged now 😄

Also-by: Bert Plonus <[email protected]>
Also-by: Martin Lepsy <[email protected]>
Also-by: Benjamin Bolte <[email protected]>
Signed-off-by: Björn Lange <[email protected]>
Copy link
Member

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks for the quick updates, @BjoernLange.

Lgtm and I guess it is finally time to merge. 🎉

A million thanks to you, your colleagues and the friends at Miele for creating this huge binding!

@kaikreuzer kaikreuzer merged commit 705f5c5 into openhab:main May 25, 2021
@kaikreuzer kaikreuzer added this to the 3.1 milestone May 25, 2021
@BjoernLange
Copy link
Contributor Author

I have to thank you for the kind review @kaikreuzer! Apart from that I have two further questions:

  1. In order to implement real time updates we implemented an SSE client based on Jetty because there was (and still seems to be) no viable option declared in the default libraries documentation. Is there something ongoing in this direction or would it make sense to generalize our implementation and add it to openHAB core?
  2. There are now multiple bindings that bring their own UI for OAuth logins (at least this one and the Spotify binding). I don't know if its possible in the general case, but wouldn't it be reasonable to add the OAuth login handling to openHAB core? Are there any efforts towards this?

@kaikreuzer
Copy link
Member

  1. The only other use of SSE within a binding that I remember is @lolodomo's work on the remote openHAB binding. He used JAX-RS for SSE, which looks pretty straight forward to me. I guess we had missed putting that to the default library documentation, but in general that makes sense. I don't really now how feature rich it is and whether it would have worked for your use case as well, though.
  2. What is available in core is the oauth2client bundle, which was actually implemented in the context of Qivicon. This client left out any UI parts, mainly because (at least to my knowledge) there isn't really a proper general way to offer it as long as you cannot have a public url as a redirect url. If you have a good suggestion on how it could/should be done, feel free to open an issue on openhab-core for further discussion!

@lolodomo
Copy link
Contributor

lolodomo commented Jun 2, 2021

  1. There are also the Nest binding (this was my reference to implement the remote openHAB binding) and the recently merged Home Connect binding. They all use JAX-RS for client SSE. I am not sure if they are the only bindings implementing a SSE client .

@BjoernLange
Copy link
Contributor Author

  1. Then I have another question: We had a look at JAX-RS and found it to be suitable, but it isn't part of the compact profile 2 which openHAB targets, or am I wrong here? According to the Oracle documentation JAX-WS, which JAX-RS is a part of, is only included in the full SE API. That's why we didn't use it and instead build something based on Jetty.
  2. I'll look into whether the "proof of concept" implementation from this binding can be used in general and will then open an issue for discussion, thank you @kaikreuzer!

@lolodomo
Copy link
Contributor

lolodomo commented Jun 3, 2021

  1. If I am not wrong, openHAB core framework is built with Apache CXF which provides an implementation of JAX-RS SSE.

@kaikreuzer
Copy link
Member

it isn't part of the compact profile 2 which openHAB targets, or am I wrong here?

Hm, this was a requirement for ESH. But for openhab-core, we didn't enforce this anymore and since we are on Java 11, there isn't such thing as compact profiles anymore...

@BjoernLange
Copy link
Contributor Author

Shouldn't the paragraph regarding compact profile 2 be dropped then?

@kaikreuzer
Copy link
Member

Absolutely. I've created openhab/openhab-docs#1602.

computergeek1507 pushed a commit to computergeek1507/openhab-addons that referenced this pull request Jul 13, 2021
…#9146)

Also-by: Bert Plonus <[email protected]>
Also-by: Martin Lepsy <[email protected]>
Also-by: Benjamin Bolte <[email protected]>
Signed-off-by: Björn Lange <[email protected]>
frederictobiasc pushed a commit to frederictobiasc/openhab-addons that referenced this pull request Oct 26, 2021
…#9146)

Also-by: Bert Plonus <[email protected]>
Also-by: Martin Lepsy <[email protected]>
Also-by: Benjamin Bolte <[email protected]>
Signed-off-by: Björn Lange <[email protected]>
thinkingstone pushed a commit to thinkingstone/openhab-addons that referenced this pull request Nov 7, 2021
…#9146)

Also-by: Bert Plonus <[email protected]>
Also-by: Martin Lepsy <[email protected]>
Also-by: Benjamin Bolte <[email protected]>
Signed-off-by: Björn Lange <[email protected]>
marcfischerboschio pushed a commit to bosch-io/openhab-addons that referenced this pull request May 5, 2022
…#9146)

Also-by: Bert Plonus <[email protected]>
Also-by: Martin Lepsy <[email protected]>
Also-by: Benjamin Bolte <[email protected]>
Signed-off-by: Björn Lange <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new binding If someone has started to work on a binding. For a new binding PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants