diff --git a/.changesets/fix-scheduler-race-condition.md b/.changesets/fix-scheduler-race-condition.md new file mode 100644 index 000000000..3c5e7d95e --- /dev/null +++ b/.changesets/fix-scheduler-race-condition.md @@ -0,0 +1,5 @@ +release: minor +summary: fix remaining scheduler race conditions and add warning when tasks are not ran in time + +The mechanism for checking if tasks are not ran in time is very simple but that also means the scheduler can only know if a task has not been called when the waiting time runs out for the second time. +This means you will know if you're too slow to execute the task in less than 2x its period but not if you're between 1 and 2x its period. 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 632c5845a..f5c3a4ddc 100644 --- a/Inc/HALAL/Models/TimerDomain/TimerDomain.hpp +++ b/Inc/HALAL/Models/TimerDomain/TimerDomain.hpp @@ -576,7 +576,8 @@ struct TimerDomain { 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"); @@ -817,6 +818,7 @@ struct TimerDomain { } } + // NOTE: This is a bit slower than timerwrapper.get_clock_frequency(), so preferrably use that static inline uint32_t get_timer_frequency(TIM_TypeDef* tim) { uint32_t result = 0; if ((tim == TIM2) || (tim == TIM3) || (tim == TIM4) || (tim == TIM5) || (tim == TIM6) || 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 dea8fe595..fd14f05fc 100644 --- a/Inc/HALAL/Services/Time/TimerWrapper.hpp +++ b/Inc/HALAL/Services/Time/TimerWrapper.hpp @@ -420,6 +420,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/Models/TimerDomain/TimerDomain.cpp b/Src/HALAL/Models/TimerDomain/TimerDomain.cpp index b1ce1df1a..3a13b905b 100644 --- a/Src/HALAL/Models/TimerDomain/TimerDomain.cpp +++ b/Src/HALAL/Models/TimerDomain/TimerDomain.cpp @@ -57,7 +57,9 @@ static void TIM_IC_CaptureCallback(const uint32_t timer_idx, uint32_t channel) { if (falling_value < info->period) info->value_falling = falling_value; } else [[unlikely]] { - ErrorHandler("TimerDomain::input_capture_info was modified"); + // TimerDomain::input_capture_info was modified or STM interrupts are all over the place + // either way, this is a no-op + // ErrorHandler("TimerDomain::input_capture_info was modified"); } } diff --git a/Src/HALAL/Services/Time/Scheduler.cpp b/Src/HALAL/Services/Time/Scheduler.cpp index 62a2333ad..ac215b4b4 100644 --- a/Src/HALAL/Services/Time/Scheduler.cpp +++ b/Src/HALAL/Services/Time/Scheduler.cpp @@ -1,12 +1,12 @@ /* - * Scheduler.hpp + * Scheduler.cpp * * Created on: 17 nov. 2025 * Author: Victor (coauthor Stephan) */ #include "HALAL/Services/Time/Scheduler.hpp" #include "HALAL/Models/TimerDomain/TimerDomain.hpp" -#include "ErrorHandler/ErrorHandler.hpp" +#include "HALAL/Services/InfoWarning/InfoWarning.hpp" #include @@ -31,22 +31,7 @@ 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; -} +uint16_t failing_id = Scheduler::INVALID_ID; // ---------------------------- @@ -73,75 +58,10 @@ void Scheduler_start(void) { ST_LIB::TimerDomain::callbacks[ST_LIB::timer_idxmap[static_cast(SCHEDULER_TIMER_DOMAIN )]] = Scheduler_global_timer_callback; - // TODO: change this to use TimerDomain::get_timer_clock()? - uint32_t prescaler = (SystemCoreClock / Scheduler::FREQUENCY); - // setup prescaler - { - // ref manual: section 8.7.7 RCC domain 1 clock configuration register - uint32_t ahb_prescaler = RCC->D1CFGR & RCC_D1CFGR_HPRE_Msk; - if ((ahb_prescaler & 0b1000) != 0) { - switch (ahb_prescaler) { - case 0b1000: - prescaler /= 2; - break; - case 0b1001: - prescaler /= 4; - break; - case 0b1010: - prescaler /= 8; - break; - case 0b1011: - prescaler /= 16; - break; - case 0b1100: - prescaler /= 64; - break; - case 0b1101: - prescaler /= 128; - break; - case 0b1110: - prescaler /= 256; - break; - case 0b1111: - prescaler /= 512; - break; - } - } - - // ref manual: section 8.7.8: RCC domain 2 clock configuration register - uint32_t apb1_prescaler = (RCC->D2CFGR & RCC_D2CFGR_D2PPRE1_Msk) >> RCC_D2CFGR_D2PPRE1_Pos; - if ((apb1_prescaler & 0b100) != 0) { - switch (apb1_prescaler) { - case 0b100: - prescaler /= 2; - break; - case 0b101: - prescaler /= 4; - break; - case 0b110: - prescaler /= 8; - break; - case 0b111: - prescaler /= 16; - break; - } - } - // tim2clk = 2 x pclk1 when apb1_prescaler != 1 - if (apb1_prescaler != 1) { - prescaler *= 2; - } - - if (prescaler > 1) { - prescaler--; - } - } - - if (prescaler == 0 || prescaler > 0xFFFF) { - ErrorHandler("Invalid prescaler value: %u", prescaler); - return; - } - - Scheduler_global_timer->PSC = (uint16_t)prescaler; + uint16_t prescaler = + (uint16_t)(ST_LIB::TimerDomain::get_timer_frequency(Scheduler_global_timer) / + Scheduler::FREQUENCY); + Scheduler_global_timer->PSC = prescaler; Scheduler_global_timer->ARR = 0; Scheduler_global_timer->DIER |= LL_TIM_DIER_UIE; Scheduler_global_timer->CR1 = @@ -149,31 +69,44 @@ void Scheduler_start(void) { Scheduler_global_timer->CNT = 0; /* Clear counter value */ - NVIC_EnableIRQ(SCHEDULER_GLOBAL_TIMER_IRQn); CLEAR_BIT(Scheduler_global_timer->SR, LL_TIM_SR_UIF); /* clear update interrupt flag */ Scheduler::schedule_next_interval(); } void Scheduler::update() { + // NOTE: Only _one_ id will be shown per call to update() + if (failing_id != Scheduler::INVALID_ID) [[unlikely]] { + WARNING("Too slow, could not execute task %u in time", failing_id); + failing_id = Scheduler::INVALID_ID; + } + 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 +205,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 +220,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 +241,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]] { + failing_id = 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/common_tests.cpp b/Tests/Time/common_tests.cpp index 847b996ef..eca45e017 100644 --- a/Tests/Time/common_tests.cpp +++ b/Tests/Time/common_tests.cpp @@ -1,5 +1,6 @@ #include #include "ErrorHandler/ErrorHandler.hpp" +#include "HALAL/Services/InfoWarning/InfoWarning.hpp" std::string ErrorHandlerModel::line; std::string ErrorHandlerModel::func; @@ -32,3 +33,33 @@ void ErrorHandlerModel::ErrorHandlerTrigger(string format, ...) { } void ErrorHandlerModel::ErrorHandlerUpdate() {} + +std::string InfoWarning::line; +std::string InfoWarning::func; +std::string InfoWarning::file; + +namespace ST_LIB::TestInfoWarning { +bool fail_on_error = false; +int call_count = 0; + +void reset() { + fail_on_error = false; + call_count = 0; +} + +void set_fail_on_error(bool enabled) { fail_on_error = enabled; } +}; // namespace ST_LIB::TestInfoWarning + +void InfoWarning::SetMetaData(int line, const char* func, const char* file) { + InfoWarning::line = to_string(line); + InfoWarning::func = string(func); + InfoWarning::file = string(file); +} + +void InfoWarning::InfoWarningTrigger(string format, ...) { + (void)format; + ST_LIB::TestInfoWarning::call_count++; + if (ST_LIB::TestInfoWarning::fail_on_error) { + EXPECT_EQ(1, 0); + } +} 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++) {