[sprinkler] Squash a few bugs + minor optimization (#12436)

esphome/components/sprinkler/automation.h

@@ -4,8 +4,7 @@
 #include "esphome/core/component.h"
 #include "esphome/components/sprinkler/sprinkler.h"
 
-namespace esphome {
+namespace esphome::sprinkler {
-namespace sprinkler {
 
 template<typename... Ts> class SetDividerAction : public Action<Ts...> {
  public:
@@ -181,5 +180,4 @@ template<typename... Ts> class ResumeOrStartAction : public Action<Ts...> {
   Sprinkler *sprinkler_;
 };
 
-}  // namespace sprinkler
+}  // namespace esphome::sprinkler
-}  // namespace esphome

esphome/components/sprinkler/sprinkler.cpp

@@ -7,8 +7,7 @@
 #include <cinttypes>
 #include <utility>
 
-namespace esphome {
+namespace esphome::sprinkler {
-namespace sprinkler {
 
 static const char *const TAG = "sprinkler";
 
@@ -411,7 +410,8 @@ void Sprinkler::loop() {
   for (auto &vo : this->valve_op_) {
     vo.loop();
   }
-  if (this->prev_req_.has_request() && this->prev_req_.valve_operator()->state() == IDLE) {
+  if (this->prev_req_.has_request() && this->prev_req_.has_valve_operator() &&
+      this->prev_req_.valve_operator()->state() == IDLE) {
     this->prev_req_.reset();
   }
 }
@@ -808,11 +808,11 @@ bool Sprinkler::standby() {
 
 void Sprinkler::start_from_queue() {
   if (this->standby()) {
-    ESP_LOGD(TAG, "start_from_queue called but standby is enabled; no action taken");
+    this->log_standby_warning_(LOG_STR("start_from_queue"));
     return;
   }
   if (this->multiplier() == 0) {
-    ESP_LOGD(TAG, "start_from_queue called but multiplier is set to zero; no action taken");
+    this->log_multiplier_zero_warning_(LOG_STR("start_from_queue"));
     return;
   }
   if (this->queued_valves_.empty()) {
@@ -832,11 +832,11 @@ void Sprinkler::start_from_queue() {
 
 void Sprinkler::start_full_cycle() {
   if (this->standby()) {
-    ESP_LOGD(TAG, "start_full_cycle called but standby is enabled; no action taken");
+    this->log_standby_warning_(LOG_STR("start_full_cycle"));
     return;
   }
   if (this->multiplier() == 0) {
-    ESP_LOGD(TAG, "start_full_cycle called but multiplier is set to zero; no action taken");
+    this->log_multiplier_zero_warning_(LOG_STR("start_full_cycle"));
     return;
   }
   if (this->auto_advance() && this->active_valve().has_value()) {
@@ -855,11 +855,11 @@ void Sprinkler::start_full_cycle() {
 
 void Sprinkler::start_single_valve(const optional<size_t> valve_number, optional<uint32_t> run_duration) {
   if (this->standby()) {
-    ESP_LOGD(TAG, "start_single_valve called but standby is enabled; no action taken");
+    this->log_standby_warning_(LOG_STR("start_single_valve"));
     return;
   }
   if (this->multiplier() == 0) {
-    ESP_LOGD(TAG, "start_single_valve called but multiplier is set to zero; no action taken");
+    this->log_multiplier_zero_warning_(LOG_STR("start_single_valve"));
     return;
   }
   if (!valve_number.has_value() || (valve_number == this->active_valve())) {
@@ -891,6 +891,11 @@ void Sprinkler::clear_queued_valves() {
 }
 
 void Sprinkler::next_valve() {
+  if (this->standby()) {
+    this->log_standby_warning_(LOG_STR("next_valve"));
+    return;
+  }
+
   if (this->state_ == IDLE) {
     this->reset_cycle_states_();  // just in case auto-advance is switched on later
   }
@@ -914,6 +919,11 @@ void Sprinkler::next_valve() {
 }
 
 void Sprinkler::previous_valve() {
+  if (this->standby()) {
+    this->log_standby_warning_(LOG_STR("previous_valve"));
+    return;
+  }
+
   if (this->state_ == IDLE) {
     this->reset_cycle_states_();  // just in case auto-advance is switched on later
   }
@@ -964,7 +974,7 @@ void Sprinkler::pause() {
 
 void Sprinkler::resume() {
   if (this->standby()) {
-    ESP_LOGD(TAG, "resume called but standby is enabled; no action taken");
+    this->log_standby_warning_(LOG_STR("resume"));
     return;
   }
 
@@ -1009,7 +1019,7 @@ optional<SprinklerValveRunRequestOrigin> Sprinkler::active_valve_request_is_from
 }
 
 optional<size_t> Sprinkler::active_valve() {
-  if (!this->valve_overlap_ && this->prev_req_.has_request() &&
+  if (!this->valve_overlap_ && this->prev_req_.has_request() && this->prev_req_.has_valve_operator() &&
       (this->prev_req_.valve_operator()->state() == STARTING || this->prev_req_.valve_operator()->state() == ACTIVE)) {
     return this->prev_req_.valve_as_opt();
   }
@@ -1029,9 +1039,7 @@ optional<size_t> Sprinkler::manual_valve() { return this->manual_valve_; }
 
 size_t Sprinkler::number_of_valves() { return this->valve_.size(); }
 
-bool Sprinkler::is_a_valid_valve(const size_t valve_number) {
+bool Sprinkler::is_a_valid_valve(const size_t valve_number) { return (valve_number < this->number_of_valves()); }
-  return ((valve_number >= 0) && (valve_number < this->number_of_valves()));
-}
 
 bool Sprinkler::pump_in_use(SprinklerSwitch *pump_switch) {
   if (pump_switch == nullptr) {
@@ -1062,8 +1070,12 @@ bool Sprinkler::pump_in_use(SprinklerSwitch *pump_switch) {
       this->active_req_.has_request() && (this->state_ != STOPPING)) {
     // ...the controller is configured to keep the pump on during a valve open delay, so just return
     //  whether or not the next valve shares the same pump
-    return (pump_switch->off_switch() == this->valve_pump_switch(this->active_req_.valve())->off_switch()) &&
+    auto *valve_pump = this->valve_pump_switch(this->active_req_.valve());
-           (pump_switch->on_switch() == this->valve_pump_switch(this->active_req_.valve())->on_switch());
+    if (valve_pump == nullptr) {
+      return false;  // valve has no pump, so this pump isn't in use by it
+    }
+    return (pump_switch->off_switch() == valve_pump->off_switch()) &&
+           (pump_switch->on_switch() == valve_pump->on_switch());
   }
   return false;
 }
@@ -1426,8 +1438,8 @@ void Sprinkler::start_valve_(SprinklerValveRunRequest *req) {
     if (vo.state() == IDLE) {
       auto run_duration = req->run_duration() ? req->run_duration() : this->valve_run_duration_adjusted(req->valve());
       ESP_LOGD(TAG, "%s is starting valve %zu for %" PRIu32 " seconds, cycle %" PRIu32 " of %" PRIu32,
-               this->req_as_str_(req->request_is_from()).c_str(), req->valve(), run_duration, this->repeat_count_ + 1,
+               LOG_STR_ARG(this->req_as_str_(req->request_is_from())), req->valve(), run_duration,
-               this->repeat().value_or(0) + 1);
+               this->repeat_count_ + 1, this->repeat().value_or(0) + 1);
       req->set_valve_operator(&vo);
       vo.set_controller(this);
       vo.set_valve(&this->valve_[req->valve()]);
@@ -1488,7 +1500,7 @@ void Sprinkler::fsm_kick_() {
 }
 
 void Sprinkler::fsm_transition_() {
-  ESP_LOGVV(TAG, "fsm_transition_ called; state is %s", this->state_as_str_(this->state_).c_str());
+  ESP_LOGVV(TAG, "fsm_transition_ called; state is %s", LOG_STR_ARG(this->state_as_str_(this->state_)));
   switch (this->state_) {
     case IDLE:  // the system was off -> start it up
       // advances to ACTIVE
@@ -1502,8 +1514,11 @@ void Sprinkler::fsm_transition_() {
 
     case STARTING: {
       // follows valve open delay interval
-      this->set_timer_duration_(sprinkler::TIMER_SM,
+      uint32_t timer_duration = this->active_req_.run_duration();
-                                this->active_req_.run_duration() - this->switching_delay_.value_or(0));
+      if (timer_duration > this->switching_delay_.value_or(0)) {
+        timer_duration -= this->switching_delay_.value_or(0);
+      }
+      this->set_timer_duration_(sprinkler::TIMER_SM, timer_duration);
       this->start_timer_(sprinkler::TIMER_SM);
       this->start_valve_(&this->active_req_);
       this->state_ = ACTIVE;
@@ -1531,7 +1546,7 @@ void Sprinkler::fsm_transition_() {
     this->set_timer_duration_(sprinkler::TIMER_SM, this->manual_selection_delay_.value_or(1));
     this->start_timer_(sprinkler::TIMER_SM);
   }
-  ESP_LOGVV(TAG, "fsm_transition_ complete; new state is %s", this->state_as_str_(this->state_).c_str());
+  ESP_LOGVV(TAG, "fsm_transition_ complete; new state is %s", LOG_STR_ARG(this->state_as_str_(this->state_)));
 }
 
 void Sprinkler::fsm_transition_from_shutdown_() {
@@ -1543,8 +1558,11 @@ void Sprinkler::fsm_transition_from_shutdown_() {
     this->active_req_.set_run_duration(this->next_req_.run_duration());
     this->next_req_.reset();
 
-    this->set_timer_duration_(sprinkler::TIMER_SM,
+    uint32_t timer_duration = this->active_req_.run_duration();
-                              this->active_req_.run_duration() - this->switching_delay_.value_or(0));
+    if (timer_duration > this->switching_delay_.value_or(0)) {
+      timer_duration -= this->switching_delay_.value_or(0);
+    }
+    this->set_timer_duration_(sprinkler::TIMER_SM, timer_duration);
     this->start_timer_(sprinkler::TIMER_SM);
     this->start_valve_(&this->active_req_);
     this->state_ = ACTIVE;
@@ -1571,8 +1589,9 @@ void Sprinkler::fsm_transition_from_valve_run_() {
   this->load_next_valve_run_request_(this->active_req_.valve());
 
   if (this->next_req_.has_request()) {  // there is another valve to run...
-    bool same_pump =
+    auto *active_pump = this->valve_pump_switch(this->active_req_.valve());
-        this->valve_pump_switch(this->active_req_.valve()) == this->valve_pump_switch(this->next_req_.valve());
+    auto *next_pump = this->valve_pump_switch(this->next_req_.valve());
+    bool same_pump = (active_pump != nullptr) && (next_pump != nullptr) && (active_pump == next_pump);
 
     this->active_req_.set_valve(this->next_req_.valve());
     this->active_req_.set_request_from(this->next_req_.request_is_from());
@@ -1581,8 +1600,11 @@ void Sprinkler::fsm_transition_from_valve_run_() {
 
     // this->state_ = ACTIVE;  // state isn't changing
     if (this->valve_overlap_ || !this->switching_delay_.has_value()) {
-      this->set_timer_duration_(sprinkler::TIMER_SM,
+      uint32_t timer_duration = this->active_req_.run_duration();
-                                this->active_req_.run_duration() - this->switching_delay_.value_or(0));
+      if (timer_duration > this->switching_delay_.value_or(0)) {
+        timer_duration -= this->switching_delay_.value_or(0);
+      }
+      this->set_timer_duration_(sprinkler::TIMER_SM, timer_duration);
       this->start_timer_(sprinkler::TIMER_SM);
       this->start_valve_(&this->active_req_);
     } else {
@@ -1605,41 +1627,49 @@ void Sprinkler::fsm_transition_to_shutdown_() {
   this->start_timer_(sprinkler::TIMER_SM);
 }
 
-std::string Sprinkler::req_as_str_(SprinklerValveRunRequestOrigin origin) {
+void Sprinkler::log_standby_warning_(const LogString *method_name) {
+  ESP_LOGW(TAG, "%s called but standby is enabled; no action taken", LOG_STR_ARG(method_name));
+}
+
+void Sprinkler::log_multiplier_zero_warning_(const LogString *method_name) {
+  ESP_LOGW(TAG, "%s called but multiplier is set to zero; no action taken", LOG_STR_ARG(method_name));
+}
+
+const LogString *Sprinkler::req_as_str_(SprinklerValveRunRequestOrigin origin) {
   switch (origin) {
     case USER:
-      return "USER";
+      return LOG_STR("USER");
 
     case CYCLE:
-      return "CYCLE";
+      return LOG_STR("CYCLE");
 
     case QUEUE:
-      return "QUEUE";
+      return LOG_STR("QUEUE");
 
     default:
-      return "UNKNOWN";
+      return LOG_STR("UNKNOWN");
   }
 }
 
-std::string Sprinkler::state_as_str_(SprinklerState state) {
+const LogString *Sprinkler::state_as_str_(SprinklerState state) {
   switch (state) {
     case IDLE:
-      return "IDLE";
+      return LOG_STR("IDLE");
 
     case STARTING:
-      return "STARTING";
+      return LOG_STR("STARTING");
 
     case ACTIVE:
-      return "ACTIVE";
+      return LOG_STR("ACTIVE");
 
     case STOPPING:
-      return "STOPPING";
+      return LOG_STR("STOPPING");
 
     case BYPASS:
-      return "BYPASS";
+      return LOG_STR("BYPASS");
 
     default:
-      return "UNKNOWN";
+      return LOG_STR("UNKNOWN");
   }
 }
 
@@ -1737,5 +1767,4 @@ void Sprinkler::dump_config() {
   }
 }
 
-}  // namespace sprinkler
+}  // namespace esphome::sprinkler
-}  // namespace esphome

esphome/components/sprinkler/sprinkler.h

@@ -8,8 +8,7 @@
 
 #include <vector>
 
-namespace esphome {
+namespace esphome::sprinkler {
-namespace sprinkler {
 
 const std::string MIN_STR = "min";
 
@@ -490,11 +489,17 @@ class Sprinkler : public Component {
   /// starts up the system from IDLE state
   void fsm_transition_to_shutdown_();
 
+  /// log error message when a method is called but standby is enabled
+  void log_standby_warning_(const LogString *method_name);
+
+  /// log error message when a method is called but multiplier is zero
+  void log_multiplier_zero_warning_(const LogString *method_name);
+
   /// return the specified SprinklerValveRunRequestOrigin as a string
-  std::string req_as_str_(SprinklerValveRunRequestOrigin origin);
+  const LogString *req_as_str_(SprinklerValveRunRequestOrigin origin);
 
   /// return the specified SprinklerState state as a string
-  std::string state_as_str_(SprinklerState state);
+  const LogString *state_as_str_(SprinklerState state);
 
   /// Start/cancel/get status of valve timers
   void start_timer_(SprinklerTimerIndex timer_index);
@@ -607,5 +612,4 @@ class Sprinkler : public Component {
   std::unique_ptr<Automation<>> sprinkler_standby_turn_on_automation_;
 };
 
-}  // namespace sprinkler
+}  // namespace esphome::sprinkler
-}  // namespace esphome