Skip to content

Fix rain count sensors' state class of Ecowitt #158204

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.

Sign up for GitHub

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

Merged
joostlek merged 9 commits into from
Jan 5, 2026
Merged

Fix rain count sensors' state class of Ecowitt #158204

Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
4943275
Revert "Fix state classes of Ecowitt rain sensors (#157409)"
upsuper Dec 7, 2025
ed20026
Revert "Remove state class for rolling window in ecowitt (#155812)"
upsuper Dec 7, 2025
0bb5c1d
Revert "Change rain count state_class to 'Total' (ecowitt) (#155358)"
upsuper Dec 7, 2025
5f3040a
Fix rain count sensors' state class of Ecowitt
upsuper Dec 7, 2025
000c9ff
fix fmt
upsuper Dec 7, 2025
c5755e7
address review comments
upsuper Dec 8, 2025
620e29c
Merge branch 'dev' into ecowitt-rain-state-class
upsuper Dec 10, 2025
5d8a17c
use comprehensive list
upsuper Dec 10, 2025
a825a5c
Merge branch 'dev' into ecowitt-rain-state-class
upsuper Dec 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 17 additions & 9 deletions homeassistant/components/ecowitt/sensor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,19 @@
)


# Hourly and 24h rain count sensors are rolling window sensors
Comment thread
upsuper marked this conversation as resolved.
Outdated
_ROLLING_WINDOW_RAIN_COUNT_SENSORS = {
Copy link
Copy Markdown
Contributor

@epenet epenet Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I stiln think this should be a positive list, not a negative list.

Copy link
Copy Markdown
Contributor Author

@upsuper upsuper Dec 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@zweckj zweckj Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor

@epenet epenet Dec 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

Copy link
Copy Markdown

@BJReplay BJReplay Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about the long-term value for "daily" or for "event"

@epenet

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

Copy link
Copy Markdown
Contributor Author

@upsuper upsuper Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

@epenet epenet Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's as short-lived as you say it will be, then no harm in making the default None

Copy link
Copy Markdown
Contributor Author

@upsuper upsuper Dec 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

@epenet epenet Dec 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

@frenck frenck Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"hourlyrainin",
"hourlyrainmm",
"hrain_piezo",
"hrain_piezomm",
"last24hrainin",
"last24hrainmm",
"last24hrain_piezo",
"last24hrain_piezomm",
}


ECOWITT_SENSORS_MAPPING: Final = {
EcoWittSensorTypes.HUMIDITY: SensorEntityDescription(
key="HUMIDITY",
Expand Down Expand Up @@ -151,12 +164,14 @@
key="RAIN_COUNT_MM",
native_unit_of_measurement=UnitOfPrecipitationDepth.MILLIMETERS,
device_class=SensorDeviceClass.PRECIPITATION,
state_class=SensorStateClass.TOTAL_INCREASING,
Comment thread
upsuper marked this conversation as resolved.
Outdated
suggested_display_precision=1,
),
EcoWittSensorTypes.RAIN_COUNT_INCHES: SensorEntityDescription(
key="RAIN_COUNT_INCHES",
native_unit_of_measurement=UnitOfPrecipitationDepth.INCHES,
device_class=SensorDeviceClass.PRECIPITATION,
state_class=SensorStateClass.TOTAL_INCREASING,
Comment thread
upsuper marked this conversation as resolved.
Outdated
suggested_display_precision=2,
),
EcoWittSensorTypes.RAIN_RATE_MM: SensorEntityDescription(
Expand Down Expand Up @@ -285,15 +300,8 @@ def _new_sensor(sensor: EcoWittSensor) -> None:
name=sensor.name,
)

# Only total rain needs state class for long-term statistics
if sensor.key in (
"totalrainin",
"totalrainmm",
):
description = dataclasses.replace(
description,
state_class=SensorStateClass.TOTAL_INCREASING,
)
if sensor.key in _ROLLING_WINDOW_RAIN_COUNT_SENSORS:
description = dataclasses.replace(description, state_class=None)
Comment thread
upsuper marked this conversation as resolved.
Outdated

async_add_entities([EcowittSensorEntity(sensor, description)])

Expand Down
Loading