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.

Sign up for GitHub

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

Jump to bottom

[linky] Handle case when data from yesterday is still NaN #9423

Merged
merged 13 commits into from
Dec 29, 2020
Merged

[linky] Handle case when data from yesterday is still NaN #9423

merged 13 commits into from
Dec 29, 2020

Conversation

lolodomo
Copy link
Contributor

Fix #9386

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

@lolodomo lolodomo added the work in progress A PR that is not yet ready to be merged label Dec 18, 2020
@lolodomo lolodomo requested a review from clinique as a code owner December 18, 2020 20:58
@lolodomo
[linky] Handle case when data from yesterday is still NaN
7e54ac2 
Fix openhab#9386

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
@lolodomo
Copy link
Contributor Author

This PR should fix all problems with channel refresh.

When data is retrieved by the binding, if the returned data for yesterday is undefined, the binding considers the data as not valid and will wait for the next refresh time to update the channels.
The frequency of retries is now more important, the binding will try once every 3 hours until it retrieves data including defined data for yesterday. The first try is now at 2 in the morning.
So if the data is not available at 2, a retry is triggered at 5, then at 8 ... until data is OK.
I will check tomorrow morning that it works as expected.

This PR also simplifies the retrieval of the week values, with correct values even the Monday.

@lolodomo
Copy link
Contributor Author

Ok, it works as expected.

The problem is that the remote data for yesterday are provided very late in the morning. At 8 in the morning, they were not yet there. At 11, it was ok. I tried just beofre 9am to produce a report with the console command and value from yesyerdau was presented. I don't know if this is the same thing everyday but it looks like the remote data are updated between 8 and 9 in the morning.

I will tune a little the current refresh settings but with my current PR we have valid data at 11 in the morning. If you look before 11am, value from yesterday channel is in fact the value from the day before yesterday.

I imagine two options:

  1. set the channels to UNDEF just after midnight and they will be set later during the morning
  2. Add a date channel so that the user can know at which date the currently values were retrieved.

@lolodomo
Copy link
Contributor Author

lolodomo commented Dec 27, 2020 •
edited
Loading

You approved while it is not yet finished ;)
As the PR at least allows the refresh of data, maybe we should merge it and make improvements in another PR.
I will just do minor changes.

@lolodomo
Refresh every 2 hours until data from yesterday are available
09327b6 
Signed-off-by: Laurent Garnier <lg.hc@free.fr>
@lolodomo lolodomo removed the work in progress A PR that is not yet ready to be merged label Dec 27, 2020
@lolodomo lolodomo changed the title [WIP][linky] Handle case when data from yesterday is still NaN [linky] Handle case when data from yesterday is still NaN Dec 27, 2020
@lolodomo lolodomo requested a review from clinique December 27, 2020 10:17
@clinique
Copy link
Contributor

Yes, I think you could split.

@lolodomo
Copy link
Contributor Author

Ready for review & merge.

@lolodomo
Log yesterday even for month and year requests
3e9362c 
Signed-off-by: Laurent Garnier <lg.hc@free.fr>
@lolodomo
Update log for day data
acbd751 
Signed-off-by: Laurent Garnier <lg.hc@free.fr>
@lolodomo
Consider review comments
5540390 
Signed-off-by: Laurent Garnier <lg.hc@free.fr>
@lolodomo
Set thing status to ONLINE when data are correctly retrieved
f267d83 
Signed-off-by: Laurent Garnier <lg.hc@free.fr>
@lolodomo
Review comment: method for common code
a02ad71 
Signed-off-by: Laurent Garnier <lg.hc@free.fr>
cpmeister
cpmeister requested changes Dec 28, 2020
View reviewed changes
...ab.binding.linky/src/main/java/org/openhab/binding/linky/internal/dto/ConsumptionReport.java Outdated
Comment on lines 45 to 69
public void log(String title, boolean withDateFin, DateTimeFormatter dateTimeFormatter, boolean onlyLast) {
if (LOGGER.isDebugEnabled()) {
int size = (datas == null || periodes == null) ? 0
: (datas.size() <= periodes.size() ? datas.size() : periodes.size());
if (onlyLast) {
if (size > 0) {
log(size - 1, title, withDateFin, dateTimeFormatter);
}
} else {
for (int i = 0; i < size; i++) {
log(i, title, withDateFin, dateTimeFormatter);
}
}
}
}

private void log(int index, String title, boolean withDateFin, DateTimeFormatter dateTimeFormatter) {
if (withDateFin) {
LOGGER.debug("{} {} {} value {}", title, periodes.get(index).dateDebut.format(dateTimeFormatter),
periodes.get(index).dateFin.format(dateTimeFormatter), datas.get(index));
} else {
LOGGER.debug("{} {} value {}", title, periodes.get(index).dateDebut.format(dateTimeFormatter),
datas.get(index));
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move this logic into LinkyHandler instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will but I don't understand why...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I suggested moving the logic is because you want to minimize the amount of logic you are doing in DTO classes since the null checker doesn't check dto classes.

@lolodomo
Review comment: move logging logic to LinkyHandler
a3a1d8b 
Signed-off-by: Laurent Garnier <lg.hc@free.fr>
@lolodomo
Copy link
Contributor Author

Wait a second, I will move checkData too.

@lolodomo
Move checkData to LinkyHandler
e85e31f 
Signed-off-by: Laurent Garnier <lg.hc@free.fr>
@lolodomo
Minor changes to restore initial code
0f2703e 
Signed-off-by: Laurent Garnier <lg.hc@free.fr>
@lolodomo
Minor change
96349ff 
Signed-off-by: Laurent Garnier <lg.hc@free.fr>
@lolodomo
New method for common code
1d69d80 
Signed-off-by: Laurent Garnier <lg.hc@free.fr>
@lolodomo
Copy link
Contributor Author

Ok, this time, it is really finished and ready for a final review.

@lolodomo
Log last day at the right place
96f5940 
Signed-off-by: Laurent Garnier <lg.hc@free.fr>
@cpmeister cpmeister added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Dec 29, 2020
@cpmeister cpmeister merged commit 4c685ad into openhab:main Dec 29, 2020
@cpmeister cpmeister added the bug An unexpected problem or unintended behavior of an add-on label Dec 29, 2020
@cpmeister cpmeister added this to the 3.1 milestone Dec 29, 2020
@lolodomo lolodomo deleted the linky_NaN branch December 30, 2020 00:05
thinkingstone pushed a commit to thinkingstone/openhab-addons that referenced this pull request Nov 7, 2021
@lolodomo @thinkingstone
[linky] Handle case when data from yesterday is still NaN (openhab#9423)
d4138f8 
* [linky] Handle case when data from yesterday is still NaN

Fix openhab#9386
* Refresh every 2 hours until data from yesterday are available
* Log yesterday even for month and year requests

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
marcfischerboschio pushed a commit to bosch-io/openhab-addons that referenced this pull request May 5, 2022
@lolodomo
[linky] Handle case when data from yesterday is still NaN (openhab#9423)
668f97c 
* [linky] Handle case when data from yesterday is still NaN

Fix openhab#9386
* Refresh every 2 hours until data from yesterday are available
* Log yesterday even for month and year requests

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cpmeister cpmeister cpmeister approved these changes

@clinique clinique Awaiting requested review from clinique clinique is a code owner

@fwolter fwolter Awaiting requested review from fwolter

Assignees
No one assigned
Labels
bug An unexpected problem or unintended behavior of an add-on
Projects
None yet
Milestone
3.1
Development

Successfully merging this pull request may close these issues.

[linky] Bad values from yesterday once the refresh is working again
3 participants
@lolodomo @clinique @cpmeister