RIOT-OS / RIOT

RIOT - The friendly OS for IoT
https://riot-os.org
GNU Lesser General Public License v2.1
4.83k stars 1.97k forks source link

MSP430: periph_timer clock config wrong #8251

Open roberthartung opened 6 years ago

roberthartung commented 6 years ago

The TelosB node and Tmote Sky node should be identical (hardware wise). However, using xtimer_sleep(2) is not sleeping two seconds, but more like 7 seconds. Will investigate.

PeterKietzmann commented 6 years ago

@roberthartung on my TelosB xtimer_sleep(1) takes about three seconds, just FYI

roberthartung commented 6 years ago

@PeterKietzmann Nevermind, it was 1 second, not two. But still, we're off the same factor!

PeterKietzmann commented 6 years ago

Whoops, yes. It helps if you can read...

roberthartung commented 6 years ago

Whoops, yes. It helps if you can read...

nono, it was wrong in the issue/my code. You didnt do any mistake :D I edited the issue sneaky beaky like ;)

kYc0o commented 5 years ago

@roberthartung if the hardware is exactly the same, then the behaviour should be the same. AFAIK we don't support TmoteSky, so I guess there should be at least one difference. As you may know, xtimer timings rely strongly on the initialisation of the hardware timer, which must be multiple of 1MHz. Thus, for me it seems that the problem might be periph_timer related.

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

PeterKietzmann commented 4 years ago

@roberthartung does the issue still exist? I'll set the "don't stale" label so it's not automatically being closed

roberthartung commented 4 years ago

@PeterKietzmann Good question. Have to test again!

roberthartung commented 4 years ago

@MichelRottleuthner Tested again with a simple xtimer_sleep(n), n \in {1,2,4}:

#include <stdio.h>
#include "xtimer.h"

int main(void)
{
    while (1) {
    printf("sleeping 1\r\n");
        xtimer_sleep(1);
    printf("done.\r\n");

    printf("sleeping 2\r\n");
        xtimer_sleep(2);
    printf("done.\r\n");

    printf("sleeping 4\r\n");
        xtimer_sleep(4);
    printf("done.\r\n");
    }
}

The configuration still seems off:

2020-01-31 08:56:19,779 # sleeping 1
2020-01-31 08:56:22,333 # done.
2020-01-31 08:56:22,348 # sleeping 2
2020-01-31 08:56:28,976 # done.
2020-01-31 08:56:28,991 # sleeping 4
2020-01-31 08:56:42,306 # done.

Tested on a MAXFOR MTM-CM5000MSP.

MichelRottleuthner commented 4 years ago

I think this is not related to xtimer itself. Changing this in cpu/msp430fxyz/periph/timer.c:

---TIMER_BASE->CTL = (TIMER_CTL_TASSEL_SMCLK | TIMER_CTL_ID_DIV8);
+++TIMER_BASE->CTL = (TIMER_CTL_TASSEL_SMCLK | TIMER_CTL_ID_DIV2);

gives me something at least close to the expected speed.

Note: changing only the above line was just meant as a quick test and will probably break configuration of other peripherals driven by SMCLK (if not already broken as is). The overall clock configuration needs to be considered for a proper fix. Looking at the clock configuration and the periph_timer implementation it looks like the code could also use some love ;) Even with a correction to the divider it may not be the best choice to drive the timer by the inaccurate DCO clock. A quick look at the datasheet indicates that using X1 or X2 as clock sources might be better alternatives.

@maribu aren't you also using MSP430s? What do you think about this?

maribu commented 4 years ago

@maribu aren't you also using MSP430s?

Not really. I worked once on some code cleanup, but I even can remember what exactly I did. But I can recommend Inrias IoT-Lab for testing, if you (like me) have no MSP430 boards to test your changes on.

What do you think about this?

I have no idea about the hardware, but if I guess correctly currently an internal clock source (DSO) is used and you suggest to use an external crystal for higher precision. This sounds like a good idea, but maybe external crystals are not available on all boards. But in any case, I don't really have the expertise in the MSP430 area to be of much help.

MichelRottleuthner commented 4 years ago

Ok, thanks for the quick response.

(...) but if I guess correctly (...)

Yep, that's pretty much what I wanted to say. I'm also no active user of the MSPs but from the datasheet it looks rather straight forward.

@roberthartung do you want to put some effort into fixing this? I'd be happy to assist.

roberthartung commented 4 years ago

I don't think there is an external crystal available on these boards.

MichelRottleuthner commented 4 years ago

Yep it seems like there is indeed no HF crystal available. But for the Tmote Sky it looks like it has at least some LF crystal X0. They also say "though the DCO frequency changes with voltage and temperature, it may be calibrated by using the 32kHz oscillator". Also it might be worth to check if there is some external resistor connected to the R_OSC to increase accuracy a bit.

maribu commented 2 months ago

I just rediscovered this issue. https://github.com/RIOT-OS/RIOT/pull/20582 fixes it