goodwe: add battery charge/discharge current limit number entities#174363
goodwe: add battery charge/discharge current limit number entities#174363matifali wants to merge 1 commit into
Conversation
![home-assistant[bot]](https://avatars.githubusercontent.com/in/97978?s=60&v=4){kind=small}
|
Hey there @mletenay, @starkillerOG, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
bb9c7a1 to
113c4c8
Compare
battery_charge_current (reg 45353) and battery_discharge_current (reg 45355) are writable settings already exposed by the goodwe library but were never wired up as HA number entities. This adds both as EntityCategory.CONFIG controls with NumberDeviceClass.CURRENT and 0.1 A step. The slider ceiling is set dynamically from the BMS-reported battery_charge_limit / battery_discharge_limit sensors (registers 37004/37005) so it reflects what the hardware actually supports (e.g. 105 A), with a 100 A fallback when the coordinator has no data yet.
113c4c8 to
d983215
Compare
mletenay
left a comment
mletenay
left a comment
There was a problem hiding this comment.
I do not agree with this change.
There are many sensors like this which are not exposed intentionally.
The charging current is parameter which is not available to ordinary PV system user, it is installation/configuration parameter meant for installers.
And average user can easily break (burn) things if (s)he does not know what is he doing.
IMHO, this feature does not belong to standard HA.
It is one of the reasons why there is a HACS version of Goodwe integration, meant for advanced users which may contain these kinds of dangerous things.
|
@mletenay Thanks. I kept this PR in draft to get initial feedback. I will close this one for now. I am adding to my list for the HACS integration instead and would see if it's suitable upstream there. Thanks again. |
Proposed change
The GoodWe inverter integration is missing two writable number entities for battery current limits. The underlying goodwe library has exposed
battery_charge_current(register 45353) andbattery_discharge_current(register 45355) as writable settings since ET inverter support was added, but they were never wired up in the HA integration. Users have no way to see or adjust these limits from Home Assistant.This PR adds both as
EntityCategory.CONFIGnumber entities withNumberDeviceClass.CURRENTand step 0.1 A.Dynamic max value from BMS: Rather than hardcoding an upper bound, the entities read
battery_charge_limitandbattery_discharge_limitfrom the runtime coordinator data (registers 37004/37005). These are reported directly by the battery BMS and reflect what the hardware actually supports — e.g. 105 A on a standard LV battery. The description'snative_max_value(100 A) is used as a fallback when the coordinator has no data yet or no battery is connected.A
filterlambda guards each entity so it is only registered when the inverter firmware actually supports the setting (the library silently drops unsupported registers duringread_device_info).Tested on: GoodWe GW8000M-ES-C10 (ESC platform, single-phase, 8.8 kW). Live readback confirmed 40.0 A charge limit and 60.0 A discharge limit; BMS reports 105 A for both, which becomes the slider ceiling.
Type of change
Additional information
Checklist
python -m pytest tests/components/goodwe/ -v(no regressions)python -m ruff format homeassistant tests)