From c3527127c6c93e5e67e54848897ae3485c2538a4 Mon Sep 17 00:00:00 2001 From: Vadim Tkachenko Date: Sat, 2 Sep 2023 20:19:30 -0700 Subject: [PATCH 1/8] Added test case identifying problems with HeatPump over MQTT switch (#271) --- .../device/actuator/MqttHeatPumpTest.java | 89 +++++++++++++++++++ dz3r-bootstrap/src/test/resources/log4j2.xml | 13 +++ 2 files changed, 102 insertions(+) create mode 100644 dz3r-bootstrap/src/test/java/net/sf/dz3r/device/actuator/MqttHeatPumpTest.java create mode 100644 dz3r-bootstrap/src/test/resources/log4j2.xml diff --git a/dz3r-bootstrap/src/test/java/net/sf/dz3r/device/actuator/MqttHeatPumpTest.java b/dz3r-bootstrap/src/test/java/net/sf/dz3r/device/actuator/MqttHeatPumpTest.java new file mode 100644 index 000000000..12e5c6138 --- /dev/null +++ b/dz3r-bootstrap/src/test/java/net/sf/dz3r/device/actuator/MqttHeatPumpTest.java @@ -0,0 +1,89 @@ +package net.sf.dz3r.device.actuator; + +import net.sf.dz3r.device.esphome.v1.ESPHomeSwitch; +import net.sf.dz3r.model.HvacMode; +import net.sf.dz3r.signal.Signal; +import net.sf.dz3r.signal.hvac.HvacCommand; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.condition.EnabledIfEnvironmentVariable; +import reactor.core.publisher.Flux; + +import java.io.IOException; +import java.time.Duration; +import java.time.Instant; + +import static org.assertj.core.api.Assertions.assertThatCode; + +/** + * Test MQTT switch race conditions when controlled by a heat pump. + * + * VT: NOTE: Careful, this works on real hardware, disconnect before testing. + */ +@EnabledIfEnvironmentVariable( + named = "TEST_DZ_ESPHOME_HEATPUMP", + matches = "safe", + disabledReason = "Only execute this test if a suitable MQTT broker and ESPHome switch device are available" +)class ESPHomeHeatPumpTest { + + private final Logger logger = LogManager.getLogger(); + + private final String MQTT_BROKER = "mqtt-esphome"; + + private final String ESPHOME_SWITCH_TOPIC_ROOT = "/esphome/0156AC/switch/"; + + @Test + void cycle() throws IOException { + + var switchFan = new ESPHomeSwitch(MQTT_BROKER, ESPHOME_SWITCH_TOPIC_ROOT + "t-relay-2-r0-fan"); + var switchRunning = new ESPHomeSwitch(MQTT_BROKER, ESPHOME_SWITCH_TOPIC_ROOT + "t-relay-2-r1-condenser"); + var switchMode = new ESPHomeSwitch(MQTT_BROKER, ESPHOME_SWITCH_TOPIC_ROOT + "t-relay-2-r2-mode"); + var delay = Duration.ofSeconds(1); + + // This should throw a wrench into everything... + Flux + .just(true, true, true) + .delayElements(delay) + .map(switchMode::setState) + .blockLast(); + + logger.info("set mode to 1"); + + try (var hp = new HeatPump( + "heatpump", + switchMode, true, + switchRunning, false, + switchFan, false, + Duration.ofSeconds(3))) { + + var commands = Flux + .just( + new HvacCommand(HvacMode.COOLING, 0d, 0d), + new HvacCommand(null, 1d, 1d), + new HvacCommand(HvacMode.HEATING, 0d, 0d), + new HvacCommand(null, 1d, 1d), + new HvacCommand(null, 0d, 0d)) + .delayElements(delay) + .doOnNext(command -> logger.info("command: {}", command)) + .map(c -> new Signal(Instant.now(), c)); + + assertThatCode(() -> { + hp + .compute(commands) + .doOnNext(s -> logger.info("output: {}", s)) + .doOnNext(s -> { + if (s.isError()) { + throw new IllegalStateException("Failure output received: " + s); + } + }) + .blockLast(); + + logger.info("done"); + }) + .doesNotThrowAnyException(); + + // Should close() here automatically + } + } +} diff --git a/dz3r-bootstrap/src/test/resources/log4j2.xml b/dz3r-bootstrap/src/test/resources/log4j2.xml new file mode 100644 index 000000000..9f4ce2f2b --- /dev/null +++ b/dz3r-bootstrap/src/test/resources/log4j2.xml @@ -0,0 +1,13 @@ + + + + + + + + + + + + + From 637c71fc23370411f4ba83033dedb9563f9a749b Mon Sep 17 00:00:00 2001 From: Vadim Tkachenko Date: Sat, 2 Sep 2023 20:46:15 -0700 Subject: [PATCH 2/8] HeatPump reactive overhaul, *almost* passes all tests (#271) --- .../view/http/gae/v3/HttpConnectorGAE.java | 6 +- .../v3/HvacDeviceMetricsConverter.java | 7 +- .../device/actuator/AbstractHvacDevice.java | 53 +-- .../dz3r/device/actuator/AbstractSwitch.java | 37 +- .../net/sf/dz3r/device/actuator/HeatPump.java | 408 +++++++++-------- .../sf/dz3r/device/actuator/NullSwitch.java | 4 + .../device/actuator/SwitchableHvacDevice.java | 30 +- .../sf/dz3r/signal/hvac/HvacDeviceStatus.java | 19 +- .../sf/dz3r/device/actuator/HeatPumpTest.java | 412 ++++++++---------- .../dz3r/device/actuator/ReconcilerTest.java | 127 ++++++ .../actuator/SwitchableHvacDeviceTest.java | 83 ++-- .../actuator/pi/autohat/HeatPumpHAT.java | 50 ++- .../dz3r/view/swing/EntitySelectorPanel.java | 4 +- .../net/sf/dz3r/view/swing/unit/UnitCell.java | 6 +- .../sf/dz3r/view/swing/unit/UnitPanel.java | 2 +- 15 files changed, 647 insertions(+), 601 deletions(-) create mode 100644 dz3r-model/src/test/java/net/sf/dz3r/device/actuator/ReconcilerTest.java diff --git a/dz3r-http-gae/src/main/java/net/sf/dz3r/view/http/gae/v3/HttpConnectorGAE.java b/dz3r-http-gae/src/main/java/net/sf/dz3r/view/http/gae/v3/HttpConnectorGAE.java index dba32697c..3168b9a3e 100644 --- a/dz3r-http-gae/src/main/java/net/sf/dz3r/view/http/gae/v3/HttpConnectorGAE.java +++ b/dz3r-http-gae/src/main/java/net/sf/dz3r/view/http/gae/v3/HttpConnectorGAE.java @@ -76,12 +76,12 @@ public void connect(UnitDirector.Feed feed) { // Zones and zone controller have no business knowing about HVAC mode; inject it var modeFlux = feed.hvacDeviceFlux .doOnNext(s -> { - if (s.getValue().requested.mode == null) { + if (s.getValue().command.mode == null) { logger.debug("null hvacMode (normal on startup): {}", s); } }) - .filter(s -> s.getValue().requested.mode != null) - .map(s -> new Signal(s.timestamp, s.getValue().requested.mode, null, s.status, s.error)); + .filter(s -> s.getValue().command.mode != null) + .map(s -> new Signal(s.timestamp, s.getValue().command.mode, null, s.status, s.error)); zoneRenderer.subscribeMode(modeFlux); // Zone ("thermostat" in its terminology) status feed is the only one supported diff --git a/dz3r-influxdb/src/main/java/net/sf/dz3r/view/influxdb/v3/HvacDeviceMetricsConverter.java b/dz3r-influxdb/src/main/java/net/sf/dz3r/view/influxdb/v3/HvacDeviceMetricsConverter.java index a13a42b2b..9e012738f 100644 --- a/dz3r-influxdb/src/main/java/net/sf/dz3r/view/influxdb/v3/HvacDeviceMetricsConverter.java +++ b/dz3r-influxdb/src/main/java/net/sf/dz3r/view/influxdb/v3/HvacDeviceMetricsConverter.java @@ -30,11 +30,10 @@ private Point convert(Signal signal) { if (status != null) { - b.tag("kind", status.kind.toString()); - b.addField("demand", status.requested.demand); - b.addField("fanSpeed", status.requested.fanSpeed); + b.addField("demand", status.command.demand); + b.addField("fanSpeed", status.command.fanSpeed); b.addField("uptimeMillis", Optional.ofNullable(status.uptime).map(Duration::toMillis).orElse(0L)); - Optional.ofNullable(status.requested.mode).ifPresent(m -> b.tag("mode", m.toString())); + Optional.ofNullable(status.command.mode).ifPresent(m -> b.tag("mode", m.toString())); } if (signal.error != null) { diff --git a/dz3r-model/src/main/java/net/sf/dz3r/device/actuator/AbstractHvacDevice.java b/dz3r-model/src/main/java/net/sf/dz3r/device/actuator/AbstractHvacDevice.java index ff46fea04..01d4883ca 100644 --- a/dz3r-model/src/main/java/net/sf/dz3r/device/actuator/AbstractHvacDevice.java +++ b/dz3r-model/src/main/java/net/sf/dz3r/device/actuator/AbstractHvacDevice.java @@ -4,8 +4,8 @@ import net.sf.dz3r.signal.hvac.HvacDeviceStatus; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; -import org.apache.logging.log4j.ThreadContext; import reactor.core.publisher.Flux; +import reactor.core.publisher.Sinks; import java.io.IOException; import java.time.Clock; @@ -25,7 +25,8 @@ public abstract class AbstractHvacDevice implements HvacDevice { private final String name; - private Flux> statusFlux; + private final Sinks.Many> statusSink; + private final Flux> statusFlux; /** * The moment this device turned on, {@code null} if currently off. @@ -41,6 +42,9 @@ protected AbstractHvacDevice(String name) { protected AbstractHvacDevice(Clock clock, String name) { this.clock = clock; this.name = name; + + statusSink = Sinks.many().multicast().onBackpressureBuffer(); + statusFlux = statusSink.asFlux(); } @Override @@ -55,48 +59,13 @@ protected void check(Switch s, String purpose) { } @Override - public final synchronized Flux> getFlux() { - - // VT: NOTE: This whole synchronized thing must be eliminated. + bucket list. - - ThreadContext.push("getFlux#" + Integer.toHexString(hashCode())); - - try { - logger.debug("getFlux(): name={} waiting...", getAddress()); - - while (statusFlux == null) { - try { - wait(); - logger.debug("getFlux(): name={} flux={}", getAddress(), statusFlux); - } catch (InterruptedException ex) { - Thread.currentThread().interrupt(); - throw new IllegalStateException("This shouldn't have happened", ex); - } - } - - logger.debug("getFlux(): name={} DONE", getAddress()); - - // VT: NOTE: Be careful when refactoring this, need correct sharing option here - return statusFlux; - - } finally { - ThreadContext.pop(); - } + public final Flux> getFlux() { + return statusFlux; } - protected final synchronized Flux> setFlux(Flux> source) { - - // VT: NOTE: This whole synchronized thing must be eliminated. + bucket list. - - ThreadContext.push("getFlux#" + Integer.toHexString(hashCode())); - try { - statusFlux = source; - notifyAll(); - logger.debug("setFlux(): name={} notified", getAddress()); - return source; - } finally { - ThreadContext.pop(); - } + protected final void broadcast(Signal signal) { + logger.debug("{}: broadcast: {}", getAddress(), signal); + statusSink.tryEmitNext(signal); } /** diff --git a/dz3r-model/src/main/java/net/sf/dz3r/device/actuator/AbstractSwitch.java b/dz3r-model/src/main/java/net/sf/dz3r/device/actuator/AbstractSwitch.java index 61360e1d7..3878dfb7e 100644 --- a/dz3r-model/src/main/java/net/sf/dz3r/device/actuator/AbstractSwitch.java +++ b/dz3r-model/src/main/java/net/sf/dz3r/device/actuator/AbstractSwitch.java @@ -6,8 +6,8 @@ import org.apache.logging.log4j.Logger; import org.apache.logging.log4j.ThreadContext; import reactor.core.publisher.Flux; -import reactor.core.publisher.FluxSink; import reactor.core.publisher.Mono; +import reactor.core.publisher.Sinks; import reactor.core.scheduler.Scheduler; import reactor.core.scheduler.Schedulers; import reactor.util.annotation.NonNull; @@ -43,8 +43,8 @@ public abstract class AbstractSwitch> implements Switch< */ private final Clock clock; + private Sinks.Many> stateSink; private Flux> stateFlux; - private FluxSink> stateSink; private Boolean lastKnownState; /** @@ -79,6 +79,9 @@ protected AbstractSwitch(@NonNull A address, @Nullable Scheduler scheduler, @Nul this.minDelay = minDelay; this.clock = clock == null ? Clock.systemUTC() : clock; + stateSink = Sinks.many().multicast().onBackpressureBuffer(); + stateFlux = stateSink.asFlux(); + logger.info("{}: created AbstractSwitch({}) with minDelay={}", Integer.toHexString(hashCode()), getAddress(), minDelay); } @@ -89,27 +92,9 @@ public final A getAddress() { @Override public final synchronized Flux> getFlux() { - - if (stateFlux != null) { - return stateFlux; - } - - logger.debug("{}: creating stateFlux:{}", Integer.toHexString(hashCode()), address); - - stateFlux = Flux - .create(this::connect) - .doOnSubscribe(s -> logger.debug("stateFlux:{} subscribed", address)) - .publishOn(Schedulers.boundedElastic()) - .publish() - .autoConnect(); - return stateFlux; } - private void connect(FluxSink> sink) { - this.stateSink = sink; - } - @Override public final Mono setState(boolean state) { @@ -171,17 +156,7 @@ private Boolean limitRate(boolean state) { } private void reportState(Signal signal) { - - if (stateSink == null) { - - // Unless something subscribes, this will be flooding the log - enable for troubleshooting - // logger.warn("stateSink:{} is still null, skipping: {}", address, signal); // NOSONAR - - getFlux(); - return; - } - - stateSink.next(signal); + stateSink.tryEmitNext(signal); } @Override diff --git a/dz3r-model/src/main/java/net/sf/dz3r/device/actuator/HeatPump.java b/dz3r-model/src/main/java/net/sf/dz3r/device/actuator/HeatPump.java index 69c8b0c6b..7aa3ace6e 100644 --- a/dz3r-model/src/main/java/net/sf/dz3r/device/actuator/HeatPump.java +++ b/dz3r-model/src/main/java/net/sf/dz3r/device/actuator/HeatPump.java @@ -5,21 +5,24 @@ import net.sf.dz3r.signal.Signal; import net.sf.dz3r.signal.hvac.HvacCommand; import net.sf.dz3r.signal.hvac.HvacDeviceStatus; +import org.apache.logging.log4j.LogManager; import reactor.core.publisher.Flux; -import reactor.core.publisher.FluxSink; import reactor.core.publisher.Mono; +import reactor.core.scheduler.Schedulers; import java.io.IOException; import java.time.Duration; import java.util.Optional; import java.util.Set; +import static net.sf.dz3r.signal.Signal.Status.FAILURE_TOTAL; + /** * Single stage heatpump, energize to heat. * * Use the reversed {@link #switchMode} for "energize to cool" heat pumps. * - * Initial mode is undefined and must be set by control logic, until that is done, any other commands are refused. + * Initial mode is undefined and must be set by control logic; until that is done, any other commands are refused. * * @author Copyright © Vadim Tkachenko 2001-2021 */ @@ -29,6 +32,7 @@ public class HeatPump extends AbstractHvacDevice { * Default mode change delay. */ private static final Duration DEFAULT_MODE_CHANGE_DELAY = Duration.ofSeconds(10); + private static final Reconciler reconciler = new Reconciler(); private final Switch switchMode; private final Switch switchRunning; @@ -56,13 +60,11 @@ public class HeatPump extends AbstractHvacDevice { * Requested device state. * * All commands fed into {@link #compute(Flux)} will result in error signals until the operating {@link HvacMode} is set. + * All decisions about the device state are made based on this state only - the actual state would be much deferred + * against the requested, and cannot be relied upon. Necessary changes are applied against this variable immediately + * so that subsequent commands in the stream know what they are dealing with. */ - private HvacCommand requested = new HvacCommand(null, null, null); - - /** - * Actual device state. - */ - private HvacCommand actual = new HvacCommand(null, null, null); + private HvacCommand requestedState = new HvacCommand(null, null, null); /** * Create an instance with all straight switches. @@ -153,189 +155,203 @@ public Set getModes() { public Flux> compute(Flux> in) { // Shut off the condenser, let the fan be as is - Flux> init = Flux.just( - new Signal<>(clock.instant(), new HvacCommand(null, 0.0, null)) - ); + var init = Flux.just(new HvacCommand(null, 0.0, null)); - // Shut off everything - Flux> shutdown = Flux.just( - new Signal<>(clock.instant(), new HvacCommand(null, 0.0, 0.0)) - ); - - return setFlux(Flux.concat(init, in, shutdown) + var commands = in .filter(Signal::isOK) - .filter(ignored -> !isClosed()) - .flatMap(s -> Flux.create(sink -> process(s, sink)))); - } - - private void process(Signal signal, FluxSink> sink) { + .map(Signal::getValue) + // We will only ignore incoming commands, but not shutdown + .filter(ignored -> !isClosed()); - try { - - logger.debug("process: {}", signal); - - checkInitialMode(signal); - trySetMode(signal, sink); - setOthers(signal, sink); - - } catch (Throwable t) { // NOSONAR Consequences have been considered - - logger.error("Failed to compute {}", signal, t); - sink.next(new Signal<>(clock.instant(), null, null, Signal.Status.FAILURE_TOTAL, t)); - - } finally { - sink.complete(); - } - - } - - private void checkInitialMode(Signal signal) { - - if (requested.mode == null - && signal.getValue().mode == null - && signal.getValue().demand > 0) { + // Shut off everything + var shutdown = Flux.just(new HvacCommand(null, 0d, 0d)); - throw new IllegalStateException("Can't accept demand > 0 before setting the operating mode, signal: " + signal); - } + return Flux + .concat(init, commands, shutdown) + .publishOn(Schedulers.newSingle("HeatPump(" + getAddress() + ")")) + .flatMap(this::process) + .doOnNext(this::broadcast); } - private void trySetMode(Signal signal, FluxSink> sink) throws IOException { + private Flux> process(HvacCommand command) { - var newMode = signal.getValue().mode; + logger.debug("{}: process: {}", getAddress(), command); - if (signal.getValue().mode == null) { - return; - } + // This is the only condition that gets checked before the requested state is updated - + // this is an invalid update and must be discarded - if (newMode == requested.mode) { - logger.debug("Mode unchanged: {}", newMode); - return; + if (!isModeSet(command)) { + return Flux.just( + new Signal<>( + clock.instant(), + null, + null, + FAILURE_TOTAL, + new IllegalStateException("Demand command issued before mode is set (likely programming error): " + command)) + ); } - logger.info("Mode changing to: {}", signal.getValue().mode); - - // Now careful, need to shut off the condenser (don't care about the fan) and wait to avoid damaging the hardware - // ... but only if it is already running + var change = reconciler.reconcile(getAddress(), requestedState, command); - if (actual.demand != null && actual.demand > 0) { + // This is the only time we touch requested state, otherwise side effects will explode the command pipeline + requestedState = change.command; - logger.info("Shutting off the condenser"); - - var requestedDemand = reconcile(actual, new HvacCommand(null, 0.0, null)); - sink.next( - new Signal<>(clock.instant(), - new HeatpumpStatus( - HvacDeviceStatus.Kind.REQUESTED, - requestedDemand, - actual, - uptime()))); - - setRunning(reverseRunning); - updateUptime(clock.instant(), false); - - // Note, #requested is not set - this is a transition - actual = reconcile(actual, requestedDemand); - - sink.next( - new Signal<>(clock.instant(), - new HeatpumpStatus( - HvacDeviceStatus.Kind.ACTUAL, - requestedDemand, - actual, - uptime()))); - logger.warn("Letting the hardware settle for modeChangeDelay={}", modeChangeDelay); - Mono.delay(modeChangeDelay).block(); - - } else { - logger.debug("Condenser is not running, skipping the pause"); - } + Flux> modeFlux = change.modeChangeRequired ? setMode(command.mode, change.delayRequired) : Flux.empty(); + var stateFlux = setState(command); - requested = reconcile( - actual, - new HvacCommand(newMode, null, null)); - sink.next( - new Signal<>(clock.instant(), - new HeatpumpStatus( - HvacDeviceStatus.Kind.REQUESTED, - requested, - actual, - uptime()))); - setMode((newMode == HvacMode.HEATING) != reverseMode); - actual = reconcile(actual, requested); - sink.next( - new Signal<>(clock.instant(), - new HeatpumpStatus( - HvacDeviceStatus.Kind.ACTUAL, - requested, - actual, - uptime()))); - logger.info("Mode changed to: {}", signal.getValue().mode); + return Flux.concat(modeFlux, stateFlux); } /** - * Reconcile the incoming command with the current state. + * Check if the initial mode set. * + * @param command Incoming command. + * @return {@code true} if the mode is set and we can proceed, {@code false} otherwise + */ + private boolean isModeSet(HvacCommand command) { + return requestedState.mode != null || command.mode != null || command.demand <= 0; + } + + /** + * Issue a command sequence to change the operating mode. No sanity checking is performed. * - * @param previous Previous command. - * @param next Incoming command. * - * @return Command that will actually be executed. + * @param mode New mode to set + * @param needDelay {@code true} if a delay before setting the new mode is required. * - * @throws IllegalArgumentException if the command indicates an unsupported mode, or illegal fan state. + * @return Flux of commands to change the operating mode. */ - private HvacCommand reconcile(HvacCommand previous, HvacCommand next) { + private Flux> setMode(HvacMode mode, boolean needDelay) { - var result = new HvacCommand( - next.mode == null? previous.mode : next.mode, - next.demand == null ? previous.demand : next.demand, - next.fanSpeed == null ? previous.fanSpeed : next.fanSpeed - ); + // May or may not be empty, see comments inside + Flux> condenserOff = needDelay + ? stopCondenser().doOnSubscribe(ignore -> logger.info("{}: mode changing to: {}", getAddress(), mode)) + : Flux.empty(); + var forceMode = forceMode(mode); - logger.debug("Reconcile: {} + {} => {}", previous, next, result); + return Flux + .concat(condenserOff, forceMode) + .doOnComplete(() -> logger.info("{}: mode changed to: {}", getAddress(), mode)); + } - return result; + /** + * Stop the condenser, then sleep for {@link #modeChangeDelay}. + */ + private Flux> stopCondenser() { + + return Flux + .just(new StateCommand(switchRunning, reverseRunning)) + .doOnNext(ignore -> logger.info("{}: stopping the condenser", getAddress())) + .flatMap(this::setState) + .flatMap(ignore -> Mono.create(sink -> { + // Can't afford to just call delayElement() of Flux or Mono, that will change the scheduler + logger.warn("{}: letting the hardware settle for modeChangeDelay={}", getAddress(), modeChangeDelay); + try { + // VT: FIXME: Need to find a lasting solution for this + // For now, this should be fine as long as the output from this flux is used in a sane way. + logger.warn("{}: BLOCKING WAIT FOR {}", getAddress(), modeChangeDelay); + Thread.sleep(modeChangeDelay.toMillis()); + } catch (InterruptedException ex) { + Thread.currentThread().interrupt(); + logger.warn("interrupted, nothing we can do about it", ex); + } finally { + // Ah, screw it + sink.success(true); + } + })) + .map(ignore -> + // If we're here, this means that the operation was carried out successfully + new Signal<>(clock.instant(), + new HvacDeviceStatus( + // Informational only, but still verifiable + reconciler.reconcile( + getAddress(), + requestedState, + new HvacCommand(null, 0.0, null)).command, + uptime())) + ); + } + + /** + * Set the mode, unconditionally. It is expected that all precautions have already been taken. + * + * @param mode Mode to set. + * @return The flux of commands to set the mode. + */ + private Flux> forceMode(HvacMode mode) { + + return Flux + .just(new StateCommand(switchMode, (mode == HvacMode.HEATING) != reverseMode)) + .doOnNext(command -> logger.debug("{}: setting mode={}", getAddress(), command)) + .flatMap(this::setState) + .map(ignore -> + // If we're here, this means that the operation was carried out successfully + new Signal<>(clock.instant(), + new HvacDeviceStatus( + reconciler.reconcile( + getAddress(), + requestedState, + new HvacCommand(mode, null, null)) + .command, + uptime())) + ); + } + + private Mono setState(StateCommand command) { + logger.debug("{}: setState({})={}", getAddress(), command.target, command.state); + return command.target.setState(command.state); } /** * Set the condenser and fan switches to proper positions. * * Note that the fan switch is only set if {@link HvacCommand#fanSpeed} is not {@code null}, - * but {@link HvacCommand#demand} is expected to have a valida value. + * but {@link HvacCommand#demand} is expected to have a valid value. * - * @param signal Signal to set the state according to. - * @param sink Sink to report hardware status to. - * @throws IOException if there was a problem talking to switch hardware. + * @param command Command to execute. */ - private void setOthers(Signal signal, FluxSink> sink) throws IOException { - - var command = signal.getValue(); - var requestedOperation = reconcile( - actual, - new HvacCommand(null, command.demand, command.fanSpeed)); - sink.next( - new Signal<>(clock.instant(), - new HeatpumpStatus( - HvacDeviceStatus.Kind.REQUESTED, - requestedOperation, - actual, - uptime()))); - - setRunning((requestedOperation.demand > 0) != reverseRunning); - updateUptime(clock.instant(), requestedOperation.demand > 0); + private Flux> setState(HvacCommand command) { + + var requestedOperation = reconciler.reconcile( + getAddress(), + requestedState, + new HvacCommand(null, command.demand, command.fanSpeed)) + .command; + + Flux runningFlux; + if (requestedOperation.demand != null) { + var running = (requestedOperation.demand > 0) != reverseRunning; + runningFlux = Flux + .just(new StateCommand(switchRunning, running)) + .flatMap(this::setState) + .doOnComplete(() -> updateUptime(clock.instant(), requestedOperation.demand > 0)); + } else { + // This will cause no action, but will prompt zip() to do what it is expected to + runningFlux = Flux.just(false); + } + + Flux fanFlux; if (requestedOperation.fanSpeed != null) { - setFan((requestedOperation.fanSpeed > 0) != reverseFan); - updateUptime(clock.instant(), requestedOperation.fanSpeed > 0); + var fan =(requestedOperation.fanSpeed > 0) != reverseFan; + fanFlux = Flux + .just(new StateCommand(switchFan, fan)) + .flatMap(this::setState) + .doOnComplete(() -> updateUptime(clock.instant(), requestedOperation.fanSpeed > 0)); + } else { + // This will cause no action, but will prompt zip() to do what it is expected to + fanFlux = Flux.just(false); } - actual = reconcile(actual, requestedOperation); - - sink.next( - new Signal<>(clock.instant(), - new HeatpumpStatus( - HvacDeviceStatus.Kind.ACTUAL, - requestedOperation, - actual, - uptime()))); + + return Flux + .zip(runningFlux, fanFlux) + .map(pair -> + // If we're here, this means that the operation was carried out successfully + new Signal<>(clock.instant(), + new HvacDeviceStatus( + requestedOperation, + uptime())) + ); } @Override @@ -353,36 +369,80 @@ protected void doClose() throws IOException { logger.warn("Shutting down: {}", getAddress()); + Flux.just( + switchRunning, + switchFan, + switchMode) + .flatMap(s -> s.setState(false)) + .blockLast(); + switchRunning.setState(false).block(); switchFan.setState(false).block(); switchMode.setState(false).block(); logger.info("Shut down: {}", getAddress()); } - public static class HeatpumpStatus extends HvacDeviceStatus { - - public final HvacCommand actual; - - protected HeatpumpStatus(Kind kind, HvacCommand requested, HvacCommand actual, Duration uptime) { - super(kind, requested, uptime); - this.actual = actual; - } - - @Override - public String toString() { - return "{kind=" + kind + ", requested=" + requested + ", actual=" + actual + ", uptime=" + uptime + "}"; - } + @Deprecated + protected Mono setMode(boolean state) { + return switchMode.setState(state); } - protected void setMode(boolean state) throws IOException { // NOSONAR Subclass throws this exception - switchMode.setState(state).block(); + @Deprecated + protected Mono setRunning(boolean state) { + return switchRunning.setState(state); } - protected void setRunning(boolean state) throws IOException { // NOSONAR Subclass throws this exception - switchRunning.setState(state).block(); + @Deprecated + protected Mono setFan(boolean state) { + return switchFanStack.getSwitch("demand").setState(state); } - protected void setFan(boolean state) throws IOException { // NOSONAR Subclass throws this exception - switchFanStack.getSwitch("demand").setState(state).block(); + static class Reconciler { + + record Result( + HvacCommand command, + boolean modeChangeRequired, + boolean delayRequired + ) {} + + /** + * Reconcile the incoming command with the current state. + * + * It is expected that the result will always take place of the {@code previous} argument. + * + * @param name Heat pump name. + * @param previous Previous command. + * @param next Incoming command. + * + * @return Command that will actually be executed, along with mode change flags. + * + * @throws IllegalArgumentException if the command indicates an unsupported mode, or illegal fan state. + */ + public Result reconcile(String name, HvacCommand previous, HvacCommand next) { + + var result = new HvacCommand( + next.mode == null? previous.mode : next.mode, + next.demand == null ? previous.demand : next.demand, + next.fanSpeed == null ? previous.fanSpeed : next.fanSpeed + ); + + var modeChangeRequired = previous.mode != result.mode; + var delayRequired = previous.mode != null && previous.mode != result.mode; + + LogManager.getLogger(HeatPump.class).debug("{}: reconcile: {} + {} => {}", name, previous, next, result); + + // Once set, mode will never go null again if the calling conventions are honored + + if (result.mode == null && result.demand != null && result.demand > 0) { + throw new IllegalArgumentException("positive demand with no mode, programming error: " + result); + } + + return new Result(result, modeChangeRequired, delayRequired); + } } + + private record StateCommand( + Switch target, + boolean state + ) {} } diff --git a/dz3r-model/src/main/java/net/sf/dz3r/device/actuator/NullSwitch.java b/dz3r-model/src/main/java/net/sf/dz3r/device/actuator/NullSwitch.java index 99b171900..8afa7f5c5 100644 --- a/dz3r-model/src/main/java/net/sf/dz3r/device/actuator/NullSwitch.java +++ b/dz3r-model/src/main/java/net/sf/dz3r/device/actuator/NullSwitch.java @@ -112,4 +112,8 @@ private void delay(long delayMillis) { } } + @Override + public String toString() { + return "NullSwitch(" + getAddress() + ")"; + } } diff --git a/dz3r-model/src/main/java/net/sf/dz3r/device/actuator/SwitchableHvacDevice.java b/dz3r-model/src/main/java/net/sf/dz3r/device/actuator/SwitchableHvacDevice.java index bccd0de1a..f0483a870 100644 --- a/dz3r-model/src/main/java/net/sf/dz3r/device/actuator/SwitchableHvacDevice.java +++ b/dz3r-model/src/main/java/net/sf/dz3r/device/actuator/SwitchableHvacDevice.java @@ -7,13 +7,9 @@ import net.sf.dz3r.signal.hvac.HvacDeviceStatus; import reactor.core.publisher.Flux; -import java.time.Duration; import java.util.Optional; import java.util.Set; -import static net.sf.dz3r.signal.hvac.HvacDeviceStatus.Kind.ACTUAL; -import static net.sf.dz3r.signal.hvac.HvacDeviceStatus.Kind.REQUESTED; - /** * A device with just one switch acting as an HVAC device that just supports one mode (either heating or cooling). * @@ -88,11 +84,11 @@ public Set getModes() { @Override public Flux> compute(Flux> in) { - return setFlux(in + return in .filter(Signal::isOK) .flatMap(signal -> { return Flux - .create(sink -> { + .>create(sink -> { try { @@ -106,7 +102,7 @@ public Flux> compute(Flux(clock.instant(), new SwitchStatus(REQUESTED, command, actual, uptime()))); + sink.next(new Signal<>(clock.instant(), new HvacDeviceStatus(command, uptime()))); // By this time, the command has been verified to be valid requested = command; @@ -115,7 +111,7 @@ public Flux> compute(Flux(clock.instant(), complete)); } catch (Throwable t) { // NOSONAR Consequences have been considered @@ -128,7 +124,8 @@ public Flux> compute(FluxVadim Tkachenko 2001-2023 */ -public abstract class HvacDeviceStatus { - public enum Kind { - REQUESTED, - ACTUAL - } +public class HvacDeviceStatus { - public final Kind kind; - public final HvacCommand requested; + /** + * The state requested by the last incoming command that resulted in this update. + */ + public final HvacCommand command; /** * Duration since the device turned on this time, {@code null} if it is currently off. */ public final Duration uptime; - protected HvacDeviceStatus(Kind kind, HvacCommand requested, Duration uptime) { - this.kind = kind; - this.requested = requested; + public HvacDeviceStatus(HvacCommand command, Duration uptime) { + this.command = command; this.uptime = uptime; } @Override public String toString() { - return "{kind=" + kind + ", requested=" + requested + ", uptime=" + uptime + "}"; + return "{requested=" + command + ", uptime=" + uptime + "}"; } } diff --git a/dz3r-model/src/test/java/net/sf/dz3r/device/actuator/HeatPumpTest.java b/dz3r-model/src/test/java/net/sf/dz3r/device/actuator/HeatPumpTest.java index a4ed734bd..17e25514e 100644 --- a/dz3r-model/src/test/java/net/sf/dz3r/device/actuator/HeatPumpTest.java +++ b/dz3r-model/src/test/java/net/sf/dz3r/device/actuator/HeatPumpTest.java @@ -3,7 +3,6 @@ import net.sf.dz3r.model.HvacMode; import net.sf.dz3r.signal.Signal; import net.sf.dz3r.signal.hvac.HvacCommand; -import net.sf.dz3r.signal.hvac.HvacDeviceStatus; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; import reactor.core.publisher.Flux; @@ -22,14 +21,59 @@ static void init() { ReactorDebugAgent.init(); } + /** + * Verify that empty command sequence is executed (implementation will issue initialization and shutdown commands). + */ @Test - void initialMode() { // NOSONAR It's not complex, it's just mundane + void empty() { // NOSONAR It's not complex, it's just mundane var switchMode = new NullSwitch("mode"); var switchRunning = new NullSwitch("running"); var switchFan = new NullSwitch("fan"); - var d = new HeatPump("hp", switchMode, switchRunning, switchFan); + var d = new HeatPump("hp-empty", + switchMode, false, + switchRunning, false, + switchFan, false, + Duration.ofSeconds(1)); + Flux> sequence = Flux.empty(); + + var result = d.compute(sequence).log(); + + StepVerifier + .create(result) + // -- + // Init sequence + .assertNext(e -> { + assertThat(e.getValue().command.mode).isNull(); + assertThat(e.getValue().command.demand).isZero(); + assertThat(e.getValue().command.fanSpeed).isNull(); + }) + // -- + // Shutdown sequence + .assertNext(e -> { + assertThat(e.getValue().command.mode).isNull(); + assertThat(e.getValue().command.demand).isZero(); + assertThat(e.getValue().command.fanSpeed).isZero(); + }) + .verifyComplete(); + } + + /** + * Verify that an attempt to set non-zero demand before the mode is set fails. + */ + @Test + void demandBeforeMode() { // NOSONAR It's not complex, it's just mundane + + var switchMode = new NullSwitch("mode"); + var switchRunning = new NullSwitch("running"); + var switchFan = new NullSwitch("fan"); + + var d = new HeatPump("hp-initial-mode", + switchMode, false, + switchRunning, false, + switchFan, false, + Duration.ofSeconds(1)); var sequence = Flux.just( // This will fail new Signal(Instant.now(), new HvacCommand(null, 0.8, null)), @@ -44,22 +88,9 @@ void initialMode() { // NOSONAR It's not complex, it's just mundane // -- // Init sequence .assertNext(e -> { - // Set demand to zero command - requested - assertThat(e.getValue().kind).isEqualTo(HvacDeviceStatus.Kind.REQUESTED); - assertThat(e.getValue().requested.mode).isNull(); - assertThat(e.getValue().requested.demand).isZero(); - assertThat(e.getValue().requested.fanSpeed).isNull(); - assertThat(((HeatPump.HeatpumpStatus)e.getValue()).actual.demand).isNull(); - assertThat(((HeatPump.HeatpumpStatus)e.getValue()).actual.fanSpeed).isNull(); - }) - .assertNext(e -> { - // Set demand to zero command - actual - assertThat(e.getValue().kind).isEqualTo(HvacDeviceStatus.Kind.ACTUAL); - assertThat(e.getValue().requested.mode).isNull(); - assertThat(e.getValue().requested.demand).isZero(); - assertThat(e.getValue().requested.fanSpeed).isNull(); - assertThat(((HeatPump.HeatpumpStatus)e.getValue()).actual.demand).isZero(); - assertThat(((HeatPump.HeatpumpStatus)e.getValue()).actual.fanSpeed).isNull(); + assertThat(e.getValue().command.mode).isNull(); + assertThat(e.getValue().command.demand).isZero(); + assertThat(e.getValue().command.fanSpeed).isNull(); }) // -- // This shall not pass... @@ -67,63 +98,82 @@ void initialMode() { // NOSONAR It's not complex, it's just mundane assertThat(e.isError()).isTrue(); assertThat(e.error) .isInstanceOf(IllegalStateException.class) - .hasMessageStartingWith("Can't accept demand > 0 before setting the operating mode, signal: "); + .hasMessageStartingWith("Demand command issued before mode is set (likely programming error)"); }) // -- // ...but this will .assertNext(e -> { - // Mode change command - requested - assertThat(e.getValue().kind).isEqualTo(HvacDeviceStatus.Kind.REQUESTED); - assertThat(e.getValue().requested.mode).isEqualTo(HvacMode.COOLING); - assertThat(e.getValue().requested.demand).isZero(); - assertThat(e.getValue().requested.fanSpeed).isNull(); - assertThat(((HeatPump.HeatpumpStatus)e.getValue()).actual.demand).isZero(); - assertThat(((HeatPump.HeatpumpStatus)e.getValue()).actual.fanSpeed).isNull(); + assertThat(e.getValue().command.mode).isEqualTo(HvacMode.COOLING); + assertThat(e.getValue().command.demand).isEqualTo(0.7); + assertThat(e.getValue().command.fanSpeed).isNull(); }) + // -- + // Demand change command - requested .assertNext(e -> { - // Mode change command - actual - assertThat(e.getValue().kind).isEqualTo(HvacDeviceStatus.Kind.ACTUAL); - assertThat(e.getValue().requested.mode).isEqualTo(HvacMode.COOLING); - assertThat(((HeatPump.HeatpumpStatus)e.getValue()).actual.demand).isZero(); - assertThat(e.getValue().requested.fanSpeed).isNull(); - assertThat(((HeatPump.HeatpumpStatus)e.getValue()).actual.demand).isZero(); - assertThat(((HeatPump.HeatpumpStatus)e.getValue()).actual.fanSpeed).isNull(); + assertThat(e.getValue().command.mode).isEqualTo(HvacMode.COOLING); + assertThat(e.getValue().command.demand).isEqualTo(0.7); + assertThat(e.getValue().command.fanSpeed).isNull(); }) // -- + // Shutdown sequence .assertNext(e -> { - // Demand change command - requested - assertThat(e.getValue().kind).isEqualTo(HvacDeviceStatus.Kind.REQUESTED); - assertThat(e.getValue().requested.mode).isEqualTo(HvacMode.COOLING); - assertThat(e.getValue().requested.demand).isEqualTo(0.7); - assertThat(e.getValue().requested.fanSpeed).isNull(); - assertThat(((HeatPump.HeatpumpStatus)e.getValue()).actual.demand).isZero(); - assertThat(((HeatPump.HeatpumpStatus)e.getValue()).actual.fanSpeed).isNull(); + assertThat(e.getValue().command.mode).isEqualTo(HvacMode.COOLING); + assertThat(e.getValue().command.demand).isZero(); + assertThat(e.getValue().command.fanSpeed).isZero(); }) + .verifyComplete(); + } + + /** + * Verify that a single mode change command executes as expected. + */ + @Test + void setMode() { // NOSONAR It's not complex, it's just mundane + + var switchMode = new NullSwitch("mode"); + var switchRunning = new NullSwitch("running"); + var switchFan = new NullSwitch("fan"); + + var d = new HeatPump("hp-change-mode", + switchMode, false, + switchRunning, false, + switchFan, false, + Duration.ofSeconds(1)); + var sequence = Flux.just( + new Signal(Instant.now(), new HvacCommand(HvacMode.HEATING, 0.8, null)) + ); + + var result = d.compute(sequence).log(); + + StepVerifier + .create(result) + // -- + // Init sequence .assertNext(e -> { - // Demand change command - actual - assertThat(e.getValue().kind).isEqualTo(HvacDeviceStatus.Kind.ACTUAL); - assertThat(e.getValue().requested.demand).isEqualTo(0.7); - assertThat(e.getValue().requested.fanSpeed).isNull(); - assertThat(((HeatPump.HeatpumpStatus)e.getValue()).actual.demand).isEqualTo(0.7); - assertThat(((HeatPump.HeatpumpStatus)e.getValue()).actual.fanSpeed).isNull(); + assertThat(e.getValue().command.mode).isNull(); + assertThat(e.getValue().command.demand).isZero(); + assertThat(e.getValue().command.fanSpeed).isNull(); }) // -- + // Set mode to HEATING .assertNext(e -> { - // Shutdown - requested - assertThat(e.getValue().kind).isEqualTo(HvacDeviceStatus.Kind.REQUESTED); - assertThat(e.getValue().requested.mode).isEqualTo(HvacMode.COOLING); - assertThat(e.getValue().requested.demand).isZero(); - assertThat(e.getValue().requested.fanSpeed).isZero(); - assertThat(((HeatPump.HeatpumpStatus)e.getValue()).actual.demand).isEqualTo(0.7); - assertThat(((HeatPump.HeatpumpStatus)e.getValue()).actual.fanSpeed).isNull(); + assertThat(e.getValue().command.mode).isEqualTo(HvacMode.HEATING); + assertThat(e.getValue().command.demand).isEqualTo(0.8); + assertThat(e.getValue().command.fanSpeed).isNull(); }) + // -- + // Turn on the condenser .assertNext(e -> { - // Shutdown - actual - assertThat(e.getValue().kind).isEqualTo(HvacDeviceStatus.Kind.ACTUAL); - assertThat(e.getValue().requested.demand).isZero(); - assertThat(e.getValue().requested.fanSpeed).isZero(); - assertThat(((HeatPump.HeatpumpStatus)e.getValue()).actual.demand).isZero(); - assertThat(((HeatPump.HeatpumpStatus)e.getValue()).actual.fanSpeed).isZero(); + assertThat(e.getValue().command.mode).isEqualTo(HvacMode.HEATING); + assertThat(e.getValue().command.demand).isEqualTo(0.8); + assertThat(e.getValue().command.fanSpeed).isNull(); + }) + // -- + // Shutdown sequence + .assertNext(e -> { + assertThat(e.getValue().command.mode).isEqualTo(HvacMode.HEATING); + assertThat(e.getValue().command.demand).isZero(); + assertThat(e.getValue().command.fanSpeed).isZero(); }) .verifyComplete(); } @@ -135,11 +185,11 @@ void changeMode() { // NOSONAR It's not complex, it's just mundane var switchRunning = new NullSwitch("running"); var switchFan = new NullSwitch("fan"); - var d = new HeatPump("hp", + var d = new HeatPump("hp-change-mode", switchMode, false, switchRunning, false, switchFan, false, - Duration.ofMillis(5)); + Duration.ofSeconds(1)); var sequence = Flux.just( new Signal(Instant.now(), new HvacCommand(HvacMode.HEATING, 0.8, null)), new Signal(Instant.now(), new HvacCommand(HvacMode.COOLING, 0.7, null)) @@ -152,112 +202,51 @@ void changeMode() { // NOSONAR It's not complex, it's just mundane // -- // Init sequence .assertNext(e -> { - // ... - }) - .assertNext(e -> { - // ... + assertThat(e.getValue().command.mode).isNull(); + assertThat(e.getValue().command.demand).isZero(); + assertThat(e.getValue().command.fanSpeed).isNull(); }) // -- + // (heating, 0.8, null) .assertNext(e -> { - // Mode change to heating command, shutting off the condenser - requested - assertThat(e.getValue().kind).isEqualTo(HvacDeviceStatus.Kind.REQUESTED); - assertThat(e.getValue().requested.mode).isEqualTo(HvacMode.HEATING); - assertThat(e.getValue().requested.demand).isZero(); - assertThat(e.getValue().requested.fanSpeed).isNull(); - assertThat(((HeatPump.HeatpumpStatus)e.getValue()).actual.demand).isZero(); - assertThat(((HeatPump.HeatpumpStatus)e.getValue()).actual.fanSpeed).isNull(); - }) - .assertNext(e -> { - // Mode change to heating command, shutting off the condenser - actual - assertThat(e.getValue().kind).isEqualTo(HvacDeviceStatus.Kind.ACTUAL); - assertThat(e.getValue().requested.mode).isEqualTo(HvacMode.HEATING); - assertThat(e.getValue().requested.demand).isZero(); - assertThat(e.getValue().requested.fanSpeed).isNull(); - assertThat(((HeatPump.HeatpumpStatus)e.getValue()).actual.demand).isZero(); - assertThat(((HeatPump.HeatpumpStatus)e.getValue()).actual.fanSpeed).isNull(); + assertThat(e.getValue().command.mode).isEqualTo(HvacMode.HEATING); + assertThat(e.getValue().command.demand).isEqualTo(0.8); + assertThat(e.getValue().command.fanSpeed).isNull(); }) // -- + // VT: FIXME: Why twice? .assertNext(e -> { - // Setting the demand after the delay - requested - assertThat(e.getValue().kind).isEqualTo(HvacDeviceStatus.Kind.REQUESTED); - assertThat(e.getValue().requested.mode).isEqualTo(HvacMode.HEATING); - assertThat(e.getValue().requested.demand).isEqualTo(0.8); - assertThat(e.getValue().requested.fanSpeed).isNull(); - assertThat(((HeatPump.HeatpumpStatus)e.getValue()).actual.demand).isZero(); - assertThat(((HeatPump.HeatpumpStatus)e.getValue()).actual.fanSpeed).isNull(); - }) - .assertNext(e -> { - // Setting the demand after the delay - actual - assertThat(e.getValue().kind).isEqualTo(HvacDeviceStatus.Kind.ACTUAL); - assertThat(e.getValue().requested.mode).isEqualTo(HvacMode.HEATING); - assertThat(e.getValue().requested.demand).isEqualTo(0.8); - assertThat(e.getValue().requested.fanSpeed).isNull(); - assertThat(((HeatPump.HeatpumpStatus)e.getValue()).actual.demand).isEqualTo(0.8); - assertThat(((HeatPump.HeatpumpStatus)e.getValue()).actual.fanSpeed).isNull(); + assertThat(e.getValue().command.mode).isEqualTo(HvacMode.HEATING); + assertThat(e.getValue().command.demand).isEqualTo(0.8); + assertThat(e.getValue().command.fanSpeed).isNull(); }) // -- + // (cooling, 0.7, null) .assertNext(e -> { // Mode change to cooling command, shutting off the condenser - requested - assertThat(e.getValue().kind).isEqualTo(HvacDeviceStatus.Kind.REQUESTED); - // Still heating... - assertThat(e.getValue().requested.mode).isEqualTo(HvacMode.HEATING); - // ...but zero demand - assertThat(e.getValue().requested.demand).isZero(); - assertThat(e.getValue().requested.fanSpeed).isNull(); - assertThat(((HeatPump.HeatpumpStatus)e.getValue()).actual.demand).isEqualTo(0.8); - assertThat(((HeatPump.HeatpumpStatus)e.getValue()).actual.fanSpeed).isNull(); - }) - .assertNext(e -> { - // Mode change to cooling command, shutting off the condenser - actual - assertThat(e.getValue().kind).isEqualTo(HvacDeviceStatus.Kind.ACTUAL); - assertThat(e.getValue().requested.demand).isZero(); - assertThat(e.getValue().requested.fanSpeed).isNull(); - assertThat(((HeatPump.HeatpumpStatus)e.getValue()).actual.demand).isZero(); - assertThat(((HeatPump.HeatpumpStatus)e.getValue()).actual.fanSpeed).isNull(); - }) - // -- - .assertNext(e -> { - // The actual mode change to cooling command - requested - assertThat(e.getValue().kind).isEqualTo(HvacDeviceStatus.Kind.REQUESTED); - assertThat(e.getValue().requested.mode).isEqualTo(HvacMode.COOLING); - assertThat(e.getValue().requested.demand).isZero(); - assertThat(e.getValue().requested.fanSpeed).isNull(); - assertThat(((HeatPump.HeatpumpStatus)e.getValue()).actual.demand).isZero(); - assertThat(((HeatPump.HeatpumpStatus)e.getValue()).actual.fanSpeed).isNull(); - }) - .assertNext(e -> { - // The actual mode change to cooling command - actual - assertThat(e.getValue().kind).isEqualTo(HvacDeviceStatus.Kind.ACTUAL); - assertThat(e.getValue().requested.demand).isZero(); - assertThat(e.getValue().requested.fanSpeed).isNull(); - assertThat(((HeatPump.HeatpumpStatus)e.getValue()).actual.demand).isZero(); - assertThat(((HeatPump.HeatpumpStatus)e.getValue()).actual.fanSpeed).isNull(); + assertThat(e.getValue().command.mode).isEqualTo(HvacMode.COOLING); + assertThat(e.getValue().command.demand).isZero(); + assertThat(e.getValue().command.fanSpeed).isNull(); }) // -- .assertNext(e -> { - // Cooling demand - requested - assertThat(e.getValue().kind).isEqualTo(HvacDeviceStatus.Kind.REQUESTED); - assertThat(e.getValue().requested.mode).isEqualTo(HvacMode.COOLING); - assertThat(e.getValue().requested.demand).isEqualTo(0.7); - assertThat(e.getValue().requested.fanSpeed).isNull(); - assertThat(((HeatPump.HeatpumpStatus)e.getValue()).actual.demand).isZero(); - assertThat(((HeatPump.HeatpumpStatus)e.getValue()).actual.fanSpeed).isNull(); + // ... and set the demand + assertThat(e.getValue().command.mode).isEqualTo(HvacMode.COOLING); + assertThat(e.getValue().command.demand).isEqualTo(0.7); + assertThat(e.getValue().command.fanSpeed).isNull(); }) .assertNext(e -> { - // Cooling demand - actual - assertThat(e.getValue().kind).isEqualTo(HvacDeviceStatus.Kind.ACTUAL); - assertThat(e.getValue().requested.demand).isEqualTo(0.7); - assertThat(e.getValue().requested.fanSpeed).isNull(); - assertThat(((HeatPump.HeatpumpStatus)e.getValue()).actual.demand).isEqualTo(0.7); - assertThat(((HeatPump.HeatpumpStatus)e.getValue()).actual.fanSpeed).isNull(); + // VT: FIXME: Why twice? + assertThat(e.getValue().command.mode).isEqualTo(HvacMode.COOLING); + assertThat(e.getValue().command.demand).isEqualTo(0.7); + assertThat(e.getValue().command.fanSpeed).isNull(); }) // -- - // Shutdown - .assertNext(e -> { - // ... - }) + // Shutdown sequence .assertNext(e -> { - // ... + assertThat(e.getValue().command.mode).isEqualTo(HvacMode.COOLING); + assertThat(e.getValue().command.demand).isZero(); + assertThat(e.getValue().command.fanSpeed).isZero(); }) .verifyComplete(); } @@ -272,11 +261,15 @@ void boot() { // NOSONAR It's not complex, it's just mundane var switchRunning = new NullSwitch("running"); var switchFan = new NullSwitch("fan"); - var d = new HeatPump("hp", switchMode, switchRunning, switchFan); + var d = new HeatPump("hp-boot", + switchMode, false, + switchRunning, false, + switchFan, false, + Duration.ofSeconds(1)); var sequence = Flux.just( - new Signal(Instant.now(), new HvacCommand(null, 0.0, null)), + new Signal(Instant.now(), new HvacCommand(null, 0d, null)), new Signal(Instant.now(), new HvacCommand(HvacMode.COOLING, null, null)), - new Signal(Instant.now(), new HvacCommand(null, 1.0, 1.0)) + new Signal(Instant.now(), new HvacCommand(null, 1d, 1d)) ); var result = d.compute(sequence).log(); @@ -284,108 +277,45 @@ void boot() { // NOSONAR It's not complex, it's just mundane StepVerifier .create(result) // -- - // Shut off the condenser - .assertNext(e -> { - assertThat(e.getValue().kind).isEqualTo(HvacDeviceStatus.Kind.REQUESTED); - assertThat(e.getValue().requested.mode).isNull(); - assertThat(e.getValue().requested.demand).isZero(); - assertThat(e.getValue().requested.fanSpeed).isNull(); - assertThat(((HeatPump.HeatpumpStatus)e.getValue()).actual.demand).isNull(); - assertThat(((HeatPump.HeatpumpStatus)e.getValue()).actual.fanSpeed).isNull(); - }) + // Init sequence .assertNext(e -> { - assertThat(e.getValue().kind).isEqualTo(HvacDeviceStatus.Kind.ACTUAL); - assertThat(e.getValue().requested.mode).isNull(); - assertThat(e.getValue().requested.demand).isZero(); - assertThat(e.getValue().requested.fanSpeed).isNull(); - assertThat(((HeatPump.HeatpumpStatus)e.getValue()).actual.demand).isZero(); - assertThat(((HeatPump.HeatpumpStatus)e.getValue()).actual.fanSpeed).isNull(); + assertThat(e.getValue().command.mode).isNull(); + assertThat(e.getValue().command.demand).isZero(); + assertThat(e.getValue().command.fanSpeed).isNull(); }) // -- - // Switch the mode to COOLING + // (null, 0d, null) .assertNext(e -> { - assertThat(e.getValue().kind).isEqualTo(HvacDeviceStatus.Kind.REQUESTED); - assertThat(e.getValue().requested.mode).isNull(); - assertThat(e.getValue().requested.demand).isZero(); - assertThat(e.getValue().requested.fanSpeed).isNull(); - assertThat(((HeatPump.HeatpumpStatus)e.getValue()).actual.demand).isZero(); - assertThat(((HeatPump.HeatpumpStatus)e.getValue()).actual.fanSpeed).isNull(); - }) - .assertNext(e -> { - assertThat(e.getValue().kind).isEqualTo(HvacDeviceStatus.Kind.ACTUAL); - assertThat(e.getValue().requested.mode).isNull(); - assertThat(e.getValue().requested.demand).isZero(); - assertThat(e.getValue().requested.fanSpeed).isNull(); - assertThat(((HeatPump.HeatpumpStatus)e.getValue()).actual.demand).isZero(); - assertThat(((HeatPump.HeatpumpStatus)e.getValue()).actual.fanSpeed).isNull(); + assertThat(e.getValue().command.mode).isNull(); + assertThat(e.getValue().command.demand).isZero(); + assertThat(e.getValue().command.fanSpeed).isNull(); }) // -- - // Start working - .assertNext(e -> { - assertThat(e.getValue().kind).isEqualTo(HvacDeviceStatus.Kind.REQUESTED); - assertThat(e.getValue().requested.mode).isEqualTo(HvacMode.COOLING); - assertThat(e.getValue().requested.demand).isZero(); - assertThat(e.getValue().requested.fanSpeed).isNull(); - assertThat(((HeatPump.HeatpumpStatus)e.getValue()).actual.demand).isZero(); - assertThat(((HeatPump.HeatpumpStatus)e.getValue()).actual.fanSpeed).isNull(); - }) - .assertNext(e -> { - assertThat(e.getValue().kind).isEqualTo(HvacDeviceStatus.Kind.ACTUAL); - assertThat(e.getValue().requested.mode).isEqualTo(HvacMode.COOLING); - assertThat(e.getValue().requested.demand).isZero(); - assertThat(e.getValue().requested.fanSpeed).isNull(); - assertThat(((HeatPump.HeatpumpStatus)e.getValue()).actual.demand).isZero(); - assertThat(((HeatPump.HeatpumpStatus)e.getValue()).actual.fanSpeed).isNull(); - }) + // (cooling, null, null) => (cooling, 0d, null) because reconcile() + // Switch the mode to COOLING without delays .assertNext(e -> { - assertThat(e.getValue().kind).isEqualTo(HvacDeviceStatus.Kind.REQUESTED); - assertThat(e.getValue().requested.mode).isEqualTo(HvacMode.COOLING); - assertThat(e.getValue().requested.demand).isZero(); - assertThat(e.getValue().requested.fanSpeed).isNull(); - assertThat(((HeatPump.HeatpumpStatus)e.getValue()).actual.demand).isZero(); - assertThat(((HeatPump.HeatpumpStatus)e.getValue()).actual.fanSpeed).isNull(); + assertThat(e.getValue().command.mode).isEqualTo(HvacMode.COOLING); + assertThat(e.getValue().command.demand).isZero(); + assertThat(e.getValue().command.fanSpeed).isNull(); }) + // VT: FIXME: Why twice? .assertNext(e -> { - assertThat(e.getValue().kind).isEqualTo(HvacDeviceStatus.Kind.ACTUAL); - assertThat(e.getValue().requested.mode).isEqualTo(HvacMode.COOLING); - assertThat(e.getValue().requested.demand).isZero(); - assertThat(e.getValue().requested.fanSpeed).isNull(); - assertThat(((HeatPump.HeatpumpStatus)e.getValue()).actual.demand).isZero(); - assertThat(((HeatPump.HeatpumpStatus)e.getValue()).actual.fanSpeed).isNull(); - }) - .assertNext(e -> { - assertThat(e.getValue().kind).isEqualTo(HvacDeviceStatus.Kind.REQUESTED); - assertThat(e.getValue().requested.mode).isEqualTo(HvacMode.COOLING); - assertThat(e.getValue().requested.demand).isEqualTo(1.0); - assertThat(e.getValue().requested.fanSpeed).isEqualTo(1.0); - assertThat(((HeatPump.HeatpumpStatus)e.getValue()).actual.demand).isZero(); - assertThat(((HeatPump.HeatpumpStatus)e.getValue()).actual.fanSpeed).isNull(); - }) - .assertNext(e -> { - assertThat(e.getValue().kind).isEqualTo(HvacDeviceStatus.Kind.ACTUAL); - assertThat(e.getValue().requested.mode).isEqualTo(HvacMode.COOLING); - assertThat(e.getValue().requested.demand).isEqualTo(1.0); - assertThat(e.getValue().requested.fanSpeed).isEqualTo(1.0); - assertThat(((HeatPump.HeatpumpStatus)e.getValue()).actual.demand).isEqualTo(1.0); - assertThat(((HeatPump.HeatpumpStatus)e.getValue()).actual.fanSpeed).isEqualTo(1.0); + assertThat(e.getValue().command.mode).isEqualTo(HvacMode.COOLING); + assertThat(e.getValue().command.demand).isZero(); + assertThat(e.getValue().command.fanSpeed).isNull(); }) // -- - // Shut down .assertNext(e -> { - assertThat(e.getValue().kind).isEqualTo(HvacDeviceStatus.Kind.REQUESTED); - assertThat(e.getValue().requested.mode).isEqualTo(HvacMode.COOLING); - assertThat(e.getValue().requested.demand).isZero(); - assertThat(e.getValue().requested.fanSpeed).isZero(); - assertThat(((HeatPump.HeatpumpStatus)e.getValue()).actual.demand).isEqualTo(1.0); - assertThat(((HeatPump.HeatpumpStatus)e.getValue()).actual.fanSpeed).isEqualTo(1.0); + assertThat(e.getValue().command.mode).isEqualTo(HvacMode.COOLING); + assertThat(e.getValue().command.demand).isEqualTo(1.0); + assertThat(e.getValue().command.fanSpeed).isEqualTo(1.0); }) + // -- + // Shutdown sequence .assertNext(e -> { - assertThat(e.getValue().kind).isEqualTo(HvacDeviceStatus.Kind.ACTUAL); - assertThat(e.getValue().requested.mode).isEqualTo(HvacMode.COOLING); - assertThat(e.getValue().requested.demand).isZero(); - assertThat(e.getValue().requested.fanSpeed).isZero(); - assertThat(((HeatPump.HeatpumpStatus)e.getValue()).actual.demand).isZero(); - assertThat(((HeatPump.HeatpumpStatus)e.getValue()).actual.fanSpeed).isZero(); + assertThat(e.getValue().command.mode).isEqualTo(HvacMode.COOLING); + assertThat(e.getValue().command.demand).isZero(); + assertThat(e.getValue().command.fanSpeed).isZero(); }) .verifyComplete(); } diff --git a/dz3r-model/src/test/java/net/sf/dz3r/device/actuator/ReconcilerTest.java b/dz3r-model/src/test/java/net/sf/dz3r/device/actuator/ReconcilerTest.java new file mode 100644 index 000000000..bc90ab77e --- /dev/null +++ b/dz3r-model/src/test/java/net/sf/dz3r/device/actuator/ReconcilerTest.java @@ -0,0 +1,127 @@ +package net.sf.dz3r.device.actuator; + +import net.sf.dz3r.model.HvacMode; +import net.sf.dz3r.signal.hvac.HvacCommand; +import org.junit.jupiter.api.Test; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatExceptionOfType; + +class ReconcilerTest { + + private final HeatPump.Reconciler reconciler = new HeatPump.Reconciler(); + + /** + * Fail. + */ + @Test + void nullToDemand() { + + var prev = new HvacCommand(null, null, null); + var next = new HvacCommand(null, 1d, null); + + assertThatExceptionOfType(IllegalArgumentException.class) + .isThrownBy(() -> { + reconciler.reconcile("null-to-demand", prev, next); + }); + } + + @Test + void modeToNull() { + + var prev = new HvacCommand(HvacMode.HEATING, 1d, null); + var next = new HvacCommand(null, 1d, null); + var result = reconciler.reconcile("mode-to-null", prev, next); + + assertThat(result.command().mode).isEqualTo(HvacMode.HEATING); + assertThat(result.command().demand).isEqualTo(1d); + assertThat(result.command().fanSpeed).isNull(); + assertThat(result.modeChangeRequired()).isFalse(); + assertThat(result.delayRequired()).isFalse(); + } + + /** + * Mode change is required, delay is not. + */ + @Test + void nullToHeating() { + + var prev = new HvacCommand(null, null, null); + var next = new HvacCommand(HvacMode.HEATING, 1d, null); + var result = reconciler.reconcile("null-to-heating", prev, next); + + assertThat(result.command().mode).isEqualTo(HvacMode.HEATING); + assertThat(result.command().demand).isEqualTo(1d); + assertThat(result.command().fanSpeed).isNull(); + assertThat(result.modeChangeRequired()).isTrue(); + assertThat(result.delayRequired()).isFalse(); + } + + /** + * Both mode change and delay are required. + */ + @Test + void coolingToHeating() { + + var prev = new HvacCommand(HvacMode.COOLING, null, null); + var next = new HvacCommand(HvacMode.HEATING, 1d, null); + var result = reconciler.reconcile("cooling-to-heating", prev, next); + + assertThat(result.command().mode).isEqualTo(HvacMode.HEATING); + assertThat(result.command().demand).isEqualTo(1d); + assertThat(result.command().fanSpeed).isNull(); + assertThat(result.modeChangeRequired()).isTrue(); + assertThat(result.delayRequired()).isTrue(); + } + + /** + * Fan by itself is OK. + */ + @Test + void nullToFan() { + + var prev = new HvacCommand(null, null, null); + var next = new HvacCommand(null, null, 1d); + var result = reconciler.reconcile("null-to-fan", prev, next); + + assertThat(result.command().mode).isNull(); + assertThat(result.command().demand).isNull(); + assertThat(result.command().fanSpeed).isEqualTo(1d); + assertThat(result.modeChangeRequired()).isFalse(); + assertThat(result.delayRequired()).isFalse(); + } + + /** + * Turning off the fan by itself is also OK. + */ + @Test + void fanToZero() { + + var prev = new HvacCommand(null, null, 1d); + var next = new HvacCommand(null, null, 0d); + var result = reconciler.reconcile("fan-to-zero", prev, next); + + assertThat(result.command().mode).isNull(); + assertThat(result.command().demand).isNull(); + assertThat(result.command().fanSpeed).isEqualTo(0d); + assertThat(result.modeChangeRequired()).isFalse(); + assertThat(result.delayRequired()).isFalse(); + } + + /** + * Null means "stay where you were", not "0". + */ + @Test + void anythingToNull() { + + var prev = new HvacCommand(HvacMode.HEATING, 1d, 1d); + var next = new HvacCommand(null, null, null); + var result = reconciler.reconcile("anything-to-null", prev, next); + + assertThat(result.command().mode).isEqualTo(HvacMode.HEATING); + assertThat(result.command().demand).isEqualTo(1d); + assertThat(result.command().fanSpeed).isEqualTo(1d); + assertThat(result.modeChangeRequired()).isFalse(); + assertThat(result.delayRequired()).isFalse(); + } +} diff --git a/dz3r-model/src/test/java/net/sf/dz3r/device/actuator/SwitchableHvacDeviceTest.java b/dz3r-model/src/test/java/net/sf/dz3r/device/actuator/SwitchableHvacDeviceTest.java index 596d4a042..01ac29778 100644 --- a/dz3r-model/src/test/java/net/sf/dz3r/device/actuator/SwitchableHvacDeviceTest.java +++ b/dz3r-model/src/test/java/net/sf/dz3r/device/actuator/SwitchableHvacDeviceTest.java @@ -3,7 +3,6 @@ import net.sf.dz3r.model.HvacMode; import net.sf.dz3r.signal.Signal; import net.sf.dz3r.signal.hvac.HvacCommand; -import net.sf.dz3r.signal.hvac.HvacDeviceStatus; import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; import reactor.core.publisher.Flux; @@ -40,42 +39,30 @@ void lifecycle() { .create(result) .assertNext(e -> { // Actual is not yet set - assertThat(e.getValue().kind).isEqualTo(HvacDeviceStatus.Kind.REQUESTED); - assertThat(e.getValue().requested.demand).isEqualTo(0.8); - assertThat(e.getValue().requested.fanSpeed).isNull(); - assertThat(((SwitchableHvacDevice.SwitchStatus)e.getValue()).actual).isNull(); + assertThat(e.getValue().command.demand).isEqualTo(0.8); + assertThat(e.getValue().command.fanSpeed).isNull(); }) .assertNext(e -> { - assertThat(e.getValue().kind).isEqualTo(HvacDeviceStatus.Kind.ACTUAL); - assertThat(e.getValue().requested.demand).isEqualTo(0.8); - assertThat(e.getValue().requested.fanSpeed).isNull(); - assertThat(((SwitchableHvacDevice.SwitchStatus)e.getValue()).actual).isTrue(); + assertThat(e.getValue().command.demand).isEqualTo(0.8); + assertThat(e.getValue().command.fanSpeed).isNull(); }) // -- .assertNext(e -> { - assertThat(e.getValue().kind).isEqualTo(HvacDeviceStatus.Kind.REQUESTED); - assertThat(e.getValue().requested.demand).isEqualTo(0.5); - assertThat(e.getValue().requested.fanSpeed).isNull(); - assertThat(((SwitchableHvacDevice.SwitchStatus)e.getValue()).actual).isTrue(); + assertThat(e.getValue().command.demand).isEqualTo(0.5); + assertThat(e.getValue().command.fanSpeed).isNull(); }) .assertNext(e -> { - assertThat(e.getValue().kind).isEqualTo(HvacDeviceStatus.Kind.ACTUAL); - assertThat(e.getValue().requested.demand).isEqualTo(0.5); - assertThat(e.getValue().requested.fanSpeed).isNull(); - assertThat(((SwitchableHvacDevice.SwitchStatus)e.getValue()).actual).isTrue(); + assertThat(e.getValue().command.demand).isEqualTo(0.5); + assertThat(e.getValue().command.fanSpeed).isNull(); }) // -- .assertNext(e -> { - assertThat(e.getValue().kind).isEqualTo(HvacDeviceStatus.Kind.REQUESTED); - assertThat(e.getValue().requested.demand).isEqualTo(0.0); - assertThat(e.getValue().requested.fanSpeed).isNull(); - assertThat(((SwitchableHvacDevice.SwitchStatus)e.getValue()).actual).isTrue(); + assertThat(e.getValue().command.demand).isEqualTo(0.0); + assertThat(e.getValue().command.fanSpeed).isNull(); }) .assertNext(e -> { - assertThat(e.getValue().kind).isEqualTo(HvacDeviceStatus.Kind.ACTUAL); - assertThat(e.getValue().requested.demand).isEqualTo(0.0); - assertThat(e.getValue().requested.fanSpeed).isNull(); - assertThat(((SwitchableHvacDevice.SwitchStatus)e.getValue()).actual).isFalse(); + assertThat(e.getValue().command.demand).isEqualTo(0.0); + assertThat(e.getValue().command.fanSpeed).isNull(); }) .verifyComplete(); } @@ -143,12 +130,10 @@ void allowFansForCooling() { StepVerifier .create(result) .assertNext(e -> { - assertThat(e.getValue().requested.fanSpeed).isEqualTo(1.0); - assertThat(((SwitchableHvacDevice.SwitchStatus)e.getValue()).actual).isNull(); + assertThat(e.getValue().command.fanSpeed).isEqualTo(1.0); }) .assertNext(e -> { - assertThat(e.getValue().requested.fanSpeed).isEqualTo(1.0); - assertThat(((SwitchableHvacDevice.SwitchStatus)e.getValue()).actual).isTrue(); + assertThat(e.getValue().command.fanSpeed).isEqualTo(1.0); }) .verifyComplete(); } @@ -198,48 +183,40 @@ void interleave() { .create(result) // Device must turn on .assertNext(e -> { - assertThat(e.getValue().requested.demand).isEqualTo(0.8); - assertThat(e.getValue().requested.fanSpeed).isNull(); - assertThat(((SwitchableHvacDevice.SwitchStatus)e.getValue()).actual).isNull(); + assertThat(e.getValue().command.demand).isEqualTo(0.8); + assertThat(e.getValue().command.fanSpeed).isNull(); }) .assertNext(e -> { - assertThat(e.getValue().requested.demand).isEqualTo(0.8); - assertThat(e.getValue().requested.fanSpeed).isNull(); - assertThat(((SwitchableHvacDevice.SwitchStatus)e.getValue()).actual).isTrue(); + assertThat(e.getValue().command.demand).isEqualTo(0.8); + assertThat(e.getValue().command.fanSpeed).isNull(); }) // Device must stay on .assertNext(e -> { // Requested demand is the previous value - assertThat(e.getValue().requested.demand).isEqualTo(0.8); - assertThat(e.getValue().requested.fanSpeed).isEqualTo(0.5); - assertThat(((SwitchableHvacDevice.SwitchStatus)e.getValue()).actual).isTrue(); + assertThat(e.getValue().command.demand).isEqualTo(0.8); + assertThat(e.getValue().command.fanSpeed).isEqualTo(0.5); }) .assertNext(e -> { - assertThat(e.getValue().requested.demand).isEqualTo(0.8); - assertThat(e.getValue().requested.fanSpeed).isEqualTo(0.5); - assertThat(((SwitchableHvacDevice.SwitchStatus)e.getValue()).actual).isTrue(); + assertThat(e.getValue().command.demand).isEqualTo(0.8); + assertThat(e.getValue().command.fanSpeed).isEqualTo(0.5); }) // Device must still stay on .assertNext(e -> { - assertThat(e.getValue().requested.demand).isEqualTo(0.8); - assertThat(e.getValue().requested.fanSpeed).isZero(); - assertThat(((SwitchableHvacDevice.SwitchStatus)e.getValue()).actual).isTrue(); + assertThat(e.getValue().command.demand).isEqualTo(0.8); + assertThat(e.getValue().command.fanSpeed).isZero(); }) .assertNext(e -> { - assertThat(e.getValue().requested.demand).isEqualTo(0.8); - assertThat(e.getValue().requested.fanSpeed).isZero(); - assertThat(((SwitchableHvacDevice.SwitchStatus)e.getValue()).actual).isTrue(); + assertThat(e.getValue().command.demand).isEqualTo(0.8); + assertThat(e.getValue().command.fanSpeed).isZero(); }) // Device must shut off .assertNext(e -> { - assertThat(e.getValue().requested.demand).isEqualTo(0.0); - assertThat(e.getValue().requested.fanSpeed).isZero(); - assertThat(((SwitchableHvacDevice.SwitchStatus)e.getValue()).actual).isTrue(); + assertThat(e.getValue().command.demand).isEqualTo(0.0); + assertThat(e.getValue().command.fanSpeed).isZero(); }) .assertNext(e -> { - assertThat(e.getValue().requested.demand).isEqualTo(0.0); - assertThat(e.getValue().requested.fanSpeed).isZero(); - assertThat(((SwitchableHvacDevice.SwitchStatus)e.getValue()).actual).isFalse(); + assertThat(e.getValue().command.demand).isEqualTo(0.0); + assertThat(e.getValue().command.fanSpeed).isZero(); }) .verifyComplete(); } diff --git a/dz3r-raspberry-pi/src/main/java/net/sf/dz3r/device/actuator/pi/autohat/HeatPumpHAT.java b/dz3r-raspberry-pi/src/main/java/net/sf/dz3r/device/actuator/pi/autohat/HeatPumpHAT.java index 6db9c827a..2d7a348a9 100644 --- a/dz3r-raspberry-pi/src/main/java/net/sf/dz3r/device/actuator/pi/autohat/HeatPumpHAT.java +++ b/dz3r-raspberry-pi/src/main/java/net/sf/dz3r/device/actuator/pi/autohat/HeatPumpHAT.java @@ -4,6 +4,7 @@ import net.sf.dz3r.device.actuator.HeatPump; import net.sf.dz3r.jmx.JmxAttribute; import net.sf.dz3r.jmx.JmxDescriptor; +import reactor.core.publisher.Mono; import java.io.IOException; @@ -122,25 +123,50 @@ public void setStatusLightsIntensity(byte statusLightsIntensity) throws IOExcept } @Override - protected void setMode(boolean state) throws IOException { - super.setMode(state); - PimoroniAutomationHAT.getInstance().status().warn().intensity().write(statusLightsIntensity); - PimoroniAutomationHAT.getInstance().status().warn().write(state); - logger.debug("mode={}", state); + protected Mono setMode(boolean state) { + + var result = super.setMode(state); + + // VT: FIXME: Temporary solution, let's eat this elephant one bite at a time + try { + PimoroniAutomationHAT.getInstance().status().warn().intensity().write(statusLightsIntensity); + PimoroniAutomationHAT.getInstance().status().warn().write(state); + logger.warn("mode={} - unconfirmed, Mono returned", state); + } catch (IOException ex) { + logger.error("Error setting status lights, ignored", ex); + } + + return result; } @Override - protected void setRunning(boolean state) throws IOException { - super.setRunning(state); - PimoroniAutomationHAT.getInstance().status().comms().write(state); - logger.debug("running={}", state); + protected Mono setRunning(boolean state) { + var result = super.setRunning(state); + + // VT: FIXME: Temporary solution, let's eat this elephant one bite at a time + try { + PimoroniAutomationHAT.getInstance().status().comms().write(state); + logger.debug("running={} - unconfirmed, Mono returned", state); + } catch (IOException ex) { + logger.error("Error setting status lights, ignored", ex); + } + + return result; } @Override - protected void setFan(boolean state) throws IOException { - super.setFan(state); + protected Mono setFan(boolean state) { + var result = super.setFan(state); + + // VT: FIXME: Temporary solution, let's eat this elephant one bite at a time + try { PimoroniAutomationHAT.getInstance().status().power().write(state); - logger.debug("fan={}", state); + logger.debug("fan={} - unconfirmed, Mono returned", state); + } catch (IOException ex) { + logger.error("Error setting status lights, ignored", ex); + } + + return result; } @Override diff --git a/dz3r-swing/src/main/java/net/sf/dz3r/view/swing/EntitySelectorPanel.java b/dz3r-swing/src/main/java/net/sf/dz3r/view/swing/EntitySelectorPanel.java index 83a6aa9a6..4c2828194 100644 --- a/dz3r-swing/src/main/java/net/sf/dz3r/view/swing/EntitySelectorPanel.java +++ b/dz3r-swing/src/main/java/net/sf/dz3r/view/swing/EntitySelectorPanel.java @@ -154,8 +154,8 @@ private CellAndPanel createZonePair( .filter(s -> zoneName.equals(s.payload)) .map(s -> new Signal(s.timestamp, s.getValue(), null, s.status, s.error)); var modeFlux = hvacDeviceFlux - .filter(s -> s.getValue().requested.mode != null) - .map(s -> new Signal(s.timestamp, s.getValue().requested.mode, null, s.status, s.error)); + .filter(s -> s.getValue().command.mode != null) + .map(s -> new Signal(s.timestamp, s.getValue().command.mode, null, s.status, s.error)); var thisScheduleFlux = scheduleFlux .filter(s -> zoneName.equals(s.getKey())) .map(Map.Entry::getValue); diff --git a/dz3r-swing/src/main/java/net/sf/dz3r/view/swing/unit/UnitCell.java b/dz3r-swing/src/main/java/net/sf/dz3r/view/swing/unit/UnitCell.java index fb883c1ac..477c163a2 100644 --- a/dz3r-swing/src/main/java/net/sf/dz3r/view/swing/unit/UnitCell.java +++ b/dz3r-swing/src/main/java/net/sf/dz3r/view/swing/unit/UnitCell.java @@ -26,7 +26,7 @@ protected void paintContent(Graphics2D g2d, Rectangle boundary) { @Override protected Color getBorderColor() { - return ColorScheme.getScheme(getSignal() == null ? null : getSignal().getValue().requested.mode).setpoint; + return ColorScheme.getScheme(getSignal() == null ? null : getSignal().getValue().command.mode).setpoint; } @Override @@ -35,8 +35,8 @@ protected Color getIndicatorColor() { return ColorScheme.offMap.error; } - var mode = getSignal().getValue().requested.mode; - return getSignal().getValue().requested.demand > 0 ? ColorScheme.getScheme(mode).setpoint : ColorScheme.getScheme(mode).setpoint.darker().darker(); + var mode = getSignal().getValue().command.mode; + return getSignal().getValue().command.demand > 0 ? ColorScheme.getScheme(mode).setpoint : ColorScheme.getScheme(mode).setpoint.darker().darker(); } @Override diff --git a/dz3r-swing/src/main/java/net/sf/dz3r/view/swing/unit/UnitPanel.java b/dz3r-swing/src/main/java/net/sf/dz3r/view/swing/unit/UnitPanel.java index 707be7e18..041de70c9 100644 --- a/dz3r-swing/src/main/java/net/sf/dz3r/view/swing/unit/UnitPanel.java +++ b/dz3r-swing/src/main/java/net/sf/dz3r/view/swing/unit/UnitPanel.java @@ -171,7 +171,7 @@ private void displayDemand(Signal signal) { if (signal.isError()) { currentDemandLabel.setText(UNDEFINED); } else { - currentDemandLabel.setText(format.format(signal.getValue().requested.demand)); + currentDemandLabel.setText(format.format(signal.getValue().command.demand)); } } From 966ff60c2f68eec47e335f68f6728933d9501ea8 Mon Sep 17 00:00:00 2001 From: Vadim Tkachenko Date: Wed, 6 Sep 2023 01:51:59 -0700 Subject: [PATCH 3/8] HeatPump reactive overhaul, now passes tests but what else is broken? (#271) --- .../java/net/sf/dz3r/device/actuator/AbstractSwitch.java | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/dz3r-model/src/main/java/net/sf/dz3r/device/actuator/AbstractSwitch.java b/dz3r-model/src/main/java/net/sf/dz3r/device/actuator/AbstractSwitch.java index 3878dfb7e..0a352f42e 100644 --- a/dz3r-model/src/main/java/net/sf/dz3r/device/actuator/AbstractSwitch.java +++ b/dz3r-model/src/main/java/net/sf/dz3r/device/actuator/AbstractSwitch.java @@ -177,8 +177,12 @@ public Mono getState() { } finally { ThreadContext.pop(); } - }) - .subscribeOn(scheduler); + }); + + // VT: NOTE: Having this here breaks stuff, but now that it's gone, + // need a thorough review because likely something else is now broken. + // More: https://github.com/home-climate-control/dz/issues/271 + // .subscribeOn(scheduler); } protected Scheduler getScheduler() { From 32c24bbcfe2940c8538d2bd664dc32eed864fe4f Mon Sep 17 00:00:00 2001 From: Vadim Tkachenko Date: Wed, 6 Sep 2023 23:16:32 -0700 Subject: [PATCH 4/8] Rearranging chairs, dangerously (#271) --- .../net/sf/dz3r/device/actuator/HeatPump.java | 19 ++- .../sf/dz3r/signal/hvac/HvacDeviceStatus.java | 2 +- .../sf/dz3r/device/actuator/HeatPumpTest.java | 115 +++++++++++++++++- 3 files changed, 127 insertions(+), 9 deletions(-) diff --git a/dz3r-model/src/main/java/net/sf/dz3r/device/actuator/HeatPump.java b/dz3r-model/src/main/java/net/sf/dz3r/device/actuator/HeatPump.java index 7aa3ace6e..564a07829 100644 --- a/dz3r-model/src/main/java/net/sf/dz3r/device/actuator/HeatPump.java +++ b/dz3r-model/src/main/java/net/sf/dz3r/device/actuator/HeatPump.java @@ -8,6 +8,7 @@ import org.apache.logging.log4j.LogManager; import reactor.core.publisher.Flux; import reactor.core.publisher.Mono; +import reactor.core.scheduler.Scheduler; import reactor.core.scheduler.Schedulers; import java.io.IOException; @@ -56,6 +57,8 @@ public class HeatPump extends AbstractHvacDevice { private final boolean reverseFan; private final Duration modeChangeDelay; + private final Scheduler scheduler; + /** * Requested device state. * @@ -144,6 +147,8 @@ public HeatPump( logger.warn("using default mode change delay of {}", DEFAULT_MODE_CHANGE_DELAY); return DEFAULT_MODE_CHANGE_DELAY; }); + + scheduler = Schedulers.newSingle("HeatPump(" + getAddress() + ")"); } @Override @@ -168,7 +173,7 @@ public Flux> compute(Flux> setMode(HvacMode mode, boolean need return Flux .concat(condenserOff, forceMode) + .doOnNext(s -> logger.debug("{}: setMode: {}", getAddress(), s.getValue().command)) .doOnComplete(() -> logger.info("{}: mode changed to: {}", getAddress(), mode)); } @@ -243,14 +249,20 @@ private Flux> stopCondenser() { .just(new StateCommand(switchRunning, reverseRunning)) .doOnNext(ignore -> logger.info("{}: stopping the condenser", getAddress())) .flatMap(this::setState) + .doOnNext(ignore -> logger.warn("{}: letting the hardware settle for modeChangeDelay={}", getAddress(), modeChangeDelay)) + + // VT: FIXME: This doesn't work where as it should (see test cases) and allows the next main sequence element to jump ahead, why? +// .delayElements(modeChangeDelay, scheduler) +// .publishOn(scheduler) + .flatMap(ignore -> Mono.create(sink -> { - // Can't afford to just call delayElement() of Flux or Mono, that will change the scheduler - logger.warn("{}: letting the hardware settle for modeChangeDelay={}", getAddress(), modeChangeDelay); + // VT: NOTE: Calling delayElement() of Flux or Mono breaks things, need to figure out why try { // VT: FIXME: Need to find a lasting solution for this // For now, this should be fine as long as the output from this flux is used in a sane way. logger.warn("{}: BLOCKING WAIT FOR {}", getAddress(), modeChangeDelay); Thread.sleep(modeChangeDelay.toMillis()); + logger.warn("{}: blocking wait for {} DONE", getAddress(), modeChangeDelay); } catch (InterruptedException ex) { Thread.currentThread().interrupt(); logger.warn("interrupted, nothing we can do about it", ex); @@ -345,6 +357,7 @@ private Flux> setState(HvacCommand command) { return Flux .zip(runningFlux, fanFlux) + .doOnNext(z -> logger.debug("{}: zip(running, fan) received: ({}, {})", getAddress(), z.getT1(), z.getT2())) .map(pair -> // If we're here, this means that the operation was carried out successfully new Signal<>(clock.instant(), diff --git a/dz3r-model/src/main/java/net/sf/dz3r/signal/hvac/HvacDeviceStatus.java b/dz3r-model/src/main/java/net/sf/dz3r/signal/hvac/HvacDeviceStatus.java index 79136fe9b..e39d4aa08 100644 --- a/dz3r-model/src/main/java/net/sf/dz3r/signal/hvac/HvacDeviceStatus.java +++ b/dz3r-model/src/main/java/net/sf/dz3r/signal/hvac/HvacDeviceStatus.java @@ -28,6 +28,6 @@ public HvacDeviceStatus(HvacCommand command, Duration uptime) { @Override public String toString() { - return "{requested=" + command + ", uptime=" + uptime + "}"; + return "{command=" + command + ", uptime=" + uptime + "}"; } } diff --git a/dz3r-model/src/test/java/net/sf/dz3r/device/actuator/HeatPumpTest.java b/dz3r-model/src/test/java/net/sf/dz3r/device/actuator/HeatPumpTest.java index 17e25514e..ddbfa31cd 100644 --- a/dz3r-model/src/test/java/net/sf/dz3r/device/actuator/HeatPumpTest.java +++ b/dz3r-model/src/test/java/net/sf/dz3r/device/actuator/HeatPumpTest.java @@ -3,9 +3,13 @@ import net.sf.dz3r.model.HvacMode; import net.sf.dz3r.signal.Signal; import net.sf.dz3r.signal.hvac.HvacCommand; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; import reactor.core.publisher.Flux; +import reactor.core.publisher.Mono; +import reactor.core.scheduler.Schedulers; import reactor.test.StepVerifier; import reactor.tools.agent.ReactorDebugAgent; @@ -16,6 +20,9 @@ class HeatPumpTest { + private final Logger logger = LogManager.getLogger(); + private final Duration delay = Duration.ofMillis(500); + @BeforeAll static void init() { ReactorDebugAgent.init(); @@ -35,7 +42,7 @@ void empty() { // NOSONAR It's not complex, it's just mundane switchMode, false, switchRunning, false, switchFan, false, - Duration.ofSeconds(1)); + delay); Flux> sequence = Flux.empty(); var result = d.compute(sequence).log(); @@ -73,7 +80,7 @@ void demandBeforeMode() { // NOSONAR It's not complex, it's just mundane switchMode, false, switchRunning, false, switchFan, false, - Duration.ofSeconds(1)); + delay); var sequence = Flux.just( // This will fail new Signal(Instant.now(), new HvacCommand(null, 0.8, null)), @@ -138,7 +145,7 @@ void setMode() { // NOSONAR It's not complex, it's just mundane switchMode, false, switchRunning, false, switchFan, false, - Duration.ofSeconds(1)); + delay); var sequence = Flux.just( new Signal(Instant.now(), new HvacCommand(HvacMode.HEATING, 0.8, null)) ); @@ -189,7 +196,7 @@ void changeMode() { // NOSONAR It's not complex, it's just mundane switchMode, false, switchRunning, false, switchFan, false, - Duration.ofSeconds(1)); + delay); var sequence = Flux.just( new Signal(Instant.now(), new HvacCommand(HvacMode.HEATING, 0.8, null)), new Signal(Instant.now(), new HvacCommand(HvacMode.COOLING, 0.7, null)) @@ -265,7 +272,7 @@ void boot() { // NOSONAR It's not complex, it's just mundane switchMode, false, switchRunning, false, switchFan, false, - Duration.ofSeconds(1)); + delay); var sequence = Flux.just( new Signal(Instant.now(), new HvacCommand(null, 0d, null)), new Signal(Instant.now(), new HvacCommand(HvacMode.COOLING, null, null)), @@ -319,4 +326,102 @@ void boot() { // NOSONAR It's not complex, it's just mundane }) .verifyComplete(); } + + @Test + void delayElementsFromFlux1() { + + var s = Schedulers.newSingle("single"); + var head = Flux.range(0, 3).publishOn(s); + var detour = Flux.just(42).delayElements(delay, s); + var tail = Flux.range(3, 3); + + var result = Flux.concat( + head, + detour.flatMap(Flux::just), + tail) + .doOnNext(e -> logger.info("{}", e)); + + StepVerifier + .create(result) + .assertNext(e -> assertThat(e).isZero()) + .assertNext(e -> assertThat(e).isEqualTo(1)) + .assertNext(e -> assertThat(e).isEqualTo(2)) + .assertNext(e -> assertThat(e).isEqualTo(42)) + .assertNext(e -> assertThat(e).isEqualTo(3)) + .assertNext(e -> assertThat(e).isEqualTo(4)) + .assertNext(e -> assertThat(e).isEqualTo(5)) + .verifyComplete(); + } + + @Test + void delayElementsFromFlux2() { + + var s = Schedulers.newSingle("single"); + var head = Flux.range(0, 3).publishOn(s); + var detour = Flux.just(42).delayElements(delay); + var tail = Flux.range(3, 3); + + var result = Flux.concat( + head, + detour.flatMap(Flux::just).publishOn(s), + tail) + .doOnNext(e -> logger.info("{}", e)); + + StepVerifier + .create(result) + .assertNext(e -> assertThat(e).isZero()) + .assertNext(e -> assertThat(e).isEqualTo(1)) + .assertNext(e -> assertThat(e).isEqualTo(2)) + .assertNext(e -> assertThat(e).isEqualTo(42)) + .assertNext(e -> assertThat(e).isEqualTo(3)) + .assertNext(e -> assertThat(e).isEqualTo(4)) + .assertNext(e -> assertThat(e).isEqualTo(5)) + .verifyComplete(); + } + + @Test + void delayElementsFromMono1() { + + var s = Schedulers.newSingle("single"); + var head = Flux.range(0, 3).publishOn(s); + var detour = Flux.just(-1).flatMap(ignore -> Mono.just(42)).delayElements(delay, s); + var tail = Flux.range(3, 3); + + var result = Flux.concat(head, detour, tail) + .doOnNext(e -> logger.info("{}", e)); + + StepVerifier + .create(result) + .assertNext(e -> assertThat(e).isZero()) + .assertNext(e -> assertThat(e).isEqualTo(1)) + .assertNext(e -> assertThat(e).isEqualTo(2)) + .assertNext(e -> assertThat(e).isEqualTo(42)) + .assertNext(e -> assertThat(e).isEqualTo(3)) + .assertNext(e -> assertThat(e).isEqualTo(4)) + .assertNext(e -> assertThat(e).isEqualTo(5)) + .verifyComplete(); + } + + @Test + void delayElementsFromMono2() { + + var s = Schedulers.newSingle("single"); + var head = Flux.range(0, 3).publishOn(s); + var detour = Flux.just(-1).flatMap(ignore -> Mono.just(42).delayElement(delay)).publishOn(s); + var tail = Flux.range(3, 3); + + var result = Flux.concat(head, detour, tail) + .doOnNext(e -> logger.info("{}", e)); + + StepVerifier + .create(result) + .assertNext(e -> assertThat(e).isZero()) + .assertNext(e -> assertThat(e).isEqualTo(1)) + .assertNext(e -> assertThat(e).isEqualTo(2)) + .assertNext(e -> assertThat(e).isEqualTo(42)) + .assertNext(e -> assertThat(e).isEqualTo(3)) + .assertNext(e -> assertThat(e).isEqualTo(4)) + .assertNext(e -> assertThat(e).isEqualTo(5)) + .verifyComplete(); + } } From 7fd7f0606f6ff5d34d3d3f545c47f2bb5616f517 Mon Sep 17 00:00:00 2001 From: Vadim Tkachenko Date: Wed, 6 Sep 2023 23:43:06 -0700 Subject: [PATCH 5/8] Brown bag fix (#271) --- .../main/java/net/sf/dz3r/device/actuator/NullSwitch.java | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/dz3r-model/src/main/java/net/sf/dz3r/device/actuator/NullSwitch.java b/dz3r-model/src/main/java/net/sf/dz3r/device/actuator/NullSwitch.java index 8afa7f5c5..3be3dc299 100644 --- a/dz3r-model/src/main/java/net/sf/dz3r/device/actuator/NullSwitch.java +++ b/dz3r-model/src/main/java/net/sf/dz3r/device/actuator/NullSwitch.java @@ -91,10 +91,7 @@ private void delay(long delayMillis) { if (delayMillis > 0) { try { - // This cannot be happening on the main scheduler - it may get blocked under some circumstances. - // The point of using a single scheduler by default is to have the *action* single threaded, - // and this is a delay, not action, so using a different scheduler is fine - Mono.delay(Duration.ofMillis(delayMillis)).subscribeOn(Schedulers.boundedElastic()).block(); + Mono.delay(Duration.ofMillis(delayMillis), getScheduler()).block(); } catch (IllegalStateException ex) { if (ex.getMessage().contains("block()/blockFirst()/blockLast() are blocking, which is not supported in thread")) { logger.warn("{}: delay() on non-blocking thread (name={}, group={}), using Thread.sleep() workaround", From 2e6adb296c4ca35b6ffd4d8cd76cfb73e9ce6641 Mon Sep 17 00:00:00 2001 From: Vadim Tkachenko Date: Thu, 7 Sep 2023 00:20:50 -0700 Subject: [PATCH 6/8] Rearranging more chairs (#271) --- .../net/sf/dz3r/device/actuator/HeatPump.java | 29 +++--- .../sf/dz3r/device/actuator/HeatPumpTest.java | 89 ++++++++++--------- 2 files changed, 65 insertions(+), 53 deletions(-) diff --git a/dz3r-model/src/main/java/net/sf/dz3r/device/actuator/HeatPump.java b/dz3r-model/src/main/java/net/sf/dz3r/device/actuator/HeatPump.java index 564a07829..40f622a3f 100644 --- a/dz3r-model/src/main/java/net/sf/dz3r/device/actuator/HeatPump.java +++ b/dz3r-model/src/main/java/net/sf/dz3r/device/actuator/HeatPump.java @@ -69,18 +69,6 @@ public class HeatPump extends AbstractHvacDevice { */ private HvacCommand requestedState = new HvacCommand(null, null, null); - /** - * Create an instance with all straight switches. - * - * @param name JMX name. - * @param switchMode Switch to pull to change the operating mode. - * @param switchRunning Switch to pull to turn on the compressor. - * @param switchFan Switch to pull to turn on the air handler. - */ - public HeatPump(String name, Switch switchMode, Switch switchRunning, Switch switchFan) { - this(name, switchMode, false, switchRunning, false, switchFan, false); - } - /** * Create an instance with some switches possibly reverse. * @@ -114,6 +102,7 @@ public HeatPump( * @param reverseRunning {@code true} if the "off" running position corresponds to logical one. * @param switchFan Switch to pull to turn on the air handler. * @param reverseFan {@code true} if the "off" fan position corresponds to logical one. + * @param changeModeDelay Delay to observe while changing the {@link HvacMode operating mode}. */ public HeatPump( String name, @@ -121,6 +110,20 @@ public HeatPump( Switch switchRunning, boolean reverseRunning, Switch switchFan, boolean reverseFan, Duration changeModeDelay) { + this(name, + switchMode, reverseMode, + switchRunning, reverseRunning, + switchFan, reverseFan, + changeModeDelay, + Schedulers.newSingle("HeatPump(" + name + ")")); + } + public HeatPump( + String name, + Switch switchMode, boolean reverseMode, + Switch switchRunning, boolean reverseRunning, + Switch switchFan, boolean reverseFan, + Duration changeModeDelay, + Scheduler scheduler) { super(name); @@ -148,7 +151,7 @@ public HeatPump( return DEFAULT_MODE_CHANGE_DELAY; }); - scheduler = Schedulers.newSingle("HeatPump(" + getAddress() + ")"); + this.scheduler = scheduler; } @Override diff --git a/dz3r-model/src/test/java/net/sf/dz3r/device/actuator/HeatPumpTest.java b/dz3r-model/src/test/java/net/sf/dz3r/device/actuator/HeatPumpTest.java index ddbfa31cd..2cb229cfa 100644 --- a/dz3r-model/src/test/java/net/sf/dz3r/device/actuator/HeatPumpTest.java +++ b/dz3r-model/src/test/java/net/sf/dz3r/device/actuator/HeatPumpTest.java @@ -9,6 +9,7 @@ import org.junit.jupiter.api.Test; import reactor.core.publisher.Flux; import reactor.core.publisher.Mono; +import reactor.core.scheduler.Scheduler; import reactor.core.scheduler.Schedulers; import reactor.test.StepVerifier; import reactor.tools.agent.ReactorDebugAgent; @@ -23,26 +24,36 @@ class HeatPumpTest { private final Logger logger = LogManager.getLogger(); private final Duration delay = Duration.ofMillis(500); + private final Scheduler scheduler = Schedulers.newSingle("heatpump-test-single"); + @BeforeAll static void init() { ReactorDebugAgent.init(); } + private SwitchPack getSwitchPack() { + + // VT: NOTE: Might need to use this for running parameterized tests with different schedulers + return new SwitchPack( + new NullSwitch("mode", scheduler), + new NullSwitch("running", scheduler), + new NullSwitch("fan", scheduler) + ); + } + /** * Verify that empty command sequence is executed (implementation will issue initialization and shutdown commands). */ @Test void empty() { // NOSONAR It's not complex, it's just mundane - var switchMode = new NullSwitch("mode"); - var switchRunning = new NullSwitch("running"); - var switchFan = new NullSwitch("fan"); - + var switchPack = getSwitchPack(); var d = new HeatPump("hp-empty", - switchMode, false, - switchRunning, false, - switchFan, false, - delay); + switchPack.mode, false, + switchPack.running, false, + switchPack.fan, false, + delay, + scheduler); Flux> sequence = Flux.empty(); var result = d.compute(sequence).log(); @@ -72,15 +83,13 @@ void empty() { // NOSONAR It's not complex, it's just mundane @Test void demandBeforeMode() { // NOSONAR It's not complex, it's just mundane - var switchMode = new NullSwitch("mode"); - var switchRunning = new NullSwitch("running"); - var switchFan = new NullSwitch("fan"); - + var switchPack = getSwitchPack(); var d = new HeatPump("hp-initial-mode", - switchMode, false, - switchRunning, false, - switchFan, false, - delay); + switchPack.mode, false, + switchPack.running, false, + switchPack.fan, false, + delay, + scheduler); var sequence = Flux.just( // This will fail new Signal(Instant.now(), new HvacCommand(null, 0.8, null)), @@ -137,15 +146,13 @@ void demandBeforeMode() { // NOSONAR It's not complex, it's just mundane @Test void setMode() { // NOSONAR It's not complex, it's just mundane - var switchMode = new NullSwitch("mode"); - var switchRunning = new NullSwitch("running"); - var switchFan = new NullSwitch("fan"); - + var switchPack = getSwitchPack(); var d = new HeatPump("hp-change-mode", - switchMode, false, - switchRunning, false, - switchFan, false, - delay); + switchPack.mode, false, + switchPack.running, false, + switchPack.fan, false, + delay, + scheduler); var sequence = Flux.just( new Signal(Instant.now(), new HvacCommand(HvacMode.HEATING, 0.8, null)) ); @@ -188,15 +195,13 @@ void setMode() { // NOSONAR It's not complex, it's just mundane @Test void changeMode() { // NOSONAR It's not complex, it's just mundane - var switchMode = new NullSwitch("mode"); - var switchRunning = new NullSwitch("running"); - var switchFan = new NullSwitch("fan"); - + var switchPack = getSwitchPack(); var d = new HeatPump("hp-change-mode", - switchMode, false, - switchRunning, false, - switchFan, false, - delay); + switchPack.mode, false, + switchPack.running, false, + switchPack.fan, false, + delay, + scheduler); var sequence = Flux.just( new Signal(Instant.now(), new HvacCommand(HvacMode.HEATING, 0.8, null)), new Signal(Instant.now(), new HvacCommand(HvacMode.COOLING, 0.7, null)) @@ -264,15 +269,13 @@ void changeMode() { // NOSONAR It's not complex, it's just mundane @Test void boot() { // NOSONAR It's not complex, it's just mundane - var switchMode = new NullSwitch("mode"); - var switchRunning = new NullSwitch("running"); - var switchFan = new NullSwitch("fan"); - + var switchPack = getSwitchPack(); var d = new HeatPump("hp-boot", - switchMode, false, - switchRunning, false, - switchFan, false, - delay); + switchPack.mode, false, + switchPack.running, false, + switchPack.fan, false, + delay, + scheduler); var sequence = Flux.just( new Signal(Instant.now(), new HvacCommand(null, 0d, null)), new Signal(Instant.now(), new HvacCommand(HvacMode.COOLING, null, null)), @@ -424,4 +427,10 @@ void delayElementsFromMono2() { .assertNext(e -> assertThat(e).isEqualTo(5)) .verifyComplete(); } + + private record SwitchPack( + Switch mode, + Switch running, + Switch fan + ) {} } From e8f700c0ec14b2f456badec7aafe7b7a55a3ad86 Mon Sep 17 00:00:00 2001 From: Vadim Tkachenko Date: Thu, 7 Sep 2023 00:38:37 -0700 Subject: [PATCH 7/8] Minor non-destructive refactoring (#271, #279) --- .../dz3r/device/actuator/AbstractSwitch.java | 4 +- .../sf/dz3r/device/actuator/NullSwitch.java | 39 ++++++++++++++++++- 2 files changed, 40 insertions(+), 3 deletions(-) diff --git a/dz3r-model/src/main/java/net/sf/dz3r/device/actuator/AbstractSwitch.java b/dz3r-model/src/main/java/net/sf/dz3r/device/actuator/AbstractSwitch.java index 0a352f42e..360f7f269 100644 --- a/dz3r-model/src/main/java/net/sf/dz3r/device/actuator/AbstractSwitch.java +++ b/dz3r-model/src/main/java/net/sf/dz3r/device/actuator/AbstractSwitch.java @@ -59,7 +59,7 @@ public abstract class AbstractSwitch> implements Switch< * @param address Switch address. */ protected AbstractSwitch(@NonNull A address) { - this(address, null, null, null); + this(address, Schedulers.newSingle("switch:" + address, true), null, null); } /** @@ -75,7 +75,7 @@ protected AbstractSwitch(@NonNull A address, @Nullable Scheduler scheduler, @Nul // VT: NOTE: @NonNull seems to have no effect, what enforces it? this.address = HCCObjects.requireNonNull(address,"address can't be null"); - this.scheduler = scheduler == null ? Schedulers.newSingle("switch:" + address, true) : scheduler; + this.scheduler = scheduler; this.minDelay = minDelay; this.clock = clock == null ? Clock.systemUTC() : clock; diff --git a/dz3r-model/src/main/java/net/sf/dz3r/device/actuator/NullSwitch.java b/dz3r-model/src/main/java/net/sf/dz3r/device/actuator/NullSwitch.java index 3be3dc299..80c3c9d91 100644 --- a/dz3r-model/src/main/java/net/sf/dz3r/device/actuator/NullSwitch.java +++ b/dz3r-model/src/main/java/net/sf/dz3r/device/actuator/NullSwitch.java @@ -35,6 +35,15 @@ public NullSwitch(String address) { this(address, 0, 0, Schedulers.newSingle("NullSwitch:" + address, true)); } + /** + * Create an instance without delay running on the provided scheduler. + * + * @param address Address to use. + * @param scheduler Scheduler to use. + */ + public NullSwitch(String address, Scheduler scheduler) { + this(address, 0, 0, scheduler); + } /** * Create an instance with delay. * @@ -91,7 +100,7 @@ private void delay(long delayMillis) { if (delayMillis > 0) { try { - Mono.delay(Duration.ofMillis(delayMillis), getScheduler()).block(); + Mono.delay(Duration.ofMillis(delayMillis), getDelayScheduler()).block(); } catch (IllegalStateException ex) { if (ex.getMessage().contains("block()/blockFirst()/blockLast() are blocking, which is not supported in thread")) { logger.warn("{}: delay() on non-blocking thread (name={}, group={}), using Thread.sleep() workaround", @@ -113,4 +122,32 @@ private void delay(long delayMillis) { public String toString() { return "NullSwitch(" + getAddress() + ")"; } + + /** + * @return A scheduler used only for {@link Mono#delayElement(Duration)}. + */ + private Scheduler getDelayScheduler() { + + // Parallel scheduler is the default for delayElement() anyway + return getScheduler() == null + ? Schedulers.parallel() + : getScheduler(); + } + + @Override + public Mono getState() { + + if (true) { // NOSONAR Shut up. I know. + return super.getState(); + } + + // VT: NOTE: While this is the Reactive compliant solution, currently it breaks at least the HeatPump. + // More: https://github.com/home-climate-control/dz/issues/279 + + if (state == null) { + return Mono.error(new IOException("setStateSync() hasn't been called yet on " + getAddress())); + } + + return Mono.just(state).delayElement(Duration.ofMillis(getDelayMillis()), getDelayScheduler()); + } } From a79e047dbe9ac06dbae86b05149d98735a57218f Mon Sep 17 00:00:00 2001 From: Vadim Tkachenko Date: Thu, 7 Sep 2023 22:17:13 -0700 Subject: [PATCH 8/8] Renamed the variable to match heartbeat/pace pattern (#47, #253) --- .../sf/dz3r/device/actuator/AbstractSwitch.java | 16 ++++++++-------- .../dz3r/device/actuator/AbstractSwitchTest.java | 4 ++-- .../dz3r/device/mqtt/v1/AbstractMqttSwitch.java | 4 ++-- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/dz3r-model/src/main/java/net/sf/dz3r/device/actuator/AbstractSwitch.java b/dz3r-model/src/main/java/net/sf/dz3r/device/actuator/AbstractSwitch.java index 360f7f269..04c875462 100644 --- a/dz3r-model/src/main/java/net/sf/dz3r/device/actuator/AbstractSwitch.java +++ b/dz3r-model/src/main/java/net/sf/dz3r/device/actuator/AbstractSwitch.java @@ -36,7 +36,7 @@ public abstract class AbstractSwitch> implements Switch< * Minimum delay between two subsequent calls to {@link #setStateSync(boolean)} if the value hasn't changed. * See Pipeline overrun with slow actuators. */ - private final Duration minDelay; + private final Duration pace; /** * Clock to use. Doesn't make sense to use a non-default clock other than for testing purposes. @@ -67,22 +67,22 @@ protected AbstractSwitch(@NonNull A address) { * * @param address Switch address. * @param scheduler Scheduler to use. {@code null} means using {@link Schedulers#newSingle(String, boolean)}. - * @param minDelay Minimum delay between sending identical commands to hardware. + * @param pace Issue identical control commands to this switch at most this often. * @param clock Clock to use. Pass {@code null} except when testing. */ - protected AbstractSwitch(@NonNull A address, @Nullable Scheduler scheduler, @Nullable Duration minDelay, @Nullable Clock clock) { + protected AbstractSwitch(@NonNull A address, @Nullable Scheduler scheduler, @Nullable Duration pace, @Nullable Clock clock) { // VT: NOTE: @NonNull seems to have no effect, what enforces it? this.address = HCCObjects.requireNonNull(address,"address can't be null"); this.scheduler = scheduler; - this.minDelay = minDelay; + this.pace = pace; this.clock = clock == null ? Clock.systemUTC() : clock; stateSink = Sinks.many().multicast().onBackpressureBuffer(); stateFlux = stateSink.asFlux(); - logger.info("{}: created AbstractSwitch({}) with minDelay={}", Integer.toHexString(hashCode()), getAddress(), minDelay); + logger.info("{}: created AbstractSwitch({}) with pace={}", Integer.toHexString(hashCode()), getAddress(), pace); } @Override @@ -127,7 +127,7 @@ private Boolean limitRate(boolean state) { try { - if (minDelay == null) { + if (pace == null) { return null; } @@ -141,8 +141,8 @@ private Boolean limitRate(boolean state) { var delay = Duration.between(lastSetAt, clock.instant()); - if (delay.compareTo(minDelay) < 0) { - logger.debug("{}: skipping setState({}), too close ({} of {})", Integer.toHexString(hashCode()), state, delay, minDelay); + if (delay.compareTo(pace) < 0) { + logger.debug("{}: skipping setState({}), too close ({} of {})", Integer.toHexString(hashCode()), state, delay, pace); return lastSetState; } diff --git a/dz3r-model/src/test/java/net/sf/dz3r/device/actuator/AbstractSwitchTest.java b/dz3r-model/src/test/java/net/sf/dz3r/device/actuator/AbstractSwitchTest.java index 30986e686..351f4fbe9 100644 --- a/dz3r-model/src/test/java/net/sf/dz3r/device/actuator/AbstractSwitchTest.java +++ b/dz3r-model/src/test/java/net/sf/dz3r/device/actuator/AbstractSwitchTest.java @@ -109,8 +109,8 @@ private static class TestSwitch extends AbstractSwitch { private Boolean state; public final AtomicInteger counter = new AtomicInteger(0); - protected TestSwitch(String address, Scheduler scheduler, Duration minDelay, Clock clock) { - super(address, scheduler, minDelay, clock); + protected TestSwitch(String address, Scheduler scheduler, Duration pace, Clock clock) { + super(address, scheduler, pace, clock); } @Override diff --git a/dz3r-mqtt/src/main/java/net/sf/dz3r/device/mqtt/v1/AbstractMqttSwitch.java b/dz3r-mqtt/src/main/java/net/sf/dz3r/device/mqtt/v1/AbstractMqttSwitch.java index 30e377181..f56e04697 100644 --- a/dz3r-mqtt/src/main/java/net/sf/dz3r/device/mqtt/v1/AbstractMqttSwitch.java +++ b/dz3r-mqtt/src/main/java/net/sf/dz3r/device/mqtt/v1/AbstractMqttSwitch.java @@ -28,10 +28,10 @@ protected AbstractMqttSwitch( MqttAdapter mqttAdapter, MqttMessageAddress address, Scheduler scheduler, - Duration minDelay, + Duration pace, Clock clock) { - super(address, scheduler, minDelay, clock); + super(address, scheduler, pace, clock); this.mqttAdapter = mqttAdapter; }