Skip to content
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

Jump to bottom

Migrate Aurora_ABB_Powerone to DataUpdateCoordinator #72363

Merged
merged 25 commits into from
Nov 22, 2023
Merged

Migrate Aurora_ABB_Powerone to DataUpdateCoordinator #72363

Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
e6a21ce
Refactor to DataUpdateCoordinator
davet2001 May 23, 2022
eccb76d
Fix tests for sunset/sunrise
davet2001 May 23, 2022
516a0df
Correct time offsets in tests
davet2001 May 23, 2022
a0d04c8
Fix time intervals (attempt 2)
davet2001 May 23, 2022
d4ea67b
Merge dev
davet2001 Jun 14, 2022
7921d67
Fix tests after rebase
davet2001 Jun 15, 2022
dab0e02
Fix isort
davet2001 Jul 14, 2022
2546087
Address review comments: const and increase cov
davet2001 Oct 12, 2022
3ae8ce2
Fix merge problems
davet2001 Feb 9, 2023
8a3ceef
Refactor, removing unnecessary file
davet2001 Apr 6, 2023
f08a1b8
Perform blocking serial IO in the executor
davet2001 Apr 6, 2023
f8b3c8b
Replace deprecated async_setup_platforms
davet2001 May 8, 2023
586f50a
Merge branch 'dev' into aurora_dataupdatecoordinator
davet2001 May 13, 2023
916aeed
Merge branch 'dev' into aurora_dataupdatecoordinator
davet2001 Aug 21, 2023
8e093b5
Merge dev into aurora_dataupdatecoordinator
davet2001 Aug 21, 2023
5b06c3d
Update based on review comments
davet2001 Aug 21, 2023
6eae5b3
Fix tests
davet2001 Aug 24, 2023
893e254
Merge remote-tracking branch 'upstream/dev' into aurora_dataupdatecoo…
davet2001 Aug 25, 2023
7760e68
Update based on review comments.
davet2001 Aug 25, 2023
2732e94
Update homeassistant/components/aurora_abb_powerone/sensor.py
davet2001 Aug 25, 2023
b3ce850
Merge branch 'dev' into aurora_dataupdatecoordinator
davet2001 Aug 25, 2023
f084896
Merge branch 'dev' into aurora_dataupdatecoordinator
davet2001 Aug 25, 2023
f92c6cf
Merge branch 'dev' into aurora_dataupdatecoordinator
davet2001 Sep 30, 2023
c36f1f5
Use freezer for time deltas.
davet2001 Oct 5, 2023
e1dae93
Address review comments
davet2001 Oct 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
66 changes: 62 additions & 4 deletions homeassistant/components/aurora_abb_powerone/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,14 @@

import logging

from aurorapy.client import AuroraSerialClient
from aurorapy.client import AuroraError, AuroraSerialClient, AuroraTimeoutError

from homeassistant.config_entries import ConfigEntry
from homeassistant.const import CONF_ADDRESS, CONF_PORT, Platform
from homeassistant.core import HomeAssistant
from homeassistant.helpers.update_coordinator import DataUpdateCoordinator

from .const import DOMAIN
from .const import DOMAIN, SCAN_INTERVAL

PLATFORMS = [Platform.SENSOR]

Expand All @@ -30,8 +31,10 @@ async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool:

comport = entry.data[CONF_PORT]
address = entry.data[CONF_ADDRESS]
ser_client = AuroraSerialClient(address, comport, parity="N", timeout=1)
hass.data.setdefault(DOMAIN, {})[entry.entry_id] = ser_client
coordinator = AuroraAbbDataUpdateCoordinator(hass, comport, address)
await coordinator.async_config_entry_first_refresh()

hass.data.setdefault(DOMAIN, {})[entry.entry_id] = coordinator
await hass.config_entries.async_forward_entry_setups(entry, PLATFORMS)

return True
Expand All @@ -47,3 +50,58 @@ async def async_unload_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool:
hass.data[DOMAIN].pop(entry.entry_id)

return unload_ok


class AuroraAbbDataUpdateCoordinator(DataUpdateCoordinator[dict[str, float]]):
"""Class to manage fetching AuroraAbbPowerone data."""

def __init__(self, hass: HomeAssistant, comport: str, address: int) -> None:
"""Initialize the data update coordinator."""
self.available_prev = False
self.available = False
self.client = AuroraSerialClient(address, comport, parity="N", timeout=1)
super().__init__(hass, _LOGGER, name=DOMAIN, update_interval=SCAN_INTERVAL)

def _update_data(self) -> dict[str, float]:
"""Fetch new state data for the sensor.

This is the only function that should fetch new data for Home Assistant.
"""
data: dict[str, float] = {}
self.available_prev = self.available
try:
self.client.connect()

# read ADC channel 3 (grid power output)
power_watts = self.client.measure(3, True)
temperature_c = self.client.measure(21)
energy_wh = self.client.cumulated_energy(5)
except AuroraTimeoutError:
self.available = False
Copy link
Member

Choose a reason for hiding this comment

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

If you migrate to using CoordinatorEntity as a base class you don't need to worry about available anymore.

raise UpdateFailed if something goes wrong and everything will automatically go unavailable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have done some experiments with this, and I'm not happy with the result when raising UpdateFailed.

The problem is that UpdateFailed logs an error every time, but going offline during the night is not an error. I don't want to fill the user's logs with errors that are impossible to prevent. By explicitly detecting online/offline I can manage what generates an error and also only show log messages when the device comes back online, even though it is trying 100s of times in the night.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe it changed in all those months, but I just checked the code of the DataUpdateCoordinator and there is a mechanism for this:

        except UpdateFailed as err:
            self.last_exception = err
            if self.last_update_success:
                if log_failures:
                    self.logger.error("Error fetching %s data: %s", self.name, err)
                self.last_update_success = False

Which is doing exactly what you're doing right now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I get what you are saying, but I'm trying to do something very specific, which is to catch the case of a timeout error, then display a single warning message on the first instance.

I think the automatic route only gives me the possibility to suppress all failure logging which isn't what I want.

In addition, this particular device is sometimes unreliable particularly in low light, so I'm planning a follow up PR that does some auto-retrying, see davet2001@f745523 and #82983

Copy link
Member

Choose a reason for hiding this comment

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

I am not talking about the log_failures but about the last_update_success. If you trigger an UpdateFailed, it will log an error, and then it will set last_update_success to False, which is checked before logging.

So in practice, if UpdateFailed is raised 5 times, 1 error is written. It won't log it again, until it actually does an update without a raised error (so it succeeded).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, they are doing very similar things. I did a quick experiment, changing the code to raise UpdateFailed on timeout. It works as you describe. But here is the log output:

Existing code:

2023-08-25 11:30:04.467 WARNING (SyncWorker_2) [homeassistant.components.aurora_abb_powerone] Communication with aurora_abb_powerone lost
2023-08-25 11:35:06.608 INFO (SyncWorker_4) [homeassistant.components.aurora_abb_powerone] Communication with aurora_abb_powerone back online

Using UpdateFailed to manage offline/online messages:

2023-08-25 11:26:35.351 ERROR (MainThread) [homeassistant.components.aurora_abb_powerone] Error fetching aurora_abb_powerone data: No response after 3 tries
2023-08-25 11:28:13.427 INFO (MainThread) [homeassistant.components.aurora_abb_powerone] Fetching aurora_abb_powerone data recovered

They are almost identical, but the key point: going offline due to sunset is not an error. So I want a way of this occurring without raising anything red in the logs (only info/warning). I can't see any other way of doing this.

Copy link
Member

Choose a reason for hiding this comment

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

Okay you got my point, now I know we're on the same wavelength.

Is there a way in the code you can differentiate on if it's because of a failure or if it's because of sunset?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately none that I know of, unplugging the cable behaves the same as sunset (both cause a timeout).

Copy link
Member

Choose a reason for hiding this comment

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

Right, I still think this is a difficult situation. imo, posting something in the logs doesn't have to be seen as something bad, just an FYI. I think we can just use the normal way like every integration does, but just have to teach the user a bit more about how it works via the documentation or the error raised. You could edit the raised error like, "Lost connection to inverter, could it be dark?". We have way more integrations that just log every time the service is responds with a 500 while the next call just succeeds.

I think it would be good to have a third opinion on this matter. Like I get where you come from, but I am just wondering what's the right solution here.

Copy link
Contributor

Choose a reason for hiding this comment

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

@joostlek I don't agree. If there are warning or error prints users will create issues and complain about it. In this case, what could be considered is to log with info level since debug prints are usually very spammy.

I think the PR is good to go as is though, adjusting to info level can be done in a follow-up.

_LOGGER.debug("No response from inverter (could be dark)")
except AuroraError as error:
self.available = False
raise error
else:
data["instantaneouspower"] = round(power_watts, 1)
data["temp"] = round(temperature_c, 1)
data["totalenergy"] = round(energy_wh / 1000, 2)
joostlek marked this conversation as resolved.
Show resolved Hide resolved
self.available = True

finally:
if self.available != self.available_prev:
if self.available:
_LOGGER.info("Communication with %s back online", self.name)
else:
_LOGGER.warning(
"Communication with %s lost",
self.name,
)
if self.client.serline.isOpen():
self.client.close()
davet2001 marked this conversation as resolved.
Show resolved Hide resolved

return data

async def _async_update_data(self) -> dict[str, float]:
"""Update inverter data in the executor."""
return await self.hass.async_add_executor_job(self._update_data)
57 changes: 0 additions & 57 deletions homeassistant/components/aurora_abb_powerone/aurora_device.py
View file
Open in desktop

This file was deleted.

3 changes: 3 additions & 0 deletions homeassistant/components/aurora_abb_powerone/const.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
"""Constants for the Aurora ABB PowerOne integration."""

from datetime import timedelta

DOMAIN = "aurora_abb_powerone"

# Min max addresses and default according to here:
Expand All @@ -8,6 +10,7 @@
MIN_ADDRESS = 2
MAX_ADDRESS = 63
DEFAULT_ADDRESS = 2
SCAN_INTERVAL = timedelta(seconds=30)

DEFAULT_INTEGRATION_TITLE = "PhotoVoltaic Inverters"
DEFAULT_DEVICE_NAME = "Solar Inverter"
Expand Down
88 changes: 35 additions & 53 deletions homeassistant/components/aurora_abb_powerone/sensor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@
import logging
from typing import Any

from aurorapy.client import AuroraError, AuroraSerialClient, AuroraTimeoutError

from homeassistant.components.sensor import (
SensorDeviceClass,
SensorEntity,
Expand All @@ -21,10 +19,21 @@
UnitOfTemperature,
)
from homeassistant.core import HomeAssistant
from homeassistant.helpers.device_registry import DeviceInfo
from homeassistant.helpers.entity_platform import AddEntitiesCallback

from .aurora_device import AuroraEntity
from .const import DOMAIN
from homeassistant.helpers.typing import StateType
from homeassistant.helpers.update_coordinator import CoordinatorEntity

from . import AuroraAbbDataUpdateCoordinator
from .const import (
ATTR_DEVICE_NAME,
ATTR_FIRMWARE,
ATTR_MODEL,
ATTR_SERIAL_NUMBER,
DEFAULT_DEVICE_NAME,
DOMAIN,
MANUFACTURER,
)

_LOGGER = logging.getLogger(__name__)

Expand Down Expand Up @@ -61,70 +70,43 @@ async def async_setup_entry(
"""Set up aurora_abb_powerone sensor based on a config entry."""
entities = []

client = hass.data[DOMAIN][config_entry.entry_id]
coordinator = hass.data[DOMAIN][config_entry.entry_id]
data = config_entry.data

for sens in SENSOR_TYPES:
entities.append(AuroraSensor(client, data, sens))
entities.append(AuroraSensor(coordinator, data, sens))

_LOGGER.debug("async_setup_entry adding %d entities", len(entities))
async_add_entities(entities, True)


class AuroraSensor(AuroraEntity, SensorEntity):
class AuroraSensor(CoordinatorEntity[AuroraAbbDataUpdateCoordinator], SensorEntity):
"""Representation of a Sensor on a Aurora ABB PowerOne Solar inverter."""
davet2001 marked this conversation as resolved.
Show resolved Hide resolved

_attr_has_entity_name = True

def __init__(
self,
client: AuroraSerialClient,
coordinator: AuroraAbbDataUpdateCoordinator,
data: Mapping[str, Any],
entity_description: SensorEntityDescription,
) -> None:
"""Initialize the sensor."""
super().__init__(client, data)
super().__init__(coordinator)
self._data = data
davet2001 marked this conversation as resolved.
Show resolved Hide resolved
self.entity_description = entity_description
self.available_prev = True

def update(self) -> None:
"""Fetch new state data for the sensor.

This is the only method that should fetch new data for Home Assistant.
"""
try:
self.available_prev = self._attr_available
self.client.connect()
if self.entity_description.key == "instantaneouspower":
# read ADC channel 3 (grid power output)
power_watts = self.client.measure(3, True)
self._attr_native_value = round(power_watts, 1)
elif self.entity_description.key == "temp":
temperature_c = self.client.measure(21)
self._attr_native_value = round(temperature_c, 1)
elif self.entity_description.key == "totalenergy":
energy_wh = self.client.cumulated_energy(5)
self._attr_native_value = round(energy_wh / 1000, 2)
self._attr_available = True

except AuroraTimeoutError:
self._attr_state = None
self._attr_native_value = None
self._attr_available = False
_LOGGER.debug("No response from inverter (could be dark)")
except AuroraError as error:
self._attr_state = None
self._attr_native_value = None
self._attr_available = False
raise error
finally:
if self._attr_available != self.available_prev:
if self._attr_available:
_LOGGER.info("Communication with %s back online", self.name)
else:
_LOGGER.warning(
"Communication with %s lost",
self.name,
)
if self.client.serline.isOpen():
self.client.close()
self._attr_unique_id = (
f"{data[ATTR_SERIAL_NUMBER]}_{self.entity_description.key}"
davet2001 marked this conversation as resolved.
Show resolved Hide resolved
)
self._attr_device_info = DeviceInfo(
identifiers={(DOMAIN, self._data[ATTR_SERIAL_NUMBER])},
davet2001 marked this conversation as resolved.
Show resolved Hide resolved
manufacturer=MANUFACTURER,
model=self._data[ATTR_MODEL],
name=self._data.get(ATTR_DEVICE_NAME, DEFAULT_DEVICE_NAME),
sw_version=self._data[ATTR_FIRMWARE],
davet2001 marked this conversation as resolved.
Show resolved Hide resolved
)

@property
def native_value(self) -> StateType:
"""Get the value of the sensor from previously collected data."""
return self.coordinator.data.get(self.entity_description.key)
4 changes: 0 additions & 4 deletions tests/components/aurora_abb_powerone/test_config_flow.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
"""Test the Aurora ABB PowerOne Solar PV config flow."""
from logging import INFO
from unittest.mock import patch

from aurorapy.client import AuroraError, AuroraTimeoutError
Expand Down Expand Up @@ -49,9 +48,6 @@ async def test_form(hass: HomeAssistant) -> None:
), patch(
"aurorapy.client.AuroraSerialClient.firmware",
return_value="1.234",
), patch(
"homeassistant.components.aurora_abb_powerone.config_flow._LOGGER.getEffectiveLevel",
return_value=INFO,
) as mock_setup, patch(
"homeassistant.components.aurora_abb_powerone.async_setup_entry",
return_value=True,
Expand Down
3 changes: 0 additions & 3 deletions tests/components/aurora_abb_powerone/test_init.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,6 @@ async def test_unload_entry(hass: HomeAssistant) -> None:
"""Test unloading the aurora_abb_powerone entry."""

with patch("aurorapy.client.AuroraSerialClient.connect", return_value=None), patch(
"homeassistant.components.aurora_abb_powerone.sensor.AuroraSensor.update",
return_value=None,
), patch(
"aurorapy.client.AuroraSerialClient.serial_number",
return_value="9876543",
), patch(
Expand Down
25 changes: 19 additions & 6 deletions tests/components/aurora_abb_powerone/test_sensor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
"""Test the Aurora ABB PowerOne Solar PV sensors."""
from datetime import timedelta
from unittest.mock import patch

from aurorapy.client import AuroraError, AuroraTimeoutError
Expand All @@ -11,6 +10,7 @@
ATTR_SERIAL_NUMBER,
DEFAULT_INTEGRATION_TITLE,
DOMAIN,
SCAN_INTERVAL,
)
from homeassistant.const import CONF_ADDRESS, CONF_PORT
from homeassistant.core import HomeAssistant
Expand Down Expand Up @@ -103,6 +103,9 @@ async def test_sensor_dark(hass: HomeAssistant) -> None:
# sun is up
with patch("aurorapy.client.AuroraSerialClient.connect", return_value=None), patch(
"aurorapy.client.AuroraSerialClient.measure", side_effect=_simulated_returns
), patch(
"aurorapy.client.AuroraSerialClient.cumulated_energy",
side_effect=_simulated_returns,
), patch(
"aurorapy.client.AuroraSerialClient.serial_number",
return_value="9876543",
Expand All @@ -123,21 +126,28 @@ async def test_sensor_dark(hass: HomeAssistant) -> None:
power = hass.states.get("sensor.mydevicename_power_output")
assert power is not None
assert power.state == "45.7"
utcnow = dt_util.utcnow()

# sunset
with patch("aurorapy.client.AuroraSerialClient.connect", return_value=None), patch(
"aurorapy.client.AuroraSerialClient.measure",
side_effect=AuroraTimeoutError("No response after 10 seconds"),
), patch(
"aurorapy.client.AuroraSerialClient.cumulated_energy",
side_effect=AuroraTimeoutError("No response after 3 tries"),
):
async_fire_time_changed(hass, utcnow + timedelta(seconds=60))
async_fire_time_changed(hass, utcnow + SCAN_INTERVAL * 2)
davet2001 marked this conversation as resolved.
Show resolved Hide resolved
await hass.async_block_till_done()
power = hass.states.get("sensor.mydevicename_power_output")
power = hass.states.get("sensor.mydevicename_total_energy")
assert power.state == "unknown"
# sun rose again
with patch("aurorapy.client.AuroraSerialClient.connect", return_value=None), patch(
"aurorapy.client.AuroraSerialClient.measure", side_effect=_simulated_returns
), patch(
"aurorapy.client.AuroraSerialClient.cumulated_energy",
side_effect=_simulated_returns,
):
async_fire_time_changed(hass, utcnow + timedelta(seconds=60))
async_fire_time_changed(hass, utcnow + SCAN_INTERVAL * 4)
await hass.async_block_till_done()
power = hass.states.get("sensor.mydevicename_power_output")
assert power is not None
Expand All @@ -146,8 +156,11 @@ async def test_sensor_dark(hass: HomeAssistant) -> None:
with patch("aurorapy.client.AuroraSerialClient.connect", return_value=None), patch(
"aurorapy.client.AuroraSerialClient.measure",
side_effect=AuroraTimeoutError("No response after 10 seconds"),
), patch(
"aurorapy.client.AuroraSerialClient.cumulated_energy",
side_effect=AuroraError("No response after 10 seconds"),
):
async_fire_time_changed(hass, utcnow + timedelta(seconds=60))
async_fire_time_changed(hass, utcnow + SCAN_INTERVAL * 6)
await hass.async_block_till_done()
power = hass.states.get("sensor.mydevicename_power_output")
assert power.state == "unknown" # should this be 'available'?
Expand All @@ -160,7 +173,7 @@ async def test_sensor_unknown_error(hass: HomeAssistant) -> None:
with patch("aurorapy.client.AuroraSerialClient.connect", return_value=None), patch(
"aurorapy.client.AuroraSerialClient.measure",
side_effect=AuroraError("another error"),
):
), patch("serial.Serial.isOpen", return_value=True):
mock_entry.add_to_hass(hass)
await hass.config_entries.async_setup(mock_entry.entry_id)
await hass.async_block_till_done()
Expand Down