Add 'max_sub_interval' option to derivative sensor

[karwosts]

Proposed change

Copying the implementation from #110685, add a max_sub_interval option 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

• [ ] Dependency upgrade
• [ ] Bugfix (non-breaking change which fixes an issue)
• [ ] New integration (thank you!)
• [x] New feature (which adds functionality to an existing integration)
• [ ] Deprecation (breaking change to happen in the future)
• [ ] 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 #83496
• This PR is related to issue:
• Link to documentation pull request:

Checklist

• [x] The code change is tested and works locally.
• [x] Local tests pass. Your PR cannot be merged unless tests pass
• [x] There is no commented out code in this PR.
• [x] I have followed the development checklist
• [x] I have followed the perfect PR recommendations
• [x] The code has been formatted using Ruff (ruff format homeassistant tests)
• [x] 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.

To help with the load of incoming pull requests:

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

[home-assistant[bot]]

Hey there @afaucogney, mind taking a look at this pull request as it has been labeled with an integration (derivative) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of derivative can trigger bot actions by commenting:

• @home-assistant close Closes the pull request.
• @home-assistant rename Awesome new title Renames the pull request.
• @home-assistant reopen Reopen the pull request.
• @home-assistant unassign derivative Removes the current integration label and assignees on the pull request, add the integration domain after the command.
• @home-assistant add-label needs-more-information Add a label (needs-more-information, problem in dependency, problem in custom component) to the pull request.
• @home-assistant remove-label needs-more-information Remove a label (needs-more-information, problem in dependency, problem in custom component) on the pull request.

[mark007]

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!

[gjohansson-ST]

Would this be solved by implementing also tracking reported and not only changed values? I assume it's not expected that the input sensors stops reporting? I guess it's not exactly the same as this PR but it would be less complicated perhaps to solve the issue.

[karwosts]

Would this be solved by implementing also tracking reported and not only changed values? I assume it's not expected that the input sensors stops reporting? I guess it's not exactly the same as this PR but it would be less complicated perhaps to solve the issue.

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.

[mekaneck]

I think using the state_reported event to update the derivative is a much needed change, but it should be a separate PR. Both changes are necessary to solve the problem correctly and completely. Both changes were implemented on the Riemann Sum integration (2 separate PR's) and I don't see a reason anything should be different for the derivative integration.

[Anton2079]

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. This issue has been observed since December 2022. https://github.com/home-assistant/core/issues/83496

[ThomDietrich]

Hey guys. The discussed issue is very similar to a problem that we have over in the statistics component. See here and here. Maybe that's why @Anton2079 mentioned me here.

I welcome this PR here. As a user of the derivative component myself, I run into this problem too.

1. I agree with @gjohansson-ST, the component should listen to the newly introduced async_track_state_report_event. I also agree with @karwosts, this should be a separate PR. However, I think the state report event PR would be the better and more significant improvement.
2. I agree with the idea to add intermediary updates, also for the mentioned reasons.
   • @karwosts did you consider a service based approach instead? I am not sure if a fixed "sub interval" will solve this cleanly. A separate automation triggering an intermediary update would allow more control to the user, e.g. triggering an update just-in-time when another action needs the derivative value.
   • The feature of a fixed sub interval is still a good fallback to the said service.

[karwosts]

did you consider a service based approach instead?

I didn't really consider anything else. I watched the development of this same feature in the integration (Riemann Sum) integration, it went through a long review process and many iterations to end up with this max_sub_interval option, so I just jumped straight to implementing the same option as there was a precedent for it (and it makes related integrations seem consistent).

[baudneo]

Patiently waiting for this to make its way to release. Really glad there's work being done to fix the derivative sensor.

[sophof]

@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?

[gr0mit1]

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. I'd be happy to test the fix once you've made the change. Many thanks

[elupus]

Looks like reported event will be seen as opt in by integrations, so we must support something like this and probably enabled by default.

[karwosts]

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.

[sophof]

https://github.com/home-assistant/core/pull/139230 should already fix this now AFAIK.

[karwosts]

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.

[sophof]

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).

[emontnemery]

@karwosts do you mean you've given up on this PR? Be as it may, consuming state_reported is the "correct" solution, although it will obviously be a long time before integrations have removed their own suppressing of equal states, so the max_sub_interval adds value for sure.

[karwosts]

@karwosts do you mean you've given up on this PR?

Not entirely sure myself. If a member confirmed that they want to take this change, and that the initial approach was a reasonable approach that could be approved, I would consider taking another go at rewriting it to merge again. But the code owner seems not around anymore, so not really getting much feedback if it's gonna be considered.

[emontnemery]

@karwosts do you mean you've given up on this PR?

Not entirely sure myself. If a member confirmed that they want to take this change, and that the initial approach was a reasonable approach that could be approved, I would consider taking another go at rewriting it to merge again. But the code owner seems not around anymore, so not really getting much feedback if it's gonna be considered.

OK. Would you be interested in stepping up as code owner for this integration?

[karwosts]

OK. Would you be interested in stepping up as code owner for this integration?

Sure I suppose I could give it a shot. I'm still pretty unfamiliar with a lot of the core concepts 😅 , but I guess this would be a quiet corner to learn.

Perhaps I'll try one more PR with just the refactoring needed, and then when that is done can come add this new option on top of that.

[karwosts]

• Rebased and fixed conflicts (due to prior refactoring by other contributors this is actually a simpler now).
• Added self to codeowners as requested.

Ready for new review.

[home-assistant[bot]]

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks :+1:

Learn more about our pull request process.

[baudneo]

Hell yeah

[Anton2079]

Omg omg omg Can’t wait to test it