Add minutes and energy sensors to Nuheat#160751
Add minutes and energy sensors to Nuheat#160751eric wants to merge 1 commit intohome-assistant:devfrom
Conversation
![home-assistant[bot]](https://avatars.githubusercontent.com/in/97978?s=60&v=4){kind=small}
home-assistant
Bot
added
cla-needed
has-tests
integration: nuheat
new-feature
new-platform
labels
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
|
Hey there @tstabrawa, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
home-assistant
Bot
added
Quality Scale: No score
cla-recheck
cla-signed
and removed
cla-recheck
cla-needed
labels
There was a problem hiding this comment.
Pull request overview
This PR adds energy monitoring capabilities to the NuHeat integration by introducing sensors for heating time and energy consumption. The energy data is fetched from the NuHeat API, with an option to calculate energy usage from user-provided floor area and watt density when API data is unavailable.
Changes:
- Added new sensor platform with heating time (minutes) and energy (kWh) sensors
- Implemented energy data coordinator to fetch data from the NuHeat API
- Added config flow options to allow users to configure floor area and watt density for energy calculations
- Extended test coverage for energy data handling scenarios
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| homeassistant/components/nuheat/init.py | Added energy data coordinator, NuHeatEnergyData dataclass, and logic to fetch/calculate energy from API or user options |
| homeassistant/components/nuheat/sensor.py | New sensor platform implementation with energy and heating time entities |
| homeassistant/components/nuheat/const.py | Added sensor platform and new configuration constants for floor area and watt density |
| homeassistant/components/nuheat/config_flow.py | Added options flow for configuring energy calculation parameters |
| homeassistant/components/nuheat/climate.py | Updated to handle new tuple structure in runtime data |
| homeassistant/components/nuheat/strings.json | Added translations for sensor entities and options flow |
| tests/components/nuheat/mocks.py | Added mock energy data and session creation helper |
| tests/components/nuheat/test_init.py | Added tests for energy sensor creation and calculation scenarios |
| tests/components/nuheat/test_config_flow.py | Added test for options flow |
| tests/components/nuheat/test_climate.py | Updated tests to mock energy API endpoint |
| async def _async_update_energy_data() -> NuHeatEnergyData: | ||
| """Fetch energy data from NuHeat API.""" | ||
| # Access session ID from library internals (no public API available) | ||
| session_id: str = thermostat._session._session_id # noqa: SLF001 |
There was a problem hiding this comment.
Accessing private attributes through multiple levels of indirection (thermostat._session._session_id) is fragile and tightly couples the code to library internals. Consider checking if the nuheat library exposes a public API for session management, or store the session ID separately during initialization to avoid repeated private attribute access.
eric
force-pushed
the
nuheat-add-energy-sensor
branch
from
17106be to
892cc1a
Compare
| "title": "Connect to the NuHeat" | ||
| } | ||
| } | ||
| }, | ||
| "options": { | ||
| "step": { | ||
| "init": { | ||
| "description": "The NuHeat API only reports energy usage if you have configured watt density in the NuHeat app. If you haven't done that, you can enter your floor specifications here to calculate energy from heating time instead.", | ||
| "data": { | ||
| "floor_area": "Heated floor area (square feet)", | ||
| "watt_density": "Watt density (watts per square foot)" | ||
| }, | ||
| "data_description": { | ||
| "floor_area": "The total area covered by your heated floor mat. Check your installation documentation or measure the heated area.", | ||
| "watt_density": "Power output per square foot. Most NuHeat mats are 12 watts/sqft. Check your mat's specifications if unsure." |
There was a problem hiding this comment.
The description should use sentence case consistently. Change 'NuHeat' references to lowercase 'Nuheat' except at the beginning of sentences, per Home Assistant writing style guidelines.
| "title": "Connect to the NuHeat" | |
| } | |
| } | |
| }, | |
| "options": { | |
| "step": { | |
| "init": { | |
| "description": "The NuHeat API only reports energy usage if you have configured watt density in the NuHeat app. If you haven't done that, you can enter your floor specifications here to calculate energy from heating time instead.", | |
| "data": { | |
| "floor_area": "Heated floor area (square feet)", | |
| "watt_density": "Watt density (watts per square foot)" | |
| }, | |
| "data_description": { | |
| "floor_area": "The total area covered by your heated floor mat. Check your installation documentation or measure the heated area.", | |
| "watt_density": "Power output per square foot. Most NuHeat mats are 12 watts/sqft. Check your mat's specifications if unsure." | |
| "title": "Connect to Nuheat" | |
| } | |
| } | |
| }, | |
| "options": { | |
| "step": { | |
| "init": { | |
| "description": "The Nuheat API only reports energy usage if you have configured watt density in the Nuheat app. If you haven't done that, you can enter your floor specifications here to calculate energy from heating time instead.", | |
| "data": { | |
| "floor_area": "Heated floor area (square feet)", | |
| "watt_density": "Watt density (watts per square foot)" | |
| }, | |
| "data_description": { | |
| "floor_area": "The total area covered by your heated floor mat. Check your installation documentation or measure the heated area.", | |
| "watt_density": "Power output per square foot. Most Nuheat mats are 12 watts/sqft. Check your mat's specifications if unsure." |
| floor_area = entry.options.get(CONF_FLOOR_AREA) | ||
| if floor_area: | ||
| watt_density = entry.options.get(CONF_WATT_DENSITY, DEFAULT_WATT_DENSITY) | ||
| # Power (watts) = floor_area (sqft) * watt_density (watts/sqft) | ||
| # Energy (kWh) = power (watts) * time (minutes) / 60 / 1000 | ||
| power_watts = floor_area * watt_density | ||
| calculated_kwh = (power_watts * total_minutes) / 60 / 1000 | ||
| energy_kwh: float | None = calculated_kwh |
There was a problem hiding this comment.
The conditional logic prioritizes user-configured floor area over API-provided kWh data. This may be unexpected behavior. Consider documenting this precedence decision or validating that this is the intended priority.
eric
force-pushed
the
nuheat-add-energy-sensor
branch
from
892cc1a to
9e056ec
Compare
tstabrawa
left a comment
tstabrawa
left a comment
There was a problem hiding this comment.
@eric First of all, I love that you want to improve the Nuheat integration with this feature. This is one that I've also wanted to see in the past, and I commend your efforts to add it, especially considering how much effort appears to have gone into it.
Unfortunately, directly accessing the device's API breaks one of the integration rules for Home Assistant. Specifically:
4. Communication with devices/services
- All API specific code has to be part of a third party library hosted on PyPi. Home Assistant should only interact with objects and not make direct calls to the API.
I don't expect that the core Home Assistant dev's would merge this PR as a result.
In theory, the way to avoid breaking this rule would be to contribue to the upstream API project. Unfortunately, that project appears to have gone unmaintained for a few years now, so I'm not sure how much success you'd have getting a PR through there either. So far, the Nuheat integration has been stable without any upstream activity, at the cost of not really being able to add new features (such as #122675).
If there's sufficient interest in the community, probably the only way forward would be for someone to fork the upstream repo and pick up maintenance on that. I've considered doing the same in the past and have also considered re-architecting it using aiohttp, but I just can't commit the amount of time needed to get started, let alone to maintain it. If you feel like you have the time and expertise to take something like that on, I would be fine with a PR that involves switching the upstream PyPi dependency to a new project that you maintain (though I may try to rope you in to help out as a code-owner of the integration, too).
| async def _async_update_energy_data() -> NuHeatEnergyData: | ||
| """Fetch energy data from NuHeat API.""" | ||
| now = dt_util.now() | ||
| date_str = now.strftime("%Y-%m-%d") | ||
|
|
||
| url = "https://mynuheat.com/api/energyusage" | ||
| params: dict[str, Any] = { | ||
| "sessionid": _get_session_id(thermostat), | ||
| "serialnumber": serial_number, | ||
| "view": "day", | ||
| "date": date_str, | ||
| "history": "0", | ||
| "calc": "yes", | ||
| "weekstart": "sunday", | ||
| } | ||
|
|
||
| try: | ||
| async with session.get(url, params=params) as response: | ||
| if response.status == 401: | ||
| _LOGGER.debug("Session expired, re-authenticating") | ||
| await hass.async_add_executor_job(_reauthenticate, thermostat) | ||
| params["sessionid"] = _get_session_id(thermostat) | ||
| async with session.get(url, params=params) as retry_response: | ||
| retry_response.raise_for_status() | ||
| data = await retry_response.json() | ||
| else: | ||
| response.raise_for_status() | ||
| data = await response.json() |
There was a problem hiding this comment.
Unfortunately, directly accessing the device's API breaks one of the integration rules for Home Assistant. Specifically:
4. Communication with devices/services
- All API specific code has to be part of a third party library hosted on PyPi. Home Assistant should only interact with objects and not make direct calls to the API.
|
You're right! We would have declined this PR for that exact reason. So I will put it back to draft to keep the discussion available. I also want to offer my help, if needed. Like I am happy to help with making sure the forked library has a way to easily release new versions, as that will decrease maintenance costs. I am not willing to fork it as I am not a user, but I am definitely able to help you get started :) |
eric
force-pushed
the
nuheat-add-energy-sensor
branch
from
9e056ec to
14fcd05
Compare
|
This pull request has been updated and now depends on broox/python-nuheat#21. I don't have the capacity at the moment to take on ownership of any of this, but the code is all here and is solving my problem for myself in my local setup. |
|
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. |
|
Now what? |
|
Euh yea it's a bit meh that the development on the library has gone stale. We could try to reach out via email to the developer to ask to merge and release. Otherwise we'd need to fork (for which we need to make sure the original library owner is indeed not active anymore) |
|
There's no point in having this PR open because it can't be merged for the reasons discussed above. There are two options to make it mergable:
@eric please open a new PR based on one of those two options if you still want to get this merged. |
|
Great job, team. |
Proposed change
The Nuheat API has the number of minutes the floor was on, which can be used to calculate an estimated energy usage. If the settings have been set on the Nuheat server, it will use that to calculate the energy usage, but if not, they can be provided in Home Assistant settings.
Type of change
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: