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][WIP] OH3 revamp - step 1 #9486

Closed
wants to merge 19 commits into from

Conversation

clinique
Copy link
Contributor

@clinique clinique commented Dec 23, 2020

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.

  • Station
  • Thermostat
  • Welcome Cam
  • Healthy Home coach => to be controlled by somebody who owns one.
  • [/] Webhook
  • [/] Presence Cam (should be working but to be controlled by somebody who owns one).
  • Adding connected Valves => started ( @Mdillman )
  • Adding doorbell => started ( @Novanic )

Latest testable version here.

@clinique clinique added enhancement An enhancement or new feature for an existing add-on (potentially) not backward compatible additional testing preferred The change works for the pull request author. A test from someone else is preferred though. work in progress A PR that is not yet ready to be merged labels Dec 23, 2020
@clinique clinique added this to the 3.1 milestone Dec 23, 2020
@clinique clinique self-assigned this Dec 23, 2020
Signed-off-by: clinique <[email protected]>
@lolodomo
Copy link
Contributor

The amount of changes is massive, this will require a lot of courage to start a review.

@clinique
Copy link
Contributor Author

clinique commented Jan 20, 2021

I agree. Swagger removal had loads of impacts but will help keeping the binding in sync with the API and also lightened a lot the size of the binding.

@robnielsen
Copy link
Contributor

@clinique, I noticed the channels for Min/Max channels for week and month have been removed from the documentation. I hope this is accidental, and are actually there. This was functionality that existed in both the OH 1 and OH 2 bindings.

@clinique
Copy link
Contributor Author

@robnielsen : dedicated min and max channels have been removed and replace by the ability to add channels with configuration :
image

@lolodomo
Copy link
Contributor

And for the ones still using files?

@clinique
Copy link
Contributor Author

Configurable channels is possible with things files.
I have an example here on openuv :

	Thing uvreport maison "OUV Maison" @ "Maison" [ location="58.999,12.009", refresh=45 ] {
		Channels:
			Type SafeExposure : SE_Parents [		 
				index=3
			]
			Type SafeExposure : SE_Enfants [		 
				index=2
			]
	}

@robnielsen
Copy link
Contributor

@clinique, when you are ready for somebody to test, let me know. I have indoor, outdoor, and rain modules.

@clinique
Copy link
Contributor Author

clinique commented Jan 21, 2021

@robnielsen : you'll find latest testable version here.

First notable change you'll identify, there is no more any "API Bridge" thing. Netatmo Account setting is done at binding level.
So, you've got to remove your currently installed netatmo binding, add this version in the addons folder, then go to "Things" section in UI, "+" to add thing and you should see the Netatmo Binding with a small gear on the right to configure it. Once properly configured, you can trigger a discovery of your things.
This has not been tested with files configurations at all, all is done from UI.

@demichve
Copy link

Hi, I made a smoke test as well (based on the version linked by @clinique .
Only one smaller issue: I cannot find the channel for Wind-Strength for the anemometer. Perhaps this is still in work.

Michael

@robnielsen
Copy link
Contributor

This has not been tested with files configurations at all, all is done from UI.

I don't want to use the API, if there isn't an API bridge anymore how do I configure it via the file?

@clinique
Copy link
Contributor Author

clinique commented Jan 21, 2021

This has not been tested with files configurations at all, all is done from UI.

I don't want to use the API, if there isn't an API bridge anymore how do I configure it via the file?

The same way it is done for other binding configurations (coming to my mind while writing Network binding).
I updated the readme.md file to reflect this. If anybody is willing to help me building the readme he's welcome, I like to code but not to document :-)

@clinique
Copy link
Contributor Author

clinique commented Jan 21, 2021

Hi, I made a smoke test as well (based on the version linked by @clinique .
Only one smaller issue: I cannot find the channel for Wind-Strength for the anemometer. Perhaps this is still in work.
Michael

You're right, they are missing. Let me add them back.

@demichve jar updated, you can download and test.

@Novanic
Copy link
Contributor

Novanic commented Jun 6, 2021

@mdillmann Hi, thank you for updating it. Now I can build and start it again. @clinique But there is already a next new bug which causes that I can't continue. My doorbell and Presence camera is found on discovery / scan, but no home thing anymore. And the Bridge is missing at these two things. When I create the home thing manually (and by finding the home id via dev.netatmo.com) there is a crash: "java.lang.ClassCastException: class org.openhab.binding.netatmo.internal.api.dto.NAHomeWeather cannot be cast to class org.openhab.binding.netatmo.internal.api.dto
.NAHomeSecurity". The home thing is important for web hook events, because this thing has the trigger channel for web hook event types. Or is the trigger channel outdated and rules should react on welcome-event#type of the specific (Presence) thing?

@mdillmann
Copy link

Discovery section of the binding changed a lot, but looking at the code it seems missing to retrieve home-topology for security. Thus it creates modules like NOC but not the proper NAHome object for security.
Not sure if @clinique missed it or if he is in progress of more deserialization magic. I think we should wait for him to fix this.

Meanwhile I changed the discovery as well to skip rooms which have no energy-equipment (rooms are currently only used for Therm/Valve, security-modules like NDB and NOC only need the proper home-object)

Signed-off-by: Gaël L'hopital <[email protected]>
Correction on handling measurable channels
Favorite weather stations detections enhanced.

Signed-off-by: Gaël L'hopital <[email protected]>
@Novanic
Copy link
Contributor

Novanic commented Jun 8, 2021

Hi, thank you for your answers and fixes. Now the discovery was completely broken in my environment. ;-) Users which just have a Presence camera got an (empty) NAHomeWeather thing and not a NAHomeSecurity and the Presence camera was also not discovered anymore. The problem is NAHomeDeserializer. I fixed/improved that with this commit within my branch, see below. But I think the concept of parsing exactly 1 NAHome object isn't working. What is with users which have both Weather and Security or also Energy things? It isn't correct to just parse 1 home object in that case. NAHomeDeserializer should also get improved to check more generic for configured things, maybe give all things a category (Weather, Energy and Security) and check by this category instead of checking specific types within NAHomeDeserializer.

Novanic@148d4e2

@clinique
Copy link
Contributor Author

clinique commented Jun 8, 2021

Hi, thank you for your answers and fixes. Now the discovery was completely broken in my environment. ;-) Users which just have a Presence camera got an (empty) NAHomeWeather thing and not a NAHomeSecurity and the Presence camera was also not discovered anymore. The problem is NAHomeDeserializer. I fixed/improved that with this commit within my branch, see below. But I think the concept of parsing exactly 1 NAHome object isn't working. What is with users which have both Weather and Security or also Energy things? It isn't correct to just parse 1 home object in that case. NAHomeDeserializer should also get improved to check more generic for configured things, maybe give all things a category (Weather, Energy and Security) and check by this category instead of checking specific types within NAHomeDeserializer.

Novanic@148d4e2

Thanks for your updates @Novanic , I take that in account. FYI in Netatmo API houses holding Weather stations are really dinstinct from those holding Security module and those containing Energy module (in my own API answer, I've got 4 distinct houses). So global behaviour seems good while I agree with your modifications.

Signed-off-by: Gaël L'hopital <[email protected]>
@Novanic
Copy link
Contributor

Novanic commented Jun 8, 2021

@clinique Ah, ok, when the API delivers separated homes, the approach may work. Now I checked the Presence support before porting the doorbell support and there seems to be more bugs. :-( The channels of the Presence aren't working. I see the NAWelcome object within the Debugger but only "type", "id", "name" and "setupDate" is set. "vpnUrl" for example is not set which causes that for example the live picture url can't get determined. The version seems still to be very unstable/uncompleted, I think it's very risky to do a complete rewrite of the binding without any automated tests and without the ability to test with all devices. I think there are many bug fixes left. :-(

@mdillmann
Copy link

Hi @clinique, just found some time to compile the latest version and do a quick test. Unfortunately my weather modules are not discovered anymore. In the debug log I see a call https://api.netatmo.com/api/getstationsdata?get_favorites=true. Does this mean that one now HAS to use favorites to get any weather module discovered ? Is this intentional ? In the last versions weather-modules were discovered.
Have not found the time yet to look via debugger what is exactly happening ...
Energy-stuff looks still good at first glance.

@mdillmann
Copy link

@clinique Could you also please cherry-pick the thing-action for NAroom from the other branch ? https://github.com/clinique/openhab-addons/pull/5/commits/91f5458af971fa48d8f7e9733ea7d2e70ecbf831 Should apply (besides the two logger statements in AuthenticationApi.java that I incorrectly put into that commit)

@clinique
Copy link
Contributor Author

clinique commented Jun 8, 2021

Hi @clinique, just found some time to compile the latest version and do a quick test. Unfortunately my weather modules are not discovered anymore. In the debug log I see a call https://api.netatmo.com/api/getstationsdata?get_favorites=true. Does this mean that one now HAS to use favorites to get any weather module discovered ? Is this intentional ? In the last versions weather-modules were discovered.
Have not found the time yet to look via debugger what is exactly happening ...
Energy-stuff looks still good at first glance.

Normally not, favorites are discovered in a second pass, so you should not be hurt :-/ On my side, I have one favorite and it works well :-/

@clinique
Copy link
Contributor Author

clinique commented Jun 8, 2021

@clinique Ah, ok, when the API delivers separated homes, the approach may work. Now I checked the Presence support before porting the doorbell support and there seems to be more bugs. :-( The channels of the Presence aren't working. I see the NAWelcome object within the Debugger but only "type", "id", "name" and "setupDate" is set. "vpnUrl" for example is not set which causes that for example the live picture url can't get determined. The version seems still to be very unstable/uncompleted, I think it's very risky to do a complete rewrite of the binding without any automated tests and without the ability to test with all devices. I think there are many bug fixes left. :-(

Come on @Novanic , we were completely stuck with the previous version based on swagger lib. This PR is used since many months by many people, including me and still the only left known bug one week ago was with the token refresh system. I had to drop all unit tests as they were targetted to previous architecture of the binding. I've started to rewrite some in the past days. I've tested as much as I could with the modules I own (even bought an home coach yesterday to proceed further). With your help, altogether, we'll achieve this. Thanks.

Signed-off-by: Gaël L'hopital <[email protected]>
@clinique
Copy link
Contributor Author

clinique commented Jun 8, 2021

@clinique Could you also please cherry-pick the thing-action for NAroom from the other branch ? https://github.com/clinique/openhab-addons/pull/5/commits/91f5458af971fa48d8f7e9733ea7d2e70ecbf831 Should apply (besides the two logger statements in AuthenticationApi.java that I incorrectly put into that commit)

@Novanic : done.
You will have to add description providers to HomeEnergyHandler so that in UI you properly display and give ability to change energy#mode and energy#planning channels. Something like this :

        ChannelUID channelUID = new ChannelUID(getThing().getUID(), GROUP_HOME_ENERGY, CHANNEL_PLANNING);
        if (descriptionProvider != null) {
            descriptionProvider.setStateOptions(channelUID, home.getThermSchedules().stream()
                    .map(p -> new StateOption(p.getId(), p.getName())).collect(Collectors.toList()));
        }

@robnielsen
Copy link
Contributor

@clinique, when I try mvn clean install, I get the following error:

[ERROR] Failed to execute goal org.apache.karaf.tooling:karaf-maven-plugin:4.2.7:verify (karaf-feature-verification) on project org.openhab.binding.netatmo: Feature resolution failed for [openhab-binding-netatmo/3.1.0.SNAPSHOT]
[ERROR] Message: Unable to resolve root: missing requirement [root] osgi.identity; osgi.identity=openhab-binding-netatmo; type=karaf.feature; version=3.1.0.SNAPSHOT; filter:="(&(osgi.identity=openhab-binding-netatmo)(type=karaf.feature)(version>=3.1.0.SNAPSHOT))" [caused by: Unable to resolve openhab-binding-netatmo/3.1.0.SNAPSHOT: missing requirement [openhab-binding-netatmo/3.1.0.SNAPSHOT] osgi.identity; osgi.identity=openhab-runtime-base; type=karaf.feature [caused by: Unable to resolve openhab-runtime-base/3.1.0.SNAPSHOT: missing requirement [openhab-runtime-base/3.1.0.SNAPSHOT] osgi.identity; osgi.identity=openhab-core-model-lsp; type=karaf.feature [caused by: Unable to resolve openhab-core-model-lsp/3.1.0.SNAPSHOT: missing requirement [openhab-core-model-lsp/3.1.0.SNAPSHOT] osgi.identity; osgi.identity=org.openhab.core.model.lsp; type=osgi.bundle; version="[3.1.0.202106070314,3.1.0.202106070314]"; resolution:=mandatory [caused by: Unable to resolve org.openhab.core.model.lsp/3.1.0.202106070314: missing requirement [org.openhab.core.model.lsp/3.1.0.202106070314] osgi.wiring.package; filter:="(&(osgi.wiring.package=org.openhab.core)(version>=3.1.0)(!(version>=4.0.0)))" [caused by: Unable to resolve org.openhab.core/3.1.0.202106070301: missing requirement [org.openhab.core/3.1.0.202106070301] osgi.wiring.package; filter:="(&(osgi.wiring.package=org.osgi.framework)(version>=1.9.0)(!(version>=2.0.0)))"]]]]]

@mdillmann
Copy link

@clinique Could you also please cherry-pick the thing-action for NAroom from the other branch ? https://github.com/clinique/openhab-addons/pull/5/commits/91f5458af971fa48d8f7e9733ea7d2e70ecbf831 Should apply (besides the two logger statements in AuthenticationApi.java that I incorrectly put into that commit)

@Novanic : done.
You will have to add description providers to HomeEnergyHandler so that in UI you properly display and give ability to change energy#mode and energy#planning channels. Something like this :

        ChannelUID channelUID = new ChannelUID(getThing().getUID(), GROUP_HOME_ENERGY, CHANNEL_PLANNING);
        if (descriptionProvider != null) {
            descriptionProvider.setStateOptions(channelUID, home.getThermSchedules().stream()
                    .map(p -> new StateOption(p.getId(), p.getName())).collect(Collectors.toList()));
        }

Hi @clinique, for thing-actions I am to blame, not @Novanic :-)
The setstateOptions code was already there in my old PR in your repo : https://github.com/clinique/openhab-addons/pull/4/files# . It looks like it got lost when you took the code and removed all the setters and getters there. Do you put it back in ?

Main problem currently is, that I can't build (same as @robnielsen) when I use your branch due to the same karaf conflicts. I always need to apply manually/cherry pick from this PR to my base which builds happyly on OH-snapshot.
My version not finding weather-modules could be well caused by an error by me during that process...
I also received my Thermostat yesterday (still have to set it up) so it would be great if your branch would build cleanly on snapshot so that I can supply proper PRs for Thermostat if needed.

@clinique
Copy link
Contributor Author

clinique commented Jun 9, 2021

https://github.com/clinique/openhab-addons/pull/4/files#

Ok, let me try to clear this issue with mvn:install. First of all, I need to reproduce it.
Done, I reproduce. I'm going to test with a fresh new PR, this should also address conflicting files below. Keep you posted.

clinique added a commit to clinique/openhab-addons that referenced this pull request Jun 9, 2021
This PR replaces PR openhab#9486

Signed-off-by: Gaël L'hopital <[email protected]>
@clinique clinique closed this Jun 9, 2021
@mdillmann
Copy link

Thanks @clinique for the effort !

@clinique clinique deleted the Netatmo_OH3_update branch June 11, 2021 08:20
clinique added a commit to clinique/openhab-addons that referenced this pull request Oct 24, 2021
This PR replaces PR openhab#9486

Signed-off-by: Gaël L'hopital <[email protected]>
clinique added a commit to clinique/openhab-addons that referenced this pull request Dec 5, 2021
This PR replaces PR openhab#9486

Signed-off-by: Gaël L'hopital <[email protected]>
clinique added a commit to clinique/openhab-addons that referenced this pull request Dec 5, 2021
This PR replaces PR openhab#9486

Signed-off-by: Gaël L'hopital <[email protected]>
Signed-off-by: clinique <[email protected]>
clinique added a commit to clinique/openhab-addons that referenced this pull request Dec 24, 2021
This PR replaces PR openhab#9486

Signed-off-by: Gaël L'hopital <[email protected]>
Signed-off-by: clinique <[email protected]>
@clinique clinique changed the title [Netatmo][WIP] OH3 revamp [Netatmo][WIP] OH3 revamp - step 1 Feb 23, 2022
@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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.