Sun WEG integration #88272
Sun WEG integration #88272
Conversation
home-assistant
bot
added
cla-signed
has-tests
new-integration
integration: sunweg
labels
|
@Lash-L can you please review it again? |
homeassistant/components/sunweg/sensor_types/sensor_entity_description.py
Outdated
Show resolved
Hide resolved
homeassistant/components/sunweg/sensor_types/sensor_entity_description.py
Outdated
Show resolved
Hide resolved
|
@frenck is it possible to make 2023.11? |
| inverter_list = [i for i in self.data.inverters if i.id == inverter_id] | ||
| if len(inverter_list) == 0: |
There was a problem hiding this comment.
not a requirement, just a suggestion/option - you could combine these using a walrus operator
| return None | ||
| inverter = inverter_list[0] | ||
|
|
||
| if device_type == "inverter": |
There was a problem hiding this comment.
might be better to make these string values enums?
| return self.data.__dict__.get(variable) | ||
|
|
||
| inverter_list = [i for i in self.data.inverters if i.id == inverter_id] | ||
| if len(inverter_list) == 0: |
There was a problem hiding this comment.
| if len(inverter_list) == 0: | |
| if not inverter_list: |
not a requirement, just an idea
| entity_description.name, | ||
| ) | ||
| variable = entity_description.api_variable_key | ||
| previous_metric = entity_description.native_unit_of_measurement |
There was a problem hiding this comment.
strange variable name. Is it not previous unit?
| login_response = await self.hass.async_add_executor_job(self.api.authenticate) | ||
|
|
||
| if not login_response: |
There was a problem hiding this comment.
another candidate for a walrus operator
| user_input = FIXTURE_USER_INPUT.copy() | ||
|
|
||
| with patch.object( | ||
| APIHelper, "authenticate", return_value=SUNWEG_LOGIN_RESPONSE |
There was a problem hiding this comment.
this one screams "convert it to a fixture" to me because it's used multiple times
| ), patch.object( | ||
| APIHelper, | ||
| "listPlants", | ||
| return_value=[deepcopy(SUNWEG_PLANT_RESPONSE)], |
There was a problem hiding this comment.
same thing re: fixture for this one. You can also avoid the deepcopying by scoping the fixture down to the test
| ): | ||
| """Retrieve from a Plant the desired variable value.""" | ||
| if device_type == "total": | ||
| return self.data.__dict__.get(variable) |
There was a problem hiding this comment.
Using __dict__ to access attributes is really bad practice.
There was a problem hiding this comment.
to be clear, getattr should be used to get an attribute by its name
| self.data = user_input | ||
| return await self.async_step_plant() | ||
|
|
||
| async def async_step_plant(self, user_input=None) -> FlowResult: |
There was a problem hiding this comment.
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: |
|
|
||
| DOMAIN = "sunweg" | ||
|
|
||
| PLATFORMS = [Platform.SENSOR] |
There was a problem hiding this comment.
Please define constants that are only used within a single module within that module. Only shared constants should go in a const.py module.
|
|
||
|
|
||
| def get_device_list( | ||
| api: APIHelper, config: MappingProxyType[str, Any] |
There was a problem hiding this comment.
Why do we need the whole config here instead of just the plant id in the config?
| self.previous_values: dict = {} | ||
|
|
||
| @Throttle(SCAN_INTERVAL) | ||
| def update(self) -> None: |
There was a problem hiding this comment.
Why aren't we using a DataUpdateCoordinator instead? That's our recommended way of polling a single api for multiple entities.
|
|
||
| @dataclass | ||
| class SunWEGRequiredKeysMixin: | ||
| """Mixin for required keys.""" |
There was a problem hiding this comment.
Nowadays we can set kw_only=True in the dataclass decorator to avoid the need for a mixin for required keys.
https://developers.home-assistant.io/docs/core/entity#example
| api = MagicMock() | ||
| api.plant = MagicMock(return_value=plant) | ||
| api.complete_inverter = MagicMock() | ||
| data = SunWEGData(api, 0) |
There was a problem hiding this comment.
Don't access integration details in the tests. Set up the integration while patching the library and assert core state and mock calls.
https://developers.home-assistant.io/docs/development_testing#writing-tests-for-integrations
|
|
||
| def get_data( | ||
| self, | ||
| entity_description: SunWEGSensorEntityDescription, |
There was a problem hiding this comment.
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.
| 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( |
There was a problem hiding this comment.
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.
| # 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 |
There was a problem hiding this comment.
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.
Proposed change
Add integration to https://sunweg.net platform for pulling solar energy production data.
Type of change
Additional information
Checklist
black --fast 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..coveragerc.To help with the load of incoming pull requests: