Merge branch 'gh321-process-control-error-propagation' into dev (#314, …

…#321)

modules/hcc-model/src/main/java/net/sf/dz3r/controller/AbstractProcessController.java

@@ -7,6 +7,7 @@
 import reactor.core.publisher.FluxSink;
 
 import java.time.Duration;
+import java.util.Optional;
 
 /**
  * Base class for reactive process controllers.
@@ -22,10 +23,10 @@ public abstract class AbstractProcessController<I, O, P> implements ProcessContr
     /**
      * The process setpoint.
      */
-    private double setpoint;
+    private Double setpoint;
 
-    private final Flux<Double> setpointFlux;
-    private FluxSink<Double> setpointSink;
+    private final Flux<Optional<Double>> setpointFlux;
+    private FluxSink<Optional<Double>> setpointSink;
 
     /**
      * The current process variable value.
@@ -43,26 +44,26 @@ public abstract class AbstractProcessController<I, O, P> implements ProcessContr
      * @param jmxName This controller's JMX name.
      * @param setpoint Initial setpoint.
      */
-    protected AbstractProcessController(String jmxName, double setpoint) {
+    protected AbstractProcessController(String jmxName, Double setpoint) {
         this.jmxName = jmxName;
 
         setpointFlux = Flux.create(this::connectSetpoint);
-        setpointFlux.subscribe(s -> this.setpoint = s);
+        setpointFlux.subscribe(s -> this.setpoint = s.orElse(null));
 
         setSetpoint(setpoint);
     }
 
-    private void connectSetpoint(FluxSink<Double> setpointSink) {
+    private void connectSetpoint(FluxSink<Optional<Double>> setpointSink) {
         this.setpointSink = setpointSink;
     }
 
     @Override
-    public void setSetpoint(double setpoint) {
-        setpointSink.next(setpoint);
+    public void setSetpoint(Double setpoint) {
+        setpointSink.next(Optional.ofNullable(setpoint));
     }
 
     @Override
-    public double getSetpoint() {
+    public Double getSetpoint() {
         return setpoint;
     }
 
@@ -72,17 +73,16 @@ public Signal<I, P> getProcessVariable() {
     }
 
     @Override
-    public final synchronized double getError() {
+    public final synchronized Double getError() {
 
-        if (pv == null) {
-            // No sample, no error
-            return 0;
+        if (setpoint == null || pv == null) {
+            return null;
         }
 
         return getError(pv, setpoint);
     }
 
-    protected abstract double getError(Signal<I, P> pv, double setpoint);
+    protected abstract double getError(Signal<I, P> pv, Double setpoint);
 
     /**
      * Get last output signal value.
@@ -100,23 +100,28 @@ public final Flux<Signal<Status<O>, P>> compute(Flux<Signal<I, P>> pv) {
         // Need to re-inject it.
         return Flux.combineLatest(
                 Flux.concat(
-                        Flux.just(setpoint),
+                        Flux.just(Optional.ofNullable(setpoint)),
                         setpointFlux
                 ),
                 pv.doOnComplete(() -> setpointSink.complete()), // or it will hang forever
                 this::compute);
     }
 
-    private Signal<Status<O>, P> compute(Double setpoint, Signal<I, P> pv) {
-
-        if (pv.isError()) {
-
-            // VT: FIXME: Ideally, even the error signal must be passed to wrapCompute() in case it needs to
-            // recalculate the state. In practice, this will have to wait.
+    /**
+     * Compute the controller signal.
+     *
+     * Using {@code Optional} for arguments is an antipattern, but an unavoidable one -
+     * the setpoint is supplied by a {@code Flux} which doesn't support null values, so some kind of wrapper
+     * would've been necessary anyway, so why not use whatever is intended for this specifically.
+     *
+     * @param setpoint Setpoint wrapped into {@code Optional}.
+     * @param pv Process variable.
+     *
+     * @return Computed signal.
+     */
+    private Signal<Status<O>, P> compute(Optional<Double> setpoint, Signal<I, P> pv) {
 
-            // For now, let's throw them a NaN, they better pay attention.
-            return new Signal<>(pv.timestamp, new Status(setpoint, null, Double.NaN), pv.payload, pv.status, pv.error);
-        }
+        // VT: NOTE: https://github.com/home-climate-control/dz/issues/321 - no more "magic numbers"
 
         if (lastOutputSignal != null && lastOutputSignal.timestamp.isAfter(pv.timestamp)) {
             logger.warn("Can't go back in time: last sample was @{}, this is @{}, {}ms difference: {}",
@@ -127,7 +132,7 @@ private Signal<Status<O>, P> compute(Double setpoint, Signal<I, P> pv) {
         }
 
         this.pv = pv;
-        lastOutputSignal = wrapCompute(setpoint, pv);
+        lastOutputSignal = wrapCompute(setpoint.orElse(null), pv);
 
         return lastOutputSignal;
     }

modules/hcc-model/src/main/java/net/sf/dz3r/controller/HysteresisController.java

@@ -120,7 +120,7 @@ public double getThresholdHigh() {
     }
 
     @Override
-    protected double getError(Signal<Double, P> pv, double setpoint) {
+    protected double getError(Signal<Double, P> pv, Double setpoint) {
         return pv.getValue() - setpoint;
     }
 

modules/hcc-model/src/main/java/net/sf/dz3r/controller/ProcessController.java

@@ -22,16 +22,16 @@ public interface ProcessController<I, O, P> extends SignalProcessor<I, ProcessCo
     /**
      * Set the setpoint.
      *
-     * @param setpoint Setpoint to set.
+     * @param setpoint Setpoint to set. {@code null} value is allowed.
      */
-    void setSetpoint(double setpoint);
+    void setSetpoint(Double setpoint);
 
     /**
      * Get the setpoint.
      *
      * @return Current setpoint value.
      */
-    double getSetpoint();
+    Double getSetpoint();
 
     /**
      * Get the process variable.
@@ -43,13 +43,15 @@ public interface ProcessController<I, O, P> extends SignalProcessor<I, ProcessCo
     /**
      * Get the current value of the error.
      *
-     * @return Current error value.
+     * @return Current error value. {@code null} indicates either {@link #getSetpoint() no setpoint}, or no signal.
      */
-    double getError();
+    Double getError();
 
     /**
      * Compute the output signal.
      *
+     * {@code null} {@link #getSetpoint()} or signal will result in an error status.
+     *
      * @param pv Process variable flux.
      *
      * @return Output signal flux. The end of this flux indicates the need for the subscriber to shut down.
@@ -59,11 +61,11 @@ public interface ProcessController<I, O, P> extends SignalProcessor<I, ProcessCo
 
     class Status<T> {
 
-        public final double setpoint;
+        public final Double setpoint;
         public final Double error;
         public final T signal;
 
-        public Status(double setpoint, Double error, T signal) {
+        public Status(Double setpoint, Double error, T signal) {
             this.setpoint = setpoint;
             this.error = error;
             this.signal = signal;

modules/hcc-model/src/main/java/net/sf/dz3r/controller/pid/AbstractPidController.java

@@ -58,7 +58,7 @@ public abstract class AbstractPidController<P> extends AbstractProcessController
      */
     private double lastD = 0;
 
-    protected AbstractPidController(String jmxName, final double setpoint, final double P, final double I, final double D, double saturationLimit) {
+    protected AbstractPidController(String jmxName, final Double setpoint, final double P, final double I, final double D, double saturationLimit) {
 
         super(jmxName, setpoint);
 
@@ -71,8 +71,12 @@ protected AbstractPidController(String jmxName, final double setpoint, final dou
     }
 
     @Override
-    protected double getError(Signal<Double, P> pv, double setpoint) {
-        return pv.getValue() - setpoint;
+    protected double getError(Signal<Double, P> pv, Double setpoint) {
+
+        // VT: NOTE: https://github.com/home-climate-control/dz/issues/321
+        // Until this is proven to be a bad idea, we're assuming that error == 0 for the error input, to recalculate I and especially D correctly.
+        // However, the output signal emitted will still be an error signal - let's not forget about that.
+        return pv.isError() ? 0 : pv.getValue() - setpoint;
     }
 
     @Override
@@ -83,15 +87,14 @@ protected Signal<Status<Double>, P> wrapCompute(Double setpoint, Signal<Double,
         var error = getError(pv, setpoint);
         var p = error * getP();
         lastP = p;
-        var signal = p;
 
         if (saturationLimit == 0) {
             var integral = getIntegral(lastOutputSignal, pv, error);
             lastI = integral * getI();
 
         } else {
 
-            if (lastOutputSignal != null && Math.abs(lastOutputSignal.getValue().signal) < saturationLimit) {
+            if (lastOutputSignal != null && !lastOutputSignal.isError() && Math.abs(lastOutputSignal.getValue().signal) < saturationLimit) {
 
                 // Integral value will only be updated if the output is not saturated
                 var integral = getIntegral(lastOutputSignal, pv, error);
@@ -110,8 +113,6 @@ protected Signal<Status<Double>, P> wrapCompute(Double setpoint, Signal<Double,
             }
         }
 
-        signal += lastI;
-
         var derivative = getDerivative(lastOutputSignal, pv, error);
 
         // One cause of this is setSetpoint(), which causes values
@@ -122,9 +123,10 @@ protected Signal<Status<Double>, P> wrapCompute(Double setpoint, Signal<Double,
                 && Double.compare(derivative, Double.POSITIVE_INFINITY) != 0) {
 
             lastD = derivative * getD();
-            signal += lastD;
         }
 
+        var signal = p + lastI + lastD;
+
         // VT: NOTE: When the hell was it NaN? I know this code wouldn't be
         // here for no reason, but can't remember the circumstances.
         // Need them to write the test case.

modules/hcc-model/src/main/java/net/sf/dz3r/controller/pid/SimplePidController.java

@@ -19,7 +19,7 @@ public class SimplePidController<P> extends AbstractPidController<P> {
      */
     private double lastError = 0;
 
-    public SimplePidController(String jmxName, double setpoint, double P, double I, double D, double saturationLimit) {
+    public SimplePidController(String jmxName, Double setpoint, double P, double I, double D, double saturationLimit) {
         super(jmxName, setpoint, P, I, D, saturationLimit);
     }
 

modules/hcc-model/src/main/java/net/sf/dz3r/device/actuator/economizer/v2/PidEconomizer.java

@@ -60,7 +60,7 @@ public PidEconomizer(
 
         super(clock, name, settings, device, timeout);
 
-        controller = new SimplePidController<>("(controller) " + getAddress(), 0, settings.P, settings.I, 0, settings.saturationLimit);
+        controller = new SimplePidController<>("(controller) " + getAddress(), 0d, settings.P, settings.I, 0, settings.saturationLimit);
         signalRenderer = new HysteresisController<>("(signalRenderer) " + getAddress(), 0, HYSTERESIS);
 
         initFluxes(ambientFlux);

modules/hcc-model/src/main/java/net/sf/dz3r/model/Thermostat.java

@@ -164,8 +164,10 @@ private Signal<Status<CallingStatus>, Void> mapOutput(Signal<Status<Double>, Sta
         var sample = source.getValue() instanceof HysteresisController.HysteresisStatus value
                 ? value.sample
                 : null;
-        var demand = source.payload.signal - signalRenderer.getThresholdLow();
-        var calling = Double.compare(source.getValue().signal, 1.0) == 0;
+
+        // Watch for the error signal
+        var demand = source.isError() ? 0 : source.payload.signal - signalRenderer.getThresholdLow();
+        var calling = !source.isError() && Double.compare(source.getValue().signal, 1.0) == 0;
 
         return new Signal<>(
                 source.timestamp,