rct: support second battery tower #25162
Conversation
|
Wow 🤩. Wenn das im nightly drin ist, teste ich das |
|
Üblicherweise adressieren und Instanzieren wir die Batterien einzeln über eine Art Index oder Umschalter, da sie ggf. unterschiedliche Eigenschaften wie z.B. Der Gesamt-SoC wird automatisch über die Wäre das hier im Sinne der Einheitlichkeit auch möglich? |
|
Dazu müsste die zweite Software Instanz Batterie wissen, dass sie immer die Nr. 2 auf dieser Adresse ist. Nur zu wissen, dass es zwei gibt, reicht nicht. |
|
Ich habe jetzt gerade eine zweite Batterie angelegt. This is the total power of batteries as in https://rctclient.readthedocs.io/en/latest/inverter_registry.html#g-sync siehe auch weltenwort/home-assistant-rct-power-integration#396 (comment) Hier nochmal die bekannten Werte für die zweite Batterie: |
|
Das ist der aktuelle Stand. Diese Register werden derzeit von evcc verwendet. @zeitisen Kannst du diese Tabelle vervollständigen? Ich hoffe, dass hierfür die RCT Power App mit der Darstellung der Batterie-Werte helfen kann.
|
|
Entweder: wir lesen alle Batterien immer aus. Oder: wir machen die Batterie konfigurierbar. Bei WW-Speichern funktioniert das über |
|
Gefunden: - name: battery
default: 1
type: choice
description:
en: Battery number
de: Batteriespeichernummer
choice: [1, 2]
usages: ["battery"] |
|
Ich würde es gerne konfigurierbar machen. Auch, damit nicht unnötige Requests gestellt werden, wenn man nur einen Batterieturm hat. |
(1) There is a better status, which is available for both batteries (see weltenwort/home-assistant-rct-power-integration#396 (comment)) and here svalouch/python-rctclient#39 (2) Battery power has to be calculated from current * voltage (3) You can get the number of batteries: power_mng.n_batteries 0x663F1452 |
|
@zeitisen Danke für die hilfreichen Informationen. |
|
PR ist PreRelease? Ich bin mit der Abkürzung nicht vertraut. Wenn es im nightly build drin ist, probiere ich es morgen früh aus. Sonst brauche ich eine Hilfe, wie ich das testen kann. Bei mir läuft evcc als Debianpaket auf dem Pi. |
|
PR steht für Pull Request. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- After querying numberOfBatteries, validate that the configured battery index falls within [1, numberOfBatteries] and return an error if it doesn’t to avoid silent misconfiguration.
- The parameter and struct field named "battery" is ambiguous—consider renaming it to "batteryNumber" or "batteryIndex" now for improved clarity.
- When using errgroup for concurrent writes/reads, consider switching to errgroup.WithContext so that a failing operation can cancel the others immediately.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- After querying numberOfBatteries, validate that the configured battery index falls within [1, numberOfBatteries] and return an error if it doesn’t to avoid silent misconfiguration.
- The parameter and struct field named "battery" is ambiguous—consider renaming it to "batteryNumber" or "batteryIndex" now for improved clarity.
- When using errgroup for concurrent writes/reads, consider switching to errgroup.WithContext so that a failing operation can cancel the others immediately.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Ich habe gerade einen update gemacht. Ist nicht drin im nightly. Selber kompilieren wollte ich eigentlich vermeiden. Ich bin mit den Werkzeugen und dem Workflow auf Github nicht wirklich vertraut. Angefangen habe ich mal vor drei Jahren, aber nie komplett durchgezogen. Und jetzt mit einer neuen Programmiersprache zu starten, die ich vor sechs Wochen das erste Mal gesehen habe, ist mir im Moment zu viel Aufwand. Vielleicht in den nächsten Wochen mal. |
|
Kein Problem. |
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The concurrent use of m.conn.Write via errgroup in batteryMode assumes rct.Connection is safe for parallel writes; if that’s not guaranteed, consider serializing these calls or adding synchronization to avoid subtle connection issues.
- When usage == "battery", you already query PowerMngNBatteries; it would be helpful to validate the configured battery index (1/2) against this value and fail fast or clamp if the requested battery is not actually available.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The concurrent use of m.conn.Write via errgroup in batteryMode assumes rct.Connection is safe for parallel writes; if that’s not guaranteed, consider serializing these calls or adding synchronization to avoid subtle connection issues.
- When usage == "battery", you already query PowerMngNBatteries; it would be helpful to validate the configured battery index (1/2) against this value and fail fast or clamp if the requested battery is not actually available.
## Individual Comments
### Comment 1
<location> `meter/rct.go:133` </location>
<code_context>
}
}
+ var eg errgroup.Group
+
switch mode {
</code_context>
<issue_to_address>
**issue (complexity):** Consider reverting the new concurrency and generic helper changes in favor of simpler sequential code and explicit typed helpers while keeping the new battery features intact.
The added concurrency and generic helper do increase complexity without clear benefit. You can keep all new battery-related behavior while simplifying the implementation.
1) Simplify batteryMode: remove errgroup and use straight-line writes
The `batteryMode` closure is still fundamentally sequential configuration over a single connection. The `errgroup` adds concurrency and error aggregation overhead for no real gain.
You can keep the new `BatteryStatus2` check and all the register writes, but revert to simple sequential calls with early returns:
```go
batteryMode = func(mode api.BatteryMode) error {
if mode != api.BatteryNormal {
batStatus, err := m.queryInt32(rct.BatteryStatus2)
if err != nil {
return err
}
// see https://github.com/weltenwort/home-assistant-rct-power-integration/issues/264#issuecomment-2124811644
if batStatus != 0 {
return errors.New("invalid battery operating mode")
}
}
switch mode {
case api.BatteryNormal:
if err := m.conn.Write(rct.PowerMngSocStrategy, []byte{rct.SOCTargetInternal}); err != nil {
return err
}
if err := m.conn.Write(rct.BatterySoCTargetMin, m.floatVal(float32(batterySocLimits.MinSoc)/100)); err != nil {
return err
}
return m.conn.Write(rct.PowerMngBatteryPowerExternW, m.floatVal(0))
case api.BatteryHold:
if err := m.conn.Write(rct.PowerMngSocStrategy, []byte{rct.SOCTargetInternal}); err != nil {
return err
}
return m.conn.Write(rct.BatterySoCTargetMin, m.floatVal(float32(batterySocLimits.MaxSoc)/100))
case api.BatteryCharge:
if err := m.conn.Write(rct.PowerMngUseGridPowerEnable, []byte{1}); err != nil {
return err
}
if err := m.conn.Write(rct.PowerMngBatteryPowerExternW, m.floatVal(float32(-maxchargepower))); err != nil {
return err
}
return m.conn.Write(rct.PowerMngSocStrategy, []byte{rct.SOCTargetExternal})
default:
return api.ErrNotAvailable
}
}
```
This keeps all battery modes and limits intact while avoiding goroutines and `errgroup` for a single connection.
2) Simplify `CurrentPower` battery branch: sequential queries with a small helper
The battery branch currently has two goroutines, plus duplicated logic for selecting identifiers for battery 1 vs 2. You can:
- Pull the identifier selection into a small helper
- Do the two queries sequentially (performance impact is negligible here)
For example:
```go
func (m *RCT) batteryIVIDs() (currentID, voltageID rct.Identifier) {
if m.battery == 2 {
return rct.BatteryPlaceholder0Current, rct.BatteryPlaceholder0Voltage
}
return rct.BatteryCurrent, rct.BatteryVoltage
}
func (m *RCT) CurrentPower() (float64, error) {
switch m.usage {
case "grid":
// unchanged...
case "battery":
currentID, voltageID := m.batteryIVIDs()
current, err := m.queryFloat(currentID)
if err != nil {
return 0, err
}
voltage, err := m.queryFloat(voltageID)
if err != nil {
return 0, err
}
return current * voltage, nil
default:
return 0, fmt.Errorf("invalid usage: %s", m.usage)
}
}
```
This preserves:
- Support for `Battery` and `BatteryPlaceholder0` IDs
- Existing behavior of computing power as `current * voltage`
without introducing goroutines or shared mutable variables.
3) Simplify retry helpers: drop generics and keep explicit typed helpers
The new `queryRCT[T any]` generic adds an abstraction layer for just three call sites. The previous pattern with a small `bo()` helper and per-type wrappers was simpler to follow.
You can keep the new `queryUint8` functionality and the `PowerMngNBatteries` query, but reintroduce the explicit helpers:
```go
func (m *RCT) bo() *backoff.ExponentialBackOff {
return backoff.NewExponentialBackOff(
backoff.WithInitialInterval(500*time.Millisecond),
backoff.WithMaxInterval(2*time.Second),
backoff.WithMaxElapsedTime(10*time.Second),
)
}
func (m *RCT) queryFloat(id rct.Identifier) (float64, error) {
res, err := backoff.RetryWithData(func() (float32, error) {
return m.conn.QueryFloat32(id)
}, m.bo())
return float64(res), err
}
func (m *RCT) queryInt32(id rct.Identifier) (int32, error) {
return backoff.RetryWithData(func() (int32, error) {
return m.conn.QueryInt32(id)
}, m.bo())
}
func (m *RCT) queryUint8(id rct.Identifier) (uint8, error) {
return backoff.RetryWithData(func() (uint8, error) {
return m.conn.QueryUint8(id)
}, m.bo())
}
```
This:
- Keeps the same retry behavior and parameters
- Preserves the new `queryUint8` functionality for `PowerMngNBatteries`
- Avoids generics while keeping the code very explicit and easy to trace
All of the above changes retain your new features (battery selection, `numberOfBatteries` division, `BatteryStatus2`, `BatteryPlaceholder0*` IDs, `queryUint8`) but remove concurrency and generic abstraction where they don’t add much value.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Tbo I'm not happy with this PR. Replacing power with current x voltage and the odd energy consumption do not feel right. |
|
Since we always know how many batteries are connected to an inverter, we could use the old approach when there is only one battery and the "odd" approach when two batteries are connected. |
|
If that gives cleaner implementation, I'd prefer that |
|
I'll try it out. |
|
Please have a look at the latest changes. I have tried to make the code as simple and readable as possible. |
| uri: {{ .host }} | ||
| usage: {{ .usage }} | ||
| cache: {{ .cache }} | ||
| battery: {{ .battery }} |
There was a problem hiding this comment.
Needed for batterySoc and currentPower.
Or am I missing something?
| batteryMode = func(mode api.BatteryMode) error { | ||
| if mode != api.BatteryNormal { | ||
| batStatus, err := m.queryInt32(rct.BatteryBatStatus) | ||
| batStatus, err := m.queryInt32(rct.BatteryStatus2) |
There was a problem hiding this comment.
(1) There is a better status, which is available for both batteries
See #25162 (comment)
I observed that the other one battery status (OID = 0x70A2AF4F, alias "battery.bat_status") does not show all status anymore
meter/rct.go
Outdated
| if m.numberOfBatteries == 1 { | ||
| return m.queryFloat(rct.BatteryPowerW) | ||
| } else { | ||
| idCurrent, idVoltage := rct.BatteryCurrent, rct.BatteryVoltage |
There was a problem hiding this comment.
why don't we just use the power for the first battery???
There was a problem hiding this comment.
If only one battery is connected, we use the power.
If two batteries are connected we have to calculate the power from current and voltage because in this case BatteryPowerW is the sum of both.
See #25162 (comment)
There was a problem hiding this comment.
because in this case BatteryPowerW is the sum of both.
Why not simply remove the battery config and always read all batteries? Removes this issue and the weird energy thing!
There was a problem hiding this comment.
I understood #25162 (comment) as a guideline to read each battery separately.
There was a problem hiding this comment.
Yes. But it doesn't properly work here...
There was a problem hiding this comment.
Oh, I see. RCT is such a pain... Can you read the capacities? Then you'd have a virtual soc. Any solution remain have-baked unfortunately.
There was a problem hiding this comment.
Having searched through this pdf.
There is
- 0xB57B59BD battery.ah_capacity Battery capacity [Ah]
- 0xBD95C46C battery_placeholder[0].ah_capacity Battery capacity [Ah]
but no registry for both batteries.
There was a problem hiding this comment.
Seems like they've intentionally made it impossible my omitting parameters ;)
There was a problem hiding this comment.
That's quite possible. :-/
There was a problem hiding this comment.
How can I proceed from here?
I would very much like to support the second battery.
Even if the solution in this PR is not the one we were looking for, it should still work and be optimized where possible.
| return (in - out) / 1000, err | ||
| totalWh := (in - out) / 1000 | ||
|
|
||
| if m.numberOfBatteries == 2 { |
There was a problem hiding this comment.
again, this is still odd. why don't we just read all connected batteries?
There was a problem hiding this comment.
If only one battery is connected, TotalEnergyBattInWh and TotalEnergyBattOutWh are perfectly matched.
If two batteries are connected, I see no other option than to read TotalEnergyBattInWh and TotalEnergyBattOutWh and then divide by two, as it appears that both registers represent the total energy of both batteries.
In other words, I have not found any separate registers that would allow me to query the energy for each individual battery.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
4 participants
Depends on evcc-io/rct#5
Ref #23450 (comment)
MUST BE TESTED IN NIGHTLY
2️⃣ support second battery power
🏃➡️ use concurrent calls whenever possible
🖱 add
batteryparameterTODO:
batterytobatterynumber(?)\cc @andig