From fc39abd55167418049bcfd86a56f7d0545f5f517 Mon Sep 17 00:00:00 2001 From: Offspring Date: Wed, 28 Aug 2024 23:28:06 -0400 Subject: [PATCH 1/2] fix virtual / override and const correctness --- src/ReactESP.cpp | 65 ++++++++++--------- src/ReactESP.h | 159 +++++++++++++++++++++++++++-------------------- 2 files changed, 123 insertions(+), 101 deletions(-) diff --git a/src/ReactESP.cpp b/src/ReactESP.cpp index 3c55b0f..c150116 100644 --- a/src/ReactESP.cpp +++ b/src/ReactESP.cpp @@ -2,7 +2,8 @@ #include #include -#include + +#include namespace reactesp { @@ -20,7 +21,7 @@ uint64_t ICACHE_RAM_ATTR micros64() { return esp_timer_get_time(); } // Reaction classes define the behaviour of each particular // Reaction -bool TimedReaction::operator<(const TimedReaction& other) { +bool TimedReaction::operator<(const TimedReaction& other) const { return (this->last_trigger_time + this->interval) < (other.last_trigger_time + other.interval); } @@ -38,15 +39,16 @@ void TimedReaction::remove(ReactESP* app) { this->enabled = false; // the object will be deleted when it's popped out of the // timer queue + (void)app; /* unused */ } -DelayReaction::DelayReaction(uint32_t interval, const react_callback callback) - : TimedReaction(interval, callback) { +DelayReaction::DelayReaction(uint32_t delay, react_callback callback) + : TimedReaction(delay, callback) { this->last_trigger_time = micros64(); } -DelayReaction::DelayReaction(uint64_t interval, const react_callback callback) - : TimedReaction(interval, callback) { +DelayReaction::DelayReaction(uint64_t delay, react_callback callback) + : TimedReaction(delay, callback) { this->last_trigger_time = micros64(); } @@ -84,7 +86,7 @@ void UntimedReaction::remove(ReactESP* app) { } void StreamReaction::tick() { - if (stream.available()) { + if (0 != stream.available()) { this->callback(); } } @@ -95,7 +97,7 @@ void TickReaction::tick() { this->callback(); } bool ISRReaction::isr_service_installed = false; void ISRReaction::isr(void* this_ptr) { - auto* this_ = (ISRReaction*)this_ptr; + auto* this_ = static_cast(this_ptr); this_->callback(); } #endif @@ -128,12 +130,11 @@ void ISRReaction::remove(ReactESP* app) { } // Need to define the static variable outside of the class -ReactESP* ReactESP::app = NULL; +ReactESP* ReactESP::app = nullptr; void ReactESP::tickTimed() { - uint64_t now = micros64(); - uint64_t trigger_t; - TimedReaction* top; + const uint64_t now = micros64(); + TimedReaction* top = nullptr; while (true) { if (timed_queue.empty()) { @@ -145,7 +146,7 @@ void ReactESP::tickTimed() { delete top; continue; } - trigger_t = top->getTriggerTimeMicros(); + const uint64_t trigger_t = top->getTriggerTimeMicros(); if (now >= trigger_t) { timed_queue.pop(); top->tick(); @@ -166,53 +167,51 @@ void ReactESP::tick() { tickTimed(); } -DelayReaction* ReactESP::onDelay(const uint32_t t, const react_callback cb) { - DelayReaction* dre = new DelayReaction(t, cb); +DelayReaction* ReactESP::onDelay(uint32_t delay, react_callback callback) { + auto* dre = new DelayReaction(delay, callback); dre->add(this); return dre; } -DelayReaction* ReactESP::onDelayMicros(const uint64_t t, - const react_callback cb) { - DelayReaction* dre = new DelayReaction(t, cb); +DelayReaction* ReactESP::onDelayMicros(uint64_t delay, + react_callback callback) { + auto* dre = new DelayReaction(delay, callback); dre->add(this); return dre; } -RepeatReaction* ReactESP::onRepeat(const uint32_t t, const react_callback cb) { - RepeatReaction* rre = new RepeatReaction(t, cb); +RepeatReaction* ReactESP::onRepeat(uint32_t interval, react_callback callback) { + auto* rre = new RepeatReaction(interval, callback); rre->add(this); return rre; } -RepeatReaction* ReactESP::onRepeatMicros(const uint64_t t, - const react_callback cb) { - RepeatReaction* rre = new RepeatReaction(t, cb); +RepeatReaction* ReactESP::onRepeatMicros(uint64_t interval, + react_callback callback) { + auto* rre = new RepeatReaction(interval, callback); rre->add(this); return rre; } -StreamReaction* ReactESP::onAvailable(Stream& stream, const react_callback cb) { - StreamReaction* sre = new StreamReaction(stream, cb); +StreamReaction* ReactESP::onAvailable(Stream& stream, react_callback callback) { + auto* sre = new StreamReaction(stream, callback); sre->add(this); return sre; } -ISRReaction* ReactESP::onInterrupt(const uint8_t pin_number, int mode, - const react_callback cb) { - ISRReaction* isrre = new ISRReaction(pin_number, mode, cb); +ISRReaction* ReactESP::onInterrupt(uint8_t pin_number, int mode, + react_callback callback) { + auto* isrre = new ISRReaction(pin_number, mode, callback); isrre->add(this); return isrre; } -TickReaction* ReactESP::onTick(const react_callback cb) { - TickReaction* tre = new TickReaction(cb); +TickReaction* ReactESP::onTick(react_callback callback) { + auto* tre = new TickReaction(callback); tre->add(this); return tre; } -void ReactESP::remove(Reaction* reaction) { - reaction->remove(this); -} +void ReactESP::remove(Reaction* reaction) { reaction->remove(this); } } // namespace reactesp diff --git a/src/ReactESP.h b/src/ReactESP.h index 01e56e9..81f9d26 100644 --- a/src/ReactESP.h +++ b/src/ReactESP.h @@ -9,8 +9,8 @@ namespace reactesp { -typedef std::function react_callback; -typedef void (*isr_react_callback)(void*); +using react_callback = std::function; +using isr_react_callback = void (*)(void*); // forward declarations @@ -24,7 +24,21 @@ uint64_t ICACHE_RAM_ATTR micros64(); /** * @brief Reactions are code to be called when a given condition is fulfilled */ -class Reaction { +struct ReactionInterface { + /** + * @brief Default virtual destructor + */ + virtual ~ReactionInterface() = default; + + virtual void add(ReactESP* app = nullptr) = 0; + virtual void remove(ReactESP* app = nullptr) = 0; + virtual void tick() = 0; +}; + +/** + * @brief Reactions are code to be called when a given condition is fulfilled + */ +class Reaction : public ReactionInterface { protected: const react_callback callback; @@ -35,10 +49,12 @@ class Reaction { * @param callback Function to be called when the reaction is triggered */ Reaction(react_callback callback) : callback(callback) {} - // FIXME: why do these have to be defined? - virtual void add(ReactESP* app = nullptr) = 0; - virtual void remove(ReactESP* app = nullptr) = 0; - virtual void tick() = 0; + + // Disabling copy and move semantics + Reaction(const Reaction&) = delete; + Reaction(Reaction&&) = delete; + Reaction& operator=(const Reaction&) = delete; + Reaction& operator=(Reaction&&) = delete; }; /** @@ -59,31 +75,33 @@ class TimedReaction : public Reaction { * @param interval Interval or delay for the reaction, in milliseconds * @param callback Function to be called when the reaction is triggered */ - TimedReaction(const uint32_t interval, const react_callback callback) - : Reaction(callback), interval((uint64_t)1000 * (uint64_t)interval) { - last_trigger_time = micros64(); - enabled = true; - } + TimedReaction(uint32_t interval, react_callback callback) + : Reaction(callback), + interval((uint64_t)1000 * (uint64_t)interval), + last_trigger_time(micros64()), + enabled(true) {} /** * @brief Construct a new Timed Reaction object * * @param interval Interval, in microseconds * @param callback Function to be called when the reaction is triggered */ - TimedReaction(const uint64_t interval, const react_callback callback) - : Reaction(callback), interval(interval) { - last_trigger_time = micros64(); - enabled = true; - } + TimedReaction(uint64_t interval, react_callback callback) + : Reaction(callback), + interval(interval), + last_trigger_time(micros64()), + enabled(true) {} - virtual ~TimedReaction() {} - bool operator<(const TimedReaction& other); + bool operator<(const TimedReaction& other) const; void add(ReactESP* app = nullptr) override; void remove(ReactESP* app = nullptr) override; - uint32_t getTriggerTime() { return (last_trigger_time + interval) / 1000; } - uint64_t getTriggerTimeMicros() { return (last_trigger_time + interval); } - bool isEnabled() { return enabled; } - virtual void tick() = 0; + uint32_t getTriggerTime() const { + return (last_trigger_time + interval) / 1000; + } + uint64_t getTriggerTimeMicros() const { + return (last_trigger_time + interval); + } + bool isEnabled() const { return enabled; } }; struct TriggerTimeCompare { @@ -101,16 +119,16 @@ class DelayReaction : public TimedReaction { * @param delay Delay, in milliseconds * @param callback Function to be called after the delay */ - DelayReaction(const uint32_t delay, const react_callback callback); + DelayReaction(uint32_t delay, react_callback callback); /** * @brief Construct a new Delay Reaction object * * @param delay Delay, in microseconds * @param callback Function to be called after the delay */ - DelayReaction(const uint64_t delay, const react_callback callback); - virtual ~DelayReaction() {} - void tick(); + DelayReaction(uint64_t delay, react_callback callback); + + void tick() override; }; /** @@ -124,7 +142,7 @@ class RepeatReaction : public TimedReaction { * @param interval Repetition interval, in milliseconds * @param callback Function to be called at every repetition */ - RepeatReaction(const uint32_t interval, const react_callback callback) + RepeatReaction(uint32_t interval, react_callback callback) : TimedReaction(interval, callback) {} /** * @brief Construct a new Repeat Reaction object @@ -132,9 +150,10 @@ class RepeatReaction : public TimedReaction { * @param interval Repetition interval, in microseconds * @param callback Function to be called at every repetition */ - RepeatReaction(const uint64_t interval, const react_callback callback) + RepeatReaction(uint64_t interval, react_callback callback) : TimedReaction(interval, callback) {} - void tick(); + + void tick() override; }; /** @@ -142,11 +161,10 @@ class RepeatReaction : public TimedReaction { */ class UntimedReaction : public Reaction { public: - UntimedReaction(const react_callback callback) : Reaction(callback) {} - virtual ~UntimedReaction() {} - virtual void add(ReactESP* app = nullptr) override; - virtual void remove(ReactESP* app = nullptr) override; - virtual void tick() = 0; + UntimedReaction(react_callback callback) : Reaction(callback) {} + + void add(ReactESP* app = nullptr) override; + void remove(ReactESP* app = nullptr) override; }; /** @@ -164,9 +182,10 @@ class StreamReaction : public UntimedReaction { * @param stream Stream to monitor * @param callback Callback to call for new input */ - StreamReaction(Stream& stream, const react_callback callback) + StreamReaction(Stream& stream, react_callback callback) : UntimedReaction(callback), stream(stream) {} - void tick(); + + void tick() override; }; /** @@ -179,8 +198,9 @@ class TickReaction : public UntimedReaction { * * @param callback Function to be called at each execution loop */ - TickReaction(const react_callback callback) : UntimedReaction(callback) {} - void tick(); + TickReaction(react_callback callback) : UntimedReaction(callback) {} + + void tick() override; }; /** @@ -193,7 +213,7 @@ class ISRReaction : public Reaction { #ifdef ESP32 // set to true once gpio_install_isr_service is called static bool isr_service_installed; - static void isr(void* arg); + static void isr(void* this_ptr); #endif public: @@ -205,7 +225,7 @@ class ISRReaction : public Reaction { * @param callback Interrupt callback. Keep this function short and add the * ICACHE_RAM_ATTR attribute. */ - ISRReaction(uint8_t pin_number, int mode, const react_callback callback) + ISRReaction(uint8_t pin_number, int mode, react_callback callback) : Reaction(callback), pin_number(pin_number), mode(mode) { #ifdef ESP32 gpio_int_type_t intr_type; @@ -227,15 +247,15 @@ class ISRReaction : public Reaction { gpio_set_intr_type((gpio_num_t)pin_number, intr_type); if (!isr_service_installed) { - gpio_install_isr_service(ESP_INTR_FLAG_LOWMED); isr_service_installed = true; + gpio_install_isr_service(ESP_INTR_FLAG_LOWMED); } #endif } - virtual ~ISRReaction() {} + void add(ReactESP* app = nullptr) override; void remove(ReactESP* app = nullptr) override; - void tick() {} + void tick() override {} }; /////////////////////////////////////// @@ -257,12 +277,14 @@ class ReactESP { * * @param singleton If true, set the singleton instance to this object */ - ReactESP(bool singleton = true) { + ReactESP(bool singleton = true) + : timed_queue(), untimed_list(), isr_reaction_list(), isr_pending_list() { if (singleton) { app = this; } } - void tick(void); + + void tick(); /// Static singleton reference to the instantiated ReactESP object static ReactESP* app; @@ -270,62 +292,62 @@ class ReactESP { /** * @brief Create a new DelayReaction * - * @param t Delay, in milliseconds - * @param cb Callback function + * @param delay Delay, in milliseconds + * @param callback Callback function * @return DelayReaction* */ - DelayReaction* onDelay(const uint32_t t, const react_callback cb); + DelayReaction* onDelay(uint32_t delay, react_callback callback); /** * @brief Create a new DelayReaction * - * @param t Delay, in microseconds - * @param cb Callback function + * @param delay Delay, in microseconds + * @param callback Callback function * @return DelayReaction* */ - DelayReaction* onDelayMicros(const uint64_t t, const react_callback cb); + DelayReaction* onDelayMicros(uint64_t delay, react_callback callback); /** * @brief Create a new RepeatReaction * - * @param t Interval, in milliseconds - * @param cb Callback function + * @param delay Interval, in milliseconds + * @param callback Callback function * @return RepeatReaction* */ - RepeatReaction* onRepeat(const uint32_t t, const react_callback cb); + RepeatReaction* onRepeat(uint32_t interval, react_callback callback); /** * @brief Create a new RepeatReaction * - * @param t Interval, in microseconds - * @param cb Callback function + * @param delay Interval, in microseconds + * @param callback Callback function * @return RepeatReaction* */ - RepeatReaction* onRepeatMicros(const uint64_t t, const react_callback cb); + RepeatReaction* onRepeatMicros(uint64_t interval, react_callback callback); /** * @brief Create a new StreamReaction * * @param stream Arduino Stream object to monitor - * @param cb Callback function + * @param callback Callback function * @return StreamReaction* */ - StreamReaction* onAvailable(Stream& stream, const react_callback cb); + StreamReaction* onAvailable(Stream& stream, react_callback callback); /** * @brief Create a new ISRReaction (interrupt reaction) * * @param pin_number GPIO pin number * @param mode One of CHANGE, RISING, FALLING - * @param cb Interrupt handler to call. This should be a very simple function, - * ideally setting a flag variable or incrementing a counter. The function - * should be defined with ICACHE_RAM_ATTR. + * @param callback Interrupt handler to call. This should be a very simple + * function, ideally setting a flag variable or incrementing a counter. The + * function should be defined with ICACHE_RAM_ATTR. * @return ISRReaction* */ - ISRReaction* onInterrupt(const uint8_t pin_number, int mode, - const react_callback cb); + ISRReaction* onInterrupt(uint8_t pin_number, int mode, + react_callback callback); /** * @brief Create a new TickReaction * - * @param cb Callback function to be called at every loop execution + * @param callback Callback function to be called at every loop execution * @return TickReaction* */ - TickReaction* onTick(const react_callback cb); + TickReaction* onTick(react_callback callback); /** * @brief Remove a reaction from the list of active reactions @@ -333,7 +355,7 @@ class ReactESP { * @param reaction Reaction to remove */ void remove(Reaction* reaction); - + private: std::priority_queue, TriggerTimeCompare> @@ -341,6 +363,7 @@ class ReactESP { std::forward_list untimed_list; std::forward_list isr_reaction_list; std::forward_list isr_pending_list; + void tickTimed(); void tickUntimed(); void tickISR(); From 7f9b6466992bc92edba995a3f242e8abdc8a65c2 Mon Sep 17 00:00:00 2001 From: Offspring Date: Thu, 29 Aug 2024 08:10:36 -0400 Subject: [PATCH 2/2] remove move sem from ReactESP --- src/ReactESP.h | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/ReactESP.h b/src/ReactESP.h index 81f9d26..089e966 100644 --- a/src/ReactESP.h +++ b/src/ReactESP.h @@ -284,6 +284,12 @@ class ReactESP { } } + // Disabling copy and move semantics + ReactESP(const ReactESP&) = delete; + ReactESP(ReactESP&&) = delete; + ReactESP& operator=(const ReactESP&) = delete; + ReactESP& operator=(ReactESP&&) = delete; + void tick(); /// Static singleton reference to the instantiated ReactESP object