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.

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

Sun WEG integration #88272

Merged
merged 43 commits into from
Dec 9, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
df7b45e
feat(sunweg): initial support
rokam Feb 16, 2023
7eb2712
chore: removed commented out code
rokam Feb 16, 2023
6bb1f62
chore: removed warning
rokam Feb 16, 2023
a8b14d1
fix: set never_resets for total sensors
rokam Feb 16, 2023
91c97cc
test: some tests
rokam Feb 16, 2023
caa511e
fix(sunweg): default plantid type
rokam Feb 17, 2023
765f781
fix(sunweg): return first plant id
rokam Feb 17, 2023
ee994b8
test(sunweg): improved code coverage
rokam Feb 17, 2023
966bffc
Merge branch 'dev' into features/sunweg
rokam Feb 17, 2023
9f88ca5
chore(sunweg): missing FlowResult return type
rokam Feb 18, 2023
9d66142
chore(sunweg): removed unused strings
rokam Feb 18, 2023
4fb10b6
perf(sunweg): using only one api instance
rokam Feb 18, 2023
6211b7c
chore(sunweg): removed uneeded atribute
rokam Feb 22, 2023
a6fb05c
refact(sunweg): small refactoring
rokam Feb 22, 2023
43ba596
refact(sunweg): typing
rokam Feb 22, 2023
1c1549c
chore(sunweg): comments
rokam Feb 24, 2023
f25c0e3
Merge branch 'dev' into features/sunweg
rokam Feb 24, 2023
59b1084
chore(sunweg): bump version
rokam Feb 24, 2023
a236f9a
Merge branch 'dev' into features/sunweg
rokam Mar 3, 2023
6e808e0
chore(sunweg): bump lib version
rokam Mar 3, 2023
8fb697b
test(sunweg): different mocking and coverage
rokam Mar 3, 2023
8301e6b
Merge branch 'dev' into features/sunweg
rokam Mar 3, 2023
7c43bac
Merge branch 'dev' into features/sunweg
rokam Mar 9, 2023
77cccc7
Merge branch 'dev' into features/sunweg
rokam Mar 9, 2023
43e8660
Merge branch 'dev' into features/sunweg
rokam Apr 2, 2023
9ac899a
Merge branch 'dev' into features/sunweg
rokam May 13, 2023
c472427
Merge branch 'dev' into features/sunweg
rokam May 16, 2023
ab820f9
Merge branch 'dev' into features/sunweg
rokam May 22, 2023
ae5c7ab
Merge branch 'dev' into features/sunweg
rokam Jun 25, 2023
5a50249
Merge branch 'dev' into features/sunweg
rokam Jul 21, 2023
c840220
Merge branch 'dev' into features/sunweg
rokam Jul 25, 2023
a2f2351
test: fixed setup component parameter
rokam Jul 25, 2023
9721e48
feat: dynamic metrics
rokam Jul 25, 2023
dcaedfc
Merge branch 'dev' into features/sunweg
rokam Jul 25, 2023
348e39d
Merge branch 'dev' into features/sunweg
rokam Oct 16, 2023
bb9629a
fix(sunweg): ruff
rokam Oct 16, 2023
0102989
fix(sunweg): mypy
rokam Oct 16, 2023
d535699
Merge branch 'dev' into features/sunweg
rokam Oct 16, 2023
a353dd0
Merge branch 'dev' into features/sunweg
rokam Oct 26, 2023
c7bec8f
Merge branch 'dev' into features/sunweg
rokam Nov 22, 2023
adbc989
refact(sunweg): codereview suggestions
rokam Nov 22, 2023
3ccc78f
chore(sunweg): removed unused string
rokam Nov 22, 2023
1ebb336
chore(sunweg): typehint and code formatting
rokam Nov 22, 2023
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
2 changes: 2 additions & 0 deletions CODEOWNERS
Validating CODEOWNERS rules …
Original file line number Diff line number Diff line change
Expand Up @@ -1243,6 +1243,8 @@ build.json @home-assistant/supervisor
/homeassistant/components/suez_water/ @ooii
/homeassistant/components/sun/ @Swamp-Ig
/tests/components/sun/ @Swamp-Ig
/homeassistant/components/sunweg/ @rokam
/tests/components/sunweg/ @rokam
/homeassistant/components/supla/ @mwegrzynek
/homeassistant/components/surepetcare/ @benleb @danielhiversen
/tests/components/surepetcare/ @benleb @danielhiversen
Expand Down
193 changes: 193 additions & 0 deletions homeassistant/components/sunweg/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,193 @@
"""The Sun WEG inverter sensor integration."""
import datetime
import json
import logging

