Utility meter, add STATE_CLASS_TOTAL_INCREASING by Danielhiversen · Pull Request #54871 · home-assistant/core

Danielhiversen · 2021-08-19T09:34:07Z

Signed-off-by: Daniel Hjelseth Høyer github@dahoiv.net

Breaking change

Proposed change

Utility meter, add STATE_CLASS_TOTAL_INCREASING

Type of change

Dependency upgrade
Bugfix (non-breaking change which fixes an issue)
New integration (thank you!)
New feature (which adds functionality to an existing integration)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

Additional information

This PR fixes or closes issue: fixes #
This PR is related to issue:
Link to documentation pull request:

Checklist

The code change is tested and works locally.
Local tests pass. Your PR cannot be merged unless tests pass
There is no commented out code in this PR.
I have followed the development checklist
The code has been formatted using Black (black --fast homeassistant tests)
Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated for www.home-assistant.io

If the code communicates with devices, web services, or third-party tools:

The manifest file has all fields filled out correctly.
Updated and included derived files by running: python3 -m script.hassfest.
New or updated dependencies have been added to requirements_all.txt.
Updated by running python3 -m script.gen_requirements_all.
For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

No score or internal
🥈 Silver
🥇 Gold
🏆 Platinum

To help with the load of incoming pull requests:

I have reviewed two other open pull requests in this repository.

Signed-off-by: Daniel Hjelseth Høyer <github@dahoiv.net>

probot-home-assistant · 2021-08-19T09:34:12Z

Hey there @dgomes, mind taking a look at this pull request as it has been labeled with an integration (utility_meter) you are listed as a code owner for? Thanks!
_{^{(message by CodeOwnersMention)}}

dgomes

We should probably leave a test for when state_class is measurement

Signed-off-by: Daniel Hjelseth Høyer <github@dahoiv.net>

fsaris · 2021-08-19T14:14:45Z

                "source": "sensor.energy",
            },
            "gas_meter": {
+                "net_consumption": True,


Is this just for the test here? Because gas_meter should also get a status_class=STATE_CLASS_TOTAL_INCREASING shouldn't ?

For all devices that can measure both decreasing and increasing values.
Changed it to energy_meter since it is more realistic to have an energy meter that can both increase and decrease.

Signed-off-by: Daniel Hjelseth Høyer <github@dahoiv.net>

frenck

Thanks, @Danielhiversen 👍

One thing I wonder, should we mark this as a breaking change?

I know it isn't, as the default net_comsumption isn't changed. But I can imaging a lot of people are using this currently without that setting... (and thus this might be unexpected behavior).

If so, lets add the breaking-change label and add a breaking changes section. Maybe mentioning it saves a bit of comments/unexpected behaviors with users.

dgomes · 2021-08-22T18:40:19Z

Thanks, @Danielhiversen 👍

One thing I wonder, should we mark this as a breaking change?

I know it isn't, as the default net_comsumption isn't changed. But I can imaging a lot of people are using this currently without that setting... (and thus this might be unexpected behavior).

If so, lets add the breaking-change label and add a breaking changes section. Maybe mentioning it saves a bit of comments/unexpected behaviors with users.

I think we should fix this by setting the proper state_class depending on the net_consumption config option

frenck · 2021-08-22T19:02:48Z

I like that suggestion even beter 👍

Danielhiversen · 2021-08-22T19:10:41Z

What is the proper state_class ?

            STATE_CLASS_MEASUREMENT
            if self._sensor_net_consumption
            else STATE_CLASS_TOTAL_INCREASING

If sensor_net_consumption is True, then the sensor value can decrease and increase, and STATE_CLASS_MEASUREMENT should be correct?

frenck · 2021-08-22T19:13:10Z

What is the proper state_class ?

Good question, I guess this is mainly about backward compatibility to reduce the number of breaking changes.

Danielhiversen · 2021-08-23T16:08:27Z

I do not see any better way to solve this. (Without reintroduce STATE_CLASS_TOTAL )
Any suggestions?

emontnemery · 2021-08-23T16:13:46Z

Can we reintroduce the STATE_CLASS_TOTAL without reintroducing the last_reset property in SensorEntity, so the sensor gets a lifetime zeropoint when first added?
I do guess that wouldn't be useful for utility_meter though.

Edit: It's not fully clear why the STATE_CLASS_TOTAL would be required here, what was the reason?

Danielhiversen · 2021-08-23T16:54:41Z

last_reset would be required with STATE_CLASS_TOTAL .

net_consumption boolean (optional, default: false)
Set this to True if you would like to treat the source as a net meter. This will allow your counter to go both positive and negative.

Currently, I do not see any better option than using STATE_CLASS_MEASUREMENT when sensor_net_consumption is True

emontnemery · 2021-08-23T17:44:48Z

@Danielhiversen Right. Are you aware of any realistic real world examples of meters which measure a net consumption, for example the sum of imported and exported energy in terms of electricity, and also periodically resets?

Danielhiversen · 2021-08-23T17:48:56Z

No, so for me the current pull request is fine :)
(I have never used sensor_net_consumption = True)

dgomes · 2021-08-23T17:53:05Z

@Danielhiversen Right. Are you aware of any realistic real world examples of meters which measure a net consumption, for example the sum of imported and exported energy in terms of electricity, and also periodically resets?

We can ask the author of #21204

But until recently in Portugal the same thing happened (we are changing meters country wide as we discuss this) old meters would run backwards.

So I guess they are still out there for some of our users

Danielhiversen · 2021-08-25T10:53:00Z

@emontnemery Do you plan to finish #55103 before the beta cut?
I think it would be great to include this pr in the next release. Either with STATE_CLASS_TOTAL or as it is now.

frenck · 2021-08-25T11:09:41Z

Do you plan to finish #55103 before the beta cut?

@Danielhiversen That will not be in the beta. At this point we are undecided. 2021.9 will have some additional information to index the needs for these things.

Danielhiversen · 2021-08-25T11:12:40Z

Ok, then I suggest merging this as it is.

emontnemery · 2021-08-25T11:35:53Z

@Danielhiversen Although #55103 will not be merged now, #55136 has been merged.
Because of that, utility_meter should drop last_reset from the extra_attributes.
Can that be done in this PR?
Edit: NVM, it makes sense to do it separately since it was basically a mistake in #55136 to not include it there

Utility meter, STATE_CLASS_TOTAL_INCREASING

92e3e78

Signed-off-by: Daniel Hjelseth Høyer <github@dahoiv.net>

Danielhiversen requested a review from dgomes as a code owner August 19, 2021 09:34

homeassistant added the cla-signed label Aug 19, 2021

probot-home-assistant Bot added has-tests integration: utility_meter new-feature small-pr PRs with less than 30 lines. labels Aug 19, 2021

probot-home-assistant Bot assigned dgomes Aug 19, 2021

dgomes approved these changes Aug 19, 2021

View reviewed changes

update test

b589473

Signed-off-by: Daniel Hjelseth Høyer <github@dahoiv.net>

fsaris reviewed Aug 19, 2021

View reviewed changes

update test

066c0d5

Signed-off-by: Daniel Hjelseth Høyer <github@dahoiv.net>

fsaris approved these changes Aug 20, 2021

View reviewed changes

Danielhiversen added the smash Indicator this PR is close to finish for merging or closing label Aug 22, 2021

frenck approved these changes Aug 22, 2021

View reviewed changes

frenck removed the smash Indicator this PR is close to finish for merging or closing label Aug 22, 2021

emontnemery approved these changes Aug 25, 2021

View reviewed changes

emontnemery merged commit 7dd169b into dev Aug 25, 2021

emontnemery deleted the utility_meter branch August 25, 2021 12:03

github-actions Bot locked and limited conversation to collaborators Aug 26, 2021

Uh oh!

Conversation

Danielhiversen commented Aug 19, 2021

Breaking change

Proposed change

Type of change

Additional information

Checklist

Uh oh!

probot-home-assistant Bot commented Aug 19, 2021

Uh oh!

dgomes left a comment

Choose a reason for hiding this comment

Uh oh!

fsaris Aug 19, 2021

Choose a reason for hiding this comment

Uh oh!

Danielhiversen Aug 19, 2021

Choose a reason for hiding this comment

Uh oh!

frenck left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dgomes commented Aug 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

frenck commented Aug 22, 2021

Uh oh!

Danielhiversen commented Aug 22, 2021

Uh oh!

frenck commented Aug 22, 2021

Uh oh!

Danielhiversen commented Aug 23, 2021

Uh oh!

emontnemery commented Aug 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Danielhiversen commented Aug 23, 2021

Uh oh!

emontnemery commented Aug 23, 2021

Uh oh!

Danielhiversen commented Aug 23, 2021

Uh oh!

dgomes commented Aug 23, 2021

Uh oh!

Danielhiversen commented Aug 25, 2021

Uh oh!

frenck commented Aug 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Danielhiversen commented Aug 25, 2021

Uh oh!

emontnemery commented Aug 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

frenck left a comment •

edited

Loading

dgomes commented Aug 22, 2021 •

edited

Loading

emontnemery commented Aug 23, 2021 •

edited

Loading

frenck commented Aug 25, 2021 •

edited

Loading

emontnemery commented Aug 25, 2021 •

edited

Loading