diff --git a/.changesets/fix-scheduler-race-condition.md b/.changesets/fix-scheduler-race-condition.md new file mode 100644 index 000000000..a811cd55c --- /dev/null +++ b/.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) diff --git a/Inc/HALAL/Models/Packets/PacketValue.hpp b/Inc/HALAL/Models/Packets/PacketValue.hpp index 1e9d373c4..f1dd6158e 100644 --- a/Inc/HALAL/Models/Packets/PacketValue.hpp +++ b/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; diff --git a/Inc/HALAL/Models/TimerDomain/TimerDomain.hpp b/Inc/HALAL/Models/TimerDomain/TimerDomain.hpp index 45d9f3d0a..d8889c954 100644 --- a/Inc/HALAL/Models/TimerDomain/TimerDomain.hpp +++ b/Inc/HALAL/Models/TimerDomain/TimerDomain.hpp @@ -546,7 +546,8 @@ TimerXList ST_LIB::compile_error("This timer is used by the scheduler"); } - if (requests[i].request != TimerRequest::AnyGeneralPurpose && + if ((requests[i].request != TimerRequest::AnyGeneralPurpose) && + (requests[i].request != TimerRequest::Any32bit) && (requests[i].request < 1 || requests[i].request > 24 || (requests[i].request > 17 && requests[i].request < 23))) { ST_LIB::compile_error("Invalid TimerRequest value for timer"); diff --git a/Inc/HALAL/Services/ADC/ADC.hpp b/Inc/HALAL/Services/ADC/ADC.hpp index 105a932b7..7356a89b1 100644 --- a/Inc/HALAL/Services/ADC/ADC.hpp +++ b/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 consteval std::size_t inscribe(Ctx& ctx) const { const auto gpio_idx = gpio.inscribe(ctx); diff --git a/Inc/HALAL/Services/Encoder/Encoder.hpp b/Inc/HALAL/Services/Encoder/Encoder.hpp index e0529ead5..8e1ddd1b9 100644 --- a/Inc/HALAL/Services/Encoder/Encoder.hpp +++ b/Inc/HALAL/Services/Encoder/Encoder.hpp @@ -63,7 +63,7 @@ template class Encoder { } tim->instance->tim->PSC = 5; - if constexpr (tim->is_32bit_instance) { + if constexpr (TimerWrapper::is_32bit_instance) { tim->instance->tim->ARR = UINT32_MAX; } else { tim->instance->tim->ARR = UINT16_MAX; diff --git a/Inc/HALAL/Services/Time/Scheduler.hpp b/Inc/HALAL/Services/Time/Scheduler.hpp index dfabc0bdb..ceb16e198 100644 --- a/Inc/HALAL/Services/Time/Scheduler.hpp +++ b/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 #include @@ -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(); diff --git a/Inc/HALAL/Services/Time/TimerWrapper.hpp b/Inc/HALAL/Services/Time/TimerWrapper.hpp index 0a81056a9..caece7b71 100644 --- a/Inc/HALAL/Services/Time/TimerWrapper.hpp +++ b/Inc/HALAL/Services/Time/TimerWrapper.hpp @@ -324,6 +324,8 @@ template struct TimerWrapper { inline TIM_TypeDef* get_cmsis_handle() { return instance->tim; } inline void set_prescaler(uint16_t psc) { instance->tim->PSC = psc; } + // TODO: 16 bit and 32 bit version (?) + inline void set_limit_value(uint32_t arr) { instance->tim->ARR = arr; } inline void configure32bit(void (*callback)(void*), void* callback_data, uint32_t period) { static_assert( diff --git a/Src/HALAL/Services/Time/Scheduler.cpp b/Src/HALAL/Services/Time/Scheduler.cpp index 62a2333ad..f8cbf1069 100644 --- a/Src/HALAL/Services/Time/Scheduler.cpp +++ b/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(__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(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(Scheduler::kMaxTasks)) [[unlikely]] + if (idx >= Scheduler::kMaxTasks) [[unlikely]] return static_cast(Scheduler::INVALID_ID); Scheduler::free_bitmap_ &= ~(1UL << idx); return static_cast(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(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(diff); } - Scheduler_global_timer->ARR = static_cast(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(global_tick_us_ + task.period_us); insert_sorted(candidate_id); } + SchedUnlock(); } schedule_next_interval(); diff --git a/Tests/Time/scheduler_test.cpp b/Tests/Time/scheduler_test.cpp index 7f468b870..ed20ab2da 100644 --- a/Tests/Time/scheduler_test.cpp +++ b/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++) { diff --git a/toolchains/stm32.cmake b/toolchains/stm32.cmake index 082d3101d..95cc954a4 100644 --- a/toolchains/stm32.cmake +++ b/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()