relay: consistent pulse requirements and startup

purge timers based on target status, do not check 'armed' state
allow to schedule timer and keep it inactive until relay loop

fixing bug with pulse api not respecting before / after delays.
activate timers when switching to an opposite state.
deactive existing timers when with the same state.

print debug after completing actions, making sure accidental yield would
not change expected state while still modifying it

code/espurna/relay.cpp

@@ -503,17 +503,7 @@ struct Timer {
         _timer.stop();
     }
 
-    void start() {
-        const auto id = _id;
-        const auto status = _status;
-        _timer.once(
-            (_duration.count() > 0)
-                ? _duration
-                : timer::SystemTimer::DurationMin,
-            [id, status]() {
-                relayStatus(id, status);
-            });
-    }
+    void start();
 
 private:
     Duration _duration;
@@ -529,7 +519,10 @@ std::forward_list<Timer> timers;
 
 } // namespace internal
 
-auto find(size_t id) -> decltype(internal::timers.begin()) {
+using Iterator = decltype(internal::timers)::iterator;
+
+// Note that timer list maintains uniqueness, there can be only one instance per relay
+Iterator find(size_t id) {
     return std::find_if(
         internal::timers.begin(),
         internal::timers.end(),
@@ -538,57 +531,61 @@ auto find(size_t id) -> decltype(internal::timers.begin()) {
         });
 }
 
-void trigger(Duration duration, size_t id, bool target) {
-    if (duration.count() == 0) {
-        bool found { false };
-        internal::timers.remove_if(
-            [&](const Timer& timer) {
-                if (id == timer.id()) {
-                    found = true;
-                    return true;
-                }
+template <typename T>
+bool find(size_t id, T&& callback) {
+    auto it = find(id);
+    if (it != internal::timers.end()) {
+        callback(*it);
+        return true;
+    }
 
-                return false;
-            });
+    return false;
+}
 
-        if (found) {
-            DEBUG_MSG_P(PSTR("[RELAY] #%zu pulse stopped\n"), id);
-        }
+void reset(size_t id) {
+    internal::timers.remove_if(
+        [&](const Timer& timer) {
+            return id == timer.id();
+        });
+}
 
-        return;
-    }
+void reset(const Timer& timer) {
+    internal::timers.remove(timer);
+}
 
-    bool rescheduled [[gnu::unused]] { false };
+// Place timer in the queue, which is going to be started when
+// relay changes status to the opposite of the one specified by the timer
+Iterator schedule(Duration duration, size_t id, bool target) {
     auto it = find(id);
     if (it != internal::timers.end()) {
         (*it).update(duration, target);
-        rescheduled = true;
     } else {
         internal::timers.emplace_front(duration, id, target);
         it = internal::timers.begin();
     }
 
-    (*it).start();
+    return it;
+}
 
-    DEBUG_MSG_P(PSTR("[RELAY] #%zu pulse %s %sscheduled in %lu (ms)\n"),
-        id, target ? "ON" : "OFF",
-        rescheduled ? "re" : "",
-        duration.count());
+// Update or create pulse timer and immediately start it
+void trigger(Duration duration, size_t id, bool target) {
+    auto it = schedule(duration, id, target);
+    (*it).start();
 }
 
-// Update the pulse counter when the relay is already in the opposite state (#454)
-void poll(size_t id, bool target) {
+// Restart when the relay is already in the opposite state (#454)
+void restart(size_t id) {
     auto it = find(id);
-    if ((it != internal::timers.end()) && ((*it).status() != target)) {
+    if (it != internal::timers.end()) {
         (*it).start();
     }
 }
 
-void expire() {
-    internal::timers.remove_if([](const Timer& timer) {
-        return !static_cast<bool>(timer)
-            || (relayStatus(timer.id()) == timer.status());
-    });
+void removeCompleted() {
+    internal::timers.remove_if(
+        [](const Timer& timer) {
+            return relayStatus(timer.id()) == timer.status();
+        });
 }
 
 [[gnu::unused]]
@@ -620,6 +617,22 @@ bool isActive(Mode pulse) {
     return pulse != Mode::None;
 }
 
+bool wouldChange(Mode mode, bool status) {
+    return isActive(mode) && !isNormalStatus(mode, status);
+}
+
+void Timer::start() {
+    const auto id = _id;
+    const auto status = _status;
+    _timer.once(
+        (_duration.count() > 0)
+            ? _duration
+            : timer::SystemTimer::DurationMin,
+        [id, status]() {
+            relayStatus(id, status);
+        });
+}
+
 } // namespace
 } // namespace pulse
 
@@ -1663,35 +1676,54 @@ bool _relayHandlePayload(size_t id, espurna::StringView payload) {
 // Initialize pulse timers after ON or OFF event
 // TODO: integrate with scheduled ON or OFF?
 
-bool _relayPulseActive(size_t id, bool status) {
-    using namespace espurna::relay::pulse;
-    if (isActive(_relays[id].pulse)) {
-        return isNormalStatus(_relays[id].pulse, status);
-    }
+void _relayProcessPulse(const Relay& relay, size_t id, bool status) {
+    using namespace espurna::relay;
 
-    return false;
-}
+    bool restarted { false };
+    pulse::find(id,
+        [&](pulse::Timer& timer) {
+            if (timer.status() == status) {
+                pulse::reset(timer);
+                DEBUG_MSG_P(PSTR("[RELAY] #%zu pulse %s stopped\n"),
+                    id, status ? PSTR("ON") : PSTR("OFF"));
+            } else {
+                restarted = true;
+                timer.start();
+                DEBUG_MSG_P(PSTR("[RELAY] #%zu pulse %s rescheduled in %lu (ms)\n"),
+                    id, timer.status() ? PSTR("ON") : PSTR("OFF"),
+                    timer.duration().count());
+            }
+        });
+
+    if (restarted) {
+        return;
+    }
 
-void _relayProcessActivePulse(const Relay& relay, size_t id, bool status) {
-    using namespace espurna::relay::pulse;
-    if (isActive(relay.pulse) && !isNormalStatus(relay.pulse, status)) {
-        trigger(relay.pulse_time, id, !status);
+    if ((relay.pulse_time.count() > 0) && pulse::wouldChange(relay.pulse, status)) {
+        pulse::trigger(relay.pulse_time, id, !status);
+        DEBUG_MSG_P(PSTR("[RELAY] #%zu pulse %s scheduled in %lu (ms)\n"),
+            id, status ? PSTR("ON") : PSTR("OFF"),
+            relay.pulse_time.count());
     }
 }
 
-// start pulse for the current status as 'target'
+// expects generic time input which is converted to float first
+// duration equal to 0 would cancel existing timer
+// duration greater than 0 would toggle relay and schedule a timer
 [[gnu::unused]]
 bool _relayHandlePulsePayload(size_t id, espurna::StringView payload) {
-    const auto status = relayStatus(id);
-    if (_relayPulseActive(id, status)) {
-        return false;
-    }
 
-    using namespace espurna::relay::pulse::settings;
-    const auto pulse = parse_time(payload);
+    using namespace espurna::relay;
+    const auto duration = pulse::settings::parse_time(payload);
 
-    if (pulse.ok) {
-        espurna::relay::pulse::trigger(native_duration(pulse), id, status);
+    if (duration.ok) {
+        const auto native = pulse::settings::native_duration(duration);
+        if (native.count() == 0) {
+            pulse::reset(id);
+            return true;
+        }
+
+        pulse::schedule(native, id, relayStatus(id));
         relayToggle(id, true, false);
 
         return true;
@@ -1701,8 +1733,8 @@ bool _relayHandlePulsePayload(size_t id, espurna::StringView payload) {
 }
 
 // Make sure expired pulse timers are removed, so any API calls don't try to re-use those
-void _relayProcessPulse() {
-    espurna::relay::pulse::expire();
+void _relayRemoveCompletedPulse() {
+    espurna::relay::pulse::removeCompleted();
 }
 
 [[gnu::unused]]
@@ -1950,10 +1982,10 @@ bool _relayStatus(size_t id, bool status, bool report, bool group_report) {
     auto& relay = _relays[id];
 
     if (!_relayStatusCheckLock(relay, status)) {
-        DEBUG_MSG_P(PSTR("[RELAY] #%u is locked to %s\n"),
-            id, relay.current_status ? PSTR("ON") : PSTR("OFF"));
         relay.report = true;
         relay.group_report = true;
+        DEBUG_MSG_P(PSTR("[RELAY] #%u is locked to %s\n"),
+            id, relay.current_status ? PSTR("ON") : PSTR("OFF"));
         return false;
     }
 
@@ -1961,7 +1993,6 @@ bool _relayStatus(size_t id, bool status, bool report, bool group_report) {
 
     if (relay.current_status == status) {
         if (relay.target_status != status) {
-            DEBUG_MSG_P(PSTR("[RELAY] #%u scheduled change cancelled\n"), id);
             relay.target_status = status;
             relay.report = false;
             relay.group_report = false;
@@ -1974,7 +2005,11 @@ bool _relayStatus(size_t id, bool status, bool report, bool group_report) {
             notify(id, status);
         }
 
-        espurna::relay::pulse::poll(id, status);
+        espurna::relay::pulse::restart(id);
+
+        if (changed) {
+            DEBUG_MSG_P(PSTR("[RELAY] #%u scheduled change cancelled\n"), id);
+        }
     } else {
         auto current_time = Relay::TimeSource::now();
         auto change_delay = status
@@ -3026,7 +3061,7 @@ bool _relayProcess(bool mode) {
             _relayReport(id, target);
 
             // try to immediately schedule 'normal' state
-            _relayProcessActivePulse(_relays[id], id, target);
+            _relayProcessPulse(_relays[id], id, target);
 
             // and make sure relay values are persisted in RAM and flash
             _relayScheduleSave(id);
@@ -3054,7 +3089,7 @@ void _relayLoop() {
     };
 
     if (changed[0] || changed[1]) {
-        _relayProcessPulse();
+        _relayRemoveCompletedPulse();
         _relayPrepareUnlock();
     }