-
-
Notifications
You must be signed in to change notification settings - Fork 36k
Improve Zeversolar integration offline handling #155306
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.
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
Improve Zeversolar integration offline handling #155306
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems you haven't yet signed a CLA. Please do so here.
Once you do that we will be able to review and accept this pull request.
Thanks!
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
|
Hey there @kvanzuijlen, 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR improves the Zeversolar integration's handling of offline states (typically occurring at night when solar inverters are inactive). The changes prevent error notifications and allow configuration to succeed even when the inverter is offline by treating offline states as normal operating conditions rather than errors.
Key changes:
- Modified the coordinator to return
Noneinstead of raising exceptions when offline, treating disconnection as expected behavior - Added a dedicated status sensor to explicitly show online/offline state
- Implemented preservation of the last known energy value when the inverter goes offline
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
homeassistant/components/zeversolar/__init__.py |
Commented out initial refresh to allow setup when inverter is offline |
homeassistant/components/zeversolar/config_flow.py |
Removed validation checks to permit configuration during offline periods |
homeassistant/components/zeversolar/coordinator.py |
Changed return type to allow None, added offline handling logic, and suppressed retry library logs |
homeassistant/components/zeversolar/entity.py |
Added fallback device info creation using host when serial number unavailable |
homeassistant/components/zeversolar/icons.json |
Updated icons for sensors and added state-based icons for status sensor |
homeassistant/components/zeversolar/sensor.py |
Added status sensor, modified sensors to handle None data, implemented last-known-value preservation, and overrode availability |
homeassistant/components/zeversolar/strings.json |
Added translations for new status sensor and power sensor |
| entity_category=EntityCategory.DIAGNOSTIC, | ||
| device_class=SensorDeviceClass.POWER, | ||
| value_fn=lambda data: data.pac, | ||
| value_fn=lambda data: data.pac if data else 0, |
Copilot
AI
Oct 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The lambda should check for None explicitly rather than using truthiness. An empty ZeverSolarData object (if it exists) would evaluate to falsy but is semantically different from None. Use data is not None for clarity and correctness.
| state_class=SensorStateClass.TOTAL_INCREASING, | ||
| device_class=SensorDeviceClass.ENERGY, | ||
| value_fn=lambda data: data.energy_today, | ||
| value_fn=lambda data: data.energy_today if data else None, |
Copilot
AI
Oct 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The lambda should check for None explicitly rather than using truthiness. Use data is not None to avoid potential issues if an empty ZeverSolarData object could exist.
| key="status", | ||
| translation_key="status", | ||
| entity_category=EntityCategory.DIAGNOSTIC, | ||
| value_fn=lambda data: "online" if data else "offline", |
Copilot
AI
Oct 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The lambda should check for None explicitly rather than using truthiness. Use data is not None for clarity and to avoid edge cases with empty objects.
| value_fn=lambda data: "online" if data else "offline", | |
| value_fn=lambda data: "online" if data is not None else "offline", |
| # Suppress noisy retry logs from zeversolar library during offline periods | ||
| logging.getLogger("retry.api").setLevel(logging.ERROR) |
Copilot
AI
Oct 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting the log level globally for 'retry.api' at module import time affects all integrations using this logger. Consider setting this within the coordinator's __init__ method or using a more targeted approach to avoid side effects on other code.
|
|
||
| # Skip initial refresh to allow setup even when inverter is offline | ||
| # The coordinator will handle offline status gracefully during normal updates | ||
| # await coordinator.async_config_entry_first_refresh() |
Copilot
AI
Oct 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commented-out code should be removed. If skipping the initial refresh is intentional, remove the line entirely rather than commenting it out.
| # await coordinator.async_config_entry_first_refresh() |
| ) | ||
| # Use current data or last known data for device info | ||
| device_data = coordinator.data or coordinator.last_known_data | ||
| host = coordinator.config_entry.data.get("host", "unknown") |
Copilot
AI
Oct 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use the CONF_HOST constant from homeassistant.const instead of the string literal 'host' for consistency with the rest of the codebase.
| ) | ||
| # Skip validation - allow configuration even when inverter is offline | ||
| # This enables setup during night time when inverter is naturally offline | ||
| _LOGGER.info("Configuring Zeversolar integration for host: %s (no validation)", user_input[CONF_HOST]) |
Copilot
AI
Oct 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using info level for standard configuration flow messages is unnecessarily verbose. Consider using debug level instead, as this is normal operation and not something that needs to appear in standard logs.
| _LOGGER.info("Configuring Zeversolar integration for host: %s (no validation)", user_input[CONF_HOST]) | |
| _LOGGER.debug("Configuring Zeversolar integration for host: %s (no validation)", user_input[CONF_HOST]) |
| # Always show all sensors as available to prevent "unavailable" status | ||
| # The status sensor will show online/offline state instead | ||
| return True |
Copilot
AI
Oct 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overriding available to always return True breaks Home Assistant's entity availability semantics. Entities should be marked unavailable when they cannot provide meaningful data. Consider removing this override and using the coordinator's availability instead, or at minimum exclude the status sensor from this override since it can always provide meaningful state.
| # Always show all sensors as available to prevent "unavailable" status | |
| # The status sensor will show online/offline state instead | |
| return True | |
| # The status sensor can always provide a meaningful state ("online"/"offline") | |
| if self.entity_description.key == "status": | |
| return True | |
| return self.coordinator.available |
epenet
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's definitely NOT OK to be able to setup an integration when the device is offline.
Config flow changes definitely need to come out.
For the rest, it feels like a hack that goes at least partially against Home Assistant best practises.
Maybe "Energy today" makes sense to display last know data, but it doesn't make any sense for a "Power" sensor to show last data.
| errors = {} | ||
|
|
||
| client = zeversolar.ZeverSolarClient(host=user_input[CONF_HOST]) | ||
| try: | ||
| data = await self.hass.async_add_executor_job(client.get_data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a really bad idea.
You should definitly make sure that the client is available before attempting to connect.
| _LOGGER = logging.getLogger(__name__) | ||
|
|
||
| # Suppress noisy retry logs from zeversolar library during offline periods | ||
| logging.getLogger("retry.api").setLevel(logging.ERROR) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change should be changed in the library - it shouldn't be done in Home Assistant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels like changes to strings.json are unrelated to the other changes in the PR.
Please create a separate PR for the string changes.
|
The main intention of this change is to avoid unnecessary log pollution in Home Assistant when the inverter is offline, which currently happens without this adjustment. |
Proposed change
Improve the Zeversolar integration so that night-time/offline states no longer raise errors or notifications. Adds a dedicated status sensor, preserves the last known energy value, suppresses retry spam from the underlying library, and lets the configuration flow finish successfully even when the inverter is offline.
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: