[netatmo] Enhance RefreshCapability #16574
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@jlaur : this is ready for your review. Will require some tests |
jlaur
added
the
additional testing preferred
|
@clinique : conflict has to be resolved. |
clinique
force-pushed
the
netatmo_refresh_review
branch
from
c01c73d
to
e38cbb0
Compare
Signed-off-by: clinique <gael@lhopital.org>
Signed-off-by: clinique <gael@lhopital.org>
Signed-off-by: gael@lhopital.org <gael@lhopital.org>
Signed-off-by: gael@lhopital.org <gael@lhopital.org>
Signed-off-by: gael@lhopital.org <gael@lhopital.org>
Rebased Signed-off-by: clinique <gael@lhopital.org> Signed-off-by: gael@lhopital.org <gael@lhopital.org>
clinique
force-pushed
the
netatmo_refresh_review
branch
from
e38cbb0
to
7ef0494
Compare
Signed-off-by: gael@lhopital.org <gael@lhopital.org>
👍 I will start reviewing. Please let me know when you are done testing. |
From my tests this seems good to me, you can give it a test |
|
I just went through all possible states of weather station refresh, I think: grep -E 'RefreshCapability|RefreshAutoCapability' /var/log/openhab/openhab.log | grep indoor_small_bathroomResult: Some of the first log lines seems repeated, which is a bit confusing. I don't know if it's caused by the mayhem when starting the binding with two weather stations and many additional modules. I unplugged the main module Perhaps it could be further fine-tuned even more with exponential backoff, distinguishing between different types of HTTP error codes vs. just old timestamps and what-not. But besides the miscalculation mentioned, overall it seems much better than before, so looks like a nice improvement! 👍 |
jlaur
requested changes
Mar 25, 2024
...src/main/java/org/openhab/binding/netatmo/internal/handler/capability/RefreshCapability.java
Outdated
Show resolved
Hide resolved
Some additional details for completeleness: Main module: "last_status_store": 1711404382,(23:06:22 CET/local time) Additional modules: "last_seen": 1711404333,
"last_seen": 1711404333,
"last_seen": 1711404333,
"last_seen": 1711404333,(23:05:33 CET/local time) |
|
@jlaur : I think I'm going to change my approach on this. It's a pitty to be searching a delay that we know : |
True. A few notes though:
|
Yes, my plan is to add |
Removed interval guessing logic. Signed-off-by: clinique <gael@lhopital.org>
Signed-off-by: clinique <gael@lhopital.org>
Signed-off-by: gael@lhopital.org <gael@lhopital.org>
Signed-off-by: gael@lhopital.org <gael@lhopital.org>
Signed-off-by: gael@lhopital.org <gael@lhopital.org>
Signed-off-by: gael@lhopital.org <gael@lhopital.org>
|
@jlaur: you're ready to go. |
Here we go with a new test session including commit 13 tracking one of my two weather stations (having four childs): grep -h -E 'executeUri|RefreshCapability|RefreshAutoCapability|Netatmo_SmallBathroom_LastSeen' /var/log/openhab/openhab.log /var/log/openhab/events.log | sort | grep -E 'indoor_small_bathroom|LastSeen|46&get_favorites=false'Filtered logs (system started around 18:32, main module taken offline after 18:41, online again before 19:55: In general it looks good - it succeeds in getting fresh values only ~10 seconds after they seems available, so that's good. It starts doing 2 minute polling when updates are missing, and then 15 minutes after one hour. It resumes normla 10 minute polling when online again. However, from the logs a few questions. First, this series of events looks a bit strange to me and results in two calls right after each other:
The same happened initially in the beginning of the logs. This was after restarting openHAB, so without anything lingering. This in itself also looks a bit strange: Can it have anything to do with additional modules? |
jlaur
requested changes
Mar 26, 2024
...main/java/org/openhab/binding/netatmo/internal/handler/capability/RefreshAutoCapability.java
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.netatmo/src/main/resources/OH-INF/config/config.xml
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.netatmo/src/main/resources/OH-INF/config/config.xml
Show resolved
Hide resolved
...ding.netatmo/src/main/java/org/openhab/binding/netatmo/internal/handler/CommonInterface.java
Outdated
Show resolved
Hide resolved
|
Not for this PR, just sharing a finding - when simulating an HTTP error: Then shortly after caused by another module: Handling of such HTTP error should be at |
These refresh requests are triggered by child modules going ONLINE. This is not great but should only happen on the first "going-ONLINE" event of a child.
We end up with two call because the timing of the two requests is not covered by the cache.
Yes, same as above. I think I can adress it. |
Avoid expiring if an expiring process is already ongoing Avoid rescheduling is a schedule already exists in the same time frame Signed-off-by: gael@lhopital.org <gael@lhopital.org>
Signed-off-by: gael@lhopital.org <gael@lhopital.org>
Signed-off-by: gael@lhopital.org <gael@lhopital.org>
jlaur
reviewed
Mar 27, 2024
....netatmo/src/main/java/org/openhab/binding/netatmo/internal/config/NAThingConfiguration.java
Outdated
Show resolved
Hide resolved
jlaur
reviewed
Mar 27, 2024
bundles/org.openhab.binding.netatmo/src/main/resources/OH-INF/config/config.xml
Show resolved
Hide resolved
Just to be sure, are you working on addressing this, or is it already fully addressed? I still see a bit of duplicated log lines, but did not observe duplicated API calls so far: |
|
@lolodomo - ready for you to review also. I have tested several iterations, and it general it looks much better than before. I have only tested weather station functionality, but the changes apply to other modules as well. There is still one clarification needed here: #16574 (comment). Besides this, and this question: #16574 (comment) - LGTM. I'm leaving tomorrow morning for a small Easter holiday in Germany, so won't be able to test again until Monday evening at the earliest. Thanks for all the effort put into this, @clinique. |
Yes, this one is still present : child module requesting their parent to give fresh data when they come ONLINE. Note that it will only happen at startup. I prefer not do changes here because it can have side effects on Security/Energy side and I'd like to keep this PR as lean as possible. This behaviour was already present before this PR so this is not a change. |
…ues. Signed-off-by: gael@lhopital.org <gael@lhopital.org>
jlaur
reviewed
Mar 28, 2024
Signed-off-by: gael@lhopital.org <gael@lhopital.org>
lolodomo
added
rebuild
|
@jlaur : I hope I did not merge too fast. As you approved, I first think it was fine for you. But now, I am asking myself if you would like to continue testing before the merge... |
No, it's fine, I approved because my tests were successful. 😉 |
|
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/netatmo-bridge-offline-99-of-time/149168/28 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Refresh is no more done based on initial dataValidity but relative to lastSeen, so delay can evoluate to fill gaps.
Resolves #16502
Extracted from #16489
Current status of the RefreshCapability published in the thing properties.