Fix rain count sensors' state class of Ecowitt #158204
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.
|
|
||
| # Hourly and 24h rain count sensors are rolling window sensors | ||
| _ROLLING_WINDOW_RAIN_COUNT_SENSOR = re.compile( | ||
| "(?:hourly|last24h)rain(?:in|mm)|(?:last24)?hrain_piezo(?:mm)?" |
There was a problem hiding this comment.
[nitpick] Consider using a raw string (r"...") for the regex pattern to improve readability and make escape sequences more explicit. Also, adding parentheses around the entire alternation could make the pattern structure clearer:
_ROLLING_WINDOW_RAIN_COUNT_SENSOR = re.compile(
r"(?:hourly|last24h)rain(?:in|mm)|(?:last24)?hrain_piezo(?:mm)?"
)This doesn't change functionality but improves code maintainability.
| "(?:hourly|last24h)rain(?:in|mm)|(?:last24)?hrain_piezo(?:mm)?" | |
| r"((?:hourly|last24h)rain(?:in|mm)|(?:last24)?hrain_piezo(?:mm)?)" |
There was a problem hiding this comment.
There is nothing need to be escaped, so raw string isn't useful.
| ) | ||
|
|
||
|
|
||
| # Hourly and 24h rain count sensors are rolling window sensors |
There was a problem hiding this comment.
The comment mentions "Hourly and 24h rain count sensors" but the regex also matches piezo rain sensor variants (hrain_piezo, last24hrain_piezo). Consider updating the comment to be more comprehensive:
# Hourly, 24h, and piezo rain count sensors are rolling window sensorsThis makes it clearer that piezo sensors are also included in the pattern.
| # Hourly and 24h rain count sensors are rolling window sensors | |
| # Hourly, 24h, and piezo rain count sensors are rolling window sensors |
There was a problem hiding this comment.
Whether the data comes from piezo or not is orthogonal to whether its time period is hourly or 24h.
|
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)?" | ||
| ) |
There was a problem hiding this comment.
The list is not that extensive, I'd prefer this, as it's not only much easier to comprehend (even with the comment, it took me some time to understand what sensors it matches), it's less complex as well.
| _ROLLING_WINDOW_RAIN_COUNT_SENSOR = re.compile( | |
| "(?:hourly|last24h)rain(?:in|mm)|(?:last24)?hrain_piezo(?:mm)?" | |
| ) | |
| _ROLLING_WINDOW_RAIN_COUNT_SENSORS = { | |
| "hourlyrainin", | |
| "hourlyrainmm", | |
| "last24hrainin", | |
| "last24hrainmm", | |
| "hrain_piezo", | |
| "hrain_piezomm", | |
| "last24hrain_piezo", | |
| "last24hrain_piezomm", | |
| } |
There was a problem hiding this comment.
Would it not make more sense to have the opposite: a list of "total increasing" sensor?
There was a problem hiding this comment.
There was a problem hiding this comment.
according to the PR description and this comment all the 24h & hourly sensors are indeed moving windows, which would mean they are not qualifying for any StateClass. The rest appears to be resetting at fixed intervals, which would make them TOTAL_INCREASING. If that is true, we'd need to remove the state class from those 8 sensors and keep the rest as TOTAL_INCREASING. But I own no Ecowitt so I have no way of verifying if that claim is really true.
There was a problem hiding this comment.
according to the PR description and this comment all the 24h & daily sensors are indeed moving windows, which would mean they are not qualifying for any
StateClass. The rest appears to be resetting at fixed intervals, which would make themTOTAL_INCREASING. If that is true, we'd need to remove the state class from those 8 sensors and keep the rest asTOTAL_INCREASING. But I own no Ecowitt so I have no way of verifying if that claim is really true.
Event = sum of all rain in consecutive blocks of 24 hours. Resets at 24 hours with 0mm rain. So if last rain stops at 9am, it will reset next day at 9am. Will sum up all rain events if they happen within 24h of each other, and so will sum up multiple days.
The others reset at the end of the period of their given name.
There was a problem hiding this comment.
Also mind, that the name and source of information may differ according to Ecowitt sensor devices.... see post #157882 (comment)
| "totalrainmm", | ||
| ): | ||
| # Rolling window sensors must use measurement state classes | ||
| if _ROLLING_WINDOW_RAIN_COUNT_SENSOR.fullmatch(sensor.key): |
There was a problem hiding this comment.
| if _ROLLING_WINDOW_RAIN_COUNT_SENSOR.fullmatch(sensor.key): | |
| if sensor.key in _ROLLING_WINDOW_RAIN_COUNT_SENSORS: |
| description = dataclasses.replace( | ||
| description, | ||
| state_class=SensorStateClass.TOTAL_INCREASING, | ||
| state_class=SensorStateClass.MEASUREMENT, |
There was a problem hiding this comment.
The consensus from previous discussions was to remove these from the LTS, given that they're largely redundant with the remaining data. I don't want to be the one to call the shots here again (with my PR I basically just wanted to avoid the rain data from being completely removed) so I'd like to leave the decision to someone else.
|
|
||
| import dataclasses | ||
| from datetime import datetime | ||
| import re |
There was a problem hiding this comment.
Wouldn't be needed with the explicit set.
| import re |
9 participants
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: