WU: bridge/thing refactoring #6121

lolodomo · 2018-08-25T22:32:52Z

Signed-off-by: Laurent Garnier lg.hc@free.fr

Signed-off-by: Laurent Garnier <lg.hc@free.fr>

lolodomo · 2018-08-25T22:51:14Z

I think everything is working better now except the background discovery which seems to be not running. I will take a look later. Manual discovery is working well.

Problem with language is fixed too.
As requested by @mhilbush, discovered things are now created with language code set from the current locale language (when found in the table).

lolodomo · 2018-08-25T23:18:45Z

I just discovered that the list of options for the language setting is wrong in the XML file ! We should find here the WU language codes, all the ones defined in the table LANG_ISO_TO_WU_CODES, or at least a subset. This is not the case !

maggu2810 · 2018-08-26T09:05:47Z

except the background discovery which seems to be not running. I will take a look later.

So, would it make sense to mark the PR title with a leading "WIP" to ensure it is not merged by accident?

Signed-off-by: Laurent Garnier <lg.hc@free.fr>

lolodomo · 2018-08-26T09:20:44Z

With my last commit, the background discovery is now started.

mhilbush · 2018-08-26T11:33:10Z

@lolodomo Thanks for this. If you publish a jar, I'd be happy to test it.

giovannt0 · 2018-08-27T16:58:08Z

It does look better, thanks! However, shouldn't the communication to the WU service be solely handled by the bridge, as mentioned by @htreu in #5501 ? This was our motive for moving the getWeatherData function to the BridgeHandler class. With the changes this PR introduces, now both the thing and the bridge connect to the WU service at some point. Imo it makes more sense to have the bridge taking care of all the API jobs, and the thing only "rendering" the data. Wdyt ?

lolodomo · 2018-08-28T06:25:45Z

We are talking about simple HTTP calls, I don't understand the logic to run them only in the bridge handler. By the way, the only call done by the bridge handler is a control of the API key, the "real" calls are run by the thing handler.
If we moved all the HTTP calls to the same class, we will have to create a new object to return information needed by each caller to set a proper thing status, something you bypassed in your code probably due to this difficulty.
I think we should keep with a simple code and avoid adding complexity for no valid reason.

htreu

Thanks @lolodomo for taking care of this. Please find some remarks inline.

htreu · 2018-08-29T13:13:05Z

.../eclipse/smarthome/binding/weatherunderground/internal/WeatherUndergroundHandlerFactory.java

+            WeatherUndergroundDiscoveryService service = (WeatherUndergroundDiscoveryService) bundleContext
+                    .getService(serviceReg.getReference());
+            service.deactivate();
+            serviceReg.unregister();


although the reference implementation in the hue binding does it the same way I guess this should be turned around: We should unregister the service first, so DS will remove service usages. After this we can safely deactivate the service.

htreu · 2018-08-29T13:15:51Z

...java/org/eclipse/smarthome/binding/weatherunderground/handler/WeatherUndergroundHandler.java


-        WeatherUndergroundConfiguration config = getConfigAs(WeatherUndergroundConfiguration.class);
+    private void initializeThingHandler(@Nullable ThingHandler thingHandler, @Nullable ThingStatus bridgeStatus) {


thingHandler should be renamed to bridgeHandler

htreu · 2018-08-29T13:21:37Z

...rg/eclipse/smarthome/binding/weatherunderground/handler/WeatherUndergroundBridgeHandler.java

@@ -412,113 +175,4 @@ public void handleCommand(ChannelUID channelUID, Command command) {
    public void setApikey(String apikey) {


I know its not the scope of this PR and cosmetic: this method is only called from within this class, please remove or change to private.

htreu · 2018-08-29T13:24:00Z

...rthome/binding/weatherunderground/internal/discovery/WeatherUndergroundDiscoveryService.java

        this.locationProvider = locationProvider;
    }

+    @Override
+    public void activate(@Nullable Map<@NonNull String, @Nullable Object> configProperties) {


Do you mind adding a small comment on why this method has to be overridden? I had to to modify the code myself to see the necessity of this change ;-)

htreu · 2018-08-29T13:24:12Z

...rthome/binding/weatherunderground/internal/discovery/WeatherUndergroundDiscoveryService.java

+    }
+
+    @Override
+    public void deactivate() {


same here, please drop a short note.

Signed-off-by: Laurent Garnier <lg.hc@free.fr>

lolodomo · 2018-08-29T20:19:26Z

@htreu ! I considered all your review comments.

I fixed the example in the documentation too.

lolodomo · 2018-08-29T22:13:28Z

Ready for merge.

openhab-bot · 2018-09-01T00:31:17Z

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

https://community.openhab.org/t/wrong-documentation-for-binding-weatherunderground-how-to-configure-bridge-and-thing/50535/2

lolodomo · 2018-09-01T07:27:20Z

@mhilbush : while it takes time to merge this PR and because some users like you are blocked, here is a jar file you can use.

lolodomo · 2018-09-01T07:40:52Z

Independently of this PR, I discovered during my tests that the WU language codes are not very reliable. As an example, for italian, the WU code should be IY but I got weather conditions in italian when I use IT, not when I use IY.
In fact, there are two pages:

We are now relying on the second page but my feeling now is that it was a mistake because this page is titled "Country to ISO Matching" and provides this description:
Weather Underground uses different country codes than standard ISO. Here is the matching from Weather Underground counties to ISO countries.
It is relative to country and not language !
So I think I should move back to the list of languages defined on this page: https://www.wunderground.com/weather/api/d/docs?d=language-support

lolodomo · 2018-09-01T08:07:34Z

As a consequence, I removed my very last commit as the list of languages in the XML file was the right language codes.
Just remains now to update the table in the code.

Automatic discovery updated to consider change of the system language Signed-off-by: Laurent Garnier <lg.hc@free.fr>

lolodomo · 2018-09-01T10:36:07Z

I finally fixed the language table and made a little change to the automatic discovery to consider change of the system language.
Everything seems to work well now.

@mhilbush : I updated the jar file you can download.

lolodomo · 2018-09-01T10:39:40Z

There is only one little thing that would need an enhancement. Currently, the binding is not able to select automatically british english or french canadian from the system locale. English and French will be rather selected. But of course, the user can select these languages using the thing setting.
This enhancement is possible but out of the scope of this PR.

mhilbush · 2018-09-01T11:29:54Z

Thanks for the changes and thanks for the jar.

htreu

Thanks @lolodomo, waiting for travis to succeed.

* WU: bridge/thing refactoring * WU: start/stop of the background discovery service fixed * Example fixed in the documentation * WU: language conversion table fixed Automatic discovery updated to consider change of the system language Signed-off-by: Laurent Garnier <lg.hc@free.fr>

WU: bridge/thing refactoring

2d23b77

Signed-off-by: Laurent Garnier <lg.hc@free.fr>

maggu2810 added the Binding-Weather-Underground label Aug 26, 2018

WU: start/stop of the background discovery service fixed

a3eaee5

Signed-off-by: Laurent Garnier <lg.hc@free.fr>

htreu mentioned this pull request Aug 28, 2018

[weatherunderground] Thing does not come online #6047

Closed

htreu suggested changes Aug 29, 2018

View reviewed changes

lolodomo added 2 commits August 29, 2018 22:06

Consider the review comments

ba499af

Signed-off-by: Laurent Garnier <lg.hc@free.fr>

Example fixed in the documentation

44947ab

Signed-off-by: Laurent Garnier <lg.hc@free.fr>

WU: language conversion table fixed

8025356

Automatic discovery updated to consider change of the system language Signed-off-by: Laurent Garnier <lg.hc@free.fr>

htreu approved these changes Sep 3, 2018

View reviewed changes

htreu merged commit 2e6bfe5 into eclipse-archived:master Sep 3, 2018

lolodomo deleted the wu_bridge branch September 3, 2018 08:03

htreu added this to the 0.10.0 milestone Oct 30, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WU: bridge/thing refactoring #6121

WU: bridge/thing refactoring #6121

lolodomo commented Aug 25, 2018

lolodomo commented Aug 25, 2018

lolodomo commented Aug 25, 2018 •

edited

maggu2810 commented Aug 26, 2018

lolodomo commented Aug 26, 2018 •

edited

mhilbush commented Aug 26, 2018

giovannt0 commented Aug 27, 2018

lolodomo commented Aug 28, 2018

htreu left a comment

htreu Aug 29, 2018

htreu Aug 29, 2018

htreu Aug 29, 2018

htreu Aug 29, 2018

htreu Aug 29, 2018

lolodomo commented Aug 29, 2018

lolodomo commented Aug 29, 2018

openhab-bot commented Sep 1, 2018

lolodomo commented Sep 1, 2018

lolodomo commented Sep 1, 2018 •

edited

lolodomo commented Sep 1, 2018

lolodomo commented Sep 1, 2018

lolodomo commented Sep 1, 2018

mhilbush commented Sep 1, 2018

htreu left a comment


		WeatherUndergroundConfiguration config = getConfigAs(WeatherUndergroundConfiguration.class);
		private void initializeThingHandler(@Nullable ThingHandler thingHandler, @Nullable ThingStatus bridgeStatus) {

		@@ -412,113 +175,4 @@ public void handleCommand(ChannelUID channelUID, Command command) {
		public void setApikey(String apikey) {

WU: bridge/thing refactoring #6121

WU: bridge/thing refactoring #6121

Conversation

lolodomo commented Aug 25, 2018

lolodomo commented Aug 25, 2018

lolodomo commented Aug 25, 2018 • edited

maggu2810 commented Aug 26, 2018

lolodomo commented Aug 26, 2018 • edited

mhilbush commented Aug 26, 2018

giovannt0 commented Aug 27, 2018

lolodomo commented Aug 28, 2018

htreu left a comment

Choose a reason for hiding this comment

htreu Aug 29, 2018

Choose a reason for hiding this comment

htreu Aug 29, 2018

Choose a reason for hiding this comment

htreu Aug 29, 2018

Choose a reason for hiding this comment

htreu Aug 29, 2018

Choose a reason for hiding this comment

htreu Aug 29, 2018

Choose a reason for hiding this comment

lolodomo commented Aug 29, 2018

lolodomo commented Aug 29, 2018

openhab-bot commented Sep 1, 2018

lolodomo commented Sep 1, 2018

lolodomo commented Sep 1, 2018 • edited

lolodomo commented Sep 1, 2018

lolodomo commented Sep 1, 2018

lolodomo commented Sep 1, 2018

mhilbush commented Sep 1, 2018

htreu left a comment

Choose a reason for hiding this comment

lolodomo commented Aug 25, 2018 •

edited

lolodomo commented Aug 26, 2018 •

edited

lolodomo commented Sep 1, 2018 •

edited