[OpenWeatherMap] Initial contribution of OpenWeatherMap binding #5694
Conversation
|
This pull request has been mentioned on openHAB Community. There might be relevant details there: |
cweitkamp
changed the title
|
A question, if I may. What's the advantage in declaring an item like this: instead of like this? |
|
I'm using your OpenWeatherMap API on a Synology NAS OH2.3 and it works SO much better than the Weather binding that comes with OH2.3, thank you for your work! The only issue I'm having is when I clear my cache and tmp directories; you binding isn't installed anymore and I have re-install it from the market place. Any suggestions? Best, Jay |
|
@jaywiseman1971 Thank you very much for your feedback. Did you see this post in the community forum? It maybe helps with your issue. |
|
Hi Christoph, That didn't work; here's the lines of code I added to /services/addon.cfg which worked for others in the file just not yours. Is it possible for you to post the JAR so I can manually add it to ADDON directory instead so I don't have to manually add it when I clear the CACHE and TMP directories? Access Remote Add-on RepositoryDefines whether the remote openHAB add-on repository should be used for browsing and installing add-ons.This not only makes latest snapshots of add-ons available, it is also required for the installation ofany legacy 1.x add-on. (default is true)remote = true A comma-separated list of bindings to install (e.g. "binding = sonos,knx,zwave")binding = sonos,zwave,mail,airquality,caldav-command1,caldav-personal1,nest,network,ntp,onkyo,plex1.samsungtv,weather1,wemo A comma-separated list of UIs to install (e.g. "ui = basic,paper")ui = basic,paper,habpanel A comma-separated list of persistence services to install (e.g. "persistence = rrd4j,jpa")persistence = rrd4j A comma-separated list of actions to install (e.g. "action = mail,pushover")action = mail A comma-separated list of transformation services to install (e.g. "transformation = map,jsonpath")transformation = javascript,jsonpath,map A comma-separated list of miscellaneous services to install (e.g. "misc = myopenhab")misc = market,openhabcloud A comma-separated list of market extension to install (see https://marketplace.eclipse.org/)market = binding-4126092 <-- this is your entry Best, Jay |
|
@jaywiseman1971 I am afraid I cannot help you very much regarding to the marketplace / addon.cfg topic. Please continue this discussion in the community forum. You can find the link to the current *.jar file behind the "download" button next to the "question mark" button in the marketplace. |
|
Thank you; got the JAR and all is good now! Best, Jay |
|
From my side the code is ready for an initial review. One open item left: Optimize README. |
|
I rebased to resolve merge conflicts. |
There was a problem hiding this comment.
Thanks @cweitkamp for this nicely done binding. I left some comments/remarks inline.
| @@ -107,7 +107,7 @@ public boolean isReadOnly() { | |||
| /** | |||
| * Returns a list of predefined states with their label. | |||
| * | |||
| * @return a list of predefined states with their label | |||
| * @return ist of predefined states with their label | |||
There was a problem hiding this comment.
this actually looked better before
| Bundle-Vendor: Eclipse.org/SmartHome | ||
| Bundle-Version: 0.10.0.qualifier | ||
| Export-Package: | ||
| org.eclipse.smarthome.binding.openweathermap, |
There was a problem hiding this comment.
In the latest bindings we decided not to export any packages if the binding is not meant to be extended by further bundles. This implies all packages/classes to be internal.
| | apikey | API key to access the OpenWeatherMap API: https://home.openweathermap.org/users/sign_up. **Mandatory**. | | ||
| | refreshInterval | Specifies the refresh interval (in minutes). Optional, the default value is 60, the minimum value is 10. | | ||
| | language | Language to be used by the OpenWeatherMap API. Optional, valid values are: `ar`, `bg`, `ca`, `zh_cn`, `zh_tw`, `hr`, `cz`, `nl`, `en`, `fi`, `fr`, `gl`, `de`, `el`, `hu`, `it`, `ja`, `kr`, `la`, `lt`, `mk`, `fa`, `pl`, `pt`, `ro`, `ru`, `sk`, `sl`, `es`, `sw`, `tr`, `ua`, `vi`. | | ||
| | hourlyForecast | Read 3 hour forecast weather data from the OpenWeatherMap API. Optional, default value is `true`. | |
There was a problem hiding this comment.
what are hourlyForecast and dailyForecast parameters good for? Should the existing things for this bridge be considered when deciding which data to fetch?
There was a problem hiding this comment.
see #5694 (comment)
| State state = UnDefType.UNDEF; | ||
| switch (channelId) { | ||
| case CHANNEL_TIME_STAMP: | ||
| state = getDateTimeTypeState(dailyForecastData.getList().get(count).getDt()); |
There was a problem hiding this comment.
personal style: factoring out dailyForecastData.getList().get(count) may enhance readability.
| State state = UnDefType.UNDEF; | ||
| switch (channelId) { | ||
| case CHANNEL_TIME_STAMP: | ||
| state = getDateTimeTypeState(hourlyForecastData.getList().get(count).getDt()); |
There was a problem hiding this comment.
same with hourlyForecastData.getList().get(count)
| OpenWeatherMapLocationDiscoveryService discoveryService = (OpenWeatherMapLocationDiscoveryService) bundleContext | ||
| .getService(serviceReg.getReference()); | ||
| discoveryService.deactivate(); | ||
| serviceReg.unregister(); |
There was a problem hiding this comment.
also in the Hue binding I switched those two lines, so the service gets unregistered first, then deactivated. Although we had no real problems with the order it feels a bit more stable.
| activate(null); | ||
| } | ||
|
|
||
| @Activate |
There was a problem hiding this comment.
Since this service is only created from the handler factory the annotation is not necessary.
| super.modified(configProperties); | ||
| } | ||
|
|
||
| @Deactivate |
There was a problem hiding this comment.
Since this service is only created from the handler factory the annotation is not necessary.
|
|
||
| @Modified | ||
| @Override | ||
| protected void modified(@Nullable Map<String, @Nullable Object> configProperties) { |
There was a problem hiding this comment.
Since this method will not be called its obsolete.
| ThingUID bridgeUID = bridgeHandler.getThing().getUID(); | ||
| createWeatherResult(locationString, bridgeUID); | ||
| OpenWeatherMapAPIConfiguration config = bridgeHandler.getOpenWeatherMapAPIConfig(); | ||
| if (config.getHourlyForecast() == Boolean.TRUE) { |
There was a problem hiding this comment.
maybe I understand now how those config params work: in the bridge config the user can decide if hourly and/or daily forecasts things should be discovered. In case one option is false auto discovery and data fetching get disabled.
But: when the daily forecast thing gets discovered, isn't it an option to just ignore the discovery result and decide data fetching by the existing things on the bridge?
There was a problem hiding this comment.
@htreu Thank you for taking time to review the binding. Yes, I agree. It makes more sense. I cannot remember why I did it this was. Maybe I should rename those parameters to discoverHourlyForecast and discoverDailyForecast. The purpose should be clearer then.
There was a problem hiding this comment.
See my comment on the description for both config parameters.
There was a problem hiding this comment.
Thanks for the binding! I'll probably also migrate to it from the WU Binding. I've added some minor comments below.
Can you also add null annotations to the remaining classes (where it is missing from) as much as possible? Partially annotated code can cause errors when people trust the compiler will always be there to save them from NPEs. 😉
extensions/binding/org.eclipse.smarthome.binding.openweathermap/META-INF/MANIFEST.MF
Show resolved
Hide resolved
| this.apikey = apikey; | ||
| } | ||
|
|
||
| public Integer getRefreshIntervall() { |
There was a problem hiding this comment.
getRefreshIntervall -> getRefreshInterval
| return refreshInterval; | ||
| } | ||
|
|
||
| public void setRefreshIntervall(Integer refreshInterval) { |
There was a problem hiding this comment.
setRefreshIntervall -> setRefreshInterval
| } | ||
|
|
||
| @Nullable | ||
| public synchronized OpenWeatherMapJsonHourlyForecastData getHourlyForecastData( |
There was a problem hiding this comment.
Please annotate the result type with @Nullable instead (see Null Annotations Conventions).
| } | ||
|
|
||
| @Nullable | ||
| public synchronized OpenWeatherMapJsonDailyForecastData getDailyForecastData( |
There was a problem hiding this comment.
Please annotate the result type with @Nullable instead (see Null Annotations Conventions).
| # thing status | ||
| offline.conf-error-missing-apikey = The 'apikey' parameter must be configured. | ||
| offline.conf-error-invalid-apikey = Invalid API key. Please see https://openweathermap.org/faq#error401 for more info. | ||
| offline.conf-error-not-supported-refreshIntervall = The 'refreshIntervall' parameter must be at least 10 minutes. |
There was a problem hiding this comment.
offline.conf-error-not-supported-refreshIntervall -> offline.conf-error-not-supported-refreshInterval
refreshIntervall - > refreshInterval
| # thing status | ||
| offline.conf-error-missing-apikey = Der Parameter 'API Schl�ssel' muss konfiguriert werden. | ||
| offline.conf-error-invalid-apikey = Ung�ltiger 'API Schl�ssel'. Mehr Infos unter https://openweathermap.org/faq#error401. | ||
| offline.conf-error-not-supported-refreshIntervall = Der Parameter 'Abfrageintervall' muss mindestens 10 min betragen. |
There was a problem hiding this comment.
offline.conf-error-not-supported-refreshIntervall -> offline.conf-error-not-supported-refreshInterval
| } | ||
| if (config.getRefreshIntervall() != null && config.getRefreshIntervall() < 10) { | ||
| updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.CONFIGURATION_ERROR, | ||
| "@text/offline.conf-error-not-supported-refreshIntervall"); |
There was a problem hiding this comment.
@text/offline.conf-error-not-supported-refreshIntervall -> @text/offline.conf-error-not-supported-refreshInterval
| connection = new OpenWeatherMapConnection(this, httpClient); | ||
|
|
||
| if (refreshJob == null || refreshJob.isCancelled()) { | ||
| logger.debug("Start refresh job at intervall {} min.", config.getRefreshIntervall()); |
| } | ||
|
|
||
| @Nullable | ||
| public synchronized OpenWeatherMapJsonWeatherData getWeatherData(OpenWeatherMapLocationConfiguration locationConfig) |
There was a problem hiding this comment.
Please annotate the result type with @Nullable instead (see Null Annotations Conventions).
|
Hi Christoph, Can I get the updated JAR for your new work? Best, Jay |
|
@jaywiseman1971 I uploaded the latest version in #5694 (comment). |
|
This merge will become urgent as the WU binding is apparently no more working. |
There was a problem hiding this comment.
I comment again on both of the discovery result parameter descriptions. I think it should be clearly stated that the "daily" thing is only available for paid subscriptions.
During testing I experienced the following: Despite my subscription being a free one, I added a "daily" thing to my bridge. This results in the "daily" thing to have status "OFFLINE, CONFIG_ERROR" but will also bring down the bridge and with it all other things.
Is it possible to only put the "daily" thing offline due to the invalid API key?
|
|
||
| | Parameter | Description | | ||
| |-----------|-------------------------------------------------------------------------------------------------------------| | ||
| | location | Multiple syntaxes are supported. Please read the binding documentation for more information. **Mandatory**. | |
There was a problem hiding this comment.
Please read the binding documentation for more information.
while doing so I was invited to read the binding documentation for more information. I got caught in an infinite loop.
| </parameter> | ||
| <parameter name="hourlyForecast" type="boolean"> | ||
| <label>Access 3 Hour Forecast</label> | ||
| <description>Read 3 hour forecast weather data from the OpenWeatherMap API.</description> |
There was a problem hiding this comment.
The description sounds as if this will affect which information is retrieved from the API. But this does only toggle the discovery, and should prevent to discover "daily forecast" for a free account I guess?
Please update the description for both parameters to make this more clear.
There was a problem hiding this comment.
Done. Yes, it toggles discovery. I changed the description and clarified the documentation. I - hopefully - clearly stated in the section for the daily-forecast that it is only available for payed accounts.
| ThingUID bridgeUID = bridgeHandler.getThing().getUID(); | ||
| createWeatherResult(locationString, bridgeUID); | ||
| OpenWeatherMapAPIConfiguration config = bridgeHandler.getOpenWeatherMapAPIConfig(); | ||
| if (config.getHourlyForecast() == Boolean.TRUE) { |
There was a problem hiding this comment.
See my comment on the description for both config parameters.
...g/eclipse/smarthome/binding/openweathermap/internal/model/OpenWeatherMapJsonWeatherData.java
Show resolved
Hide resolved
|
@htreu Thanks again for your review. I looked into the bridge status handling and have optimized it. |
|
This pull request has been mentioned on openHAB Community. There might be relevant details there: |
| @@ -60,14 +60,14 @@ thing-type.openweathermap.weather.label = Wetterinformationen | |||
| thing-type.openweathermap.weather.description = Erm�glicht die Anzeige der aktuellen Wetterinformationen. | |||
|
|
|||
| thing-type.openweathermap.hourly-forecast.label = 3 Stunden Wettervorhersage | |||
| thing-type.openweathermap.hourly-forecast.description = Erm�glicht die Anzeige einer 3 Stunden Wettervorhersage. | |||
| thing-type.openweathermap.hourly-forecast.description = Aktiviert / Deaktiviert die automatische Erstellung einer 3 Stunden Wettervorhersage. | |||
There was a problem hiding this comment.
I think in the heat of action you accidentally changed the wrong lines:
The
bridge-type.config.openweathermap.weather-api.hourlyForecast.description
...
bridge-type.config.openweathermap.weather-api.dailyForecast.description
are the ones describing the config parameter of the bridge.
There was a problem hiding this comment.
😓 Ups, in the lunch break between two meetings ... I am eager to get this finally merged ...
@htreu What do you mean? The thing-type description? |
|
Yes. I didn't have a closer look. But the two parameters are only used on the bridge. So the description on the bridge-type should be sufficient. |
|
Sorry, its just the label & description of the 3 hours forecast. Forget my last comment. |
| <parameter name="apikey" type="text" required="true"> | ||
| <context>password</context> | ||
| <label>API Key</label> | ||
| <description>API key to access the OpenWeatherMap API: https://home.openweathermap.org/users/sign_up.</description> |
| </parameter> | ||
| <parameter name="hourlyForecast" type="boolean"> | ||
| <label>3 Hour Forecast</label> | ||
| <description>Enable / Disable discovery of the 3 hour forecast thing.</description> |
There was a problem hiding this comment.
"3 hour forecast thing" - sounds pretty weird (especially as a user facing text) and it feels as if there's something not ideal. You might have discussed the modelling with others already, so forgive me if you have to repeat it here. But imho it would make much more sense to have the forecasts being a part of the "weather" thing, but not a separate one for which you have to configure the location individually.
There was a problem hiding this comment.
I second your opinion on the description. I will think about it.
The way how I modeled bridges and things has not been discussed in detail with others yet. The idea behind my model is mainly based in the different forecast options provided by OWM and the underlying accounting / price model (The hourly forecast is free for everyone and the daily forecast restricted to payed accounts).
Additionally I am planning to do some extensions once this will have been merged. OWM provides UV Index, Air pollution and weather alerts too. I want to add them as new things too because I do not think that everyone needs them or is allowed to access them. Why overload one thing with channels / data which are not necessary?
There was a problem hiding this comment.
Why overload one thing with channels / data which are not necessary?
Because Things are not a means to express a functional structure, but are the entities that can physically be added to a system and which can potentially provide many functionalities in one.
Certainly you should only add the channels to the Thing that really ARE available. So if it is a free account, the daily forecast channels would not be added. Btw, can this be determined automatically?
There was a problem hiding this comment.
Btw, can this be determined automatically?
OWM does not expose any information about the account. I can try to ping the daily forecast API and evaluate the result to determine the details of the account. But we cannot say it for 100% sure.
There was a problem hiding this comment.
My current approach tries to request daily forecast data from the OWM API. If the request fails, all channel groups are removed from the thing and further request are omitted.
| <label>Weather Station</label> | ||
| <description>This is a weather station.</description> | ||
| <channels> | ||
| <channel id="id" typeId="station-id" /> |
There was a problem hiding this comment.
Can the id ever change dynamically? If not, this should be a property, not a channel.
There was a problem hiding this comment.
See #5694 (comment)
There was a problem hiding this comment.
Still, the id is a pretty technical information. Is there really a valid use case for it?
There was a problem hiding this comment.
Yes, you can call the weather / forecast data for one location either by geographic coordinates or by city id (which I am planning to implement in the future).
extensions/binding/org.eclipse.smarthome.binding.openweathermap/ESH-INF/thing/channel-types.xml
Show resolved
Hide resolved
extensions/binding/org.eclipse.smarthome.binding.openweathermap/ESH-INF/thing/channel-types.xml
Show resolved
Hide resolved
|
|
||
| ## Discovery | ||
|
|
||
| At first the auto-discovery will add an "OpenWeatherMap API" `weather-api` bridge. |
There was a problem hiding this comment.
This must not be discovered as you do not know whether the user has an account at all.
There was a problem hiding this comment.
That is something I do not understand. Why? We did the same in #5501.
There was a problem hiding this comment.
That is something I do not understand. Why?
Because discovery should only discover what really exists. Imagine you and your spouse each have an OWM account. Which one do you discover? Imagine myself having no account? Which one did you discover? Discovery must not be misused as a shortcut to prompt users to manually create a thing of a certain type.
We did the same in #5501.
Did we? I can only see that it discovers the weather for the system location, once a bridge (=account) has been (manually) created.
| If a system location is set, a "Local Weather" (`weather`) thing and a "Local 3 Hour Forecast" (`hourly-forecast`) thing will be automatically discovered for this location. | ||
| The thing for "Local Daily Forecast" will not be added by default (see `dailyForecast` parameter to enable it). | ||
|
|
||
| Auto-discovery is enabled by default. |
There was a problem hiding this comment.
This is not binding specific, hence it should not be specifically mentioned here. (see #6224)
Especially, there must not be any reference to openHAB here.
| public class OpenWeatherMapLocationConfiguration { | ||
|
|
||
| private String location; | ||
| private Double altitude; |
There was a problem hiding this comment.
Double is not a good choice for precise values.
| return altitude; | ||
| } | ||
|
|
||
| public OpenWeatherMapLocationConfiguration parseLocation() { |
There was a problem hiding this comment.
You should use PointType, which already provides everything you need.
There was a problem hiding this comment.
I switched this logic to use existing features in f2eaef7. Thanks for the hint.
A question from my side which has not be solved in this PR: Is it possible to convert a config param with context location to PointType by the framework automatically? It seems to be a little overhead to parse the location String every time before doing a request.
There was a problem hiding this comment.
No, this is not possible, because configurations only have the text, boolean of decimal values by definition. But filling the string into a PointType should not be much overhead. It only needs to be done once and not every time "before doing a request".
| } | ||
| } catch (ExecutionException e) { | ||
| String errorMessage = e.getLocalizedMessage(); | ||
| if (errorMessage.startsWith("org.eclipse.jetty.client.HttpResponseException")) { |
There was a problem hiding this comment.
Does this have to be a string matching here? What is different about the 401 handling that you have in line 199 already?
There was a problem hiding this comment.
This is one of the most ugliest part I discovered during the implementation of the binding. Funny thing is once I try to request the OMW API with an invalid a API key I do not get a 401 Unauthorized status code but Jetty internally fails with an org.eclipse.jetty.client.HttpResponseException -> ExecutionException. The same request in a browser fails in the normal way returning a 401 Unauthorized status code. If you have any idea why or how to solve it in a better way your help is very much appreciated. Try it yourself:
There was a problem hiding this comment.
What is the cause of the ExecutionException?
There was a problem hiding this comment.
Here is a stack trace for the failing request:
2018-09-25 13:54:30.494 [TRACE] [.connection.OpenWeatherMapConnection] - Exception occurred during execution: org.eclipse.jetty.client.HttpResponseException: HTTP protocol violation: Authentication challenge without WWW-Authenticate header
java.util.concurrent.ExecutionException: org.eclipse.jetty.client.HttpResponseException: HTTP protocol violation: Authentication challenge without WWW-Authenticate header
at org.eclipse.jetty.client.util.FutureResponseListener.getResult(FutureResponseListener.java:118) ~[?:?]
at org.eclipse.jetty.client.util.FutureResponseListener.get(FutureResponseListener.java:101) ~[?:?]
at org.eclipse.jetty.client.HttpRequest.send(HttpRequest.java:684) ~[?:?]
at org.eclipse.smarthome.binding.openweathermap.internal.connection.OpenWeatherMapConnection.getResponse(OpenWeatherMapConnection.java:190) ~[?:?]
at org.eclipse.smarthome.binding.openweathermap.internal.connection.OpenWeatherMapConnection.lambda$2(OpenWeatherMapConnection.java:182) ~[?:?]
at org.eclipse.smarthome.core.cache.ExpiringCache.refreshValue(ExpiringCache.java:91) ~[?:?]
at org.eclipse.smarthome.core.cache.ExpiringCache.getValue(ExpiringCache.java:61) ~[?:?]
at org.eclipse.smarthome.core.cache.ExpiringCacheMap.get(ExpiringCacheMap.java:202) ~[?:?]
at org.eclipse.smarthome.core.cache.ExpiringCacheMap.putIfAbsentAndGet(ExpiringCacheMap.java:133) ~[?:?]
at org.eclipse.smarthome.core.cache.ExpiringCacheMap.putIfAbsentAndGet(ExpiringCacheMap.java:118) ~[?:?]
at org.eclipse.smarthome.binding.openweathermap.internal.connection.OpenWeatherMapConnection.getResponseFromCache(OpenWeatherMapConnection.java:182) ~[?:?]
at org.eclipse.smarthome.binding.openweathermap.internal.connection.OpenWeatherMapConnection.getDailyForecastData(OpenWeatherMapConnection.java:124) ~[?:?]
at org.eclipse.smarthome.binding.openweathermap.internal.handler.OpenWeatherMapDailyForecastHandler.requestData(OpenWeatherMapDailyForecastHandler.java:56) ~[?:?]
at org.eclipse.smarthome.binding.openweathermap.internal.handler.AbstractOpenWeatherMapHandler.updateData(AbstractOpenWeatherMapHandler.java:125) ~[?:?]
at org.eclipse.smarthome.binding.openweathermap.internal.handler.OpenWeatherMapAPIHandler.updateThing(OpenWeatherMapAPIHandler.java:154) ~[?:?]
at org.eclipse.smarthome.binding.openweathermap.internal.handler.OpenWeatherMapAPIHandler.lambda$2(OpenWeatherMapAPIHandler.java:121) ~[?:?]
at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511) [?:?]
at java.util.concurrent.FutureTask.run(FutureTask.java:266) [?:?]
at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.access$201(ScheduledThreadPoolExecutor.java:180) [?:?]
at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:293) [?:?]
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149) [?:?]
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624) [?:?]
at java.lang.Thread.run(Thread.java:748) [?:?]
Caused by: org.eclipse.jetty.client.HttpResponseException: HTTP protocol violation: Authentication challenge without WWW-Authenticate header
at org.eclipse.jetty.client.AuthenticationProtocolHandler$AuthenticationListener.onComplete(AuthenticationProtocolHandler.java:197) ~[?:?]
at org.eclipse.jetty.client.ResponseNotifier.notifyComplete(ResponseNotifier.java:216) ~[?:?]
at org.eclipse.jetty.client.ResponseNotifier.notifyComplete(ResponseNotifier.java:208) ~[?:?]
at org.eclipse.jetty.client.HttpReceiver.terminateResponse(HttpReceiver.java:470) ~[?:?]
at org.eclipse.jetty.client.HttpReceiver.responseSuccess(HttpReceiver.java:416) ~[?:?]
at org.eclipse.jetty.client.http.HttpReceiverOverHTTP.messageComplete(HttpReceiverOverHTTP.java:316) ~[?:?]
at org.eclipse.jetty.http.HttpParser.handleContentMessage(HttpParser.java:605) ~[?:?]
at org.eclipse.jetty.http.HttpParser.parseContent(HttpParser.java:1675) ~[?:?]
at org.eclipse.jetty.http.HttpParser.parseNext(HttpParser.java:1523) ~[?:?]
at org.eclipse.jetty.client.http.HttpReceiverOverHTTP.parse(HttpReceiverOverHTTP.java:172) ~[?:?]
at org.eclipse.jetty.client.http.HttpReceiverOverHTTP.process(HttpReceiverOverHTTP.java:135) ~[?:?]
at org.eclipse.jetty.client.http.HttpReceiverOverHTTP.receive(HttpReceiverOverHTTP.java:73) ~[?:?]
at org.eclipse.jetty.client.http.HttpChannelOverHTTP.receive(HttpChannelOverHTTP.java:133) ~[?:?]
at org.eclipse.jetty.client.http.HttpConnectionOverHTTP.onFillable(HttpConnectionOverHTTP.java:155) ~[?:?]
at org.eclipse.jetty.io.AbstractConnection$ReadCallback.succeeded(AbstractConnection.java:281) ~[?:?]
at org.eclipse.jetty.io.FillInterest.fillable(FillInterest.java:102) ~[?:?]
at org.eclipse.jetty.io.ssl.SslConnection.onFillable(SslConnection.java:291) ~[?:?]
at org.eclipse.jetty.io.ssl.SslConnection$3.succeeded(SslConnection.java:151) ~[?:?]
at org.eclipse.jetty.io.FillInterest.fillable(FillInterest.java:102) ~[?:?]
at org.eclipse.jetty.io.ChannelEndPoint$2.run(ChannelEndPoint.java:118) ~[?:?]
at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.runTask(EatWhatYouKill.java:333) ~[?:?]
at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.doProduce(EatWhatYouKill.java:310) ~[?:?]
at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.tryProduce(EatWhatYouKill.java:168) ~[?:?]
at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.run(EatWhatYouKill.java:126) ~[?:?]
at org.eclipse.jetty.util.thread.ReservedThreadExecutor$ReservedThread.run(ReservedThreadExecutor.java:366) ~[?:?]
at org.eclipse.jetty.util.thread.QueuedThreadPool.runJob(QueuedThreadPool.java:762) ~[?:?]
at org.eclipse.jetty.util.thread.QueuedThreadPool$2.run(QueuedThreadPool.java:680) ~[?:?]
... 1 more
There was a problem hiding this comment.
Just to make clear: The same request with a valid API key works like a charm.
There was a problem hiding this comment.
My point has been: Shouldn't we check the e.g. the cause using instanceof instead of check the message string?
There was a problem hiding this comment.
I get it. You mean we should replace
if (errorMessage.startsWith("org.eclipse.jetty.client.HttpResponseException")) {
...
with
if (e.getCause() instanceof HttpResponseException) {
...
That is nice and should work too.
|
@cweitkamp Thanks for your updates - are you done with everything and the PR is ready for a final review round? |
|
@kaikreuzer No, unfortunately not. The main part - redesign of things for current weather and forecast - is still in progress. |
|
@kaikreuzer @htreu I finalized my work. I hope I have catched everything. The binding is back in a stable mode and ready for a follow-up review. Thanks in advance. |
There was a problem hiding this comment.
Just a quick 2nd round with the new architecture. The code looks pretty good, just minor text stuff and one conceptual question so far.
Thanks @cweitkamp.
| channel-type.openweathermap.forecasted-rain.description = Zeigt den vorhergesagten, kumulierten Regen der n�chsten drei Stunden an. | ||
|
|
||
| channel-type.openweathermap.snow.label = Schnee | ||
| channel-type.openweathermap.snow.description = Zeigt den kumulierten Schnee der n�chsten drei Stunden an. |
There was a problem hiding this comment.
"der nächsten drei Stunden" oder hier "der letzten drei Stunden"?
| <channel-type id="forecasted-snow"> | ||
| <item-type>Number:Length</item-type> | ||
| <label>Forecasted Snow</label> | ||
| <description>Forecasted snow volume for the last 3 hours.</description> |
There was a problem hiding this comment.
this one should be "for the next 3 hours."
| | forecastHours03, forecastHours06, ... forecastHours120 | wind-direction | Number:Angle | Forecasted wind direction. | | ||
| | forecastHours03, forecastHours06, ... forecastHours120 | gust-speed | Number:Speed | Forecasted gust speed. **Advanced** | | ||
| | forecastHours03, forecastHours06, ... forecastHours120 | cloudiness | Number:Dimensionless | Forecasted cloudiness. | | ||
| | forecastHours03, forecastHours06, ... forecastHours120 | rain | Number:Length | Expected rain volume of a day. | |
There was a problem hiding this comment.
expected volume for the next 3 hours
| | forecastHours03, forecastHours06, ... forecastHours120 | gust-speed | Number:Speed | Forecasted gust speed. **Advanced** | | ||
| | forecastHours03, forecastHours06, ... forecastHours120 | cloudiness | Number:Dimensionless | Forecasted cloudiness. | | ||
| | forecastHours03, forecastHours06, ... forecastHours120 | rain | Number:Length | Expected rain volume of a day. | | ||
| | forecastHours03, forecastHours06, ... forecastHours120 | snow | Number:Length | Expected snow volume of a day. | |
There was a problem hiding this comment.
expected volume for the next 3 hours
| configValid = false; | ||
| } | ||
|
|
||
| if (configValid) { |
There was a problem hiding this comment.
In initialization you dynamically create the channel groups for hourly and daily forecasts as defined in the config. Why are hourly forecast channels 03 - 24 predefined in the thing-type xml? Same for daily forecast channels 2 - 6?
There was a problem hiding this comment.
My initial idea was to have those channel groups by default and only change them dynamically if the user changes the thing configuration. Thus I had implemented a logic inside the handleConfigurationUpdate() method. But that did not work because when I tried to configure the things via textual configuration with different numbers of forecasted hours / days the method never had been triggered - which leads to a wrong count of channel groups. Afterwards I moved the logic to the initialization of the thing.
To be honest I do not think that it is the best way how to solve it. But for now I have no better solution in my mind. Maybe you have an idea how to do it?
So in general we now can remove them from the thing type definition.
There was a problem hiding this comment.
Like every time - once you start talking about it the idea starts to develop inside your mind.
I reworked the dynamically handling. Now only new channels will be added or superflous will be removed. This way the predefined ones in the thing-type xml as default makes sense again.
|
Done. Thanks. |
|
The WU binding will become useless first of January 2019. So we need this binding available in snapshots before the end of the year. |
This is not said - the notification from WU told a lot about "stay tuned" and "apis for non-commercial developers"... But I think if you have a look at the progress of this PR, there is no doubt that it'll be finished soon, even without pushing anybody ;-) |
There was a problem hiding this comment.
lgtm, thanks!
@htreu I leave the final feedback to you - if everything is fine, @cweitkamp should squash the commits and we can create a CQ for it.
There was a problem hiding this comment.
Thanks a lot for this contribution @cweitkamp, nice work!
Please squash your commits so we can issue the CQ for IP check.
Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>
|
I am finished. Thanks everyone. |
|
FTR: CQ18113 created. |
|
CQ has been approved! |
|
Great! Thank you very much. 👍 |
|
@cweitkamp I just tried to release a new ESH stable, but the build failed due to this binding - could you please have a look at https://ci.eclipse.org/smarthome/job/SmartHomeDistribution-Stable/312/console? |
|
@kaikreuzer will be fixed with #6521. |
|
First thank you for your development work. I've been trying to make the openweather binding work for a few days now. The integration of the org.eclipse.smarthome.binding.openweathermap-0.10.0-SNAPSHOT.jar worked so far. I had a free OWM API generated and could receive data with the account binding. Unfortunately this only worked once. Afterwards the data was not updated anymore. The status of the OWM account changed to Unknown. Now I can't receive any more data. The events.log 2018-11-15 21:11:48.058 [hingStatusInfoChangedEvent] - 'openweathermap:weather-and-forecast:6b382647:local' changed from UNINITIALIZED (BRIDGE_UNINITIALIZED) to INITIALIZING |
|
@woanne67 May I ask you to use code fences for your log abstract (three backticks before and after the data)? Thanks. If I understood correctly you did two tests on different OH2 versions. Which test/version belongs to this log? Can you please clarify? |
|
@cweitkamp Yes, after I read here that it also works under OH2.3 on a Synology I installed the OWM binding there and it ran exactly once. For test purposes I installed OH2.4 on a Raspi. |
@woanne67 Is it possible to append a log extract for that situation? That would be nice. For the rest of your issue I do not see a technical reason. May I ask you to continue our discussion in the community forum. Thanks. |
|
Changed the OH2.3 OWM-Config from 0 to 3 days forecast:
After saving the settings:
The status changed from online to unknown:
event.log
current status
|
|
Your core ESH bundles are not up to date. At least this error is not caused by the binding but by your runtime. You should use an up to date runtime. |
|
@maggu2810 @cweitkamp Thanks. Before installing OH2.3 on the synology I make a runtime update to Java 1.8.0_191. What else can I do? |
|
@woanne67 You can switch your installation to the testing or snapshot release. |
Initial contribution of OpenWeatherMap binding. See https://openweathermap.org/api. Valid candidate for #5659.
HttpClientinstead ofHttpUtil.executeUrl().TODOs after merging:
Any kind of feedback will be very much appreciated.
Beta version:
Signed-off-by: Christoph Weitkamp github@christophweitkamp.de