Improve EStop Manager's atomics + set to Idle when changing pin/enabled.

[nullstalgia]

This PR aims to reduce the use of atomics and global static variables within the EStop manager, and improve the resiliency of the ones that are remain required.

Additionally, the EStop manager now resets back to the Idle state when being disabled or when changing the pin (otherwise you could not disable it if it was falsely triggered prior to disabling, or if the previously-chosen pin was incorrect and would always read Low).

[github-actions]

Cpp-Linter Report ⚠️

Some files did not pass the configured checks!

clang-tidy (v21.1.8) reports: 31 concern(s)

• include/estop/EStopManager.h:1:1: warning: [portability-avoid-pragma-once]

  avoid 'pragma once' directive; use include guards instead

      1 | #pragma once
        | ^

• include/estop/EStopManager.h:10:22: warning: [modernize-use-trailing-return-type]

  use a trailing return type for this function

     10 |   [[nodiscard]] bool Init();
        |                 ~~~~ ^     
        |                 auto        -> bool

• include/estop/EStopManager.h:11:8: warning: [modernize-use-trailing-return-type]

  use a trailing return type for this function

     11 |   bool SetEStopEnabled(bool enabled);
        |   ~~~~ ^                            
        |   auto                               -> bool

• include/estop/EStopManager.h:12:8: warning: [modernize-use-trailing-return-type]

  use a trailing return type for this function

     12 |   bool SetEStopPin(gpio_num_t pin);
        |   ~~~~ ^                          
        |   auto                             -> bool

• include/estop/EStopManager.h:13:8: warning: [modernize-use-trailing-return-type]

  use a trailing return type for this function

     13 |   bool IsEStopped();
        |   ~~~~ ^           
        |   auto              -> bool

• include/estop/EStopManager.h:14:11: warning: [modernize-use-trailing-return-type]

  use a trailing return type for this function

     14 |   int64_t LastEStopped();
        |           ^

• src/EStopManager.cpp:36:31: warning: [cppcoreguidelines-avoid-non-const-global-variables]

  variable 's_estopMutex' is non-const and globally accessible, consider making it const

     36 | static OpenShock::SimpleMutex s_estopMutex = {};
        |                               ^

• src/EStopManager.cpp:38:21: warning: [cppcoreguidelines-avoid-non-const-global-variables]

  variable 's_estopTask' is non-const and globally accessible, consider making it const

     38 | static TaskHandle_t s_estopTask = nullptr;
        |                     ^

• src/EStopManager.cpp:39:19: warning: [cppcoreguidelines-avoid-non-const-global-variables]

  variable 's_estopPin' is non-const and globally accessible, consider making it const

     39 | static gpio_num_t s_estopPin    = GPIO_NUM_NC;  // Passed to task via pointer argument
        |                   ^

• src/EStopManager.cpp:42:29: warning: [cppcoreguidelines-avoid-non-const-global-variables]

  variable 's_estopActivatedAt' is non-const and globally accessible, consider making it const

     42 | static std::atomic<int64_t> s_estopActivatedAt       = 0;  // When == 0, EStop not active. When != 0, EStop is active.
        |                             ^

• src/EStopManager.cpp:43:26: warning: [cppcoreguidelines-avoid-non-const-global-variables]

  variable 's_externallyTriggered' is non-const and globally accessible, consider making it const

     43 | static std::atomic<bool> s_externallyTriggered       = false;
        |                          ^

• src/EStopManager.cpp:44:26: warning: [cppcoreguidelines-avoid-non-const-global-variables]

  variable 's_killEStopManagerRequested' is non-const and globally accessible, consider making it const

     44 | static std::atomic<bool> s_killEStopManagerRequested = false;
        |                          ^

• src/EStopManager.cpp:46:13: warning: [cppcoreguidelines-avoid-non-const-global-variables]

  variable 's_estopInitialized' is non-const and globally accessible, consider making it const

     46 | static bool s_estopInitialized = false;
        |             ^

• src/EStopManager.cpp:65:13: warning: [readability-function-cognitive-complexity]

  function 'estopmgr_managerTask' has cognitive complexity of 26 (threshold 25)

     65 | static void estopmgr_managerTask(void* pvParameters)
        |             ^
  src/EStopManager.cpp:88:3: note: +1, including nesting penalty of 0, nesting level increased to 1
     88 |   while (!s_killEStopManagerRequested.load(std::memory_order_relaxed)) {
        |   ^
  src/EStopManager.cpp:96:5: note: +2, including nesting penalty of 1, nesting level increased to 2
     96 |     if (s_externallyTriggered.exchange(false, std::memory_order_relaxed)) {
        |     ^
  src/EStopManager.cpp:117:34: note: +1
    117 |     bool pressedEdge = (btnState && !lastBtnState);
        |                                  ^
  src/EStopManager.cpp:120:5: note: +2, including nesting penalty of 1, nesting level increased to 2
    120 |     switch (state) {
        |     ^
  src/EStopManager.cpp:124:9: note: +3, including nesting penalty of 2, nesting level increased to 3
    124 |         if (rearmBlocked) {
        |         ^
  src/EStopManager.cpp:125:11: note: +4, including nesting penalty of 3, nesting level increased to 4
    125 |           if (now < rearmAt) {
        |           ^
  src/EStopManager.cpp:134:9: note: +3, including nesting penalty of 2, nesting level increased to 3
    134 |         if (btnState) {
        |         ^
  src/EStopManager.cpp:142:9: note: +3, including nesting penalty of 2, nesting level increased to 3
    142 |         if (pressedEdge) {
        |         ^
  src/EStopManager.cpp:149:9: note: +3, including nesting penalty of 2, nesting level increased to 3
    149 |         if (!btnState) {  // released before hold time -> go back to Active
        |         ^
  src/EStopManager.cpp:151:16: note: +1, nesting level increased to 3
    151 |         } else if (now >= deactivatesAt) {
        |                ^
  src/EStopManager.cpp:158:9: note: +3, including nesting penalty of 2, nesting level increased to 3
    158 |         if (!btnState) {  // fully released -> clear E-Stop
        |         ^

• src/EStopManager.cpp:68:3: warning: [modernize-use-auto]

  use auto when initializing with a cast to avoid duplicating the type name

     68 |   gpio_num_t estopPin = static_cast<gpio_num_t>(reinterpret_cast<uintptr_t>(pvParameters));
        |   ^~~~~~~~~~
        |   auto

• src/EStopManager.cpp:68:49: warning: [cppcoreguidelines-pro-type-reinterpret-cast]

  do not use reinterpret_cast

     68 |   gpio_num_t estopPin = static_cast<gpio_num_t>(reinterpret_cast<uintptr_t>(pvParameters));
        |                                                 ^

• src/EStopManager.cpp:76:22: warning: [cppcoreguidelines-avoid-magic-numbers]

  0xFFFF is a magic number; consider replacing it with a named constant

     76 |   uint16_t history = 0xFFFF;  // Bit history of samples, 0 is pressed
        |                      ^

• src/EStopManager.cpp:187:13: warning: [modernize-use-trailing-return-type]

  use a trailing return type for this function

    187 | static bool estopmgr_configurePin(gpio_num_t pin)
        |        ~~~~ ^                                    
        |        auto                                       -> bool

• src/EStopManager.cpp:224:13: warning: [modernize-use-trailing-return-type]

  use a trailing return type for this function

    224 | static bool estopmgr_taskStart()
        |        ~~~~ ^                   
        |        auto                      -> bool

• src/EStopManager.cpp:262:18: warning: [cppcoreguidelines-pro-type-reinterpret-cast]

  do not use reinterpret_cast

    262 |   void* argPtr = reinterpret_cast<void*>(static_cast<uintptr_t>(s_estopPin));
        |                  ^

• src/EStopManager.cpp:262:18: warning: [performance-no-int-to-ptr]

  integer to pointer cast pessimizes optimization opportunities

• src/EStopManager.cpp:273:13: warning: [modernize-use-trailing-return-type]

  use a trailing return type for this function

    273 | static bool estopmgr_taskStop()
        |        ~~~~ ^                  
        |        auto                     -> bool

• src/EStopManager.cpp:291:20: warning: [modernize-use-trailing-return-type]

  use a trailing return type for this function

    291 | bool EStopManager::Init()
        | ~~~~               ^     
        | auto                      -> bool

• src/EStopManager.cpp:308:25: warning: [bugprone-reserved-identifier]

  declaration uses identifier 'lock__', which is a reserved identifier

    308 |   OpenShock::ScopedLock lock__(&s_estopMutex);
        |                         ^~~~~~
        |                         lock_

• src/EStopManager.cpp:319:20: warning: [modernize-use-trailing-return-type]

  use a trailing return type for this function

    319 | bool EStopManager::SetEStopEnabled(bool enabled)
        | ~~~~               ^                            
        | auto                                             -> bool

• src/EStopManager.cpp:321:25: warning: [bugprone-reserved-identifier]

  declaration uses identifier 'lock__', which is a reserved identifier

    321 |   OpenShock::ScopedLock lock__(&s_estopMutex);
        |                         ^~~~~~
        |                         lock_

• src/EStopManager.cpp:330:20: warning: [modernize-use-trailing-return-type]

  use a trailing return type for this function

    330 | bool EStopManager::SetEStopPin(gpio_num_t pin)
        | ~~~~               ^                          
        | auto                                           -> bool

• src/EStopManager.cpp:332:25: warning: [bugprone-reserved-identifier]

  declaration uses identifier 'lock__', which is a reserved identifier

    332 |   OpenShock::ScopedLock lock__(&s_estopMutex);
        |                         ^~~~~~
        |                         lock_

• src/EStopManager.cpp:369:20: warning: [modernize-use-trailing-return-type]

  use a trailing return type for this function

    369 | bool EStopManager::IsEStopped()
        | ~~~~               ^           
        | auto                            -> bool

• src/EStopManager.cpp:374:23: warning: [modernize-use-trailing-return-type]

  use a trailing return type for this function

    374 | int64_t EStopManager::LastEStopped()
        |                       ^

• src/message_handlers/websocket/gateway/Trigger.cpp:19:3: warning: [readability-qualified-auto]

  'auto msg' can be declared as 'const auto *msg'

     19 |   auto msg = root->payload_as_Trigger();
        |   ^~~~
        |   const auto *

Have any feedback or feature suggestions? Share it here.

[Copilot]

Pull request overview

This PR refactors the EStop manager to reduce shared global state/atomics, introduces a more explicit task start/stop lifecycle, and renames the software-initiated trigger API. It also aims to ensure the EStop state returns to Idle when disabling the manager or changing the configured GPIO pin.

Changes:

• Renamed the network/software EStop entrypoint from Trigger() to SoftwareTrigger() and updated the websocket trigger handler accordingly.
• Refactored the EStop manager task lifecycle into estopmgr_taskStart() / estopmgr_taskStop(), with a kill-request flag to exit the task loop.
• Adjusted EStop state publication to track last-published state locally and attempt to reset to Idle on task shutdown.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

File	Description
src/message_handlers/websocket/gateway/Trigger.cpp	Updates gateway trigger handling to call the renamed software EStop API.
src/EStopManager.cpp	Refactors EStop manager task lifecycle, atomics usage, pin handling, and state publication/reset behavior.
include/estop/EStopManager.h	Renames public API from Trigger() to SoftwareTrigger().

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.