-
-
Notifications
You must be signed in to change notification settings - Fork 33k
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
Add solar charging options to Wallbox integration #139286
base: dev
Are you sure you want to change the base?
Add solar charging options to Wallbox integration #139286
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.
Hi @jorisdrenth
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!
Hey there @hesselonline, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
PR cliviu74/wallbox#63 has been merged so I'll open this PR for review. |
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
@jorisdrenth code looks fine, but please add sufficient tests for the new component. |
@hesselonline I've added tests, based on the test_switch.py test and select test I've found in another component folder. It was kind of a steep learning curve so I'm not sure whether it makes any sense. Can you take a look at 1a3cd1c? |
1a3cd1c
to
a35843a
Compare
looks good on first glance, can you check the code coverage of these tests (wallbox integration should have 100% code cov)? |
@hesselonline Ah thanks for pointing that out, that's a nice feature. I've added extra tests:
The last percent are these lines. Do you have an idea how I can test them because it's just a value I set here in the test response? |
Yes, set 3 different values in test responses |
@hesselonline It has 100% code coverage now. |
59e9988
to
0c53ab7
Compare
@abmantis I think this needs your review before it can be merged? |
async_add_entities( | ||
[WallboxSelect(coordinator, SELECT_TYPES[CHARGER_ECO_SMART_KEY])] | ||
) |
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.
Since SELECT_TYPES
is a dict, I kinda expected the code here to iterate over it (instead of hardcode picking an item from it.
async_add_entities( | |
[WallboxSelect(coordinator, SELECT_TYPES[CHARGER_ECO_SMART_KEY])] | |
) | |
async_add_entities( | |
[WallboxSelect(coordinator, description)] | |
for description in SELECT_TYPES | |
) |
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.
Fixed this: 6fb9681
Your suggested code gave me an error because it only returns the key and not the value. I used the same coding style as other entities in the Wallbox component (see number.py and lock.py.
Note: in switch.py, which I used as base for the select.py file, it's also picking a value hardcoded. I'll fix that in a different PR.
super().__init__(coordinator) | ||
self.entity_description = description | ||
self._attr_unique_id = f"{description.key}-{coordinator.data[CHARGER_DATA_KEY][CHARGER_SERIAL_NUMBER_KEY]}" | ||
self._attr_icon = "mdi:solar-power" |
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.
Icons ideally go into the icons translation file (icons.json
)
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.
Check! d142208
self._attr_options = [ | ||
EcoSmartMode.OFF, | ||
EcoSmartMode.ECO_MODE, | ||
EcoSmartMode.FULL_SOLAR, | ||
] |
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.
These can be set in the entity description, right?
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 is fixed too: jorisdrenth@7a025df
in self.coordinator.data[CHARGER_DATA_KEY][CHARGER_PLAN_KEY][ | ||
CHARGER_FEATURES_KEY | ||
] |
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.
Hmmm, is this is check we need to do before creating the entity? As in, this makes it sounds like this feature isn't always existing?
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.
That's right. Solar charging is only available if a Wallbox Load Balancer is installed, which reads other energy usage in the building by (in my case) reading data from the P1 meter. Your question makes me wonder: is a "exist" option available which I should use in stead of "available"?
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.
@jorisdrenth in the number component we do a try...except construction to check the user rights before adding the number component. Maybe you can do something similar before adding the select component. TLDR, move this check to the setup_entry, add only if the right charger features are present. (maybe combine this with the user rights check, becasue a limited Wallbox portal user will not be able to do this)
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.
Like this? 42b68ae
@property | ||
def current_option(self) -> str | None: | ||
"""Return the current selected option.""" | ||
return str(self.coordinator.data[CHARGER_SOLAR_CHARGING_MODE]) |
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.
Let's implement a lambda in the entity description to handle this. Right now, the entity description dict is created to be extendable, but this method isn't (it is hardcoded to a single feature).
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.
Fixed: jorisdrenth@7a025df
await hass.services.async_call( | ||
SELECT_DOMAIN, | ||
SERVICE_SELECT_OPTION, | ||
{ | ||
ATTR_ENTITY_ID: MOCK_SELECT_ENTITY_ID, | ||
ATTR_OPTION: EcoSmartMode.FULL_SOLAR, | ||
}, | ||
blocking=True, | ||
) | ||
|
||
await hass.services.async_call( | ||
SELECT_DOMAIN, | ||
SERVICE_SELECT_OPTION, | ||
{ | ||
ATTR_ENTITY_ID: MOCK_SELECT_ENTITY_ID, | ||
ATTR_OPTION: EcoSmartMode.OFF, | ||
}, | ||
blocking=True, | ||
) |
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.
Same as above.
await hass.services.async_call( | ||
SELECT_DOMAIN, | ||
SERVICE_SELECT_OPTION, | ||
{ | ||
ATTR_ENTITY_ID: MOCK_SELECT_ENTITY_ID, | ||
ATTR_OPTION: EcoSmartMode.ECO_MODE, | ||
}, | ||
blocking=True, | ||
) | ||
|
||
await hass.services.async_call( | ||
SELECT_DOMAIN, | ||
SERVICE_SELECT_OPTION, | ||
{ | ||
ATTR_ENTITY_ID: MOCK_SELECT_ENTITY_ID, | ||
ATTR_OPTION: EcoSmartMode.OFF, | ||
}, | ||
blocking=True, | ||
) |
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.
Samea as above
status_code=404, | ||
) | ||
|
||
with pytest.raises(ConnectionError): |
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.
Errors with services should raise a ConnectionError. They should have been caught and raise an translated HomeAssistantError
instead.
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.
status_code=403, | ||
) | ||
|
||
with pytest.raises(ConfigEntryAuthFailed): |
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.
Same as above
with pytest.raises(ConnectionError): | ||
await hass.services.async_call( | ||
SELECT_DOMAIN, | ||
SERVICE_SELECT_OPTION, | ||
{ | ||
ATTR_ENTITY_ID: MOCK_SELECT_ENTITY_ID, | ||
ATTR_OPTION: EcoSmartMode.FULL_SOLAR, | ||
}, | ||
blocking=True, | ||
) |
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 is really the same thing as above, as well as the one below. This means this test can be parameterized to reduce the repetitive code.
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.
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.
please move the checking for the right requirements from the 'available' property to the setup entry function.
in self.coordinator.data[CHARGER_DATA_KEY][CHARGER_PLAN_KEY][ | ||
CHARGER_FEATURES_KEY | ||
] |
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.
@jorisdrenth in the number component we do a try...except construction to check the user rights before adding the number component. Maybe you can do something similar before adding the select component. TLDR, move this check to the setup_entry, add only if the right charger features are present. (maybe combine this with the user rights check, becasue a limited Wallbox portal user will not be able to do this)
@frenck @hesselonline @abmantis Can you re-review this PR? I addressed all points of feedback. |
Proposed change
Wallbox has an option in their app to charge a car using energy from a users solar panels. In the Wallbox app this can be turned off, or either one of two modes can be selected: eco mode, which starts charging at 2A and fills with energy from the net till 6A, or full solar, which starts charging at 6A energy from solar panels. A Wallbox Load balancer is needed for this functionality.
This PR adds an "Solar charging" entity to the Wallbox integration. It features a select with three options: "Off", "Eco mode" or "Full solar". This way the solar charging mode can be controlled via Home Assistant. Entity availability is checked with the Power Boost availability (Wallbox' confusing name for the load balancer).
For the API communication I also opened a PR to the Wallbox python project: cliviu74/wallbox#63. The merging of that PR is blocking for this PR.
Disclaimer: it's been 5 year since I worked with Python and this is my first PR for Home Assistant. Looking forward to receive feedback in a constructive way.
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: