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
Fix Huisbaasje negative periodic gas readings (#103457) #108090
Conversation
Avoid negative gas readings in energy dashboard when using Huisbaasje / EnergyFlip integration. Use state_class "TOTAL_INCREASING" for entities related to periodic gas usage (day, week, month, year).
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.
Hi @JeroenvIS
It seems you haven't yet signed a CLA. Please do so here.
Once you do that we will be able to review and accept this pull request.
Thanks!
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
Hey there @dennisschroer, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
Last night I finally managed to get a dev checkout of home assistant core up & running, so ran it with my patch over night. With this change, the energy dashboard doesn't start the new day with a negative gas reading anymore. I consider the PR as tested now, so ticked the box for that. I'm not sure why @bouwew made the change in the first place; what stands out to me in his PR is that there is a revert where the change from 'TOTAL_INCREASING' to 'TOTAL' is undone for single-directivity-interval sensors, but there the entities for the various intervals with gas readings (daily, weekly etc) are not reverted. I believe that gas metering is always single direction, it's not like we can bore our own gas well in our back yard and start feeding natural gas back into the pipes ;-) |
I made the change to TOTAL because these period-sensors all reset at the end of the period. Key point in solving the issue is this: |
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.
As stated in above comment, this change is OK, so approved.
@JeroenvIS can you please also update the relevant test-code? |
Will do ASAP! |
And please sign the CLA too. See here #108090 (review) |
CLA was signed 2 weeks ago, I believe that the bot also corrected the labels. |
OK, found the relevant test and updated it - I wasn't aware that |
@frenck this PR should be ready now. |
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.
LGTM!
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.
Thanks both! 👍
../Frenck
Avoid negative gas readings in energy dashboard when using Huisbaasje / EnergyFlip integration. Use state_class "TOTAL_INCREASING" for entities related to periodic gas usage (day, week, month, year).
Proposed change
Fix issue #103457 in Huisbaasje integration
Type of change
Additional information
Checklist
ruff format homeassistant tests
)This is my first Home Assistant PR. I've read the "Contributing" document and the details of this minor fix were discussed in the ticket. It's still unclear to me how to test the changes locally; however, I believe that this change is so minor (and related tests have been performed by other users, see issue) that that doesn't need to be blocking.
If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest
.requirements_all.txt
.Updated by running
python3 -m script.gen_requirements_all
..coveragerc
.To help with the load of incoming pull requests: