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

Catch ClientConnectorError and TimeOutError in APSystems #132027

Merged
merged 9 commits into from
Dec 23, 2024
Merged

Catch ClientConnectorError and TimeOutError in APSystems #132027

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
e034ad2
Catch ClientConnectorError in APSystems
Thomas55555 Dec 1, 2024
f519c9e
fix
Thomas55555 Dec 1, 2024
27664c9
also catch TimeoutError
Thomas55555 Dec 2, 2024
0b665ae
Merge branch 'dev' into ClientConnectorError
Thomas55555 Dec 4, 2024
ba82284
Merge branch 'dev' into ClientConnectorError
Thomas55555 Dec 16, 2024
e224819
Merge branch 'dev' into ClientConnectorError
Thomas55555 Dec 18, 2024
3796ebb
add tests
Thomas55555 Dec 18, 2024
11572eb
Merge branch 'dev' into ClientConnectorError
Thomas55555 Dec 22, 2024
2386c79
Merge branch 'dev' into ClientConnectorError
Thomas55555 Dec 23, 2024
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
10 changes: 9 additions & 1 deletion homeassistant/components/apsystems/number.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

from __future__ import annotations

from aiohttp import ClientConnectorError

from homeassistant.components.number import NumberDeviceClass, NumberEntity, NumberMode
from homeassistant.const import UnitOfPower
from homeassistant.core import HomeAssistant
Expand Down Expand Up @@ -45,7 +47,13 @@ def __init__(

async def async_update(self) -> None:
"""Set the state with the value fetched from the inverter."""
self._attr_native_value = await self._api.get_max_power()
try:
status = await self._api.get_max_power()
except (TimeoutError, ClientConnectorError):
self._attr_available = False
Copy link
Contributor

@epenet epenet Dec 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

See https://developers.home-assistant.io/docs/core/integration-quality-scale/rules/log-when-unavailable

If internet/device/service is unavailable, log once when unavailable and once when back connected

It looks to me like you never log...

Edit: Or maybe it's because you don't want to log for each entity...

Copy link
Contributor

Choose a reason for hiding this comment

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

This is true, the integration never logs the change. How is it supposed to be? Would it be reasonable to log for every platform (switch, number and sensor)? As each of them uses a kind of different method regarding the data fetching

Copy link
Contributor

Choose a reason for hiding this comment

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

Standard way would be something like this:

Suggested change
self._attr_available = False
if self._attr_available:
_LOGGER.info(...offline)
self._attr_available = False
else:
if not self._attr_available:
_LOGGER.info(...back online)
self._attr_available = True

But I don't know enough about apsystems to make a proper recommendation.
We don't want to flood, but I think we do recommend visibility for the user.

Copy link
Contributor

Choose a reason for hiding this comment

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

But this would run on every update, wouldn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But this would run on every update, wouldn't it?

Yes, but we would implement a variable to remember if we have already logged the availability change.

Copy link
Contributor Author

@Thomas55555 Thomas55555 Dec 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

See https://developers.home-assistant.io/docs/core/integration-quality-scale/rules/log-when-unavailable

If internet/device/service is unavailable, log once when unavailable and once when back connected

It looks to me like you never log...

Edit: Or maybe it's because you don't want to log for each entity...

This integration uses partly a DataUpdateCoordinator (https://developers.home-assistant.io/docs/integration_fetching_data?_highlight=fetching&_highlight=data#coordinated-single-api-poll-for-data-for-all-entities) and partly separate polling (https://developers.home-assistant.io/docs/integration_fetching_data?_highlight=fetching&_highlight=data#separate-polling-for-each-individual-entity).
With #131930 implemented we would log the availability for the coordinator. Which should in most cases also apply for the single entities. Maybe we can raise UpdatedFailed in #131930 for some more Exceptions, to get more cases covered?

else:
self._attr_available = True
self._attr_native_value = status

async def async_set_native_value(self, value: float) -> None:
"""Set the desired output power."""
Expand Down
13 changes: 12 additions & 1 deletion tests/components/apsystems/test_number.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
DOMAIN as NUMBER_DOMAIN,
SERVICE_SET_VALUE,
)
from homeassistant.const import ATTR_ENTITY_ID, Platform
from homeassistant.const import ATTR_ENTITY_ID, STATE_UNAVAILABLE, Platform
from homeassistant.core import HomeAssistant
from homeassistant.helpers import entity_registry as er

Expand Down Expand Up @@ -46,6 +46,17 @@ async def test_number(
await hass.async_block_till_done()
state = hass.states.get(entity_id)
assert state.state == "50"
mock_apsystems.get_max_power.side_effect = TimeoutError()
await hass.services.async_call(
NUMBER_DOMAIN,
SERVICE_SET_VALUE,
service_data={ATTR_VALUE: 50.1},
target={ATTR_ENTITY_ID: entity_id},
blocking=True,
)
await hass.async_block_till_done()
state = hass.states.get(entity_id)
assert state.state == STATE_UNAVAILABLE


@pytest.mark.usefixtures("mock_apsystems")
Expand Down