from sunweg.api import APIHelper
from sunweg.plant import Plant

from homeassistant import config_entries
from homeassistant.config_entries import ConfigEntry
from homeassistant.const import CONF_PASSWORD, CONF_USERNAME
from homeassistant.core import HomeAssistant
from homeassistant.helpers.typing import StateType
from homeassistant.util import Throttle

from .const import CONF_PLANT_ID, DOMAIN, PLATFORMS
from .sensor_types.sensor_entity_description import SunWEGSensorEntityDescription

SCAN_INTERVAL = datetime.timedelta(minutes=5)

_LOGGER = logging.getLogger(__name__)


async def async_setup_entry(
hass: HomeAssistant, entry: config_entries.ConfigEntry
) -> bool:
"""Load the saved entities."""
api = APIHelper(entry.data[CONF_USERNAME], entry.data[CONF_PASSWORD])
if not await hass.async_add_executor_job(api.authenticate):
_LOGGER.error("Username or Password may be incorrect!")
rokam marked this conversation as resolved.
Show resolved Hide resolved
return False
hass.data.setdefault(DOMAIN, {})[entry.entry_id] = SunWEGData(
api, entry.data[CONF_PLANT_ID]
)
await hass.config_entries.async_forward_entry_setups(entry, PLATFORMS)
return True


async def async_unload_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool:
"""Unload a config entry."""
hass.data[DOMAIN].pop(entry.entry_id)
if len(hass.data[DOMAIN]) == 0:
hass.data.pop(DOMAIN)
return await hass.config_entries.async_unload_platforms(entry, PLATFORMS)
rokam marked this conversation as resolved.
Show resolved Hide resolved


class SunWEGData:
"""The class for handling data retrieval."""

def __init__(
self,
api: APIHelper,
plant_id: int,
) -> None:
"""Initialize the probe."""

self.api = api
self.plant_id = plant_id
self.data: Plant = None
self.previous_values: dict = {}
rokam marked this conversation as resolved.
Show resolved Hide resolved

@Throttle(SCAN_INTERVAL)
def update(self) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

Why aren't we using a DataUpdateCoordinator instead? That's our recommended way of polling a single api for multiple entities.

https://developers.home-assistant.io/docs/integration_fetching_data#coordinated-single-api-poll-for-data-for-all-entities

"""Update probe data."""
_LOGGER.debug("Updating data for plant %s", self.plant_id)
try:
self.data = self.api.plant(self.plant_id)
for inverter in self.data.inverters:
self.api.complete_inverter(inverter)
except json.decoder.JSONDecodeError:
_LOGGER.error("Unable to fetch data from SunWEG server")
_LOGGER.debug("Finished updating data for plant %s", self.plant_id)

def get_api_value(
self,
variable: str,
device_type: str,
inverter_id: int = 0,
deep_name: str | None = None,
):
"""Retrieve from a Plant the desired variable value."""
if device_type == "total":
return self.data.__dict__.get(variable)
Copy link
Member

Choose a reason for hiding this comment

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

Using __dict__ to access attributes is really bad practice.

Copy link
Contributor

Choose a reason for hiding this comment

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

to be clear, getattr should be used to get an attribute by its name


inverter_list = [i for i in self.data.inverters if i.id == inverter_id]
if len(inverter_list) == 0:
Comment on lines +85 to +86
Copy link
Contributor

Choose a reason for hiding this comment

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

not a requirement, just a suggestion/option - you could combine these using a walrus operator

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if len(inverter_list) == 0:
if not inverter_list:

not a requirement, just an idea

return None
inverter = inverter_list[0]

if device_type == "inverter":
Copy link
Contributor

Choose a reason for hiding this comment

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

might be better to make these string values enums?

