ARM-software / CMSIS_5

CMSIS Version 5 Development Repository
http://arm-software.github.io/CMSIS_5/index.html
Apache License 2.0
1.33k stars 1.08k forks source link

osKernelGetTickCount from IRQ #625

Open kjbracey opened 5 years ago

kjbracey commented 5 years ago

In CMSIS-RTOS 2.1.1, osKernelGetTickCount was changed to officially support being called from IRQ. (And was narrowed to 32-bit to achieve this, which I grumbled about at the time).

We hit an issue yesterday with some Mbed OS code that was calling Kernel::get_ms_count from a serial interrupt. It caused an assert in some glue code, which highlighted a problem.

If you are trying to get the tick count from interrupt routine, then it is quite likely that the interrupt routine doing the read woke the processor from a idle sleep.

If the system has an idle routine for tickless operation, it will look like this:

    uint32_t ticks_to_sleep = osKernelSuspend();
    // osKernelSuspend will call OS_Tick_Disable, cancelling the tick, which frees
    // up the os timer for the timed sleep
    uint64_t ticks_slept = mbed::internal::do_timed_sleep_relative(ticks_to_sleep, rtos_event_pending);
    MBED_ASSERT(ticks_slept < osWaitForever);
    osKernelResume((uint32_t) ticks_slept);

The interrupt that woke the processor will run in the exit path from the inner sleep, before we call osKernelResume.

So at that point, the kernel's tick count will be significantly out-of-date - it will be the time we entered sleep - we haven't yet added (or maybe even computed) ticks_slept.

This means that osKernelTickCount can be very misleading from interrupt, unless you are running a fully ticking build.

So I would suggest that the safety of osKernelTickCount from ISR be reconsidered, or at least given some significant qualification to its accuracy.

For Mbed OS, our Kernel::get_ms_count is already documented as not safe from ISR (based on pre-2.1.1 CMSIS-RTOS docs), and we can stick with that.

JonatanAntoni commented 5 years ago

Thanks, @kjbracey-arm for highlighting this issue.

The function osKernelTickCount is documented to read the current kernel tick. In case of a tickless low-power system the kernel tick is suspended. This needs to be considered when writing application code.

In case of a suspended kernel (i.e. systick timer stopped) the time spent sleeping needs to be determined using another timer. Hence the user application would need to provide some sort of high level get_time function that takes the current kernel state into account.

Do you think its possible to enhance your Kernel::get_ms_count in that sense?

Cheers, Jonatan

kjbracey commented 5 years ago

This needs to be considered when writing application code.

Well, more the "driver" code, if it's ISRs, but it's a significant issue - I think worth highlighting in the documentation.

I've been working in this area, implementing the sleep for years now, and this particular trap only occurred to me when I hit it. (Or at least a related issue)

People will naturally assume that "just read the kernel tick count" is a handy high-speed thing you can do to get a medium-resolution time without doing another hardware timer read - good for ISR use! - but it can fail significantly.

Seems at least as worth highlighting as the 32-bit wrap thing. Maybe something like "This function may be called from Interrupt Service Routines, but see note below" ..."Note about ISR use: if the system is a tickless low power system, then an ISR may have woken the system from sleep, in which case the kernel tick count read from that ISR will not yet have been updated to reflect the sleep time. It may be necessary to use an alternative timer in that situation - possibly the one that the platform uses to compute the sleep time.

Do you think its possible to enhance your Kernel::get_ms_count in that sense?

Yes, maybe. But is there an IRQ-safe API to interrogate the kernel state?

But for us, we never documented that as safe from IRQ anyway - we didn't track the 2.1.1 change - so I'm inclined to modify the code calling it. (And I'd have to make my non-RTOS alternative implementation IRQ safe...)

See the PR I just pushed for mbed event queue: https://github.com/kjbracey-arm/mbed-os/commit/566f784dc73b56c529e3c380a5ea3a5b023ddd0a

Refining the condition to "if kernel suspended" or "if in IRQ and kernel suspended" would be nice. But can we do that IRQ-safely?

In the case I'm hitting, that's wasting CPU time anyway - the serial ISR wants to post something "now", so there's no point reading the current time here in the ISR, and comparing again in dispatch. Would be better to have an "immediate" flag on the event, and skip both timer reads.

JonatanAntoni commented 5 years ago

People will naturally assume that "just read the kernel tick count" is a handy high-speed thing you can do [..]

Yes, but that's a wrong assumption on a tick-less system. So I think highlighting this dependency between osGetTickCount and osKernelSuspend/osKernelResume might be beneficial.

[..] is there an IRQ-safe API to interrogate the kernel state?

Yes, you can call osKernelGetState from interrupt. If this one gives you osKernelSuspended you should assume osGetTickCount returning an outdated value, i.e. the old value which needs to be updated with ticks_slept from your idle thread.

JonatanAntoni commented 5 years ago

Hi @kjbracey-arm,

Is it reasonable for you if we just see this issue as documentative?

Cheers, Jonatan

kjbracey commented 5 years ago

Yes, I think this can just be a documentation issue - it's usable from IRQs, but with this significant proviso in tickless systems.