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-format (v21.1.8) reports: 1 file(s) not formatted

• src/EStopManager.cpp

clang-tidy (v21.1.8) reports: 35 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:33:31: warning: [cppcoreguidelines-avoid-non-const-global-variables]

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

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

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

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

     35 | static TaskHandle_t s_estopTask = nullptr;
        |                     ^

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

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

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

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

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

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

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

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

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

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

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

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

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

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

     43 | static bool s_estopInitialized = false;
        |             ^

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

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

     62 | static void estopmgr_managerTask(void* pvParameters)
        |             ^
  src/EStopManager.cpp:85:3: note: +1, including nesting penalty of 0, nesting level increased to 1
     85 |   while (!s_killEStopManagerRequested.load(std::memory_order_relaxed)) {
        |   ^
  src/EStopManager.cpp:93:5: note: +2, including nesting penalty of 1, nesting level increased to 2
     93 |     if (s_externallyTriggered.exchange(false, std::memory_order_relaxed)) {
        |     ^
  src/EStopManager.cpp:114:34: note: +1
    114 |     bool pressedEdge = (btnState && !lastBtnState);
        |                                  ^
  src/EStopManager.cpp:117:5: note: +2, including nesting penalty of 1, nesting level increased to 2
    117 |     switch (state) {
        |     ^
  src/EStopManager.cpp:121:9: note: +3, including nesting penalty of 2, nesting level increased to 3
    121 |         if (rearmBlocked) {
        |         ^
  src/EStopManager.cpp:122:11: note: +4, including nesting penalty of 3, nesting level increased to 4
    122 |           if (now < rearmAt) {
        |           ^
  src/EStopManager.cpp:131:9: note: +3, including nesting penalty of 2, nesting level increased to 3
    131 |         if (btnState) {
        |         ^
  src/EStopManager.cpp:139:9: note: +3, including nesting penalty of 2, nesting level increased to 3
    139 |         if (pressedEdge) {
        |         ^
  src/EStopManager.cpp:146:9: note: +3, including nesting penalty of 2, nesting level increased to 3
    146 |         if (!btnState) {  // released before hold time -> go back to Active
        |         ^
  src/EStopManager.cpp:148:16: note: +1, nesting level increased to 3
    148 |         } else if (now >= deactivatesAt) {
        |                ^
  src/EStopManager.cpp:155:9: note: +3, including nesting penalty of 2, nesting level increased to 3
    155 |         if (!btnState) {  // fully released -> clear E-Stop
        |         ^

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

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

     65 |   gpio_num_t estopPin = static_cast<gpio_num_t>(reinterpret_cast<intptr_t>(pvParameters));
        |   ^~~~~~~~~~
        |   auto

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

  do not use reinterpret_cast

     65 |   gpio_num_t estopPin = static_cast<gpio_num_t>(reinterpret_cast<intptr_t>(pvParameters));
        |                                                 ^

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

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

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

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

  use a trailing return type for this function

    180 | static bool estopmgr_setPinImpl(gpio_num_t pin)
        |        ~~~~ ^                                  
        |        auto                                     -> bool

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

  use a trailing return type for this function

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

• src/EStopManager.cpp:233:16: warning: [cppcoreguidelines-init-variables]

  variable 'pin' is not initialized

    233 |     gpio_num_t pin;
        |                ^

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

  do not use reinterpret_cast

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

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

  integer to pointer cast pessimizes optimization opportunities

• src/EStopManager.cpp:264:65: warning: [cppcoreguidelines-avoid-magic-numbers]

  4096 is a magic number; consider replacing it with a named constant

    264 |   if (TaskUtils::TaskCreateUniversal(estopmgr_managerTask, TAG, 4096, argPtr, 5, &s_estopTask, 1) != pdPASS) {  // TODO: Profile stack size and set priority
        |                                                                 ^

• src/EStopManager.cpp:264:79: warning: [cppcoreguidelines-avoid-magic-numbers]

  5 is a magic number; consider replacing it with a named constant

    264 |   if (TaskUtils::TaskCreateUniversal(estopmgr_managerTask, TAG, 4096, argPtr, 5, &s_estopTask, 1) != pdPASS) {  // TODO: Profile stack size and set priority
        |                                                                               ^

• 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:318:20: warning: [modernize-use-trailing-return-type]

  use a trailing return type for this function

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

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

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

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

• src/EStopManager.cpp:324:5: warning: [readability-else-after-return]

  do not use 'else' after 'return'

    324 |   } else {
        |     ^~~~~~
    325 |     return estopmgr_taskStop();
        |     ~~~~~~~~~~~~~~~~~~~~~~~~~~~
    326 |   }
        |   ~

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

  use a trailing return type for this function

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

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

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

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

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

  use a trailing return type for this function

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

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

  use a trailing return type for this function

    364 | 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.