Add 'max_sub_interval' option to derivative sensor #125870
Conversation
|
Hey there @afaucogney, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
karwosts
force-pushed
the
derivative_max_interval
branch
from
e9e8675 to
1dc8731
Compare
karwosts
force-pushed
the
derivative_max_interval
branch
from
ff8f0ed to
2fd5b97
Compare
|
Hoping this one can be reviewed / merged. It affects many of us, especially in the likes of our house where many sensors update very slowly/infrequently, eg where temperatures drop/rise very slowly. All I can ask is that those with the power to do so, review/merge it sooner rather than later, thanks! |
|
Would this be solved by implementing also tracking reported and not only changed values? |
That might help somewhat for some cases, but I'm not confident if that covers all the scenarios. Like consider in the image in the description, there is a counter helper that just stops updating. I'm not convinced it would ever send more "reports"? In general I'm not sure to what extent I can rely on an arbitrary sensor to keep sending reports of unchanged values, that seems very implementation dependent. I could imagine especially like battery powered devices would not send unnecessary reports to save battery. |
|
I think using the |
|
Sorry to ping when you're not listed, however @ThomDietrich are you able to review this, it is against @afaucogney but he hasn't been seen on Github since August. |
|
Hey guys. I welcome this PR here. As a user of the derivative component myself, I run into this problem too.
|
I didn't really consider anything else. I watched the development of this same feature in the |
|
Patiently waiting for this to make its way to release. Really glad there's work being done to fix the derivative sensor. |
|
@karwosts Hi, I've started on reviewing your PR after talking to you on discord, but I have some generic questions first before I go ahead and write up my suggested changes and such. To be honest, I think there will be few that will take some work.. I think it is clear that extra codeowners are needed for this integration based on the time this has spent in limbo currently. Besides that, according to the architecture guidelines, when adding a feature (such as here) one should also add themselves as codeowner. Are you willing to do that? You also implied that you didn't really know much anymore because of the time that had passed since submitting the PR. I interpreted from that, that you might not be that motivated. As an alternative I could submit my PR and add myself as codeowner. It has similar ideas, but refactors the code even more (I might remove some of it to put it in a later PR to make things simpler). The only work that is left for me is to copy your unit tests and naming scheme, which I like better than mine. Then you could review my code. So my question boils down to: are you willing to be code-owner? And if not, are you willing to review my PR to help it along instead? |
|
This would be really good to see working. I've got an mqtt gas meter that reports my gas usage in kWh. I'm trying to plot 'power' and it never drops to zero, even when no gas is being consumed for hours. |
|
Looks like reported event will be seen as opt in by integrations, so we must support something like this and probably enabled by default. |
|
I'm thinking this has reached a dead-end. There was perhaps too much refactoring included in the initial PR, makes it probably too hard to review, and worse it is too hard to keep up to date with other changes. The recent merging of state_reported now will require a significant amount of work to rewrite this again, and I'm not sure if it would do any good to attempt it. Could try to split the necessary refactoring out into a separate PR before attempting any functional changes, but given that this integration has been abandoned by the codeowner I have no reason to think that would ever be reviewed either. @sophof - if you have a version of this ready to go that has incorporated the state_reported change, I'm happy to close this and let you take it over if you would like to try to do so. If not, I think this needs some direction from a maintainer on where to go from here, before I'm willing to invest any more time on it. |
|
#139230 should already fix this now AFAIK. |
|
I think the thinking is that it fixes it for some cases, but not all. Getting state_reported events from a constant sensor can't be guaranteed. |
|
Yeah, I was just checking the code and I'm also unsure if it will always work. Depends on the implementation of the sensor I guess, but maybe that is the right approach? all sensors I own have the possibility to set both a min value to force reporting AND a fixed time. This approach keeps the code 'clean' (although I think that maybe I will still do some refactoring). |
| }, | ||
| "data_description": { | ||
| "round": "Controls the number of decimal digits in the output.", | ||
| "time_window": "If set, the sensor's value is a time weighted moving average of derivatives within this window.", | ||
| "unit_prefix": "The output will be scaled according to the selected metric prefix and time unit of the derivative." | ||
| "unit_prefix": "The output will be scaled according to the selected metric prefix and time unit of the derivative.", | ||
| "max_sub_interval": "Recalculates derivative if the source did not change for this duration. Use 0 for no time based updates." |
There was a problem hiding this comment.
Adding a hyphen to "time-based" should improve readability and (machine) translations.
Proposed change
Copying the implementation from #110685, add a
max_sub_intervaloption to derivative sensor, which will force the derivative recalculation at the requested time interval if no updates come from the source sensor during that time.Enabling this option will allow the derivative sensor to return to zero when the source stops changing. Presently the derivative will just forever keep the last non-zero value, which is not mathematically expected.
E.g.:
Type of change
Additional information
Checklist
ruff format homeassistant tests)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.To help with the load of incoming pull requests: