From 914cad00c107f4ce2df59c3edbe82826fc76716d Mon Sep 17 00:00:00 2001 From: tushabe Date: Mon, 3 Aug 2026 18:32:36 +0300 Subject: [PATCH 1/4] feat(srne): add guarded settings write infrastructure --- .../edge/ess/srne/batteryinverter/Config.java | 27 +++++ .../batteryinverter/SafeWriteHandler.java | 66 +++++++++++ .../batteryinverter/SrneBatteryInverter.java | 12 +- .../SrneBatteryInverterImpl.java | 105 +++++++++++++++++- .../ess/srne/batteryinverter/MyConfig.java | 64 +++++++++++ .../batteryinverter/SafeWriteHandlerTest.java | 40 +++++++ 6 files changed, 308 insertions(+), 6 deletions(-) create mode 100644 io.openems.edge.ess.srne/src/io/openems/edge/ess/srne/batteryinverter/SafeWriteHandler.java create mode 100644 io.openems.edge.ess.srne/test/io/openems/edge/ess/srne/batteryinverter/SafeWriteHandlerTest.java diff --git a/io.openems.edge.ess.srne/src/io/openems/edge/ess/srne/batteryinverter/Config.java b/io.openems.edge.ess.srne/src/io/openems/edge/ess/srne/batteryinverter/Config.java index e3a3c769dc..b3d453c7af 100644 --- a/io.openems.edge.ess.srne/src/io/openems/edge/ess/srne/batteryinverter/Config.java +++ b/io.openems.edge.ess.srne/src/io/openems/edge/ess/srne/batteryinverter/Config.java @@ -32,5 +32,32 @@ @AttributeDefinition(name = "Maximum apparent power", description = "Maximum inverter apparent power in [VA].") int maxApparentPower() default 12_000; + @AttributeDefinition(name = "Enable settings writes", description = "Safety gate for FC16 settings writes. Disabled by default.") + boolean controlEnabled() default false; + + @AttributeDefinition(name = "Discharge cutoff SoC", description = "Target for E00F in percent; -1 leaves unchanged.") + int dischargeCutoffSoc() default -1; + + @AttributeDefinition(name = "Stop-charge current", description = "Target for E01C in amperes; -1 leaves unchanged.") + int stopChargeCurrent() default -1; + + @AttributeDefinition(name = "Stop-charge SoC", description = "Target for E01D in percent; -1 leaves unchanged.") + int stopChargeSoc() default -1; + + @AttributeDefinition(name = "Low-SoC alarm", description = "Target for E01E in percent; -1 leaves unchanged.") + int lowSocAlarm() default -1; + + @AttributeDefinition(name = "Switch to line SoC", description = "Target for E01F in percent; -1 leaves unchanged.") + int switchToLineSoc() default -1; + + @AttributeDefinition(name = "Switch to battery SoC", description = "Target for E020 in percent; -1 leaves unchanged.") + int switchToBatterySoc() default -1; + + @AttributeDefinition(name = "AC charge-current limit", description = "Target for E205 in amperes; -1 leaves unchanged.") + int acChargeCurrentLimit() default -1; + + @AttributeDefinition(name = "Maximum charge-current limit", description = "Target for E20A in amperes; -1 leaves unchanged.") + int maxChargeCurrentLimit() default -1; + String webconsole_configurationFactory_nameHint() default "SRNE Battery-Inverter [{id}]"; } diff --git a/io.openems.edge.ess.srne/src/io/openems/edge/ess/srne/batteryinverter/SafeWriteHandler.java b/io.openems.edge.ess.srne/src/io/openems/edge/ess/srne/batteryinverter/SafeWriteHandler.java new file mode 100644 index 0000000000..08f7496ba3 --- /dev/null +++ b/io.openems.edge.ess.srne/src/io/openems/edge/ess/srne/batteryinverter/SafeWriteHandler.java @@ -0,0 +1,66 @@ +package io.openems.edge.ess.srne.batteryinverter; + +import io.openems.common.types.OptionsEnum; +import io.openems.edge.bridge.modbus.api.task.Task.ExecuteState; + +/** + * Coordinates one idempotent settings write and its read-back verification. + * A failed or mismatched write is never retried automatically. + */ +final class SafeWriteHandler { + + public enum State implements OptionsEnum { + UNDEFINED(-1), IDLE(0), QUEUED(1), AWAITING_READBACK(2), VERIFIED(3), FAILED(4); + + private final int value; + + private State(int value) { + this.value = value; + } + + @Override + public int getValue() { + return this.value; + } + + @Override + public String getName() { + return this.name(); + } + + @Override + public OptionsEnum getUndefined() { + return UNDEFINED; + } + } + + private State state = State.IDLE; + private Integer target; + + public boolean queueIfChanged(Integer actual, int target) { + if (this.state != State.IDLE || actual == null || actual == target) { + return false; + } + this.target = target; + this.state = State.QUEUED; + return true; + } + + public void onExecute(ExecuteState executeState) { + if (this.state != State.QUEUED || executeState == ExecuteState.NO_OP) { + return; + } + this.state = executeState == ExecuteState.OK ? State.AWAITING_READBACK : State.FAILED; + } + + public void verify(Integer actual) { + if (this.state != State.AWAITING_READBACK || actual == null) { + return; + } + this.state = actual.equals(this.target) ? State.VERIFIED : State.FAILED; + } + + public State getState() { + return this.state; + } +} diff --git a/io.openems.edge.ess.srne/src/io/openems/edge/ess/srne/batteryinverter/SrneBatteryInverter.java b/io.openems.edge.ess.srne/src/io/openems/edge/ess/srne/batteryinverter/SrneBatteryInverter.java index 6ce7676857..f7f5c9706b 100644 --- a/io.openems.edge.ess.srne/src/io/openems/edge/ess/srne/batteryinverter/SrneBatteryInverter.java +++ b/io.openems.edge.ess.srne/src/io/openems/edge/ess/srne/batteryinverter/SrneBatteryInverter.java @@ -21,7 +21,17 @@ public enum ChannelId implements io.openems.edge.common.channel.ChannelId { MACHINE_STATE(Doc.of(MachineState.values()) // .text("Machine state from holding register 0x0210")), // STATE_MACHINE(Doc.of(State.values()) // - .text("Grid/off-grid lifecycle derived from the machine state")); // + .text("Grid/off-grid lifecycle derived from the machine state")), // + DISCHARGE_CUTOFF_SOC(Doc.of(OpenemsType.INTEGER).unit(Unit.PERCENT)), // + STOP_CHARGE_CURRENT(Doc.of(OpenemsType.INTEGER).unit(Unit.MILLIAMPERE)), // + STOP_CHARGE_SOC(Doc.of(OpenemsType.INTEGER).unit(Unit.PERCENT)), // + LOW_SOC_ALARM(Doc.of(OpenemsType.INTEGER).unit(Unit.PERCENT)), // + SWITCH_TO_LINE_SOC(Doc.of(OpenemsType.INTEGER).unit(Unit.PERCENT)), // + SWITCH_TO_BATTERY_SOC(Doc.of(OpenemsType.INTEGER).unit(Unit.PERCENT)), // + AC_CHARGE_CURRENT_LIMIT(Doc.of(OpenemsType.INTEGER).unit(Unit.MILLIAMPERE)), // + MAX_CHARGE_CURRENT_LIMIT(Doc.of(OpenemsType.INTEGER).unit(Unit.MILLIAMPERE)), // + SAFE_WRITE_STATE(Doc.of(SafeWriteHandler.State.values()) // + .text("Aggregate state of the guarded settings write operation")); // private final Doc doc; diff --git a/io.openems.edge.ess.srne/src/io/openems/edge/ess/srne/batteryinverter/SrneBatteryInverterImpl.java b/io.openems.edge.ess.srne/src/io/openems/edge/ess/srne/batteryinverter/SrneBatteryInverterImpl.java index d72ff88851..1140c932de 100644 --- a/io.openems.edge.ess.srne/src/io/openems/edge/ess/srne/batteryinverter/SrneBatteryInverterImpl.java +++ b/io.openems.edge.ess.srne/src/io/openems/edge/ess/srne/batteryinverter/SrneBatteryInverterImpl.java @@ -30,6 +30,8 @@ import io.openems.edge.bridge.modbus.api.element.SignedWordElement; import io.openems.edge.bridge.modbus.api.element.UnsignedWordElement; import io.openems.edge.bridge.modbus.api.task.FC3ReadRegistersTask; +import io.openems.edge.bridge.modbus.api.task.FC16WriteRegistersTask; +import io.openems.edge.common.channel.IntegerReadChannel; import io.openems.edge.common.channel.value.Value; import io.openems.edge.common.component.OpenemsComponent; import io.openems.edge.common.startstop.StartStop; @@ -53,6 +55,18 @@ public class SrneBatteryInverterImpl extends AbstractOpenemsModbusComponent private final AtomicReference targetGridMode = new AtomicReference<>(TargetGridMode.GO_ON_GRID); private final AtomicReference startStopTarget = new AtomicReference<>(StartStop.UNDEFINED); + private final UnsignedWordElement dischargeCutoffSocWrite = new UnsignedWordElement(0xE00F); + private final UnsignedWordElement stopChargeCurrentWrite = new UnsignedWordElement(0xE01C); + private final UnsignedWordElement stopChargeSocWrite = new UnsignedWordElement(0xE01D); + private final UnsignedWordElement lowSocAlarmWrite = new UnsignedWordElement(0xE01E); + private final UnsignedWordElement switchToLineSocWrite = new UnsignedWordElement(0xE01F); + private final UnsignedWordElement switchToBatterySocWrite = new UnsignedWordElement(0xE020); + private final UnsignedWordElement acChargeCurrentLimitWrite = new UnsignedWordElement(0xE205); + private final UnsignedWordElement maxChargeCurrentLimitWrite = new UnsignedWordElement(0xE20A); + private final SafeWriteHandler[] writeHandlers = { new SafeWriteHandler(), new SafeWriteHandler(), + new SafeWriteHandler(), new SafeWriteHandler(), new SafeWriteHandler(), new SafeWriteHandler(), + new SafeWriteHandler(), new SafeWriteHandler() }; + private Config config; @Override @Reference(// @@ -77,6 +91,7 @@ public SrneBatteryInverterImpl() { @Activate private void activate(ComponentContext context, Config config) throws OpenemsException { + this.config = config; super.activate(context, config.id(), config.alias(), config.enabled(), config.modbusUnitId()); this._setMaxApparentPower(config.maxApparentPower()); this.getMachineStateChannel().onSetNextValue(ignore -> this.updateLifecycle()); @@ -129,13 +144,40 @@ private void updateLifecycle() { @Override protected ModbusProtocol defineModbusProtocol() { return new ModbusProtocol(this, // + new FC3ReadRegistersTask(0xE00F, Priority.LOW, // + m(SrneBatteryInverter.ChannelId.DISCHARGE_CUTOFF_SOC, + new UnsignedWordElement(0xE00F))), // + new FC3ReadRegistersTask(0xE01C, Priority.LOW, // + m(SrneBatteryInverter.ChannelId.STOP_CHARGE_CURRENT, new UnsignedWordElement(0xE01C), + SCALE_FACTOR_2), // + m(SrneBatteryInverter.ChannelId.STOP_CHARGE_SOC, new UnsignedWordElement(0xE01D)), // + m(SrneBatteryInverter.ChannelId.LOW_SOC_ALARM, new UnsignedWordElement(0xE01E)), // + m(SrneBatteryInverter.ChannelId.SWITCH_TO_LINE_SOC, new UnsignedWordElement(0xE01F)), // + m(SrneBatteryInverter.ChannelId.SWITCH_TO_BATTERY_SOC, + new UnsignedWordElement(0xE020))), // + new FC3ReadRegistersTask(0xE205, Priority.LOW, // + m(SrneBatteryInverter.ChannelId.AC_CHARGE_CURRENT_LIMIT, + new UnsignedWordElement(0xE205), SCALE_FACTOR_2)), // + new FC3ReadRegistersTask(0xE20A, Priority.LOW, // + m(SrneBatteryInverter.ChannelId.MAX_CHARGE_CURRENT_LIMIT, + new UnsignedWordElement(0xE20A), SCALE_FACTOR_2)), // new FC3ReadRegistersTask(0x0101, Priority.HIGH, // m(SrneBatteryInverter.ChannelId.BATTERY_VOLTAGE, new UnsignedWordElement(0x0101), SCALE_FACTOR_2), // m(SrneBatteryInverter.ChannelId.BATTERY_CURRENT, new SignedWordElement(0x0102), SCALE_FACTOR_2)), // new FC3ReadRegistersTask(0x0210, Priority.HIGH, // - m(SrneBatteryInverter.ChannelId.MACHINE_STATE, new UnsignedWordElement(0x0210)))); + m(SrneBatteryInverter.ChannelId.MACHINE_STATE, new UnsignedWordElement(0x0210))), // + new FC16WriteRegistersTask(this.writeHandlers[0]::onExecute, 0xE00F, this.dischargeCutoffSocWrite), // + new FC16WriteRegistersTask(this.writeHandlers[1]::onExecute, 0xE01C, this.stopChargeCurrentWrite), // + new FC16WriteRegistersTask(this.writeHandlers[2]::onExecute, 0xE01D, this.stopChargeSocWrite), // + new FC16WriteRegistersTask(this.writeHandlers[3]::onExecute, 0xE01E, this.lowSocAlarmWrite), // + new FC16WriteRegistersTask(this.writeHandlers[4]::onExecute, 0xE01F, this.switchToLineSocWrite), // + new FC16WriteRegistersTask(this.writeHandlers[5]::onExecute, 0xE020, this.switchToBatterySocWrite), // + new FC16WriteRegistersTask(this.writeHandlers[6]::onExecute, 0xE205, + this.acChargeCurrentLimitWrite), // + new FC16WriteRegistersTask(this.writeHandlers[7]::onExecute, 0xE20A, + this.maxChargeCurrentLimitWrite)); } @Override @@ -150,11 +192,64 @@ public void setStartStop(StartStop value) throws OpenemsNamedException { @Override public void run(Battery battery, int setActivePower, int setReactivePower) throws OpenemsNamedException { - /* - * Story 18 is intentionally read-only. Story 19 will consume targetGridMode and - * apply the power set-points through the validated FC16 control path. - */ this.updateLifecycle(); + this.reconcileSafeSettings(); + } + + private void reconcileSafeSettings() { + if (this.config == null || !this.config.controlEnabled()) { + return; + } + MachineState machineState = this.getMachineStateChannel().value().asEnum(); + if (machineState == null || !machineState.isVerified()) { + return; + } + + this.reconcile(0, SrneBatteryInverter.ChannelId.DISCHARGE_CUTOFF_SOC, + this.config.dischargeCutoffSoc(), 0, 100, 1, this.dischargeCutoffSocWrite); + this.reconcile(1, SrneBatteryInverter.ChannelId.STOP_CHARGE_CURRENT, + this.config.stopChargeCurrent(), 0, 120, 10, this.stopChargeCurrentWrite); + this.reconcile(2, SrneBatteryInverter.ChannelId.STOP_CHARGE_SOC, + this.config.stopChargeSoc(), 0, 100, 1, this.stopChargeSocWrite); + this.reconcile(3, SrneBatteryInverter.ChannelId.LOW_SOC_ALARM, + this.config.lowSocAlarm(), 0, 100, 1, this.lowSocAlarmWrite); + this.reconcile(4, SrneBatteryInverter.ChannelId.SWITCH_TO_LINE_SOC, + this.config.switchToLineSoc(), 0, 100, 1, this.switchToLineSocWrite); + this.reconcile(5, SrneBatteryInverter.ChannelId.SWITCH_TO_BATTERY_SOC, + this.config.switchToBatterySoc(), 0, 100, 1, this.switchToBatterySocWrite); + this.reconcile(6, SrneBatteryInverter.ChannelId.AC_CHARGE_CURRENT_LIMIT, + this.config.acChargeCurrentLimit(), 0, 120, 10, this.acChargeCurrentLimitWrite); + this.reconcile(7, SrneBatteryInverter.ChannelId.MAX_CHARGE_CURRENT_LIMIT, + this.config.maxChargeCurrentLimit(), 0, 120, 10, this.maxChargeCurrentLimitWrite); + this.channel(SrneBatteryInverter.ChannelId.SAFE_WRITE_STATE).setNextValue(this.aggregateWriteState()); + } + + private void reconcile(int index, SrneBatteryInverter.ChannelId channelId, int configuredTarget, int min, int max, + int rawMultiplier, UnsignedWordElement writeElement) { + if (configuredTarget < 0) { + return; + } + var handler = this.writeHandlers[index]; + Integer actual = ((IntegerReadChannel) this.channel(channelId)).value().get(); + handler.verify(actual); + int target = Math.max(min, Math.min(max, configuredTarget)); + int channelTarget = target * (rawMultiplier == 10 ? 1_000 : 1); + if (handler.queueIfChanged(actual, channelTarget)) { + writeElement.setNextWriteValue(target * rawMultiplier); + } + } + + private SafeWriteHandler.State aggregateWriteState() { + var result = SafeWriteHandler.State.IDLE; + for (var handler : this.writeHandlers) { + if (handler.getState() == SafeWriteHandler.State.FAILED) { + return SafeWriteHandler.State.FAILED; + } + if (handler.getState().ordinal() > result.ordinal()) { + result = handler.getState(); + } + } + return result; } @Override diff --git a/io.openems.edge.ess.srne/test/io/openems/edge/ess/srne/batteryinverter/MyConfig.java b/io.openems.edge.ess.srne/test/io/openems/edge/ess/srne/batteryinverter/MyConfig.java index 91d86b9ece..36b59b9331 100644 --- a/io.openems.edge.ess.srne/test/io/openems/edge/ess/srne/batteryinverter/MyConfig.java +++ b/io.openems.edge.ess.srne/test/io/openems/edge/ess/srne/batteryinverter/MyConfig.java @@ -11,6 +11,15 @@ protected static class Builder { private String modbusId; private int modbusUnitId; private int maxApparentPower; + private boolean controlEnabled; + private int dischargeCutoffSoc = -1; + private int stopChargeCurrent = -1; + private int stopChargeSoc = -1; + private int lowSocAlarm = -1; + private int switchToLineSoc = -1; + private int switchToBatterySoc = -1; + private int acChargeCurrentLimit = -1; + private int maxChargeCurrentLimit = -1; private Builder() { } @@ -35,6 +44,16 @@ public Builder setMaxApparentPower(int maxApparentPower) { return this; } + public Builder setControlEnabled(boolean value) { + this.controlEnabled = value; + return this; + } + + public Builder setDischargeCutoffSoc(int value) { + this.dischargeCutoffSoc = value; + return this; + } + public MyConfig build() { return new MyConfig(this); } @@ -75,4 +94,49 @@ public int modbusUnitId() { public int maxApparentPower() { return this.builder.maxApparentPower; } + + @Override + public boolean controlEnabled() { + return this.builder.controlEnabled; + } + + @Override + public int dischargeCutoffSoc() { + return this.builder.dischargeCutoffSoc; + } + + @Override + public int stopChargeCurrent() { + return this.builder.stopChargeCurrent; + } + + @Override + public int stopChargeSoc() { + return this.builder.stopChargeSoc; + } + + @Override + public int lowSocAlarm() { + return this.builder.lowSocAlarm; + } + + @Override + public int switchToLineSoc() { + return this.builder.switchToLineSoc; + } + + @Override + public int switchToBatterySoc() { + return this.builder.switchToBatterySoc; + } + + @Override + public int acChargeCurrentLimit() { + return this.builder.acChargeCurrentLimit; + } + + @Override + public int maxChargeCurrentLimit() { + return this.builder.maxChargeCurrentLimit; + } } diff --git a/io.openems.edge.ess.srne/test/io/openems/edge/ess/srne/batteryinverter/SafeWriteHandlerTest.java b/io.openems.edge.ess.srne/test/io/openems/edge/ess/srne/batteryinverter/SafeWriteHandlerTest.java new file mode 100644 index 0000000000..1c68026250 --- /dev/null +++ b/io.openems.edge.ess.srne/test/io/openems/edge/ess/srne/batteryinverter/SafeWriteHandlerTest.java @@ -0,0 +1,40 @@ +package io.openems.edge.ess.srne.batteryinverter; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import org.junit.jupiter.api.Test; + +import io.openems.edge.bridge.modbus.api.task.Task.ExecuteState; + +public class SafeWriteHandlerTest { + + @Test + void unchangedValueDoesNotWrite() { + var sut = new SafeWriteHandler(); + assertFalse(sut.queueIfChanged(10, 10)); + assertEquals(SafeWriteHandler.State.IDLE, sut.getState()); + } + + @Test + void successfulWriteRequiresMatchingReadback() { + var sut = new SafeWriteHandler(); + assertTrue(sut.queueIfChanged(10, 20)); + sut.onExecute(ExecuteState.OK); + assertEquals(SafeWriteHandler.State.AWAITING_READBACK, sut.getState()); + sut.verify(20); + assertEquals(SafeWriteHandler.State.VERIFIED, sut.getState()); + assertFalse(sut.queueIfChanged(10, 20)); + } + + @Test + void mismatchFailsWithoutRetry() { + var sut = new SafeWriteHandler(); + assertTrue(sut.queueIfChanged(10, 20)); + sut.onExecute(ExecuteState.OK); + sut.verify(15); + assertEquals(SafeWriteHandler.State.FAILED, sut.getState()); + assertFalse(sut.queueIfChanged(15, 20)); + } +} From 1b162c79b1747634a6ff712dc6d493fd16865517 Mon Sep 17 00:00:00 2001 From: tushabe Date: Mon, 3 Aug 2026 19:11:35 +0300 Subject: [PATCH 2/4] fix(srne): wait for fresh settings readback --- .../SrneBatteryInverterImpl.java | 17 ++++++++++++++++- .../batteryinverter/SafeWriteHandlerTest.java | 14 ++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/io.openems.edge.ess.srne/src/io/openems/edge/ess/srne/batteryinverter/SrneBatteryInverterImpl.java b/io.openems.edge.ess.srne/src/io/openems/edge/ess/srne/batteryinverter/SrneBatteryInverterImpl.java index 1140c932de..401bb71cd5 100644 --- a/io.openems.edge.ess.srne/src/io/openems/edge/ess/srne/batteryinverter/SrneBatteryInverterImpl.java +++ b/io.openems.edge.ess.srne/src/io/openems/edge/ess/srne/batteryinverter/SrneBatteryInverterImpl.java @@ -112,6 +112,22 @@ private void activate(ComponentContext context, Config config) throws OpenemsExc }; this.getBatteryVoltageChannel().onSetNextValue(updateActivePower); this.getBatteryCurrentChannel().onSetNextValue(updateActivePower); + + this.registerReadback(0, SrneBatteryInverter.ChannelId.DISCHARGE_CUTOFF_SOC); + this.registerReadback(1, SrneBatteryInverter.ChannelId.STOP_CHARGE_CURRENT); + this.registerReadback(2, SrneBatteryInverter.ChannelId.STOP_CHARGE_SOC); + this.registerReadback(3, SrneBatteryInverter.ChannelId.LOW_SOC_ALARM); + this.registerReadback(4, SrneBatteryInverter.ChannelId.SWITCH_TO_LINE_SOC); + this.registerReadback(5, SrneBatteryInverter.ChannelId.SWITCH_TO_BATTERY_SOC); + this.registerReadback(6, SrneBatteryInverter.ChannelId.AC_CHARGE_CURRENT_LIMIT); + this.registerReadback(7, SrneBatteryInverter.ChannelId.MAX_CHARGE_CURRENT_LIMIT); + } + + private void registerReadback(int index, SrneBatteryInverter.ChannelId channelId) { + ((IntegerReadChannel) this.channel(channelId)).onSetNextValue(value -> { + this.writeHandlers[index].verify(value.get()); + this.channel(SrneBatteryInverter.ChannelId.SAFE_WRITE_STATE).setNextValue(this.aggregateWriteState()); + }); } @Override @@ -231,7 +247,6 @@ private void reconcile(int index, SrneBatteryInverter.ChannelId channelId, int c } var handler = this.writeHandlers[index]; Integer actual = ((IntegerReadChannel) this.channel(channelId)).value().get(); - handler.verify(actual); int target = Math.max(min, Math.min(max, configuredTarget)); int channelTarget = target * (rawMultiplier == 10 ? 1_000 : 1); if (handler.queueIfChanged(actual, channelTarget)) { diff --git a/io.openems.edge.ess.srne/test/io/openems/edge/ess/srne/batteryinverter/SafeWriteHandlerTest.java b/io.openems.edge.ess.srne/test/io/openems/edge/ess/srne/batteryinverter/SafeWriteHandlerTest.java index 1c68026250..96ef088d3a 100644 --- a/io.openems.edge.ess.srne/test/io/openems/edge/ess/srne/batteryinverter/SafeWriteHandlerTest.java +++ b/io.openems.edge.ess.srne/test/io/openems/edge/ess/srne/batteryinverter/SafeWriteHandlerTest.java @@ -28,6 +28,20 @@ void successfulWriteRequiresMatchingReadback() { assertFalse(sut.queueIfChanged(10, 20)); } + @Test + void remainsPendingUntilFreshReadbackArrives() { + var sut = new SafeWriteHandler(); + assertTrue(sut.queueIfChanged(10, 20)); + sut.onExecute(ExecuteState.OK); + + // Merely reconciling against the cached value must not verify or fail the write. + assertFalse(sut.queueIfChanged(10, 20)); + assertEquals(SafeWriteHandler.State.AWAITING_READBACK, sut.getState()); + + sut.verify(20); + assertEquals(SafeWriteHandler.State.VERIFIED, sut.getState()); + } + @Test void mismatchFailsWithoutRetry() { var sut = new SafeWriteHandler(); From 0bd88068440e9d286575f6f3b34d54482fc1381a Mon Sep 17 00:00:00 2001 From: arindahills <293051436+arindahills@users.noreply.github.com> Date: Wed, 5 Aug 2026 12:07:48 +0530 Subject: [PATCH 3/4] fix(srne): thread-safe SafeWriteHandler + cap current limits at battery continuous rating Review follow-ups on the guarded settings-write path (Aaron offline): - SafeWriteHandler: queueIfChanged runs on the Edge cycle thread while onExecute/verify fire on the Modbus bridge worker thread; the mutable state/target had no synchronization, so transitions were not guaranteed visible across threads. Make the four methods synchronized. - Current-limit settings (stopChargeCurrent, acChargeCurrentLimit, maxChargeCurrentLimit) were clamped to 120A. The SR-SE10B (205Ah LFP) continuous max charge/discharge is 100A; 120A is only the 3-second peak (datasheet SRNE_SE series V1.5). Cap at 100A via a named constant so a configured continuous limit can never exceed the battery rating. Signed-off-by: arindahills <293051436+arindahills@users.noreply.github.com> --- .../ess/srne/batteryinverter/SafeWriteHandler.java | 14 ++++++++++---- .../batteryinverter/SrneBatteryInverterImpl.java | 14 +++++++++++--- 2 files changed, 21 insertions(+), 7 deletions(-) diff --git a/io.openems.edge.ess.srne/src/io/openems/edge/ess/srne/batteryinverter/SafeWriteHandler.java b/io.openems.edge.ess.srne/src/io/openems/edge/ess/srne/batteryinverter/SafeWriteHandler.java index 08f7496ba3..a140906bdb 100644 --- a/io.openems.edge.ess.srne/src/io/openems/edge/ess/srne/batteryinverter/SafeWriteHandler.java +++ b/io.openems.edge.ess.srne/src/io/openems/edge/ess/srne/batteryinverter/SafeWriteHandler.java @@ -6,6 +6,12 @@ /** * Coordinates one idempotent settings write and its read-back verification. * A failed or mismatched write is never retried automatically. + * + *

Thread-safety: {@code queueIfChanged} is called from the Edge cycle thread + * (via {@code run()}), while {@code onExecute} and {@code verify} are called from + * the Modbus bridge worker thread (task execute callback and read-back channel + * callback). The mutable {@code state}/{@code target} are therefore guarded by + * intrinsic locking so transitions are atomic and visible across those threads. */ final class SafeWriteHandler { @@ -37,7 +43,7 @@ public OptionsEnum getUndefined() { private State state = State.IDLE; private Integer target; - public boolean queueIfChanged(Integer actual, int target) { + public synchronized boolean queueIfChanged(Integer actual, int target) { if (this.state != State.IDLE || actual == null || actual == target) { return false; } @@ -46,21 +52,21 @@ public boolean queueIfChanged(Integer actual, int target) { return true; } - public void onExecute(ExecuteState executeState) { + public synchronized void onExecute(ExecuteState executeState) { if (this.state != State.QUEUED || executeState == ExecuteState.NO_OP) { return; } this.state = executeState == ExecuteState.OK ? State.AWAITING_READBACK : State.FAILED; } - public void verify(Integer actual) { + public synchronized void verify(Integer actual) { if (this.state != State.AWAITING_READBACK || actual == null) { return; } this.state = actual.equals(this.target) ? State.VERIFIED : State.FAILED; } - public State getState() { + public synchronized State getState() { return this.state; } } diff --git a/io.openems.edge.ess.srne/src/io/openems/edge/ess/srne/batteryinverter/SrneBatteryInverterImpl.java b/io.openems.edge.ess.srne/src/io/openems/edge/ess/srne/batteryinverter/SrneBatteryInverterImpl.java index 401bb71cd5..c9eddacbfd 100644 --- a/io.openems.edge.ess.srne/src/io/openems/edge/ess/srne/batteryinverter/SrneBatteryInverterImpl.java +++ b/io.openems.edge.ess.srne/src/io/openems/edge/ess/srne/batteryinverter/SrneBatteryInverterImpl.java @@ -212,6 +212,14 @@ public void run(Battery battery, int setActivePower, int setReactivePower) throw this.reconcileSafeSettings(); } + /** + * Continuous max charge/discharge current of the SR-SE10B (205Ah LFP) is 100A; + * 120A is only the 3-second peak (datasheet SRNE_SE series V1.5). Current-limit + * settings are capped at the continuous rating so a configured limit can never + * exceed what the battery allows. + */ + private static final int MAX_BATTERY_CURRENT_A = 100; + private void reconcileSafeSettings() { if (this.config == null || !this.config.controlEnabled()) { return; @@ -224,7 +232,7 @@ private void reconcileSafeSettings() { this.reconcile(0, SrneBatteryInverter.ChannelId.DISCHARGE_CUTOFF_SOC, this.config.dischargeCutoffSoc(), 0, 100, 1, this.dischargeCutoffSocWrite); this.reconcile(1, SrneBatteryInverter.ChannelId.STOP_CHARGE_CURRENT, - this.config.stopChargeCurrent(), 0, 120, 10, this.stopChargeCurrentWrite); + this.config.stopChargeCurrent(), 0, MAX_BATTERY_CURRENT_A, 10, this.stopChargeCurrentWrite); this.reconcile(2, SrneBatteryInverter.ChannelId.STOP_CHARGE_SOC, this.config.stopChargeSoc(), 0, 100, 1, this.stopChargeSocWrite); this.reconcile(3, SrneBatteryInverter.ChannelId.LOW_SOC_ALARM, @@ -234,9 +242,9 @@ private void reconcileSafeSettings() { this.reconcile(5, SrneBatteryInverter.ChannelId.SWITCH_TO_BATTERY_SOC, this.config.switchToBatterySoc(), 0, 100, 1, this.switchToBatterySocWrite); this.reconcile(6, SrneBatteryInverter.ChannelId.AC_CHARGE_CURRENT_LIMIT, - this.config.acChargeCurrentLimit(), 0, 120, 10, this.acChargeCurrentLimitWrite); + this.config.acChargeCurrentLimit(), 0, MAX_BATTERY_CURRENT_A, 10, this.acChargeCurrentLimitWrite); this.reconcile(7, SrneBatteryInverter.ChannelId.MAX_CHARGE_CURRENT_LIMIT, - this.config.maxChargeCurrentLimit(), 0, 120, 10, this.maxChargeCurrentLimitWrite); + this.config.maxChargeCurrentLimit(), 0, MAX_BATTERY_CURRENT_A, 10, this.maxChargeCurrentLimitWrite); this.channel(SrneBatteryInverter.ChannelId.SAFE_WRITE_STATE).setNextValue(this.aggregateWriteState()); } From a1ec8048158117063374eb15d8037965ab191e81 Mon Sep 17 00:00:00 2001 From: tushabe Date: Wed, 5 Aug 2026 12:37:46 +0300 Subject: [PATCH 4/4] fix(srne): harden settings write lifecycle --- .../batteryinverter/SafeWriteHandler.java | 29 ++++++++++++ .../SrneBatteryInverterImpl.java | 42 ++++++++++++++--- .../batteryinverter/SafeWriteHandlerTest.java | 46 +++++++++++++++++++ 3 files changed, 111 insertions(+), 6 deletions(-) diff --git a/io.openems.edge.ess.srne/src/io/openems/edge/ess/srne/batteryinverter/SafeWriteHandler.java b/io.openems.edge.ess.srne/src/io/openems/edge/ess/srne/batteryinverter/SafeWriteHandler.java index a140906bdb..c6d54ded7f 100644 --- a/io.openems.edge.ess.srne/src/io/openems/edge/ess/srne/batteryinverter/SafeWriteHandler.java +++ b/io.openems.edge.ess.srne/src/io/openems/edge/ess/srne/batteryinverter/SafeWriteHandler.java @@ -42,6 +42,7 @@ public OptionsEnum getUndefined() { private State state = State.IDLE; private Integer target; + private int awaitingReadbackCycles; public synchronized boolean queueIfChanged(Integer actual, int target) { if (this.state != State.IDLE || actual == null || actual == target) { @@ -56,9 +57,37 @@ public synchronized void onExecute(ExecuteState executeState) { if (this.state != State.QUEUED || executeState == ExecuteState.NO_OP) { return; } + this.awaitingReadbackCycles = 0; this.state = executeState == ExecuteState.OK ? State.AWAITING_READBACK : State.FAILED; } + /** + * Advances the bounded readback wait without ever retrying the write. + * + * @param timeoutCycles number of Edge cycles allowed for a fresh readback + */ + public synchronized void onCycle(int timeoutCycles) { + if (this.state != State.AWAITING_READBACK) { + return; + } + if (++this.awaitingReadbackCycles >= timeoutCycles) { + this.state = State.FAILED; + } + } + + /** + * Rejects an invalid setting before any Modbus write is queued. + * + * @return true only for the first rejection, for one-shot audit logging + */ + public synchronized boolean reject() { + if (this.state != State.IDLE) { + return false; + } + this.state = State.FAILED; + return true; + } + public synchronized void verify(Integer actual) { if (this.state != State.AWAITING_READBACK || actual == null) { return; diff --git a/io.openems.edge.ess.srne/src/io/openems/edge/ess/srne/batteryinverter/SrneBatteryInverterImpl.java b/io.openems.edge.ess.srne/src/io/openems/edge/ess/srne/batteryinverter/SrneBatteryInverterImpl.java index c9eddacbfd..6aae6b7c9c 100644 --- a/io.openems.edge.ess.srne/src/io/openems/edge/ess/srne/batteryinverter/SrneBatteryInverterImpl.java +++ b/io.openems.edge.ess.srne/src/io/openems/edge/ess/srne/batteryinverter/SrneBatteryInverterImpl.java @@ -15,6 +15,8 @@ import org.osgi.service.component.annotations.Deactivate; import org.osgi.service.component.annotations.Reference; import org.osgi.service.metatype.annotations.Designate; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import io.openems.common.exceptions.OpenemsError.OpenemsNamedException; import io.openems.common.exceptions.OpenemsException; @@ -52,6 +54,8 @@ public class SrneBatteryInverterImpl extends AbstractOpenemsModbusComponent implements SrneBatteryInverter, Srne, OffGridBatteryInverter, ManagedSymmetricBatteryInverter, SymmetricBatteryInverter, ModbusComponent, OpenemsComponent, StartStoppable { + private static final int READBACK_TIMEOUT_CYCLES = 30; + private final Logger log = LoggerFactory.getLogger(SrneBatteryInverterImpl.class); private final AtomicReference targetGridMode = new AtomicReference<>(TargetGridMode.GO_ON_GRID); private final AtomicReference startStopTarget = new AtomicReference<>(StartStop.UNDEFINED); @@ -92,6 +96,12 @@ public SrneBatteryInverterImpl() { @Activate private void activate(ComponentContext context, Config config) throws OpenemsException { this.config = config; + /* + * There is intentionally no @Modified method. A settings configuration update + * causes DS to replace this component instance, giving every setting a fresh + * SafeWriteHandler. VERIFIED and FAILED therefore remain terminal for one + * configured request and cannot trigger repeated writes in the same activation. + */ super.activate(context, config.id(), config.alias(), config.enabled(), config.modbusUnitId()); this._setMaxApparentPower(config.maxApparentPower()); this.getMachineStateChannel().onSetNextValue(ignore -> this.updateLifecycle()); @@ -221,6 +231,10 @@ public void run(Battery battery, int setActivePower, int setReactivePower) throw private static final int MAX_BATTERY_CURRENT_A = 100; private void reconcileSafeSettings() { + for (var handler : this.writeHandlers) { + handler.onCycle(READBACK_TIMEOUT_CYCLES); + } + this.channel(SrneBatteryInverter.ChannelId.SAFE_WRITE_STATE).setNextValue(this.aggregateWriteState()); if (this.config == null || !this.config.controlEnabled()) { return; } @@ -255,7 +269,14 @@ private void reconcile(int index, SrneBatteryInverter.ChannelId channelId, int c } var handler = this.writeHandlers[index]; Integer actual = ((IntegerReadChannel) this.channel(channelId)).value().get(); - int target = Math.max(min, Math.min(max, configuredTarget)); + if (configuredTarget < min || configuredTarget > max) { + if (handler.reject()) { + this.logWarn(this.log, "Rejected out-of-range setting [" + channelId.name() + "] value [" + + configuredTarget + "]; validated range is [" + min + ".." + max + "]"); + } + return; + } + int target = configuredTarget; int channelTarget = target * (rawMultiplier == 10 ? 1_000 : 1); if (handler.queueIfChanged(actual, channelTarget)) { writeElement.setNextWriteValue(target * rawMultiplier); @@ -265,16 +286,25 @@ private void reconcile(int index, SrneBatteryInverter.ChannelId channelId, int c private SafeWriteHandler.State aggregateWriteState() { var result = SafeWriteHandler.State.IDLE; for (var handler : this.writeHandlers) { - if (handler.getState() == SafeWriteHandler.State.FAILED) { - return SafeWriteHandler.State.FAILED; - } - if (handler.getState().ordinal() > result.ordinal()) { - result = handler.getState(); + var state = handler.getState(); + if (writeStatePrecedence(state) > writeStatePrecedence(result)) { + result = state; } } return result; } + static int writeStatePrecedence(SafeWriteHandler.State state) { + return switch (state) { + case FAILED -> 5; + case AWAITING_READBACK -> 4; + case QUEUED -> 3; + case VERIFIED -> 2; + case IDLE -> 1; + case UNDEFINED -> 0; + }; + } + @Override public int getPowerPrecision() { return 1; diff --git a/io.openems.edge.ess.srne/test/io/openems/edge/ess/srne/batteryinverter/SafeWriteHandlerTest.java b/io.openems.edge.ess.srne/test/io/openems/edge/ess/srne/batteryinverter/SafeWriteHandlerTest.java index 96ef088d3a..0499008c6f 100644 --- a/io.openems.edge.ess.srne/test/io/openems/edge/ess/srne/batteryinverter/SafeWriteHandlerTest.java +++ b/io.openems.edge.ess.srne/test/io/openems/edge/ess/srne/batteryinverter/SafeWriteHandlerTest.java @@ -51,4 +51,50 @@ void mismatchFailsWithoutRetry() { assertEquals(SafeWriteHandler.State.FAILED, sut.getState()); assertFalse(sut.queueIfChanged(15, 20)); } + + @Test + void missingReadbackTimesOutWithoutRetry() { + var sut = new SafeWriteHandler(); + assertTrue(sut.queueIfChanged(10, 20)); + sut.onExecute(ExecuteState.OK); + for (var i = 1; i < 30; i++) { + sut.onCycle(30); + assertEquals(SafeWriteHandler.State.AWAITING_READBACK, sut.getState()); + } + sut.onCycle(30); + assertEquals(SafeWriteHandler.State.FAILED, sut.getState()); + assertFalse(sut.queueIfChanged(10, 20)); + } + + @Test + void rejectedTargetNeverQueuesAWrite() { + var sut = new SafeWriteHandler(); + assertTrue(sut.reject()); + assertEquals(SafeWriteHandler.State.FAILED, sut.getState()); + assertFalse(sut.reject()); + assertFalse(sut.queueIfChanged(10, 20)); + } + + @Test + void newActivationStartsWithFreshHandler() { + var previousActivation = new SafeWriteHandler(); + assertTrue(previousActivation.queueIfChanged(10, 20)); + previousActivation.onExecute(ExecuteState.OK); + previousActivation.verify(20); + assertEquals(SafeWriteHandler.State.VERIFIED, previousActivation.getState()); + + var nextActivation = new SafeWriteHandler(); + assertEquals(SafeWriteHandler.State.IDLE, nextActivation.getState()); + assertTrue(nextActivation.queueIfChanged(20, 30)); + } + + @Test + void aggregatePrecedenceIsIndependentOfEnumOrder() { + assertTrue(SrneBatteryInverterImpl.writeStatePrecedence(SafeWriteHandler.State.FAILED) // + > SrneBatteryInverterImpl.writeStatePrecedence(SafeWriteHandler.State.AWAITING_READBACK)); + assertTrue(SrneBatteryInverterImpl.writeStatePrecedence(SafeWriteHandler.State.AWAITING_READBACK) // + > SrneBatteryInverterImpl.writeStatePrecedence(SafeWriteHandler.State.QUEUED)); + assertTrue(SrneBatteryInverterImpl.writeStatePrecedence(SafeWriteHandler.State.QUEUED) // + > SrneBatteryInverterImpl.writeStatePrecedence(SafeWriteHandler.State.VERIFIED)); + } }