keatis / dwt_delay

Microseconds delay lib for STM32 (or whatever ARM) based on DWT
MIT License
80 stars 28 forks source link

What happens if overflow DWT? #1

Closed ovsel43 closed 5 years ago

ovsel43 commented 5 years ago

What happens if overflow DWT? The function will not work correctly.

An example of the required code: currentTick = startTick = DWT->CYCCNT; endTick = startTtick + delay;

if ( endTick < startTtick ) while( currentTick > startTick || currentTick < endTick ) currentTick = DWT->CYCCNT; else while( currentTick > startTick && currentTick < endTick ) currentTick = DWT->CYCCNT;

keatis commented 5 years ago

You are right, DWT->CYCCNT overflows, and thus targetTick can overflow too. In this case DWT_Delay() will exit immediately since a condition in while loop would be false. Thanks for pointing that out! Will fix that soon.

Nable80 commented 5 years ago

There is a much simpler way to handle overflow:

  1. Avoid signed types when they are not needed. Avoid them as much as possible - signed overflow is UB (undefined behavior, one of the worst things in C).
  2. Compare delay amount with the difference between a current moment and a start one. Due to the defined (it's in the C standard) nature of unsigned overflow it would work automatically. In my projects I'm using similar code that is based on SysTick timer, e.g.:
    
    /* UNIX timestamp in milliseconds (JS-style time). */
    static volatile union {
    struct {
    uint32_t ms_low;
    uint32_t ms_high;
    };
    uint64_t ms_full;
    } __uptime;

void SysTick_Handler() { /* Update time counters by adding SYSTEMTICK_PERIOD_MS each SysTick interrupt.

/* Return number of milliseconds modulo 2**32 since some moment in past.

void stm32time_ms_sleep(uint32_t delay) { / Here we are dealing with the overflow in a right way. / const uint32_t start = stm32time_ms32(); while (stm32time_ms32() - start < delay) { __WFI(); } }


Thank you very much for sharing the idea of ``DWT->CYCCNT``, additional portable timer would help me in my current projects.
keatis commented 5 years ago

Oh, I meant uint32_t. But made a typo from the beginning and didn't notice that. Thanks for noting! Using signed is, of course, a mistake, and can potentially lead to errors as in #3.

And thanks for the second time for your example. I used similar approach in earlier version. But calculating target tick instead of the difference was a bad idea, since resulting condition for while() didn't work well after overflow. MCUs are not my profession so I realized that a bit late :)

Anyway, i will keep both ideas. Current code is good for those who is educating, and yours - for those who already calculate binaries right in their head. Thx :)

Nable80 commented 5 years ago

Btw, there are one or two more potentially interesting places where arithmetic can bite. The first one is SystemCoreClock/1000000 - I think SystemCoreClock doesn't have to be a multiple of 1e6, i.e. some precision may be lost here. At the same time one can't just fix this part in a usual way like (us * SystemCoreClock) / 1000000 because it would trigger overflow too early. The second problem is that this multiplication is a potential source of overflow anyway.

One can try to resolve these issues by using longer types for temporary variables but I think some words of warning (and a bit of information with exact numbers like in https://github.com/keatis/dwt_delay/issues/3#issuecomment-476879391) would be more than enough.