return inverter.__dict__.get(variable)
if device_type == "phase":
for phase in inverter.phases:
if phase.name == deep_name:
return phase.__dict__.get(variable)
elif device_type == "string":
for mppt in inverter.mppts:
for string in mppt.strings:
if string.name == deep_name:
return string.__dict__.get(variable)
return None

def get_data(
self,
entity_description: SunWEGSensorEntityDescription,
Copy link
Member

Choose a reason for hiding this comment

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

The sensor description is a detail of the sensor platform. Either we should move this function to the sensor platform or the function should be refactored to be unaware of the sensor entity description and just work on a base entity description or some other abstraction.

device_type: str,
inverter_id: int = 0,
deep_name: str | None = None,
) -> StateType | datetime.datetime:
"""Get the data."""
_LOGGER.debug(
"Data request for: %s",
entity_description.name,
)
variable = entity_description.api_variable_key
previous_metric = entity_description.native_unit_of_measurement
Copy link
Contributor

Choose a reason for hiding this comment

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

strange variable name. Is it not previous unit?

api_value = self.get_api_value(variable, device_type, inverter_id, deep_name)
previous_value = self.previous_values.get(variable)
return_value = api_value
if entity_description.api_variable_metric is not None:
entity_description.native_unit_of_measurement = self.get_api_value(
Copy link
Member

@MartinHjelmare MartinHjelmare Dec 17, 2023

Choose a reason for hiding this comment

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

Entity descriptions should not be modified. They are considered frozen.

Generally if the attribute needs to be calculated dynamically it doesn't belong in the entity description. Then it's better to set it inside the entity or pass it to the entity after calculating the attribute.

entity_description.api_variable_metric,
device_type,
inverter_id,
deep_name,
)

# If we have a 'drop threshold' specified, then check it and correct if needed
if (
entity_description.previous_value_drop_threshold is not None
and previous_value is not None
and api_value is not None
and previous_metric == entity_description.native_unit_of_measurement
):
_LOGGER.debug(
(
"%s - Drop threshold specified (%s), checking for drop... API"
" Value: %s, Previous Value: %s"
),
entity_description.name,
entity_description.previous_value_drop_threshold,
api_value,
previous_value,
)
diff = float(api_value) - float(previous_value)

# Check if the value has dropped (negative value i.e. < 0) and it has only
# dropped by a small amount, if so, use the previous value.
# Note - The energy dashboard takes care of drops within 10%
# of the current value, however if the value is low e.g. 0.2
# and drops by 0.1 it classes as a reset.
if -(entity_description.previous_value_drop_threshold) <= diff < 0:
_LOGGER.debug(
(
"Diff is negative, but only by a small amount therefore not a"
" nightly reset, using previous value (%s) instead of api value"
" (%s)"
),
previous_value,
api_value,
)
return_value = previous_value
else:
_LOGGER.debug(
"%s - No drop detected, using API value", entity_description.name
)

# Lifetime total values should always be increasing, they will never reset,
# however the API sometimes returns 0 values when the clock turns to 00:00
# local time in that scenario we should just return the previous value
# Scenarios:
# 1 - System has a genuine 0 value when it it first commissioned:
# - will return 0 until a non-zero value is registered
# 2 - System has been running fine but temporarily resets to 0 briefly
# at midnight:
# - will return the previous value
# 3 - HA is restarted during the midnight 'outage' - Not handled:
# - Previous value will not exist meaning 0 will be returned
# - This is an edge case that would be better handled by looking
# up the previous value of the entity from the recorder
if entity_description.never_resets and api_value == 0 and previous_value:
_LOGGER.debug(
(
"API value is 0, but this value should never reset, returning"
" previous value (%s) instead"
),
previous_value,
)
return_value = previous_value
Comment on lines +128 to +189
Copy link
Contributor

@emontnemery emontnemery Dec 18, 2023

Choose a reason for hiding this comment

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

The workarounds do not seem bullet proof, for example if Home Assistant is restarted when we get a wrong reading the accumulated sum will be permanently wrong.

Considering these complications, I don't think it's correct to use SensorStateClass.TOTAL_INCREASING, the state class should be set to SensorStateClass.TOTAL instead for inverter_energy_today and total_energy_today.

Another option would be to not set any state class at all for the *_today readings, there's not really any reason to accumulate statistics for both a grand total and a daily reading.

More details here: https://developers.home-assistant.io/docs/core/entity/sensor#how-to-choose-state_class-and-last_reset

