feedback on the SYST API

[japaric]

"Some nits about the SYST implementation: fn clear () does not make a whole lot of sense as clearing actually disables the SysTick timer because the exception is triggered when the count changes from 1 -> 0 which is also what causes the reload. Similarly the manuals suggest you should load the counter with the value 1 (for which there's currently no safe API) to ensure that the timer will not start with some offset in the first turn and also the correct value for the reload is actually - 1."

from rust-embedded/cortex-m-quickstart#15 (comment)

cc @whitequark

[whitequark]

Yeah, I screwed this up. The proposed changes all seem good to me, can you apply them?

[japaric]

I'm finally visiting the details of this issue.

Similarly the manuals suggest you should load the counter with the value 1 (for which there's currently no safe API) to ensure that the timer will not start with some offset in the first turn

Where did you get this info from, @therealprof? I'm looking the official ARM documentation for Cortex-M0 devices and it says "A write of any value clears the field to 0, and also clears the SYST_CSR.COUNTFLAG bit to 0." (emphasis mine) about the CURRENT field (first 24 bits) of the CVR register. Based on that clear_current() is the right API to provide because a set_current(value: u8) would have the same semantics as the written value would be ignored so there would be no gain in picking the value to write to the CVR register.

fn clear () does not make a whole lot of sense as clearing actually disables the SysTick timer because the exception is triggered when the count changes from 1 -> 0 which is also what causes the reload

The documentation says:

The correct initialization sequence for the SysTick counter is:

• Program reload value.
• Clear current value.
• Program Control and Status register.

So syst.set_reload(value); syst.clear_current(); syst.enable_counter() is the proper way to initialize the clock. According to my tests, it seems that clear_current can be omitted and the clock still works correctly. I tested with this code:

    syst.set_clock_source(SystClkSource::Core);
    syst.set_reload(8_000_000 - 1);
    // syst.set_current(0); // <- doesn't make any difference
    syst.enable_counter();
    let start = syst.get_current();

    let mut cnt = 0;
    while !syst.has_wrapped() {
        cnt += 1;
    }

    asm::bkpt();

And got values of start = 7999939 and cnt = 86021 regardless of whether set_current was present (I added the set_current API just for this test; set_current(0) is the same as the clear_current we have today).

and also the correct value for the reload is actually - 1.

This is indeed stated in the documentation. I would personally lean towards documenting this fact in the API docs rather than trying to do something smart like implicitly performing the subtraction in set_reload (or the addition in the case of get_reload) though.

TL;DR I lean towards leaving the API as it is and documenting the "reload formula" in {set,get}_reload. Thoughts?

[therealprof]

@japaric

I'm not quite sure where it was mentioned you should set it to 1 but that certainly makes sense from the perspective of getting determinable timings.

Also the "STM32F0xxx Cortex-M0 programming manual" contains one more sentence in the paragraph from your citing which is quite relevant:

The SysTick counter reload and current value are undefined at reset, the correct initialization
sequence for the SysTick counter is:

• Program reload value.
• Clear current value.
• Program Control and Status register.

You definitely do not want to omit the clear_counter(). If you do and run the SysTick clock at low frequency and/or the counter at a high frequency and the random initial value is high you'll potentially end up with a long delay before it'll trigger the exception for the first time...

[japaric]

I'm not quite sure where it was mentioned you should set it to 1 but that certainly makes sense from the perspective of getting determinable timings.

But the thing is that reference manual says that the value you write to RVR doesn't matter because the write operation will clear the register. I have confirmed that on hardware: any write sets the RVR value to 0.

I lean towards leaving the API as it is and documenting the "reload formula"

Both the reload formula and the correct initalization sequence are being added to documentation in #75