microsoft / jacdac-stm32x0

Jacdac implementation for STM32F0 and similar
MIT License
11 stars 10 forks source link

add cpu freq check to timer set #50

Closed xmeow closed 10 months ago

xmeow commented 10 months ago

The timer pre-scaler could be varying during runtime, especially for such case: https://github.com/microsoft/jacdac/issues/1293

The servo enable update the pll from 16MHz to 48M, which is 3 times faster than origin one. And leads to subsequence packet receiver working in a wrong timeout params, that is header wait timeout from 250us to 86us and full packet timeout from around 300us to 100us.

image

mmoskal commented 10 months ago

We already update the prescaler when we change the clock speed. However, it seems the new prescaler is only taken into account on update event. Could you try the following change, instead of yours?

void tim_update_prescaler(void) {
    target_disable_irq();
    // first store, the time
    uint64_t us = tim_get_micros();

    // set the prescaler
    LL_TIM_SetPrescaler(TIMx, cpu_mhz - 1);

    // arrange for the update event to be generated very soon to load the new prescaler
    timeoff = us - 0xfffd;
    LL_TIM_DisableCounter(TIMx);
    TIMx->CNT = 0xfffe;
    LL_TIM_EnableCounter(TIMx);
    LL_TIM_ClearFlag_UPDATE(TIMx);

    target_enable_irq();
}
xmeow commented 10 months ago

@mmoskal You are right about the prescaler. It looks like the only thing we need to modify is to add a generation event to tell the hardware to reset the counter and load the registers.

xmeow commented 10 months ago

And some verify on tim_get_micros continues:

void tim_update_prescaler(void) {
    uint64_t m = tim_get_micros();

    target_disable_irq();
    LL_TIM_DisableCounter(TIMx);
    LL_TIM_SetPrescaler(TIMx, cpu_mhz - 1);
    uint64_t us = tim_get_micros();
    timeoff = us - 0xffff;
    LL_TIM_GenerateEvent_UPDATE(TIMx); // the update evt will also clear timer cnt
    LL_TIM_EnableCounter(TIMx);
    target_enable_irq();

    uint64_t m1 = tim_get_micros();
    uint8_t *p = (uint8_t *)&m;
    DMESG("before %x %x %x %x", p[0], p[1], p[2], p[3]);

    p = (uint8_t *)&m1;
    DMESG("after  %x %x %x %x", p[0], p[1], p[2], p[3]);

    DMESG("tim %d", (int)(m1 - m));

}

The log output:

before 0x77 0x24 0x92 0x0
after  0x7F 0x24 0x92 0x0
tim 8
mmoskal commented 10 months ago

@xmeow did you try my code from my comment above? it should do approximately what your code does but be a little more resilient for the update event happening close the generated update event. Thank you for testing!

xmeow commented 10 months ago

Yeah, I tried both and looks the same to me. Sorry I could not tell the difference between the two implementations, it is beyond my knowledge. I just have a feeling that using something from the SDK would be better, fall back to your implementation in the PR.

xmeow commented 10 months ago

@mmoskal Sorry to bother, maybe you have missed the last message. I have tried your code and fall back to it in lastest commit. Everything works great after the prescaler take effect.

mmoskal commented 10 months ago

Thank you very much!