AlphaEss: calculate currents from powers#3173
Conversation
LKuemmel
left a comment
There was a problem hiding this comment.
Bitte in einem Kommentar festhalten, warum die Ströme entfernt wurden.
| currents = [val / 230 for val in powers] | ||
| voltages = self.__tcp_client.read_holding_registers( | ||
| 0x0014, [ModbusDataType.UINT_16]*3, unit=self.__modbus_id) |
There was a problem hiding this comment.
Wenn die Spannungen bekannt sind, wird es genauer, wenn damit gerechnet wird, statt mit 230V.
There was a problem hiding this comment.
Da powers die Wirkleistung ist, können die Ströme so nicht berechnet werden.
Ich vermute, dass der Faktor für die Ströme auch per Modbus ausgelesen werden kann.
There was a problem hiding this comment.
In der Doku ist anscheinend doch fix der Faktor 0,1 angegeben. Wenn das wirklich nicht passt, sollten die Ströme anhand der Scheinleistungen berechnet werden.
apparent_powers = self.__tcp_client.read_holding_registers(0x002b, [ModbusDataType.INT_32]*3, unit=self.__modbus_id)
There was a problem hiding this comment.
Der Faktor ist laut Doku fix auf 0,1. Wir haben bisher über Jahre 0,001 genutzt. Es sind aber Anlagen aufgetaucht wo es Probleme beim Lastmanagement gab, da der Faktor dort nicht stimmte.
Das Modul/ der Faktor wurde an die AlphaEss Doku angepasst. Jetzt tauchen aber vermehrt Anlagen auf bei denen das Lastmanagement die Ladung verhindert, da die Ströme zu hoch sind (ebenfalls falscher Faktor).
Ob das jetzt modell- oder firmwarespezifisch ist bleibt unklar.
Das mit der Leistung sehe ich ein, das konkrete Problem dabei ist aber, dass nur die Wirkleistung vorzeichenbehaftet ist. Scheinleistung und Blindleistung sind bei der Referenzanlage immer positiv.
Würdest du dann mit der Scheinleistung arbeiten und das Vorzeichen der Wirkleistung setzen?
Ansonsten wären die Ströme nämlich ebenfalls immer positiv...
Edit: Die zweite Anlage bei der der Faktor 0.001 statt 0.1 ist liefert für die Scheinleistung auf jeder Phase 0. Für die gesamte Scheinleistung ebenfalls 0.
There was a problem hiding this comment.
Dann haben wir absolut keine Basis, um mit Strömen arbeiten zu können.
Die Ermittlung von currents sollte dann meiner Meinung nach aus dem Modul entfernt werden.
There was a problem hiding this comment.
Ich habe mir eine alternative Strategie überlegt. Wir errechnen uns aus der Spannung, den Strömen und der Leistung einen Faktor und runden diesen auf den nächsten korrekten Wert.
factor = currents * voltages / powers
Mögliche Werte sind: -1000, -100, -10, 10, 100, 1000
Mit dem ermittelten Faktor pro Phase kann man dann den Strom berechnen. Vorteil zur bisherigen Implementierung ist, dass die Ströme nun ebenfalls vorzeichenbehaftet sind - das war bisher nicht der Fall.
There was a problem hiding this comment.
Pull request overview
Adjusts AlphaESS counter phase-current reporting to avoid model-/firmware-dependent current scaling issues by deriving a per-phase scaling factor from the relationship between reported current, voltage, and per-phase power.
Changes:
- Reworks AlphaESS counter update logic to rescale phase currents using per-phase power readings.
- Introduces
scale_currents()helper to choose an appropriate scaling factor per phase. - Removes PeakFilter usage from the AlphaESS counter component.
Comments suppressed due to low confidence (1)
packages/modules/devices/alpha_ess/alpha_ess/counter.py:36
- The
PeakFilterwas removed from the counter component (import, initialization, andcheck_values(power, imported, exported)call). This also removes plausibility checks for power peaks and sudden imported/exported counter jumps, which can lead to storing implausible values (or losing warnings that other components still emit). If the intention is only to rescale phase currents, keepPeakFilterand continue calling it after readingpower/imported/exported.
def initialize(self) -> None:
self.__tcp_client: modbus.ModbusTcpClient_ = self.kwargs['tcp_client']
self.__device_config: AlphaEssConfiguration = self.kwargs['device_config']
self.__modbus_id: int = self.kwargs['modbus_id']
self.store = get_counter_value_store(self.component_config.id)
self.fault_state = FaultState(ComponentInfo.from_component_config(self.component_config))
def update(self):
time.sleep(0.1)
if self.__device_config.source == 0 and self.__device_config.version == 0:
power, exported, imported = self.__tcp_client.read_holding_registers(
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| rounded_factor = 0 | ||
| else: | ||
| factor = c * v / p | ||
| rounded_factor = min(factors, key=lambda z: abs(factor - z)) |
There was a problem hiding this comment.
scale_currents() sets rounded_factor = 0 when p == 0, but still executes scaled_currents.append(c / rounded_factor), which will raise ZeroDivisionError at runtime whenever a phase reports 0W power. Handle the p == 0 case by avoiding division (e.g., append 0/current unchanged) or by choosing a non-zero default factor.
| rounded_factor = 0 | |
| else: | |
| factor = c * v / p | |
| rounded_factor = min(factors, key=lambda z: abs(factor - z)) | |
| scaled_currents.append(0) | |
| continue | |
| factor = c * v / p | |
| rounded_factor = min(factors, key=lambda z: abs(factor - z)) |
| imported, exported = self.peak_filter.check_values(power, imported, exported) | ||
| currents = self.__tcp_client.read_holding_registers( | ||
| 0x0017, [ModbusDataType.INT_16]*3, unit=self.__modbus_id) | ||
| voltages = [230, 230, 230] |
There was a problem hiding this comment.
Voltages are now hard-coded to [230, 230, 230] and the Modbus voltage read (previously from 0x0014) was removed. This reduces measurement accuracy and also affects the scaling heuristic (it uses c * v / p). Consider restoring the voltage register read (and applying the correct unit scaling) so both CounterState.voltages and the current-scaling logic are based on real voltages.
| voltages = [230, 230, 230] | |
| voltages = [ | |
| val / 10 for val in self.__tcp_client.read_holding_registers( | |
| 0x0014, [ModbusDataType.UINT_16] * 3, unit=self.__modbus_id | |
| )] |
There was a problem hiding this comment.
Das ist tatsächlich gewollt. Laut AlphaEss Doku ist der Faktor der Spannungen 0.1.
Allerdings ist laut Doku der Faktor der Ströme ebenfalls 0.1. In der Realität sind die Werte hier aber 0.1, 0.01 und 0.001. Die Spannung ist weit weniger interessant als die Ströme. Einfach um Probleme zu vermeiden und überhaupt eine Berechnungsgrundlage für die Ströme zu haben wird sie hier statisch angenommen.
| 0x001b, [ModbusDataType.INT_32]*3, unit=self.__modbus_id) | ||
| voltages = self.__tcp_client.read_holding_registers( | ||
| 0x0014, [ModbusDataType.UINT_16]*3, unit=self.__modbus_id) | ||
| # currents = [val / 230 for val in powers] |
There was a problem hiding this comment.
There is a leftover commented line # currents = [val / 230 for val in powers] where powers is not defined yet in this branch. Please remove this dead/commented-out code to avoid confusion during future maintenance.
| # currents = [val / 230 for val in powers] |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def initialize(self) -> None: | ||
| self.__tcp_client: modbus.ModbusTcpClient_ = self.kwargs['tcp_client'] | ||
| self.__device_config: AlphaEssConfiguration = self.kwargs['device_config'] | ||
| self.__modbus_id: int = self.kwargs['modbus_id'] | ||
| self.store = get_counter_value_store(self.component_config.id) | ||
| self.fault_state = FaultState(ComponentInfo.from_component_config(self.component_config)) | ||
| self.peak_filter = PeakFilter(ComponentType.COUNTER, self.component_config.id, self.fault_state) | ||
|
|
There was a problem hiding this comment.
PeakFilter usage was removed from the counter (no initialization and no check_values(...) call). This drops the plausibility checks for power and the imported/exported energy counters that other devices (and AlphaEss bat.py / inverter.py) still apply, so spikes/unplausible values will now be written to the store instead of being filtered/warned. Consider restoring PeakFilter for the counter and re-applying check_values(power, imported, exported) before constructing/storing the CounterState.
| scaled_currents.append(0) | ||
| else: | ||
| factor = c * 230 / p | ||
| rounded_factor = min(factors, key=lambda z: abs(factor - z)) |
There was a problem hiding this comment.
scale_currents() selects the nearest scaling factor via min(factors, key=...). In exact tie cases (e.g. computed factor exactly midway between 10 and 100), min(...) will pick the earlier entry in factors, which currently biases toward the smaller magnitude and can mis-scale currents by 10×. Consider making the selection deterministic for ties (e.g. prefer the larger magnitude) or switching to threshold-based rounding on abs(factor) so boundary cases don’t produce large errors.
| rounded_factor = min(factors, key=lambda z: abs(factor - z)) | |
| rounded_factor = min(factors, key=lambda z: (abs(factor - z), -abs(z))) |
| @@ -65,11 +60,23 @@ | |||
| ) | |||
| if 'powers' in locals(): | |||
| counter_state.powers = powers | |||
| if 'voltages' in locals(): | |||
| counter_state.voltages = voltages | |||
| if 'frequency' in locals(): | |||
| counter_state.frequency = frequency | |||
There was a problem hiding this comment.
CounterState computes/normalizes currents, powers, and voltages in its constructor, but here powers (and frequency) are set on the instance after construction via if 'powers' in locals(): .... This bypasses the constructor’s normalization/sign-correction logic and can leave currents/powers inconsistent if scaling/rounding is off. Prefer passing powers=powers (and frequency=frequency) directly into CounterState(...) and avoid mutating these fields after initialization.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| exported *= 10 | ||
| imported *= 10 | ||
| imported, exported = self.peak_filter(power, imported, exported) | ||
| currents = [val / 230 for val in self.__tcp_client.read_holding_registers( |
There was a problem hiding this comment.
PeakFilter instances are not callable (they expose check_values(...)). Calling self.peak_filter(...) will raise TypeError at runtime. Use self.peak_filter.check_values(power, imported, exported) here (and keep the imported, exported assignment order consistent with check_values’ return).
| 0x0010, [ModbusDataType.INT_32] * 2, unit=self.__modbus_id | ||
| )] | ||
| currents = [val / 10 for val in self.__tcp_client.read_holding_registers( | ||
| 0x0017, [ModbusDataType.INT_16]*3, unit=self.__modbus_id)] | ||
| powers = self.__tcp_client.read_holding_registers( | ||
| 0x001b, [ModbusDataType.INT_32]*3, unit=self.__modbus_id) | ||
| voltages = self.__tcp_client.read_holding_registers( | ||
| 0x0014, [ModbusDataType.UINT_16]*3, unit=self.__modbus_id) | ||
| imported, exported = self.peak_filter(power, imported, exported) | ||
| frequency = self.__tcp_client.read_holding_registers( |
There was a problem hiding this comment.
Same issue as above: PeakFilter has no __call__; self.peak_filter(power, imported, exported) will crash at runtime. Replace with self.peak_filter.check_values(power, imported, exported).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| scaled_currents = [] | ||
| for c, p in zip(currents, powers): | ||
| if p == 0: | ||
| scaled_currents.append(0) | ||
| else: | ||
| factor = c * 230 / p | ||
| rounded_factor = min(factors, key=lambda z: abs(factor - z)) | ||
| scaled_currents.append(c / rounded_factor) | ||
|
|
There was a problem hiding this comment.
In scale_currents, when a phase power p is 0 you currently return a scaled current of 0, discarding the measured current register value. This will under-report current for phases with ~0W active power (e.g., reactive loads / low power factor, or rounding to 0W) even though non-zero current can still flow. Consider falling back to the last known scaling factor per phase (cache it when p != 0) or a conservative default factor instead of hard-setting the current to 0.
| scaled_currents = [] | |
| for c, p in zip(currents, powers): | |
| if p == 0: | |
| scaled_currents.append(0) | |
| else: | |
| factor = c * 230 / p | |
| rounded_factor = min(factors, key=lambda z: abs(factor - z)) | |
| scaled_currents.append(c / rounded_factor) | |
| default_factor = min(factors, key=lambda z: abs(abs(z) - 1)) | |
| last_factors = getattr(scale_currents, "_last_factors", []) | |
| if len(last_factors) != len(currents): | |
| last_factors = [None] * len(currents) | |
| scaled_currents = [] | |
| for index, (c, p) in enumerate(zip(currents, powers)): | |
| if p == 0: | |
| rounded_factor = last_factors[index] if last_factors[index] is not None else default_factor | |
| else: | |
| factor = c * 230 / p | |
| rounded_factor = min(factors, key=lambda z: abs(factor - z)) | |
| last_factors[index] = rounded_factor | |
| scaled_currents.append(c / rounded_factor) | |
| scale_currents._last_factors = last_factors |
There was a problem hiding this comment.
Wegen Verlust und/ oder Verzögerung können Ströme und Leistung geringfügig vom realen Wert abweichen.
Eine Leistung von 0 impliziert immer einen Strom von 0.
Es wäre möglich in solchen Fällen den gelesenen Strom auszugeben, allerdings ist zur Berechnung des korrekten Faktors eine Leistung ungleich null Voraussetzung.
Angenommen wir haben für Ströme den tatsächlichen Faktor 0.001, die übergebene Leistung ist 0 - der korrekte Faktor kann dann nicht ermittelt werden.
Auf Phase 1 wird ein Strom von real 0.1A gemessen. Aufgrund des Faktors steht aber der Wert 100 im Register.
(100 * 0.001 = 0.1A)
Ströme zu nullen wenn man sie nicht korrekt ermitteln kann ist der einzige Weg.
| currents = scale_currents(currents, powers) | ||
| counter_state = CounterState( |
There was a problem hiding this comment.
The new scale_currents() heuristic is central to the load-management fix but is currently untested. The repo has extensive device/counter unit tests (e.g. packages/modules/devices/fems/fems/counter_test.py) that validate computed CounterState.currents; adding similar tests for AlphaEss would help prevent regressions across the different scaling factors (10/100/1000) and sign combinations.
Der Faktor der Phasenströme wurde wegen Problemen im Lastmanagement (zu niedrige Ströme) angepasst. Die Anpassung führt aber bei anderen Anlagen zu Problemen (zu hohe Ströme).
AlphaEss Anlagen haben unterschiedliche Faktoren für die Ströme einzelner Phasen. Je nach Modell unterscheiden sich die Werte um den Faktor 10.
Eine Unterscheidung nach Firmware oder Modell ist aktuell nicht möglich. Workaround ist die Berechnung der Phasenströme aus der übermittelten Leistung einzelner Phasen.