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.

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

[daikin][wip] add energy readings #7471

Closed
wants to merge 5 commits into from
Closed

[daikin][wip] add energy readings #7471

wants to merge 5 commits into from

Conversation

gitwouter
Copy link
Contributor

Adding support for energy usage reading to the binding

Reading the aircon/get_week_power_ex endpoint to get energy usage reading for heating & cooling for today, this week & last week.

Wouter Denayer added 2 commits April 25, 2020 17:15
Signed-off-by: Wouter Denayer <[email protected]>
@TravisBuddy
Copy link

Travis tests were successful

Hey @gitwouter,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

Signed-off-by: Wouter Denayer <[email protected]>
@TravisBuddy
Copy link

Travis tests were successful

Hey @gitwouter,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

Signed-off-by: Wouter Denayer <[email protected]>
@TravisBuddy
Copy link

Travis tests were successful

Hey @gitwouter,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

Copy link
Member

@Hilbrand Hilbrand left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution. I've just some small comments. It looks like you need to do a rebase to solve the conflicts: With git pull --rebase and when pushing back to GitHub use force: git push --force-with-lease

return new String[] { key, value };
}).collect(Collectors.toMap(x -> x[0], x -> x[1]));

if (responseMap.get("ret") != null && responseMap.get("ret").contentEquals("OK")) {
Copy link
Member

Choose a reason for hiding this comment

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

Swap parameters and null check can be omitted:

Suggested change
if (responseMap.get("ret") != null && responseMap.get("ret").contentEquals("OK")) {
if ("OK".equals(responseMap.get("ret"))) {

thisWeekEnergy += Integer.parseInt(heatingValues[i]);
}
double previousWeekEnergy = 0;
for (int i = thisWeekLastDayIndex; i < thisWeekLastDayIndex + 7; i += 1) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
for (int i = thisWeekLastDayIndex; i < thisWeekLastDayIndex + 7; i += 1) {
for (int i = thisWeekLastDayIndex; i < thisWeekLastDayIndex + 7; i++) {

and same comment for other loops.

@@ -184,6 +186,12 @@ protected void updateTemperatureChannel(String channel, Optional<Double> maybeTe
.orElse(UnDefType.UNDEF));
}

protected void updateEnergyChannel(String channel, Optional<Double> maybeEnergy) {
updateState(channel,
maybeEnergy.<State>map(t -> new QuantityType<>(new DecimalType(t), SmartHomeUnits.KILOWATT_HOUR))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
maybeEnergy.<State>map(t -> new QuantityType<>(new DecimalType(t), SmartHomeUnits.KILOWATT_HOUR))
maybeEnergy.<State>map(t -> new QuantityType<>(t, SmartHomeUnits.KILOWATT_HOUR))

@Hilbrand Hilbrand added the enhancement An enhancement or new feature for an existing add-on label Aug 27, 2020
// /aircon/get_week_power_ex
// ret=OK,s_dayw=0,week_heat=1/1/1/1/1/5/2/1/1/1/1/2/1/1,week_cool=0/0/0/0/0/0/0/0/0/0/0/0/0/0
// week_heat=<today>/<today-1>/<today-2>/<today-3>/...
Map<String, String> responseMap = Arrays.asList(response.split(",")).stream().filter(kv -> kv.contains("="))
Copy link
Member

Choose a reason for hiding this comment

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

You should use InfoParser.parse(response); The InfoParser class also has some new methods: parseArrayofInt that you could use to parse the specific values below. That might simplify the code too.

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 should remove this pull request. While I was in discussion/feedback on this PR another one was merged and made my work obsolete. Cf. https://community.openhab.org/t/daikin-binding-logging-energy-usage/96964

Copy link
Member

Choose a reason for hiding this comment

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

I might be wrong, but if you're referring to #7708. It doesn't look like that pr overlapped with your pr. That pr added monthly reports, while you added daily/weekly data. That seems other data. So I think your changes are still relevant and from a quick look that pr added some methods that might could make your code simpler by using the added helper methods for parsing the data.

@gitwouter gitwouter closed this Aug 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature for an existing add-on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants