Skip to content

k_sleep() expires sooner than expected on STM32F4 (Cortex M4) #15216

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
avisconti opened this issue Apr 5, 2019 · 2 comments
Closed

k_sleep() expires sooner than expected on STM32F4 (Cortex M4) #15216

avisconti opened this issue Apr 5, 2019 · 2 comments
Assignees
Labels
area: Timer Timer bug The issue is a bug, or the PR is fixing a bug priority: medium Medium impact/importance bug
Milestone

Comments

@avisconti
Copy link
Collaborator

Description
I'm adding support for a new sensor shield (shield-iks01a3) which should be plugged
on Nucleo board (for example F411RE, which is based on Cortex M4).
I have a sample thread that reads sensor data every 2 secs using a k_sleep().
The sensors are programmed to generate triggers at 100hz or 200hz.
I see that k_sleep() very often takes less than 2secs.

I opened a PR (#15024) with a temptative fix that make disappear the issue.
My observation is that there is a much higher rate of z_clock_set_timeout() calls with
last_load < CYC_PER_TICK. If I force last_load to be at least CYC_PER_TICK the issue
again disappear.

To Reproduce
I'm currently not able to reproduce it w/o a proper h/w.
Only using Nucleo board plus x_nucleo_iks01a3 shield.
I can try to find an easy way.

Expected behavior
k_sleep() should wait the proper amount of time in all conditions.

Impact
Bug is annoying but not really severe for my development.

Screenshots or console output

Environment (please complete the following information):

  • OS: linux
  • Toolchain (e.g Zephyr SDK, ...)
  • latest code

Additional context

@avisconti avisconti added the bug The issue is a bug, or the PR is fixing a bug label Apr 5, 2019
@avisconti avisconti added this to the v1.14.0 milestone Apr 5, 2019
@andyross
Copy link
Collaborator

andyross commented Apr 5, 2019

So... the bug that I thought I saw during review of the first patch turned out to not be one. But on careful reflection I found a situation that could potentially be causing the behavior you're seeing.

While as explained, a LOAD value set to less than one tick is normal and desired, the driver was allowing VERY small LOAD values to be set (as low as 512 cycles), which create problems if you are running a busy app doing lots of interrupt masking for 512+ cycles. The patch in #15219 adjusts the defaults a little, and also cleans up the code to be able to handle multiple-wraparounds-while-interrupt-is-masked conditions more gracefully. Can you try it?

@nashif nashif added the priority: medium Medium impact/importance bug label Apr 6, 2019
@avisconti
Copy link
Collaborator Author

Sorry @andyross . I'm stuck with issues on system I use for development.
I hope to solve them asap and try this out.

@nashif nashif added the area: Timer Timer label Apr 10, 2019
andyross pushed a commit to andyross/zephyr that referenced this issue Apr 10, 2019
The SysTick logic looked logically sound, but it was allowing us to
set a LOAD value as low as 512 cycles.  On other platforms, that
minimum future interrupt delay is there to protect the "read, compute,
write, unmask" cycle that sets the new interrupt from trying to set
one sooner than it can handle.

But with SysTick, that value then becomes the value of the LOAD
register, which is effectively the frequency with which timer
interrupts arrive.  This has two side effects:

1. It opens up the possibility that future code that masks interrupts
   longer than 512 cycles will miss more than one overflow, slipping
   the clock backward as viewed by z_clock_announce().

2. The original code only understood one overflow cycle, so in the
   event we do set one of these very near timeouts and then mask
   interrupts, we'll only add at most one overflow to the "elapsed()"
   time, slipping the CURRENT time backward (actually turning it into
   a non-monotonic sawtooth which would slip every LOAD cycle) and
   thus messing up future scheduled interrupts, slipping those forward
   relative to what the ISR was counting.

This patch simplifies the logic for reading SysTick VAL/CTRL (the loop
wasn't needed), handles the case where we see more than one overflow,
and increases the MIN_DELAY cycles from 512 to 1/16th of a tick.

Fixes zephyrproject-rtos#15216

Signed-off-by: Andy Ross <[email protected]>
@nashif nashif closed this as completed in 110cab8 Apr 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Timer Timer bug The issue is a bug, or the PR is fixing a bug priority: medium Medium impact/importance bug
Projects
None yet
Development

No branches or pull requests

3 participants