fix(solis): derive nominal_voltage from live battery data instead of hard-coded 48V - #4502
Conversation
…hard-coded 48V (#4493) SolisAPI.nominal_voltage was hard-coded to 48.0 and never updated from the actual battery, making every derived power and capacity value wrong by roughly the ratio of the real pack voltage to 48V (~11x on a 533V HV battery). Reported and confirmed live under load by @jibbej, who also found the capacity path needs a *different* voltage (nominal pack, not live measured) plus parallel_battery_count, which the max current calculation already applies but capacity didn't. - get_nominal_voltage(): power/current conversions (max charge/discharge power, amp<->watt round trips) now use the live measured batteryVoltage, retaining the last known-good reading if it becomes unavailable rather than reverting to 48V. - get_capacity_voltage() + new solis_nominal_voltage apps.yaml option: the capacity calculation uses the nominal pack voltage (a fixed physical property) when configured. Without it, capacity still falls back to the live voltage - existing installs already had this sensor published and dropping it outright had no migration path - but is flagged unreliable (reliable/voltage_source attributes, a one-time warning) since that value drifts with charge state. - battery_capacity now applies parallel_battery_count, matching the existing max current calculation. - soc_max auto-binding to battery_capacity stays deliberately disabled: without solis_nominal_voltage configured the sensor can still wobble with charge state, which soc_max never did before.
|
I have read through the diff. Splitting live measured voltage (for amp/watt Applying I can confirm One observation that may be useful for the fallback path you added: on my Incidentally, that same page confirms the ordering constraint behind the other I am currently running with |
|
Thanks for confirming with real numbers - 50Ah × 2 parallel × 512V / 1000 = 51.2kWh matching your actual battery size is exactly the validation this needed. Good to know CID 172 comes from the BMS with no local override available on your Battery1 Setting page - that confirms Look forward to the post-merge report once you've dropped the manual patch and set |
|
@jibbej I'm not sure we captured the implications of that - could you check it is covered in it's own issue if there is outstanding work please. |
Checked - it is not covered anywhere, and my cross-reference was wrong. Apologies: I It is also not specific to my system. #4184 already contains an independent report of
Same symptom, same manual workaround, different limit. Theirs rejects below 10. Mine I should correct something in my earlier note, though. I wrote that the Battery1 Setting So the general statement is not "recovery must exceed over-discharge". It is that CID The path, on v8.47.7:
So the default path lands exactly on the boundary value the inverter refuses, and there I checked whether the API could supply the range itself, since So the limits are enforced in inverter firmware and only visible in the local UI. That
Separately, while reading that area I think value = await self.read_cid(device_sn, SOLIS_CID_BATTERY_OVER_DISCHARGE_SOC)
try:
value = float(value)
except (ValueError, TypeError):
value = 0
if value > 20:
Worth deciding what it should do before fixing it, rather than just correcting the Happy to open a single issue covering the range/clamp problem with both reports, and a |
There was a problem hiding this comment.
Pull request overview
This PR fixes Solis Cloud battery power/capacity calculations by removing the hard-coded 48V assumption and deriving conversion voltages from live inverter data, while introducing an optional configured nominal pack voltage for stable capacity reporting.
Changes:
- Add
get_nominal_voltage()(live measured, with last-known fallback) for all amp↔watt conversions and number write-backs. - Add
solis_nominal_voltage(nominal pack voltage) for capacity calculation; applyparallel_battery_countto capacity. - Update docs/templates and extend Solis tests to cover voltage source selection, reliability flags, and one-time warning behaviour.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| templates/solis_cloud.yaml | Documents the new solis_nominal_voltage option and clarifies soc_max guidance. |
| docs/components.md | Adds the Solis nominal_voltage option documentation (mapped to solis_nominal_voltage). |
| docs/apps-yaml.md | Documents solis_nominal_voltage behaviour and the unreliable fallback path. |
| apps/predbat/tests/test_solis.py | Adds new tests for voltage selection/memoization and capacity reliability/warn-once; updates existing assertions for live-voltage conversions. |
| apps/predbat/solis.py | Implements live-voltage conversions, capacity-voltage selection, reliability metadata, and applies parallel_battery_count to capacity. |
| apps/predbat/config.py | Adds solis_nominal_voltage to config schema. |
| apps/predbat/components.py | Wires solis_nominal_voltage into Solis component initialization. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Summary
Fixes #4493 -
SolisAPI.nominal_voltagewas hard-coded to 48.0 and never updated from the actual battery, so every derived power and capacity value was wrong by roughly the ratio of the real pack voltage to 48V (~11x on the reporter's 533V HV battery: 960W computed vs ~10.7kW actual). Reported and confirmed live under load by @jibbej, who also identified a second defect while testing: the capacity calculation needs a genuinely different voltage (nominal pack, not live measured) plusparallel_battery_count, which the max-current calculation already applies but capacity didn't.get_nominal_voltage(inverter_sn): power/current conversions (max charge/discharge power sensors, amp↔watt round trips for writing to the inverter) now use the live measuredbatteryVoltage, retaining the last known-good reading if it becomes momentarily unavailable rather than reverting to 48V.get_capacity_voltage(inverter_sn)+ newsolis_nominal_voltageapps.yaml option: the capacity calculation prefers the nominal pack voltage (a fixed physical property, e.g. cell count × nominal cell voltage) when configured.battery_capacitynow appliesparallel_battery_count, matching the existing max-current calculation (_calculate_max_currents()) which already did.soc_maxauto-binding tobattery_capacitystays deliberately disabled - withoutsolis_nominal_voltageconfigured the sensor can still wobble with charge state, whichsoc_maxnever did before, even with the old wrong constant.A decision worth reviewer pushback
DEYE already solved the exact same "nominal vs measured voltage" problem for its own capacity calculation (
deye.pynominal_pack_voltage()/derive_battery_capacity()) and its answer is to refuse to publish a capacity at all rather than guess, on the basis that a wrongsoc_maxsource is worse than none.I initially matched that for Solis, but changed it after discussion: without
solis_nominal_voltageconfigured,battery_capacityis still published, falling back to the live measured voltage, flagged unreliable viareliable/voltage_sourceattributes and a one-time warning log (not repeated every cycle - it runs inpublish_entities(), roughly once a minute). The reasoning: every existing Solis Cloud install already has this sensor published today (using the old, worse, hard-coded 48V), and nobody hassolis_nominal_voltageset yet since it's brand new - dropping the sensor outright the moment this ships would regress 100% of existing installs with no migration path, for a sensor that isn't wired intosoc_maxinternally anyway (that binding is, and stays, commented out). Cloud integration users skew less technical, which weighed into being cautious here rather than optimising for architectural purity.This is a genuine tradeoff, not an obviously-correct call - happy to switch to DEYE's stricter behaviour (or something in between) if reviewers feel differently. See
get_capacity_voltage()'s docstring insolis.pyfor the same note in-code.Test plan
test_solis.py:test_get_nominal_voltage_and_capacity_voltage(live voltage used and remembered; falls back to last known-good, then 48V),test_publish_entities_capacity_voltage_reliability(capacity always published, correctly flagged reliable/unreliable, warning logs exactly once not every cycle,parallel_battery_countapplied)test_publish_entitiespower/capacity assertions updated to reflect live-voltage-based calculations./run_all --quickpasses./run_pre_commitpasses🤖 Generated with Claude Code