Please fix this in a follow-up.


self.previous_values[variable] = return_value

return return_value
74 changes: 74 additions & 0 deletions homeassistant/components/sunweg/config_flow.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
"""Config flow for Sun WEG integration."""
from sunweg.api import APIHelper
import voluptuous as vol

from homeassistant import config_entries
from homeassistant.const import CONF_NAME, CONF_PASSWORD, CONF_USERNAME
from homeassistant.core import callback
from homeassistant.data_entry_flow import FlowResult

from .const import CONF_PLANT_ID, DOMAIN


class SunWEGConfigFlow(config_entries.ConfigFlow, domain=DOMAIN):
"""Config flow class."""

VERSION = 1

def __init__(self) -> None:
"""Initialise sun weg server flow."""
self.api: APIHelper = None
self.data: dict = {}
rokam marked this conversation as resolved.
Show resolved Hide resolved

@callback
def _async_show_user_form(self, errors=None) -> FlowResult:
"""Show the form to the user."""
data_schema = vol.Schema(
{
vol.Required(CONF_USERNAME): str,
vol.Required(CONF_PASSWORD): str,
}
)

return self.async_show_form(
rokam marked this conversation as resolved.
Show resolved Hide resolved
step_id="user", data_schema=data_schema, errors=errors
)

async def async_step_user(self, user_input=None) -> FlowResult:
"""Handle the start of the config flow."""
if not user_input:
return self._async_show_user_form()

# Initialise the library with the username & password
self.api = APIHelper(user_input[CONF_USERNAME], user_input[CONF_PASSWORD])
login_response = await self.hass.async_add_executor_job(self.api.authenticate)

if not login_response:
Comment on lines +44 to +46
Copy link
Contributor

Choose a reason for hiding this comment

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

another candidate for a walrus operator

return self._async_show_user_form({"base": "invalid_auth"})

# Store authentication info
self.data = user_input
return await self.async_step_plant()
rokam marked this conversation as resolved.
Show resolved Hide resolved

async def async_step_plant(self, user_input=None) -> FlowResult:
Copy link
Member

Choose a reason for hiding this comment

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

The type annotation is incomplete. Please type the whole signature, including parameters, when adding type annotations.

"""Handle adding a "plant" to Home Assistant."""
plant_list = await self.hass.async_add_executor_job(self.api.listPlants)

if len(plant_list) == 0:
Copy link
Member

Choose a reason for hiding this comment

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

if not plant_list:

return self.async_abort(reason="no_plants")

plants = {plant.id: plant.name for plant in plant_list}

if user_input is None and len(plant_list) > 1:
data_schema = vol.Schema({vol.Required(CONF_PLANT_ID): vol.In(plants)})

return self.async_show_form(step_id="plant", data_schema=data_schema)

if user_input is None and len(plant_list) == 1:
user_input = {CONF_PLANT_ID: plant_list[0].id}

user_input[CONF_NAME] = plants[user_input[CONF_PLANT_ID]]
await self.async_set_unique_id(user_input[CONF_PLANT_ID])
self._abort_if_unique_id_configured()
self.data.update(user_input)
return self.async_create_entry(title=self.data[CONF_NAME], data=self.data)
rokam marked this conversation as resolved.
Show resolved Hide resolved
12 changes: 12 additions & 0 deletions homeassistant/components/sunweg/const.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
"""Define constants for the Sun WEG component."""
from homeassistant.const import Platform

CONF_PLANT_ID = "plant_id"

DEFAULT_PLANT_ID = 0

DEFAULT_NAME = "Sun WEG"

DOMAIN = "sunweg"

PLATFORMS = [Platform.SENSOR]
Copy link
Member

Choose a reason for hiding this comment

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

Please define constants that are only used within a single module within that module. Only shared constants should go in a const.py module.

10 changes: 10 additions & 0 deletions homeassistant/components/sunweg/manifest.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"domain": "sunweg",
"name": "Sun WEG",
"codeowners": ["@rokam"],
"config_flow": true,
"documentation": "https://www.home-assistant.io/integrations/sunweg/",
"iot_class": "cloud_polling",
"loggers": ["sunweg"],
"requirements": ["sunweg==2.0.0"]
}
Loading