fix (tested on PCU, LV-BMS, HV-BMS and LCU)

.changesets/fix-scheduler-race-condition.md

@@ -0,0 +1,4 @@
+release: patch
+summary: fix remaining scheduler race conditions
+
+Currently only tested on LCU despite title saying otherwise (too many changes to say it was tested)

Inc/HALAL/Models/Packets/PacketValue.hpp

@@ -11,7 +11,7 @@ template <> class PacketValue<> {
 public:
     using value_type = empty_type;
     PacketValue() = default;
-    ~PacketValue() = default;
+    virtual ~PacketValue() = default;
     virtual void* get_pointer() = 0;
     virtual void set_pointer(void* pointer) = 0;
     virtual size_t get_size() = 0;

Inc/HALAL/Services/ADC/ADC.hpp

@@ -189,15 +189,7 @@ struct ADCDomain {
             ClockPrescaler prescaler = ClockPrescaler::DIV1,
             uint32_t sample_rate_hz = 0
         )
-            : ADC(
-                  pin,
-                  resolution,
-                  sample_time,
-                  prescaler,
-                  sample_rate_hz,
-                  peripheral,
-                  channel
-              ) {}
+            : ADC(pin, resolution, sample_time, prescaler, sample_rate_hz, peripheral, channel) {}
 
         template <class Ctx> consteval std::size_t inscribe(Ctx& ctx) const {
             const auto gpio_idx = gpio.inscribe(ctx);

Inc/HALAL/Services/Encoder/Encoder.hpp

@@ -63,7 +63,7 @@ template <const TimerDomain::Timer& dev> class Encoder {
         }
 
         tim->instance->tim->PSC = 5;
-        if constexpr (tim->is_32bit_instance) {
+        if constexpr (TimerWrapper<dev>::is_32bit_instance) {
             tim->instance->tim->ARR = UINT32_MAX;
         } else {
             tim->instance->tim->ARR = UINT16_MAX;

Inc/HALAL/Services/Time/Scheduler.hpp

@@ -9,7 +9,15 @@
 /* Uso del scheduler, descrito en la wiki:
  * https://wiki.hyperloopupv.com/es/firmware/Timing/Scheduler */
 
+/* To allow debugging of inline functions only when testing */
+#ifdef SIM_ON
+#define HYPER_INLINE inline
+#else
+#define HYPER_INLINE
+#endif
+
 #include "stm32h7xx_ll_tim_wrapper.h"
+#include "HALAL/Models/Packets/Packet.hpp"
 
 #include <array>
 #include <cstdint>
@@ -33,9 +41,7 @@ struct Scheduler {
     // temporary, will be removed
     [[deprecated]] static inline void start() {}
     static void update();
-    static inline uint64_t get_global_tick() {
-        return global_tick_us_ + Scheduler_global_timer->CNT;
-    }
+    static uint64_t get_global_tick();
 
     static uint16_t register_task(uint32_t period_us, callback_t func);
     static bool unregister_task(uint16_t id);
@@ -44,7 +50,7 @@ struct Scheduler {
     static bool cancel_timeout(uint16_t id);
 
     // internal
-    static void on_timer_update();
+    static inline void on_timer_update();
     static void schedule_next_interval();
     static constexpr uint32_t FREQUENCY = 1'000'000u; // 1 MHz -> 1us precision
 #ifndef SIM_ON
@@ -92,10 +98,19 @@ struct Scheduler {
     static void remove_sorted(uint8_t id);
 
     // helpers
-    static inline uint8_t get_at(uint8_t idx);
-    static inline void set_at(uint8_t idx, uint8_t id);
-    static inline void pop_front();
-    static inline uint8_t front_id();
+    static HYPER_INLINE uint8_t get_at(uint8_t idx) {
+        return (uint8_t)((sorted_task_ids_ >> (idx * 4)) & 0xF);
+    }
+    static HYPER_INLINE void set_at(uint8_t idx, uint8_t id) {
+        uint64_t shift = idx * 4;
+        uint64_t clearmask = ~(0x0FULL << shift);
+        Scheduler::sorted_task_ids_ = (sorted_task_ids_ & clearmask) | ((uint64_t)id << shift);
+    }
+    static HYPER_INLINE uint8_t front_id() { return *((uint8_t*)&sorted_task_ids_) & 0xF; }
+    static HYPER_INLINE void pop_front() {
+        Scheduler::active_task_count_--;
+        Scheduler::sorted_task_ids_ >>= 4;
+    }
 
     static inline void global_timer_disable();
     static inline void global_timer_enable();

Src/HALAL/Services/Time/Scheduler.cpp

@@ -1,5 +1,5 @@
 /*
- * Scheduler.hpp
+ * Scheduler.cpp
  *
  * Created on: 17 nov. 2025
  *     Author: Victor (coauthor Stephan)
@@ -31,23 +31,6 @@ uint64_t Scheduler::global_tick_us_{0};
 uint32_t Scheduler::current_interval_us_{0};
 uint16_t Scheduler::timeout_idx_{1};
 
-inline uint8_t Scheduler::get_at(uint8_t idx) {
-    int word_idx = idx > 7;
-    uint32_t shift = (idx & 7) << 2;
-    return (((uint32_t*)&sorted_task_ids_)[word_idx] & (0x0F << shift)) >> shift;
-}
-inline void Scheduler::set_at(uint8_t idx, uint8_t id) {
-    uint32_t shift = idx * 4;
-    uint64_t clearmask = ~(0xFF << shift);
-    Scheduler::sorted_task_ids_ = (sorted_task_ids_ & clearmask) | (id << shift);
-}
-inline uint8_t Scheduler::front_id() { return *((uint8_t*)&sorted_task_ids_) & 0xF; }
-inline void Scheduler::pop_front() {
-    // O(1) remove of logical index 0
-    Scheduler::active_task_count_--;
-    Scheduler::sorted_task_ids_ >>= 4;
-}
-
 // ----------------------------
 
 inline void Scheduler::global_timer_disable() {
@@ -159,21 +142,29 @@ void Scheduler::update() {
     while (ready_bitmap_ != 0u) {
         uint32_t bit_index = static_cast<uint32_t>(__builtin_ctz(ready_bitmap_));
 
-        CLEAR_BIT(ready_bitmap_, 1u << bit_index);
-
         Task& task = tasks_[bit_index];
+
         task.callback();
+
+        SchedLock();
+        CLEAR_BIT(ready_bitmap_, 1u << bit_index);
         if (!task.repeating) [[unlikely]] {
-            SchedLock();
             release_slot(static_cast<uint8_t>(bit_index));
-            SchedUnlock();
         }
+        SchedUnlock();
     }
 }
 
+uint64_t Scheduler::get_global_tick() {
+    SchedLock();
+    uint64_t val = global_tick_us_ + Scheduler_global_timer->CNT;
+    SchedUnlock();
+    return val;
+}
+
 inline uint8_t Scheduler::allocate_slot() {
     uint32_t idx = __builtin_ffs(Scheduler::free_bitmap_) - 1;
-    if (idx > static_cast<int>(Scheduler::kMaxTasks)) [[unlikely]]
+    if (idx >= Scheduler::kMaxTasks) [[unlikely]]
         return static_cast<uint8_t>(Scheduler::INVALID_ID);
     Scheduler::free_bitmap_ &= ~(1UL << idx);
     return static_cast<uint8_t>(idx);
@@ -272,9 +263,12 @@ void Scheduler::schedule_next_interval() {
         return;
     }
 
+    SchedLock();
     uint8_t next_id = Scheduler::front_id(); // sorted_task_ids_[0]
     Task& next_task = tasks_[next_id];
     int32_t diff = (int32_t)(next_task.next_fire_us - static_cast<uint32_t>(global_tick_us_));
+    SchedUnlock();
+
     if (diff >= -1 && diff <= 1) [[unlikely]] {
         current_interval_us_ = 1;
         SET_BIT(Scheduler_global_timer->EGR, TIM_EGR_UG); // This should cause an interrupt
@@ -284,18 +278,18 @@ void Scheduler::schedule_next_interval() {
         } else {
             current_interval_us_ = static_cast<uint32_t>(diff);
         }
-        Scheduler_global_timer->ARR = static_cast<uint32_t>(current_interval_us_ - 1u);
-        while (Scheduler_global_timer->CNT > Scheduler_global_timer->ARR) [[unlikely]] {
-            uint32_t offset = Scheduler_global_timer->CNT - Scheduler_global_timer->ARR;
-            current_interval_us_ = offset;
-            SET_BIT(Scheduler_global_timer->EGR, TIM_EGR_UG); // This should cause an interrupt
-            Scheduler_global_timer->CNT = Scheduler_global_timer->CNT + offset;
+
+        Scheduler_global_timer->ARR = current_interval_us_ - 1u;
+        if (Scheduler_global_timer->CNT > Scheduler_global_timer->ARR) [[unlikely]] {
+            uint32_t cnt_temp = Scheduler_global_timer->CNT;
+            Scheduler_global_timer->CNT = 0;
+            global_tick_us_ += cnt_temp;
         }
     }
     Scheduler::global_timer_enable();
 }
 
-void Scheduler::on_timer_update() {
+inline void Scheduler::on_timer_update() {
     global_tick_us_ += current_interval_us_;
 
     while (active_task_count_ > 0) { // Pop all due tasks, several might be due in the same tick
@@ -305,15 +299,20 @@ void Scheduler::on_timer_update() {
         if (diff > 0) [[likely]] {
             break; // Task is in the future, stop processing
         }
-        pop_front();
+        uint32_t task_bit = 1u << candidate_id;
 
+        SchedLock();
+        pop_front();
         // mark task as ready
-        SET_BIT(ready_bitmap_, 1u << candidate_id);
-
+        if ((ready_bitmap_ & task_bit) != 0) [[unlikely]] {
+            ErrorHandler("Too slow, could not execute task %u in time", candidate_id);
+        }
+        SET_BIT(ready_bitmap_, task_bit);
         if (task.repeating) [[likely]] {
             task.next_fire_us = static_cast<uint32_t>(global_tick_us_ + task.period_us);
             insert_sorted(candidate_id);
         }
+        SchedUnlock();
     }
 
     schedule_next_interval();

Tests/Time/scheduler_test.cpp

@@ -32,6 +32,43 @@ class SchedulerTests : public ::testing::Test {
     }
 };
 
+TEST_F(SchedulerTests, GetAt) {
+    Scheduler::sorted_task_ids_ = 0xFEDCBA9876543210ULL;
+    for (uint64_t i = 0; i < 16; i++) {
+        uint64_t val = Scheduler::get_at(i);
+        EXPECT_EQ(val, i);
+    }
+}
+
+TEST_F(SchedulerTests, SetAt) {
+    uint64_t original = 0x1EDCBA9876543210ULL;
+    for (uint64_t i = 0; i < 16; i++) {
+        Scheduler::sorted_task_ids_ = original;
+        Scheduler::set_at(i, 0xF);
+        EXPECT_EQ(Scheduler::sorted_task_ids_, original | (0xFULL << (i * 4)));
+    }
+}
+
+TEST_F(SchedulerTests, FrontId_PopFront) {
+    Scheduler::sorted_task_ids_ = 0xFEDCBA9876543210ULL;
+    for (uint8_t i = 0; i < 16; i++) {
+        uint8_t id = Scheduler::front_id();
+        Scheduler::pop_front();
+        EXPECT_EQ(id, i);
+    }
+}
+
+TEST_F(SchedulerTests, Max16Tasks) {
+    uint16_t task_id;
+    for (int i = 0; i < 16; i++) {
+        task_id = Scheduler::register_task(1, &fake_workload);
+        EXPECT_EQ(task_id == Scheduler::INVALID_ID, false);
+    }
+    // 17th task should not give a valid i
+    task_id = Scheduler::register_task(1, &fake_workload);
+    EXPECT_EQ(task_id == Scheduler::INVALID_ID, true);
+}
+
 TEST_F(SchedulerTests, FreeBitmap) {
     Scheduler::register_task(10, &fake_workload);
     EXPECT_EQ(Scheduler::free_bitmap_, 0xFFFF'FFFE);
@@ -44,13 +81,16 @@ TEST_F(SchedulerTests, TaskRegistration) {
 
 TEST_F(SchedulerTests, TaskExecutionShort) {
     Scheduler::register_task(10, &fake_workload);
-    Scheduler::start();
     TIM2_BASE->PSC = 2; // quicker test
 
     constexpr int NUM_TICKS = 1'000;
     for (int i = 0; i < NUM_TICKS; i++) {
+        uint64_t tick = Scheduler::get_global_tick();
+        EXPECT_EQ(tick, i);
+
         for (int j = 0; j <= TIM2_BASE->PSC; j++)
             TIM2_BASE->inc_cnt_and_check(1);
+
         Scheduler::update();
     }
     // 1000 ticks / 10 ticks/task = 100 executions.
@@ -59,7 +99,6 @@ TEST_F(SchedulerTests, TaskExecutionShort) {
 
 TEST_F(SchedulerTests, TaskExecutionLong) {
     Scheduler::register_task(10, &fake_workload);
-    Scheduler::start();
     // TIM2_BASE->ARR = 500;
     TIM2_BASE->generate_update();
     TIM2_BASE->PSC = 2; // quicker test
@@ -75,7 +114,6 @@ TEST_F(SchedulerTests, TaskExecutionLong) {
 
 TEST_F(SchedulerTests, SetTimeout) {
     Scheduler::set_timeout(10, &fake_workload);
-    Scheduler::start();
     TIM2_BASE->PSC = 2; // quicker test
 
     constexpr int NUM_TICKS = 100;
@@ -90,7 +128,6 @@ TEST_F(SchedulerTests, SetTimeout) {
 TEST_F(SchedulerTests, GlobalTickOverflow) {
     Scheduler::global_tick_us_ = 0xFFFFFFF0ULL; // Near 32-bit max
     Scheduler::register_task(20, &fake_workload);
-    Scheduler::start();
     TIM2_BASE->PSC = 2; // quicker test
 
     constexpr int NUM_TICKS = 100;
@@ -133,7 +170,6 @@ TEST_F(SchedulerTests, GlobalTickOverflowManyTasks) {
     Scheduler::register_task(10, &multiple_task_1);
     Scheduler::register_task(20, &multiple_task_2);
     Scheduler::register_task(30, &multiple_task_3);
-    Scheduler::start();
     TIM2_BASE->PSC = 2; // quicker test
 
     constexpr int NUM_TICKS = 100;
@@ -151,7 +187,6 @@ TEST_F(SchedulerTests, GlobalTickOverflowManyTasks) {
 
 TEST_F(SchedulerTests, TimeoutClearAddTask) {
     uint8_t timeout_id = Scheduler::set_timeout(10, &fake_workload);
-    Scheduler::start();
     TIM2_BASE->PSC = 2; // quicker test
 
     constexpr int NUM_TICKS = 100;
@@ -190,7 +225,6 @@ TEST_F(SchedulerTests, TaskDe_ReRegistration) {
     uint8_t connecting_task = Scheduler::register_task(10, &connecting_cyclic);
     uint8_t operational_task = 0;
     uint8_t fault_task = 0;
-    Scheduler::start();
     TIM2_BASE->PSC = 2; // quicker test
 
     constexpr int NUM_TICKS = 100;
@@ -231,7 +265,6 @@ TEST_F(SchedulerTests, MultipleTasks) {
     Scheduler::register_task(5, &multiple_task_5);
     Scheduler::register_task(6, &multiple_task_6);
 
-    Scheduler::start();
     TIM2_BASE->PSC = 2; // quicker test
     constexpr int NUM_TICKS = 300;
     for (int i = 0; i < NUM_TICKS; i++) {
@@ -258,7 +291,6 @@ TEST_F(SchedulerTests, SameTaskMultipleTimes) {
     Scheduler::register_task(6, &multiple_task_1);
 
     multiple_task1count = 0;
-    Scheduler::start();
     TIM2_BASE->PSC = 2; // quicker test
     constexpr int NUM_TICKS = 300;
     for (int i = 0; i < NUM_TICKS; i++) {

toolchains/stm32.cmake

@@ -34,7 +34,8 @@ endfunction()
 
 function(_stm32_extract_version _path _out_var)
     _stm32_resolve_path("${_path}" _resolved)
-    string(REGEX MATCH "STM32CubeCLT[_-]([0-9]+\\.[0-9]+\\.[0-9]+)" _match "${_resolved}")
+    string(TOUPPER "${_resolved}" _resolved_upper)
+    string(REGEX MATCH "STM32CUBECLT[_-]([0-9]+\\.[0-9]+\\.[0-9]+)" _match "${_resolved_upper}")
     set(${_out_var} "${CMAKE_MATCH_1}" PARENT_SCOPE)
 endfunction()