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

[Netatmo] Binding rewrite without external dependencies - step 2 #10831

Closed
wants to merge 59 commits into from

Conversation

clinique
Copy link
Contributor

@clinique clinique commented Jun 9, 2021

This rework of the Netatmo binding was initiated in order to remove external dependencies (swagger, okio, okhttp...), it ended as a complete rework of it.

Should now be reasonably stable :

  • Station
  • Thermostat
  • Welcome Cam
  • Connected Valves => for extensive tests now ( @Mdillman )
  • Healthy Home coach

Most of work done but to be tested more :

  • [/] Webhook
  • [/] Presence Cam (should be working but to be controlled by somebody who owns one).
  • Smoke detector (won't be full part of it due to a bug in Netatmo API)

To be started (if someone volunteers):

  • Adding doorbell => inception ( @Novanic )

Latest testable version here.

This PR replaces PR #9486

Signed-off-by: Gaël L'hopital gael@lhopital.org

@clinique clinique self-assigned this Jun 9, 2021
@clinique clinique added (potentially) not backward compatible additional testing preferred The change works for the pull request author. A test from someone else is preferred though. cre Coordinated Review Effort enhancement An enhancement or new feature for an existing add-on help wanted test labels Jun 9, 2021
@clinique
Copy link
Contributor Author

clinique commented Jun 9, 2021

@mdillman, @Novanic , @robnielsen : you should not have any Karaf issue now.

@mdillmann
Copy link

Hi @clinique, just tried to build this new branch but "OpenHAB Annotation hell" struck:
[ERROR] /Users/markusdillmann/development/openhab-addons/bundles/org.openhab.binding.netatmo/src/main/java/org/openhab/binding/netatmo/internal/NetatmoHandlerFactory.java:[103,33] Null type mismatch: required 'org.openhab.binding.netatmo.internal.channelhelper.@NonNull AbstractChannelHelper' but the provided value is inferred as @org.eclipse.jdt.annotation.Nullable
Do you have a quick fix for this ?

@mdillmann
Copy link

Created a PR in your repo to adress the build-issue (also seen here in Jenkins) and fixes for Thermostat/Valves. Support for both is considered now stable by me.

@mdillmann
Copy link

mdillmann commented Jun 11, 2021

@clinique : Discovery still does not find any Weather things for me (I do not have any favorites set on the Netatmo site). Debugging thru the discovery-part of the binding, it looks like it tries to create the NAMain-Bridge under the then current NAHome (NAHomeEnergy).
Shouldn't the code do a check there and create the NAMain-bridge without specifing its bridge (pass null) the same way you create the NAMains for favorite stations ?

@clinique
Copy link
Contributor Author

@lolodomo , @cweitkamp : we're doing our best with @mdillmann to have this PR properly ready in regard of OH3.1 While we're still working on some issue, would one of you agree to start reviewing code ?

@robnielsen
Copy link
Contributor

@clinique, my binding has been offline for about 5 hours. I see this in the logs.

openhab.log:

2021-06-23 06:00:14.603 [WARN ] [tatmo.internal.api.AuthenticationApi] - Unable to refresh access token : Rest call failed: statusCode=-1, message=Exception while calling https://api.netatmo.com/oauth2/token, trying to reopen connection.

events.log:

2021-06-23 06:00:24.636 [INFO ] [ab.event.ThingStatusInfoChangedEvent] - Thing 'netatmo:NAMain:indoorModule' changed from ONLINE to OFFLINE (BRIDGE_OFFLINE): Will retry to connect Netatmo API, this one failed : Rest call failed: statusCode=-1, message=Exception while calling https://api.netatmo.com/oauth2/token
2021-06-23 06:00:24.641 [INFO ] [ab.event.ThingStatusInfoChangedEvent] - Thing 'netatmo:NAModule1:indoorModule:outdoorModule' changed from ONLINE to OFFLINE (BRIDGE_OFFLINE)
2021-06-23 06:00:24.652 [INFO ] [ab.event.ThingStatusInfoChangedEvent] - Thing 'netatmo:NAModule3:indoorModule:rainModule' changed from ONLINE to OFFLINE (BRIDGE_OFFLINE)
2021-06-23 06:00:24.658 [INFO ] [ab.event.ThingStatusInfoChangedEvent] - Thing 'netatmo:NAModule1:indoorModule:outdoorModule' changed from OFFLINE (BRIDGE_OFFLINE) to OFFLINE (BRIDGE_OFFLINE): Will retry to connect Netatmo API, this one failed : Rest call failed: statusCode=-1, message=Exception while calling https://api.netatmo.com/oauth2/token
2021-06-23 06:00:24.662 [INFO ] [ab.event.ThingStatusInfoChangedEvent] - Thing 'netatmo:NAModule3:indoorModule:rainModule' changed from OFFLINE (BRIDGE_OFFLINE) to OFFLINE (BRIDGE_OFFLINE): Will retry to connect Netatmo API, this one failed : Rest call failed: statusCode=-1, message=Exception while calling https://api.netatmo.com/oauth2/token

And nothing else for the Netatmo binding after these entries.

@openhab openhab deleted a comment from robnielsen Jun 23, 2021
@clinique
Copy link
Contributor Author

@robnielsen : what version of the binding are you currently using ? Is it a recent one ?
Plese note that I recently introduced a Device action that can help relaunch API connexion from rules :

val actions = getActions("netatmo","netatmo:NHC:ddddxxx")
if(actions === null) {
  logInfo("actions", "Actions is null")
} else {
  actions.reconnectApi()
    logInfo("actions","Reconnecting")
  }

This action can be based on any discovered thing (and reconnects the whole binding)

@lolodomo
Copy link
Contributor

With the last version, enabling camera monitoring is no more working. It looks like there is even a kind of (infinite ?) loop:

	at org.openhab.binding.netatmo.internal.handler.DeviceHandler.tryApiCall(DeviceHandler.java:245) ~[?:?]
	at org.openhab.binding.netatmo.internal.handler.DeviceHandler.lambda$12(DeviceHandler.java:245) ~[?:?]
	at java.util.Optional.ifPresentOrElse(Optional.java:201) ~[?:?]
	at org.openhab.binding.netatmo.internal.handler.DeviceHandler.tryApiCall(DeviceHandler.java:245) ~[?:?]
	at org.openhab.binding.netatmo.internal.handler.DeviceHandler.lambda$12(DeviceHandler.java:245) ~[?:?]
	at java.util.Optional.ifPresentOrElse(Optional.java:201) ~[?:?]
	at org.openhab.binding.netatmo.internal.handler.DeviceHandler.tryApiCall(DeviceHandler.java:245) ~[?:?]
	at org.openhab.binding.netatmo.internal.handler.DeviceHandler.lambda$12(DeviceHandler.java:245) ~[?:?]
	at java.util.Optional.ifPresentOrElse(Optional.java:201) ~[?:?]
	at org.openhab.binding.netatmo.internal.handler.DeviceHandler.tryApiCall(DeviceHandler.java:245) ~[?:?]
	at org.openhab.binding.netatmo.internal.handler.DeviceHandler.lambda$12(DeviceHandler.java:245) ~[?:?]

@lolodomo
Copy link
Contributor

Fixed for Camera. Can you be more specific on what you expect on home ?

Something like "Home ID" + "Unique identifier of the home defined by Netatmo.".

@lolodomo
Copy link
Contributor

You just added the signal group for the camera but I am not sure at all it is relevant. It is not there in the current binding. The values are set to 0 dBm + Excellent. What is it ? The WiFi signal ? 0 dBm is strange. For example, my main weather station has a non zero value (42 dBm).

@lolodomo
Copy link
Contributor

For the camera, what looks missing in the refactored binding is all channels that are providing the different information of the last received camera event (message, type, timestamp, snapshot, ...).

@jlaur
Copy link
Contributor

jlaur commented Dec 27, 2021

@clinique - general: i18n strings are not maintained after text changes. Maybe you can try to run mvn i18n:generate-default-translations.

@lolodomo
Copy link
Contributor

lolodomo commented Dec 27, 2021

I really do not like your approach with ULTRA generic channel types like "string" channel type ! I do not believe this is the original philosophy behind a channel type. And this is not what we have in the current binding.
As an example, you have the channel type "snapshot", that is fine for me. Could you please create channel types "snapshot-url" and "videostream-url" with appropriate label/description.
Maybe, just keep the existing channel types from the current binding.

@clinique
Copy link
Contributor Author

clinique commented Dec 28, 2021

@lolodomo

With the last version, enabling camera monitoring is no more working. It looks like there is even a kind of (infinite ?) loop:

Ok, corrected.

Something like "Home ID" + "Unique identifier of the home defined by Netatmo.".

Enhanced

You just added the signal group for the camera but I am not sure at all it is relevant. It is not there in the current binding.
The values are set to 0 dBm + Excellent. What is it ? The WiFi signal ? 0 dBm is strange.
For example, my main weather station has a non zero value (42 dBm).

Yes, this value is now available. Corrected the fact it was at 0.

Could you please create channel types "snapshot-url" and "video-url" with appropriate label/description.

Ok, done. I did that because the binding heavily relies on channel groups where you can't define channels in details, so channel types did tend to multiply becoming unmaintainable.

For the camera, what looks missing in the refactored binding is all channels that are providing the different information of the last received camera event (message, type, timestamp, snapshot, ...).

Corrected.

@jlaur

@clinique - general: i18n strings are not maintained after text changes. Maybe you can try to run mvn i18n:generate-default-translations.

I'll adress this once the binding is in the last reviewing steps.

@lolodomo
Copy link
Contributor

@clinique: you did not yet push your last changes

@clinique
Copy link
Contributor Author

@clinique: you did not yet push your last changes

No, I'm still working on it.

@clinique
Copy link
Contributor Author

@clinique: you did not yet push your last changes

No, I'm still working on it.

Hi gents, I did not forgot you and am still working. In the meantime I discovered that Netatmo deprecated a lot of API endpoints, so adapting the binding to it.

@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/any-support-for-netatmo-smart-video-doorbell/132022/3

@lolodomo lolodomo added the work in progress A PR that is not yet ready to be merged label Feb 9, 2022
@robnielsen
Copy link
Contributor

Hi @clinique, just checking to see how updating the deprecated api's is going.

@clinique
Copy link
Contributor Author

clinique commented Feb 21, 2022

Hi @clinique, just checking to see how updating the deprecated api's is going.

I will be able to push something in a few days. I'm currently trying the binding progressivly. As there is many conflicting files due to year change and snapshot version change, I will perhaps close this one and open a new PR.

@clinique clinique changed the title [Netatmo] Binding rewrite without external dependencies [Netatmo] Binding rewrite without external dependencies - step 2 Feb 23, 2022
@clinique clinique closed this Feb 23, 2022
@VL2019
Copy link

VL2019 commented May 1, 2022

Hi
Is there any Update about the Support of the Netatmo doorbell in openHAB? Would be great to have the Information „someone pushed the doorbell Button Outsider“ im openhab to inform the House by Hue, Telegram of something Else.

Thanks a lot

@clinique
Copy link
Contributor Author

clinique commented May 1, 2022

Hi Is there any Update about the Support of the Netatmo doorbell in openHAB? Would be great to have the Information „someone pushed the doorbell Button Outsider“ im openhab to inform the House by Hue, Telegram of something Else.

Thanks a lot

Doorbell is accepted and recognized by this version but no specific functionality right now. BTW I suppose than doorbell button push will go through webhook, but not investigated at the time. We are targetted on finalizing what currently exists in order to merge, and then we'll see to implement new things.

@rbonvino
Copy link

@clinique I have this error in openHabian 3.2.0. I removed all netatmo things and put the addon in the addons folder.
Any hints? Thank you.

[ERROR] [tatmo.internal.NetatmoHandlerFactory] - bundle org.openhab.binding.netatmo:3.3.0.202204170649 (28)[org.openhab.binding.netatmo.internal.NetatmoHandlerFactory(15)] : Error during instantiation of the implementation object

java.lang.reflect.InvocationTargetException: null

@clinique
Copy link
Contributor Author

@clinique I have this error in openHabian 3.2.0. I removed all netatmo things and put the addon in the addons folder. Any hints? Thank you.

[ERROR] [tatmo.internal.NetatmoHandlerFactory] - bundle org.openhab.binding.netatmo:3.3.0.202204170649 (28)[org.openhab.binding.netatmo.internal.NetatmoHandlerFactory(15)] : Error during instantiation of the implementation object
java.lang.reflect.InvocationTargetException: null

I'm not sure you use the correct version. Last one is available in milestone

@rbonvino
Copy link

Are you talking about openHab or the netatmo binding? I updated openHab yesterday and that was the latest build available on master.

@clinique
Copy link
Contributor Author

So if you updated openhab yesterday, being on a milestone version 3.3 you have available the last version of the binding, no need to install a jar in addons folder.

@rbonvino
Copy link

Ok , I didn't check the milestone version. As soon I'll get it up and running I'll let you know if it is ok. Thank you!

@jlaur
Copy link
Contributor

jlaur commented May 25, 2022

@rbonvino - you mentioned that you had this problem with openHABian 3.2. I assume openHABian 1.7 and openHAB 3.2. So just for clarification: Are you testing the new binding with openHAB 3.2 or with a 3.3 milestone/snapshot release?

@rbonvino
Copy link

@jlaur I updated today at the version 3.3.0M5 and installed the binding from the market, but I have a weird behavour of the binding. While I am able to set correctly the netatmo account, it does not show when I configure the relay plug and the thermostat things.

@jlaur
Copy link
Contributor

jlaur commented May 25, 2022

@rbonvino - please create an issue as this PR is merged. Also it was authored by @clinique. :) However, I did notice that some changes that might be related to plug and thermostat were made in #12805, but not sure if those changes directly address your issue.

@rbonvino
Copy link

I'll do it as soon as possible. Which informations am I supposed to provide? Are there any logs?

@wborn wborn removed the additional testing preferred The change works for the pull request author. A test from someone else is preferred though. label Dec 23, 2022
@clinique clinique deleted the NetatmoOH31revamp branch December 28, 2022 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cre Coordinated Review Effort enhancement An enhancement or new feature for an existing add-on (potentially) not backward compatible work in progress A PR that is not yet ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[netatmo] NATherm1 wrong temperature at frost-guard setpoint [netatmo] items returning to default state