Systick handler

[ghost]

It is annoying that modm preoccupies the systick handler. I have a project where it's needed elsewhere and the only option in the moment is to rename it and call modm's handler from external code. It's not difficult to create a option in lbuild to implement such a feature or to improve the systick clock code. I can see there is a "custom irq handler" project, Is this the strategy to solve this problem?

[salkinium]

The modm:platform:clock module (should probably be renamed to modm:platform:systick) is optional (except that modm:platform:rcc depends on it accidentally). Only the board support packages depend on it directly.

So the idea is that you can not include this module in your project via a custom BSP and then replace this module with your own modm::Clock implementation that uses a different timer (I think STM32 HAL sacrifices TIM5).

You can probably even make it a modm:platform:clock-timer module with a lbuild option to choose the STM32 timer you want to use (in case you need TIM5 too). I would expect the code to be similiar to SysTick incl. timer value interpolation to get the microsecond timer.

[salkinium]

Oh, I just realized something way simpler:

You can overwrite the weak modm::chrono::{milli, micro}_clock::now() functions and not call SysTickTimer::initialize(), then the SysTick is free.

[salkinium]

Ah, but that doesn't let go of the SysTick_Handler() of course. So you still need to go with the original plan.

[ghost]

Thanks i will look into it.

I noticed the deprecated volatile's in modm::platform::clock the correct thing would be to replace them with atomic's with the correct memory_order for performance reasons (platform dependent). I know that atomic's have normal operators but i don't use them, they are not fast on all platforms and you can't see something special is going then reading the code. Do you want a patch?

static std::atomic_uint32_t milli_time{0};
static std::atomic_uint32_t micro_time{0};
static std::atomic_uint8_t interrupt{false};

//Writing
milli_time.fetch_add(systick_step, std::memory_order_release);

//Reading
ms = milli_time.load(std::memory_order_acquire);

[salkinium]

I'm not sure if atomics is useful, since the SysTick->VAL overflows in hardware regardless of interrupt state. That's why there is the polling loop via the volatile bool interrupt to read the values, rather than an atomic.

However, since the SysTick interrupt is set at the lowest interrupt priority, there exists the possibility to delay the interrupt for too long and have the clock "jump back" by 1ms or 250ms. You would also have to poll the interrupt flag and manually call the interrupt handler in that case.

I'm a bit unhappy about that…

[chris-durand]

I think volatile will continue work in GCC but you have no guarantee

Actually only some parts of volatile have been deprecated, p1152r4 has the details. The only language feature I see here that was deprecated in C++20 is the volatile compound assignment (milli_time += …). It might be needed to write it as milli_time = milli_time + … in some future language version but it would most likely retain the same semantics.

For memory mapped IO volatile will not go away soon, with the meaning of "don't optimize this memory access, it has side effects". All the peripheral registers in the device headers are declared volatile (ST uses __IO, which is a #define for volatile). It's the only way to tell the compiler to not mess with something like UART->DR = 0x01; UART->DR = 0x02;. The last thing one wants is that the compiler optimizes away the first write because it thinks it's a redundant store. Or it could skip writes to peripherals and cache them in a register instead.

It even looks like bitwise compound operations (|=, &=) on volatile will be de-deprecated again in C++23: P2327R1: De-deprecating volatile compound operations.

[ghost]

Atomics will only prevent a race condition then now() is called in a legal C++20 way (I think volatile will continue work in GCC but you have no guarantee). It won't affect the "jump back" problem.

[salkinium]

Don't let me discourage you from fixing this, I'm by no means an expert on the C++ memory model. Perhaps using load-store-exclusive to access the value is better than the boolean thing we have now (probably less code) resulting in better latency. Just keep in mind that atomic operations on ARMv6-M are implemented via atomic lock regardless of memory order (which is perhaps wrong)