Fix: Component script not stopped in certain situations (#1004)

* Move stop/is_running implementation to Action base class

Try to fix issue #1105.  Until now if an UpdateComponentAction, a
LambdaAction, or any action that can contain those inside their
"else" or "then" action lists, resulted in a call to script.stop on
the script that contains them, the script would continue running,
because they didn't implement a stop() method.  Basically only
the asynchronous ones did: DelayAction, WaitUntilAction and
ScriptWaitAction.

With this change num_running_ in Action replaces
DelayAction::num_running_ and WaitUntilAction::triggered_ to provide
the same is_running logic to other actions.

* Make some Action methods protected

Apparently play()/stop() etc. are not meant to be called directly by
users of the class and if they're called directly that would not give
the expected result for the classes that have an empty play().

Make all methods except play_complex, stop_comples and is_running
protected.  While there also make RemoteTransmitterActionBase::encode
protected.

* lint

* format

Co-authored-by: Guillermo Ruffino <glm.net@gmail.com>

esphome/components/api/homeassistant_service.h

@@ -29,6 +29,7 @@ template<typename... Ts> class HomeAssistantServiceCallAction : public Action<Ts
   template<typename T> void add_variable(std::string key, T value) {
     this->variables_.push_back(TemplatableKeyValuePair<Ts...>(key, value));
   }
+
   void play(Ts... x) override {
     HomeassistantServiceResponse resp;
     resp.service = this->service_.value(x...);

esphome/components/binary_sensor/automation.h

@@ -137,6 +137,7 @@ template<typename... Ts> class BinarySensorPublishAction : public Action<Ts...>
  public:
   explicit BinarySensorPublishAction(BinarySensor *sensor) : sensor_(sensor) {}
   TEMPLATABLE_VALUE(bool, state)
+
   void play(Ts... x) override {
     auto val = this->state_.value(x...);
     this->sensor_->publish_state(val);

esphome/components/cover/automation.h

@@ -41,6 +41,10 @@ template<typename... Ts> class ControlAction : public Action<Ts...> {
  public:
   explicit ControlAction(Cover *cover) : cover_(cover) {}
 
+  TEMPLATABLE_VALUE(bool, stop)
+  TEMPLATABLE_VALUE(float, position)
+  TEMPLATABLE_VALUE(float, tilt)
+
   void play(Ts... x) override {
     auto call = this->cover_->make_call();
     if (this->stop_.has_value())
@@ -52,10 +56,6 @@ template<typename... Ts> class ControlAction : public Action<Ts...> {
     call.perform();
   }
 
-  TEMPLATABLE_VALUE(bool, stop)
-  TEMPLATABLE_VALUE(float, position)
-  TEMPLATABLE_VALUE(float, tilt)
-
  protected:
   Cover *cover_;
 };
@@ -63,6 +63,10 @@ template<typename... Ts> class ControlAction : public Action<Ts...> {
 template<typename... Ts> class CoverPublishAction : public Action<Ts...> {
  public:
   CoverPublishAction(Cover *cover) : cover_(cover) {}
+  TEMPLATABLE_VALUE(float, position)
+  TEMPLATABLE_VALUE(float, tilt)
+  TEMPLATABLE_VALUE(CoverOperation, current_operation)
+
   void play(Ts... x) override {
     if (this->position_.has_value())
       this->cover_->position = this->position_.value(x...);
@@ -73,10 +77,6 @@ template<typename... Ts> class CoverPublishAction : public Action<Ts...> {
     this->cover_->publish_state();
   }
 
-  TEMPLATABLE_VALUE(float, position)
-  TEMPLATABLE_VALUE(float, tilt)
-  TEMPLATABLE_VALUE(CoverOperation, current_operation)
-
  protected:
   Cover *cover_;
 };

esphome/components/dfplayer/dfplayer.h

@@ -104,7 +104,6 @@ class DFPlayer : public uart::UARTDevice, public Component {
 
 #define DFPLAYER_SIMPLE_ACTION(ACTION_CLASS, ACTION_METHOD) \
   template<typename... Ts> class ACTION_CLASS : public Action<Ts...>, public Parented<DFPlayer> { \
-   public: \
     void play(Ts... x) override { this->parent_->ACTION_METHOD(); } \
   };
 
@@ -115,6 +114,7 @@ template<typename... Ts> class PlayFileAction : public Action<Ts...>, public Par
  public:
   TEMPLATABLE_VALUE(uint16_t, file)
   TEMPLATABLE_VALUE(boolean, loop)
+
   void play(Ts... x) override {
     auto file = this->file_.value(x...);
     auto loop = this->loop_.value(x...);
@@ -131,6 +131,7 @@ template<typename... Ts> class PlayFolderAction : public Action<Ts...>, public P
   TEMPLATABLE_VALUE(uint16_t, folder)
   TEMPLATABLE_VALUE(uint16_t, file)
   TEMPLATABLE_VALUE(boolean, loop)
+
   void play(Ts... x) override {
     auto folder = this->folder_.value(x...);
     auto file = this->file_.value(x...);
@@ -146,6 +147,7 @@ template<typename... Ts> class PlayFolderAction : public Action<Ts...>, public P
 template<typename... Ts> class SetDeviceAction : public Action<Ts...>, public Parented<DFPlayer> {
  public:
   TEMPLATABLE_VALUE(Device, device)
+
   void play(Ts... x) override {
     auto device = this->device_.value(x...);
     this->parent_->set_device(device);
@@ -155,6 +157,7 @@ template<typename... Ts> class SetDeviceAction : public Action<Ts...>, public Pa
 template<typename... Ts> class SetVolumeAction : public Action<Ts...>, public Parented<DFPlayer> {
  public:
   TEMPLATABLE_VALUE(uint8_t, volume)
+
   void play(Ts... x) override {
     auto volume = this->volume_.value(x...);
     this->parent_->set_volume(volume);
@@ -164,6 +167,7 @@ template<typename... Ts> class SetVolumeAction : public Action<Ts...>, public Pa
 template<typename... Ts> class SetEqAction : public Action<Ts...>, public Parented<DFPlayer> {
  public:
   TEMPLATABLE_VALUE(EqPreset, eq)
+
   void play(Ts... x) override {
     auto eq = this->eq_.value(x...);
     this->parent_->set_eq(eq);

esphome/components/display/display_buffer.h

@@ -400,6 +400,7 @@ class Image {
 template<typename... Ts> class DisplayPageShowAction : public Action<Ts...> {
  public:
   TEMPLATABLE_VALUE(DisplayPage *, page)
+
   void play(Ts... x) override {
     auto *page = this->page_.value(x...);
     if (page != nullptr) {
@@ -411,18 +412,18 @@ template<typename... Ts> class DisplayPageShowAction : public Action<Ts...> {
 template<typename... Ts> class DisplayPageShowNextAction : public Action<Ts...> {
  public:
   DisplayPageShowNextAction(DisplayBuffer *buffer) : buffer_(buffer) {}
+
   void play(Ts... x) override { this->buffer_->show_next_page(); }
 
- protected:
   DisplayBuffer *buffer_;
 };
 
 template<typename... Ts> class DisplayPageShowPrevAction : public Action<Ts...> {
  public:
   DisplayPageShowPrevAction(DisplayBuffer *buffer) : buffer_(buffer) {}
+
   void play(Ts... x) override { this->buffer_->show_prev_page(); }
 
- protected:
   DisplayBuffer *buffer_;
 };
 

esphome/components/esp8266_pwm/esp8266_pwm.h

@@ -43,7 +43,6 @@ template<typename... Ts> class SetFrequencyAction : public Action<Ts...> {
     this->parent_->update_frequency(freq);
   }
 
- protected:
   ESP8266PWM *parent_;
 };
 

esphome/components/fan/automation.h

@@ -25,7 +25,6 @@ template<typename... Ts> class TurnOnAction : public Action<Ts...> {
     call.perform();
   }
 
- protected:
   FanState *state_;
 };
 
@@ -35,7 +34,6 @@ template<typename... Ts> class TurnOffAction : public Action<Ts...> {
 
   void play(Ts... x) override { this->state_->turn_off().perform(); }
 
- protected:
   FanState *state_;
 };
 
@@ -45,7 +43,6 @@ template<typename... Ts> class ToggleAction : public Action<Ts...> {
 
   void play(Ts... x) override { this->state_->toggle().perform(); }
 
- protected:
   FanState *state_;
 };
 

esphome/components/mhz19/mhz19.h

@@ -37,6 +37,7 @@ class MHZ19Component : public PollingComponent, public uart::UARTDevice {
 template<typename... Ts> class MHZ19CalibrateZeroAction : public Action<Ts...> {
  public:
   MHZ19CalibrateZeroAction(MHZ19Component *mhz19) : mhz19_(mhz19) {}
+
   void play(Ts... x) override { this->mhz19_->calibrate_zero(); }
 
  protected:
@@ -46,6 +47,7 @@ template<typename... Ts> class MHZ19CalibrateZeroAction : public Action<Ts...> {
 template<typename... Ts> class MHZ19ABCEnableAction : public Action<Ts...> {
  public:
   MHZ19ABCEnableAction(MHZ19Component *mhz19) : mhz19_(mhz19) {}
+
   void play(Ts... x) override { this->mhz19_->abc_enable(); }
 
  protected:
@@ -55,6 +57,7 @@ template<typename... Ts> class MHZ19ABCEnableAction : public Action<Ts...> {
 template<typename... Ts> class MHZ19ABCDisableAction : public Action<Ts...> {
  public:
   MHZ19ABCDisableAction(MHZ19Component *mhz19) : mhz19_(mhz19) {}
+
   void play(Ts... x) override { this->mhz19_->abc_disable(); }
 
  protected:

esphome/components/mqtt/mqtt_client.h

@@ -316,6 +316,7 @@ template<typename... Ts> class MQTTPublishJsonAction : public Action<Ts...> {
   TEMPLATABLE_VALUE(bool, retain)
 
   void set_payload(std::function<void(Ts..., JsonObject &)> payload) { this->payload_ = payload; }
+
   void play(Ts... x) override {
     auto f = std::bind(&MQTTPublishJsonAction<Ts...>::encode_, this, x..., std::placeholders::_1);
     auto topic = this->topic_.value(x...);

esphome/components/output/automation.h

@@ -33,6 +33,7 @@ template<typename... Ts> class SetLevelAction : public Action<Ts...> {
   SetLevelAction(FloatOutput *output) : output_(output) {}
 
   TEMPLATABLE_VALUE(float, level)
+
   void play(Ts... x) override { this->output_->set_level(this->level_.value(x...)); }
 
  protected:

esphome/components/pid/pid_climate.h

@@ -71,6 +71,10 @@ template<typename... Ts> class PIDAutotuneAction : public Action<Ts...> {
  public:
   PIDAutotuneAction(PIDClimate *parent) : parent_(parent) {}
 
+  void set_noiseband(float noiseband) { noiseband_ = noiseband; }
+  void set_positive_output(float positive_output) { positive_output_ = positive_output; }
+  void set_negative_output(float negative_output) { negative_output_ = negative_output; }
+
   void play(Ts... x) {
     auto tuner = make_unique<PIDAutotuner>();
     tuner->set_noiseband(this->noiseband_);
@@ -79,10 +83,6 @@ template<typename... Ts> class PIDAutotuneAction : public Action<Ts...> {
     this->parent_->start_autotune(std::move(tuner));
   }
 
-  void set_noiseband(float noiseband) { noiseband_ = noiseband; }
-  void set_positive_output(float positive_output) { positive_output_ = positive_output; }
-  void set_negative_output(float negative_output) { negative_output_ = negative_output; }
-
  protected:
   float noiseband_;
   float positive_output_;

esphome/components/remote_base/jvc_protocol.h

@@ -23,6 +23,7 @@ DECLARE_REMOTE_PROTOCOL(JVC)
 template<typename... Ts> class JVCAction : public RemoteTransmitterActionBase<Ts...> {
  public:
   TEMPLATABLE_VALUE(uint32_t, data)
+
   void encode(RemoteTransmitData *dst, Ts... x) override {
     JVCData data{};
     data.data = this->data_.value(x...);

esphome/components/remote_base/lg_protocol.h

@@ -26,6 +26,7 @@ template<typename... Ts> class LGAction : public RemoteTransmitterActionBase<Ts.
  public:
   TEMPLATABLE_VALUE(uint32_t, data)
   TEMPLATABLE_VALUE(uint8_t, nbits)
+
   void encode(RemoteTransmitData *dst, Ts... x) override {
     LGData data{};
     data.data = this->data_.value(x...);

esphome/components/remote_base/nec_protocol.h

@@ -25,6 +25,7 @@ template<typename... Ts> class NECAction : public RemoteTransmitterActionBase<Ts
  public:
   TEMPLATABLE_VALUE(uint16_t, address)
   TEMPLATABLE_VALUE(uint16_t, command)
+
   void encode(RemoteTransmitData *dst, Ts... x) override {
     NECData data{};
     data.address = this->address_.value(x...);

esphome/components/remote_base/panasonic_protocol.h

@@ -26,6 +26,7 @@ template<typename... Ts> class PanasonicAction : public RemoteTransmitterActionB
  public:
   TEMPLATABLE_VALUE(uint16_t, address)
   TEMPLATABLE_VALUE(uint32_t, command)
+
   void encode(RemoteTransmitData *dst, Ts... x) override {
     PanasonicData data{};
     data.address = this->address_.value(x...);

esphome/components/remote_base/pioneer_protocol.h

@@ -25,6 +25,7 @@ template<typename... Ts> class PioneerAction : public RemoteTransmitterActionBas
  public:
   TEMPLATABLE_VALUE(uint16_t, rc_code_1)
   TEMPLATABLE_VALUE(uint16_t, rc_code_2)
+
   void encode(RemoteTransmitData *dst, Ts... x) override {
     PioneerData data{};
     data.rc_code_1 = this->rc_code_1_.value(x...);

esphome/components/remote_base/rc5_protocol.h

@@ -26,6 +26,7 @@ template<typename... Ts> class RC5Action : public RemoteTransmitterActionBase<Ts
  public:
   TEMPLATABLE_VALUE(uint8_t, address)
   TEMPLATABLE_VALUE(uint8_t, command)
+
   void encode(RemoteTransmitData *dst, Ts... x) override {
     RC5Data data{};
     data.address = this->address_.value(x...);

esphome/components/remote_base/remote_base.h

@@ -323,6 +323,9 @@ template<typename... Ts> class RemoteTransmitterActionBase : public Action<Ts...
  public:
   void set_parent(RemoteTransmitterBase *parent) { this->parent_ = parent; }
 
+  TEMPLATABLE_VALUE(uint32_t, send_times);
+  TEMPLATABLE_VALUE(uint32_t, send_wait);
+
   void play(Ts... x) override {
     auto call = this->parent_->transmit();
     this->encode(call.get_data(), x...);
@@ -331,12 +334,9 @@ template<typename... Ts> class RemoteTransmitterActionBase : public Action<Ts...
     call.perform();
   }
 
+ protected:
   virtual void encode(RemoteTransmitData *dst, Ts... x) = 0;
 
-  TEMPLATABLE_VALUE(uint32_t, send_times);
-  TEMPLATABLE_VALUE(uint32_t, send_wait);
-
- protected:
   RemoteTransmitterBase *parent_{};
 };
 

esphome/components/remote_base/samsung_protocol.h

@@ -24,6 +24,7 @@ DECLARE_REMOTE_PROTOCOL(Samsung)
 template<typename... Ts> class SamsungAction : public RemoteTransmitterActionBase<Ts...> {
  public:
   TEMPLATABLE_VALUE(uint32_t, data)
+
   void encode(RemoteTransmitData *dst, Ts... x) override {
     SamsungData data{};
     data.data = this->data_.value(x...);

esphome/components/remote_base/sony_protocol.h

@@ -26,6 +26,7 @@ template<typename... Ts> class SonyAction : public RemoteTransmitterActionBase<T
  public:
   TEMPLATABLE_VALUE(uint32_t, data)
   TEMPLATABLE_VALUE(uint8_t, nbits)
+
   void encode(RemoteTransmitData *dst, Ts... x) override {
     SonyData data{};
     data.data = this->data_.value(x...);

esphome/components/rotary_encoder/rotary_encoder.h

@@ -74,6 +74,7 @@ template<typename... Ts> class RotaryEncoderSetValueAction : public Action<Ts...
  public:
   RotaryEncoderSetValueAction(RotaryEncoderSensor *encoder) : encoder_(encoder) {}
   TEMPLATABLE_VALUE(int, value)
+
   void play(Ts... x) override { this->encoder_->set_value(this->value_.value(x...)); }
 
  protected:

esphome/components/script/script.h

@@ -53,41 +53,34 @@ template<typename... Ts> class ScriptWaitAction : public Action<Ts...>, public C
  public:
   ScriptWaitAction(Script *script) : script_(script) {}
 
-  void play(Ts... x) { /* ignore - see play_complex */
-  }
-
   void play_complex(Ts... x) override {
+    this->num_running_++;
     // Check if we can continue immediately.
     if (!this->script_->is_running()) {
-      this->triggered_ = false;
-      this->play_next(x...);
+      this->play_next_(x...);
       return;
     }
     this->var_ = std::make_tuple(x...);
-    this->triggered_ = true;
     this->loop();
   }
 
-  void stop() override { this->triggered_ = false; }
-
   void loop() override {
-    if (!this->triggered_)
+    if (this->num_running_ == 0)
       return;
 
     if (this->script_->is_running())
       return;
 
-    this->triggered_ = false;
-    this->play_next_tuple(this->var_);
+    this->play_next_tuple_(this->var_);
   }
 
   float get_setup_priority() const override { return setup_priority::DATA; }
 
-  bool is_running() override { return this->triggered_ || this->is_running_next(); }
+  void play(Ts... x) override { /* ignore - see play_complex */
+  }
 
  protected:
   Script *script_;
-  bool triggered_{false};
   std::tuple<Ts...> var_{};
 };
 

esphome/components/sensor/automation.h

@@ -25,6 +25,7 @@ template<typename... Ts> class SensorPublishAction : public Action<Ts...> {
  public:
   SensorPublishAction(Sensor *sensor) : sensor_(sensor) {}
   TEMPLATABLE_VALUE(float, state)
+
   void play(Ts... x) override { this->sensor_->publish_state(this->state_.value(x...)); }
 
  protected:

esphome/components/servo/servo.h

@@ -64,6 +64,7 @@ template<typename... Ts> class ServoWriteAction : public Action<Ts...> {
  public:
   ServoWriteAction(Servo *servo) : servo_(servo) {}
   TEMPLATABLE_VALUE(float, value)
+
   void play(Ts... x) override { this->servo_->write(this->value_.value(x...)); }
 
  protected:
@@ -73,6 +74,7 @@ template<typename... Ts> class ServoWriteAction : public Action<Ts...> {
 template<typename... Ts> class ServoDetachAction : public Action<Ts...> {
  public:
   ServoDetachAction(Servo *servo) : servo_(servo) {}
+
   void play(Ts... x) override { this->servo_->detach(); }
 
  protected:

esphome/components/switch/automation.h

@@ -73,6 +73,7 @@ template<typename... Ts> class SwitchPublishAction : public Action<Ts...> {
  public:
   SwitchPublishAction(Switch *a_switch) : switch_(a_switch) {}
   TEMPLATABLE_VALUE(bool, state)
+
   void play(Ts... x) override { this->switch_->publish_state(this->state_.value(x...)); }
 
  protected:

esphome/components/text_sensor/automation.h

@@ -30,6 +30,7 @@ template<typename... Ts> class TextSensorPublishAction : public Action<Ts...> {
  public:
   TextSensorPublishAction(TextSensor *sensor) : sensor_(sensor) {}
   TEMPLATABLE_VALUE(std::string, state)
+
   void play(Ts... x) override { this->sensor_->publish_state(this->state_.value(x...)); }
 
  protected: