Fix stm32 timer periodic

[Enoch247]

Contribution description

From the commit msg:

cpu/stm32/periph/timer: remove unneeded header

I see no reason this header should be included. It does not exist in
RIOT's source tree. This patch removes the include.

and

cpu/stm32/periph/timer: fix execution flow

The implmentation of `timer_set_absolute()` has The following problems.
First, it attempts to restore the auto reload register (ARR) to it's
default if the ARR was previosly set by `timer_set_periodic()` by
comparing it to the channel's capture compare (CC) register _after_ it
has already set the CC register. Secondly, it clears spurious IRQs
_after_ the CC register has been set. If the value being set is equal to
the timer's current count (or the two become equal before the supurios
IRQ clearing happens), this could cause a legitimate IRQ to be cleared.

The implmentation of `timer_set()` has the same error in handling the
ARR as described above.

This patch reorders the operations of both functions to do:

1. handle ARR
2. clear spurious IRQs
3. set channel's CC
4. enable IRQ

Additionally, the calulation of `value` in `timer_set()` is moved
earlier in the function's exec path as a pedantic measure.

Testing procedure

I tested by doing the following:

1. make -C tests/periph_timer BOARD=nucleo-f767zi all flash term
2. press s
3. press [ENTER]
4. observe test passes
5. make -C tests/periph_timer_periodic BOARD=nucleo-f767zi all flash term
6. press s
7. press [ENTER]
8. observe test passes
9. make -C tests/periph_timer_short_relative_set BOARD=nucleo-f767zi all flash term
10. press s
11. press [ENTER]
12. observe test passes

Issues/PRs references

• none known

[benpicco]

Thank you for the fix!
Just one thing:

[benpicco]

You want to move that below the irq_disable() otherwise if this thread is interrupted, the value will no longer be correct.

[Enoch247]

I debated doing that before submitting the PR. I didn't feel strongly either way, but my reasoning for making it as it is follows. Please check my thinking for flaws.

I reasoned that from a caller's stand point, the value of timeout is an offset from "now". "Now" is a moving target until value is computed, therefore it should be computed as soon as possible, but it doesn't have to be in the critical section. There is no harm in servicing IRQs or higher priority threads between calculating value and scheduling the timer to fire once it reaches value, so long as the timer doesn't go past value + one rollover period of the timer. If the caller needs this guarantee, I think the IRQs need disabled at a higher level then any part of the body of this function can provide.

If this line were moved to the critical section, this function could sill be blocked before entering the critical section. Once unblocked and inside the critical section value could be computed, but it would be not what the caller expected. It would be "now" + however long the blocked time was before the critical section. Note that my current implementation also has this flaw too since it can be blocked between being called and the first line of c code of the body of the function.

In both cases, if the caller is worried about interrupts and other threads messing with setting a timer, the caller better handle it, because the driver will do something bad either way. So best to just keep the critical section as small as possible.

[Enoch247]

As I understand it. The critical sections in this driver exists so that the read-modify-writes on the timers' registers and _oneshot are safely synchronized between the timer's ISR and any concurrent threads accessing the driver. The critical sections aren't meant to ensure any kind of timing/blocking guarantees.

[benpicco]

That makes sense - and we do have a check for value > timeout when in the meantime the timeout already passed, so this is indeed a valid solution.

[riot-ci]

Murdock results

✔️ PASSED

eeb359e cpu/stm32/periph/timer: fix execution flow

Success	Failures	Total	Runtime
6919	0	6919	11m:26s

Artifacts

• Documentation preview

[maribu]

This looks like clangd header insertion artifact. I had a lot of those until I finally just globally disabled it. Maybe we should add some sane default clangd configuration to the repo, so that others don't run into the same issue.

[benpicco]

bors merge

[bors]

Build succeeded:

• Murdock
• static-tests
• tools-build-success