-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[metOfficeDataHub] Initial contribution #15367
[metOfficeDataHub] Initial contribution #15367
Conversation
b368b77
to
7687840
Compare
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/uk-met-office-weather-service-binding/148346/1 |
9c43098
to
bb3b226
Compare
[metOfficeDataHub] WIP: Corrections to refactoring. Signed-off-by: dag81 <david.goodyear@gmail.com>
[metOfficeDataHub] WIP: Testing corrections and adjustments Signed-off-by: dag81 <david.goodyear@gmail.com>
[metOfficeDataHub] WIP: Corrections to refactoring. Signed-off-by: dag81 <david.goodyear@gmail.com>
[metOfficeDataHub] WIP: Corrections to refactoring. Signed-off-by: dag81 <david.goodyear@gmail.com>
[metOfficeDataHub] WIP: Corrections to refactoring. Signed-off-by: dag81 <david.goodyear@gmail.com>
[metOfficeDataHub] WIP: RF feedback adjustments to refactoring. Signed-off-by: dag81 <david.goodyear@gmail.com>
Hi @lsiepel , Hopefully you had a good weekend sorry for the delay as you can see quite a bit of refactoring based on the feedback over the last week. Some of it I missed until later, but it looks like you were thinking along the same lines, now I went through the comments tonight. I'm still testing the code and going through various boundary condition testing here, when my old laptop doesn't hard freeze :(, so apologies that's why its taken even longer as I've had to order new hardware to use, as this is now really unstable, as it crashes when I move it :( (I barely get to use my desk with my setup as others in the house use it :( ). The code has been refactored to try and compartmentalize different parts of functionality. As you said in one of the comments I went with a listener based approach generally - mainly so each layer could plugin as necessary. The polling has been moved into its own area, and reused for the two purposes in this binding. Rate limiting now attempts to persist the data to the storage service, so if people do constantly reboot at least it will attempt to prevent them overrunning their allowance. Reading the data and bits have remained the same, the other functionality has been moved around and made more async I guess is the easiest way to explaining it. If you could give it a look over while I continue to test and adjust any silly bits that would be great as you've made some great feedback before. As I'm weary of the Dec release it would be good to get some eye's on it asap when you have time while I clean up any boundary conditions / missed messy code, just to speed up getting it ready. (While I test it here on the testbeds). Hopefully the above is okay - and please don't take any of the quick pass through comments rudely it was just a quick yeah that's covered I think, thumbs up and move on when I just went through them - so apologies if they came across the wrong way. I'll hopefully have time to revisit this more on Thursday / Friday evening 🤞. All the best David |
[metOfficeDataHub] WIP: Corrections to refactoring. Signed-off-by: dag81 <david.goodyear@gmail.com>
The refactoring sounds promesing, i do have quite some work on my plate at the moment so it may take some time, possibly a week or so before i come back to this. |
No problem at all - in the meantime I'll continue testing the edge cases, and runtime to. |
[metOfficeDataHub] WIP: Cleanup unused logger Signed-off-by: dag81 <david.goodyear@gmail.com>
[metOfficeDataHub] WIP: Init cleanup Signed-off-by: dag81 <david.goodyear@gmail.com>
[metOfficeDataHub] WIP: Init double poll fix Signed-off-by: dag81 <david.goodyear@gmail.com>
[metOfficeDataHub] Boundary condition adjustments Signed-off-by: dag81 <david.goodyear@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last push is a great step forward. Based on that push i only have a few new ones. I guess this is the last review. Please also confirm this is well tested.
...c/main/java/org/openhab/binding/metofficedatahub/internal/MetOfficeDataHubBridgeHandler.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/openhab/binding/metofficedatahub/internal/MetOfficeDataHubSiteHandler.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/openhab/binding/metofficedatahub/internal/MetOfficeDataHubSiteHandler.java
Outdated
Show resolved
Hide resolved
...c/main/java/org/openhab/binding/metofficedatahub/internal/api/IConnectionStatusListener.java
Outdated
Show resolved
Hide resolved
...ub/src/main/java/org/openhab/binding/metofficedatahub/internal/api/IRateLimiterListener.java
Show resolved
Hide resolved
...b/src/main/java/org/openhab/binding/metofficedatahub/internal/api/ISiteResponseListener.java
Outdated
Show resolved
Hide resolved
...etofficedatahub/src/main/java/org/openhab/binding/metofficedatahub/internal/api/SiteApi.java
Outdated
Show resolved
Hide resolved
[metOfficeDataHub] Interfaces javadoc Signed-off-by: dag81 <david.goodyear@gmail.com>
[metOfficeDataHub] MET Office Ts Simplification Signed-off-by: dag81 <david.goodyear@gmail.com>
[metOfficeDataHub] unkown error text updated Signed-off-by: dag81 <david.goodyear@gmail.com>
[metOfficeDataHub] httpClient caching Signed-off-by: dag81 <david.goodyear@gmail.com>
Hi @lsiepel, thank you again so much for the time reviewing and suggestions they helped clean it up more! :) Hopefully workwise its calmed down a bit for you now. The code is well tested, however I would like to soak test (on various different setups) and cross check 2 bits to make sure those edge conditions were due to the previous snapshot builds. If all is good with everything done so far - can we delay merging until next weekend (2024-11-23), just so I can validate these last final observations, with different resources and time zones to make sure this is all good please :) Have a great weekend and thank you again! |
[metOfficeDataHub] Start-up bug fix Signed-off-by: dag81 <david.goodyear@gmail.com>
[metOfficeDataHub] Start-up bug fix Signed-off-by: dag81 <david.goodyear@gmail.com>
I have found the bit's I saw from the previous soak runs. I believe its all good now. I will monitor and cross check over the next week over a few different VM's simulating different performance before the 2024-11-23 date, I believe its now consistent no matter what the system loading / performance, or data supplied by MET Office. (Ironically it would have worked before - but I didn't like the odd switch the offline for 20ms before it went to online depending on the systems performance, and slicing of threads. It should now be spot on I hope! 🤞 Have a good weekend - and thanks again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again thanks for contributing. This PR seems finished. I'll wait for your tests to finish, ping me when you are ready.
Next step would be to add the binding logo to the openHAB website. See https://www.openhab.org/docs/developer/addons/#add-your-add-on-s-logo-to-the-openhab-website
[metOfficeDataHub] Readme example fix Signed-off-by: dag81 <david.goodyear@gmail.com>
Hi @lsiepel, nothing found apart from a minor doc mistake. Thank you for all the time and guidance with the review. I think this should be good to merge now. I've also opened the PR for the logo here: openhab/openhab-docs#2409 I will change to non-draft now as hopefully you can merge it after this one. Thanks again and have a great weekend |
* [metOfficeDataHub] Initial Commit for v4 [metOfficeDataHub] Initial code commit. Signed-off-by: David Goodyear <david.goodyear@gmail.com>
[metOfficeDataHub] Initial code commit.
Addition of new Met Office Data Hub binding
-[metOfficeDataHub] Addition of new binding
Description
This is a new binding, which I have been using at home for more accurate temperature forecasts within the UK (I believe it does provide global data from the API definitions). Since moving to OH4 from OH3, I decided it had proven itself, over the last 10 months, so started doing the final pieces to raise this PR, so others can benefit from it as well.
The binding polls with careful consideration to the limitations of the met office API for free users (limited poll's per day), to retrieve forecast data, for each hour up to 24 hours, and for several days using daily forecasts. (For the duration of the systems runtime).
Testing
The core code base I have run at home for 10 months I think. Testing for this PR, has been checking the documented other channels that I do not use, and adjust as necessary comparing the raw values from the API in postman to those polled for the London example given.