Add individual battery banks as devices #108339
Conversation
home-assistant
bot
added
cla-signed
dependency
integration: powerwall
new-feature
labels
|
Hey there @bdraco, @jrester, @daniel-simpson, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
bdraco
left a comment
bdraco
left a comment
There was a problem hiding this comment.
Please see comments above.
Thanks!
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
bubonicbob
force-pushed
the
powerwall_batterydevice
branch
from
f3cdd18 to
8fb7d46
Compare
|
I need some help testing this out. I am only able to run unit tests. |
bubonicbob
force-pushed
the
powerwall_batterydevice
branch
from
f94154d to
989593b
Compare
I did end up testing this by creating a simulated powerwall server using cached results from a multi-powerwall installation. It shows each battery bank as a separate device with individual sensor entities for each as expected. |
Oh I'll change that. I was waiting for the other PRs to merge, but now they're done. |
Whenever you feel comfortable. Doesn't have to be. |
|
I'll try to load this on my production before my flight takes off |
bdraco
added
smash
|
One small review comment, and should be good to merge once a docs PR is opened and linked since we do document which sensors are expected https://www.home-assistant.io/integrations/powerwall/ |
No problem. I am a little confused by the terminology on the docs page though. When somebody says "Powerwall" in the context of the integration, they are actually referring to a gateway whereas the "powerwall" I would think of is the actual battery bank + inverter. If I look at the Tesla marketing page for purchasing a Powerwall, they list specifically a single battery bank and inverter spec and they say you can "stack up to 10 Powerwalls" in a single installation. I presume that all those Powerwalls would be accessed via a single gateway. |
Normally that's the most common to use a single gateway. Apparently you can have multiple gateways though I've never seen that configuration in person. It would be fine to adjust the docs to make it more clear that we are connecting to a gateway |
Add documentation for new sensors added in home-assistant/core#108339. Also clarifies the different sensors per Backup Gateway and per Powerwall
Add documentation for new sensors added in home-assistant/core#108339. Also clarifies the different sensors per Backup Gateway and per Powerwall
Ok, I should have made a docs PR targeting the "next" branch. home-assistant/home-assistant.io#31158 Hope that's the right thing to do. |
| _attr_has_entity_name = True | ||
|
|
||
| def __init__( | ||
| self, powerwall_data: PowerwallRuntimeData, battery: BatteryResponse |
There was a problem hiding this comment.
It would be so much better if PowerwallRuntimeData was a dataclass as we wouldn't have to use as many type hints. This integration predates us being able to use dataclasses so it's never been converted.
|
Thanks @bubonicbob |
|
|
||
| def _get_battery_charge(battery_data: BatteryResponse) -> float: | ||
| """Get the current value in %.""" | ||
| ratio = float(battery_data.energy_remaining) / float(battery_data.capacity) |
There was a problem hiding this comment.
We don't allow calculating state of entities in integrations that integrate devices or services. The API data should be shown as is, while following our entity design guidelines. Only exception is around time state. Please remove this sensor.
There was a problem hiding this comment.
For sure! What is the alternative to provide the same information to the user?
There was a problem hiding this comment.
State of charge for batteries is naturally a computation based on models for the battery chemistry, so wouldn't it make sense that it is a computed value? Either the computation is done explicitly here or buried lower in the stack in support libraries or in the firmware of the powerwall that this is representing.
I'm happy to update this to meet any guidelines, but please go easy on me since these are my first contributions and I'd like to get to learn HA. :)
There was a problem hiding this comment.
The other option is to have the underlying library return the value so we don't do the calculation in HA
3 participants
Proposed change
Add batteries as devices with their own individual sensors. Making this PR to get early feedback on the direction of adding the devices and basic structure for adding a sensor.
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..coveragerc.To help with the load of incoming pull requests: