Fix rain count sensors' state class of Ecowitt#158204
Fix rain count sensors' state class of Ecowitt#158204joostlek merged 9 commits intohome-assistant:devfrom
Conversation
)" This reverts commit 8fe79a8.
…ant#155812)" This reverts commit cb029e0.
…stant#155358)" This reverts commit 7ead8f9.
home-assistant
bot
added
bugfix
cla-signed
integration: ecowitt
small-pr
|
Hey there @pvizeli, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
There was a problem hiding this comment.
Pull request overview
This PR fixes incorrect state class assignments for Ecowitt rain count sensors by reverting three previous PRs and implementing proper logic to distinguish between sensor types. Rain sensors are categorized as: total/resetting sensors (TOTAL_INCREASING) and rolling window sensors (MEASUREMENT). The fix adds default TOTAL_INCREASING state class to rain count sensor descriptions, then overrides it to MEASUREMENT for sensors matching a rolling window pattern (hourly, 24h, and piezo variants).
Key changes:
- Adds regex pattern to identify rolling window rain sensors (hourly, last24h, and piezo variants)
- Sets TOTAL_INCREASING as default state class for RAIN_COUNT_MM and RAIN_COUNT_INCHES sensor descriptions
- Implements logic to override state class to MEASUREMENT for rolling window sensors during sensor setup
| if _ROLLING_WINDOW_RAIN_COUNT_SENSOR.fullmatch(sensor.key): | ||
| description = dataclasses.replace( | ||
| description, | ||
| state_class=SensorStateClass.TOTAL_INCREASING, | ||
| state_class=SensorStateClass.MEASUREMENT, | ||
| ) | ||
|
|
There was a problem hiding this comment.
This logic for detecting and overriding state classes for rolling window sensors lacks test coverage. Given the critical nature of state class assignments for long-term statistics (as mentioned in the PR description), tests should be added to verify:
- Rolling window sensors (matching the regex) correctly get MEASUREMENT state class
- Non-rolling window rain count sensors correctly retain TOTAL_INCREASING state class
- The regex pattern matches all expected sensor key formats (hourlyrainmm, hourlyrainin, last24hrainmm, last24hrainin, hrain_piezo variants, etc.)
Consider adding tests in tests/components/ecowitt/test_sensor.py following the Home Assistant testing patterns with fixtures and snapshots.
There was a problem hiding this comment.
For sure ecowitt tests are extremely limited!
I strongly suggest to add snapshot tests (maybe in a preliminary PR?) to ensure this is properly tested and follow-up PRs do not accidentaly mess it up.
|
I suggest using the |
|
Also related #158070 |
|
Assigning @sairon for review. |
That one may not be used on aggregations, thus not a valid suggestion; see also our developer documentation. |
|
This will also fix #157903 |
sairon
left a comment
sairon
left a comment
There was a problem hiding this comment.
To give some context, when I opened my PR, it was aimed at fixing the removal of total rain from the long term statistics. At that point it wasn't known that this sensor is not always present. But now I agree it is reasonable to keep the other values in LTS as TOTAL_INCREASING too, unless they cause any problems.
Not entirely sure if it makes sense to preserve/reintroduce the rolling window ones in long-term statistics - the consensus from the previous discussions was to remove them from there, which my change only made consistent for hourly sensors as well.
The major change I'd do though is that I'd use an explicit list of the sensors instead of the regexp, the only advantage of the regexp is that it's slightly more concise, but it's harder to read and orders of magnitude slower. And in longer run, I think it would be best if the difference between rolling window sensors was implemented on the aioecowitt library level, but I'd not go through that complexity for the purpose of this fix.
| _ROLLING_WINDOW_RAIN_COUNT_SENSOR = re.compile( | ||
| "(?:hourly|last24h)rain(?:in|mm)|(?:last24)?hrain_piezo(?:mm)?" | ||
| ) | ||
| _ROLLING_WINDOW_RAIN_COUNT_SENSORS = { |
There was a problem hiding this comment.
I stiln think this should be a positive list, not a negative list.
There was a problem hiding this comment.
I strongly disagree.
There are 30 rain count sensors in total, in which 8 are rolling window and 22 are accumulating.
If we want to really be comprehensive, then I'd go with something like
_RAIN_COUNT_SENSORS_STATE_CLASS_MAPPING: Final = {
"eventrainin": SensorStateClass.TOTAL_INCREASING,
"hourlyrainin": None,
"totalrainin": SensorStateClass.TOTAL_INCREASING,
"dailyrainin": SensorStateClass.TOTAL_INCREASING,
"weeklyrainin": SensorStateClass.TOTAL_INCREASING,
"monthlyrainin": SensorStateClass.TOTAL_INCREASING,
"yearlyrainin": SensorStateClass.TOTAL_INCREASING,
"last24hrainin": None,
"eventrainmm": SensorStateClass.TOTAL_INCREASING,
"hourlyrainmm": None,
"totalrainmm": SensorStateClass.TOTAL_INCREASING,
"dailyrainmm": SensorStateClass.TOTAL_INCREASING,
"weeklyrainmm": SensorStateClass.TOTAL_INCREASING,
"monthlyrainmm": SensorStateClass.TOTAL_INCREASING,
"yearlyrainmm": SensorStateClass.TOTAL_INCREASING,
"last24hrainmm": None,
"erain_piezo": SensorStateClass.TOTAL_INCREASING,
"hrain_piezo": None,
"drain_piezo": SensorStateClass.TOTAL_INCREASING,
"wrain_piezo": SensorStateClass.TOTAL_INCREASING,
"mrain_piezo": SensorStateClass.TOTAL_INCREASING,
"yrain_piezo": SensorStateClass.TOTAL_INCREASING,
"last24hrain_piezo": None,
"erain_piezomm": SensorStateClass.TOTAL_INCREASING,
"hrain_piezomm": None,
"drain_piezomm": SensorStateClass.TOTAL_INCREASING,
"wrain_piezomm": SensorStateClass.TOTAL_INCREASING,
"mrain_piezomm": SensorStateClass.TOTAL_INCREASING,
"yrain_piezomm": SensorStateClass.TOTAL_INCREASING,
"last24hrain_piezomm": None,
}but then we need to introduce another warning for a sensor not in this list, which just makes this change increasing more complicated.
Given that
- we are fixing a serious regression many users face that can cause data loss,
- previous code was using a negative list,
- there are far more accumulating sensors than rolling window sensors, and
- the long term direction is to move this discrimination into upstream aioecowitt library,
I'd suggest we don't add complexity here and keep the fix as simple and minimal as possible to avoid introducing even more issues. We can always evaluate any improvement separately in the future if needed.
There was a problem hiding this comment.
but then we need to introduce another warning for a sensor not in this list
Sorry, I don't see it, which one are you talking about?
There was a problem hiding this comment.
I'm not saying you need to set a full list, but I think all sensors should default to state_class=None
Then only the "valid" sensors should get the state_class overriden to SensorStateClass.TOTAL_INCREASING
_TOTAL_INCREASING_RAIN_COUNT_SENSORS: Final = {
# Lifetime
"totalrainin": SensorStateClass.TOTAL_INCREASING,
"totalrainmm": SensorStateClass.TOTAL_INCREASING,
# Yearly, resets 1st day of the year
"yearlyrainin": SensorStateClass.TOTAL_INCREASING,
"yearlyrainmm": SensorStateClass.TOTAL_INCREASING,
"yrain_piezo": SensorStateClass.TOTAL_INCREASING,
"yrain_piezomm": SensorStateClass.TOTAL_INCREASING,
# Monthly, resets 1st day of the month
"monthlyrainin": SensorStateClass.TOTAL_INCREASING,
"monthlyrainmm": SensorStateClass.TOTAL_INCREASING,
"mrain_piezo": SensorStateClass.TOTAL_INCREASING,
"mrain_piezomm": SensorStateClass.TOTAL_INCREASING,
# Weekly, resets 1st day of the week
"weeklyrainin": SensorStateClass.TOTAL_INCREASING,
"weeklyrainmm": SensorStateClass.TOTAL_INCREASING,
"wrain_piezo": SensorStateClass.TOTAL_INCREASING,
"wrain_piezomm": SensorStateClass.TOTAL_INCREASING,
}I'm not sure about the long-term value for "daily" or for "event"
There was a problem hiding this comment.
I'm not sure about the long-term value for "daily" or for "event"
As a user of the integration:
- Event represents rain that doesn't have a break of more than 24 hours. It is rain that continues on and off - not continuous - but with no more than 23 hours between rainfall being recorded. A rain event may span days, or even weeks, while day and week sensors reset on their boundaries.
- Daily is of use for planning irrigation (sprinkler) automation; if you have had a certain amount of rain total in a certain number of days, then there is lesser or no need for irrigation.
That's the near-term need for irrigation automation, but the long-term need is for adjusting the amount of irrigation (duration of cycle).
There was a problem hiding this comment.
And again, this discrimination shouldn't live in Home Assistant code, so this list should be rather short-lived before necessary arrangement is done on aioecowitt library.
There was a problem hiding this comment.
If it's as short-lived as you say it will be, then no harm in making the default None
There was a problem hiding this comment.
There is. The short-live can still be a while, and it also puts more code in Home Assistant unnecessarily.
I don't want to continue this unproductive discussion. I have been trying to address your arguments but you ignore mine. And it seems both of us are rather determined on this.
If you strongly believe it shouldn't be this way, feel free to raise a different PR and leave it to someone else's judgement on which one should be merged. I'm not going to change mine without seeing a convincing argument.
There was a problem hiding this comment.
If you strongly believe it shouldn't be this way
I do, because your way can introduce actual bugs:
- a missing state class is a feature request, which can be improved upon
- a invalid state class is a bug
feel free to raise a different PR
As you wish - I have opened alternative #158528
There was a problem hiding this comment.
I'm unresolving this conversation. I thin it isn't finished.
If we want to really be comprehensive, then I'd go with something like
Honestly, I think that solution is the best approach.
EXPLICIT ALWAYS WINS.
I suggest define them all fully explicitly as full integration descriptions.
Hacks like: if sensor.key in _ROLLING_WINDOW_RAIN_COUNT_SENSORS is the source of all weirdness to begin with.
We need to start extensively defining every sensor. I'm going to put my feet down on this one.
../Frenck
|
Wait for review. Please don't ping for that. If you want to learn more about our review processes, please check out developer documentation here: https://developers.home-assistant.io/docs/review-process |
frenck
left a comment
frenck
left a comment
There was a problem hiding this comment.
As per discussion above. We need to make this all explicit, as the magic approach is clearly not working.
This needs to come out as a solid solution, that addresses the issue for now and the future.
../Frenck
Blogging my personal ramblings at frenck.dev
|
Okay, I changed it to the comprehensive list. When I post #158204 (comment) I was under the impression that adding a warning is more cumbersome that requires extra boilerplate like introducing translations etc. but it turns out that just a single Thanks for pushing on that, @frenck. |
frenck
left a comment
frenck
left a comment
There was a problem hiding this comment.
This is not what I meant. Let get rid of the constant fully and extend the entity descriptions instead. Make the list of expected entities and their behavior explicit, removing the need for the whole state class replacing magic.
I don't think that's a good idea. aioecowitt has in total 288 sensors in its The The alternative is we abandon this quick fix, and try to merge an upstream change then update the dependency and mapping code here. |
|
I've also raised home-assistant-libs/aioecowitt#312 to change the upstream code. It's not clear to me how the process would look like, but my understanding is to fix it in the proper way, we need to
|
|
Even if my review was stale, the solution here has already been rejected by Frenck The logic should be moved to the backend library |
|
I think in the end, yes we do want entity descriptions for every data point. Except removing the state class from all the values was a regression and while in the end I would also like to see an entity description based setup, it currently doesn't record statistics for all entities and I think that's way more inconvenient for the end user. So let's merge this for now, and let's review an entity description based setup in the future. |
|
Fixes #159188 |
Proposed change
Ecowtt products produce many rain count sensors, there are largely three types:
Total sensors and resetting sensors should have state class of
TOTAL_INCREASING, and the rolling window ones should haveMEASUREMENTas their state class correspondingly.Previously, the code only counted hourly sensors as rolling window sensors, and #155321 was raised because 24h sensors are also rolling window but the code didn't take that into consideration.
As described in #157882 (comment), #155358 by @ogruendel was raised to fix the above issue, however, it was done in a wrong approach by changing all resetting sensors to
TOTAL, and subsequent PRs #155812 and #157409 are heading down the wrong way further by completely removing the state class, causing long-term statistics for those resetting sensors to be deleted.This PR reverts the three aforementioned PRs, and tries to assign the right state class for all the sensors.
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: