cpu/mips32r2_common: use periph_common timer_set

[cladmi]

Use 'timer_set' provided by 'periph_common' instead of re-implementing it.

It also fixes 'mips32r2_generic' that should have been defining
'PERIPH_TIMER_PROVIDES_SET' as 'mips32r2_common' uses 'periph_common' but is not
necessary if removing the function.

Issues/PRs references

Its required for #8711

It was also disabled by #5757 in periph_conf.h but the function can be removed as it has the same behavior as periph_common https://github.com/RIOT-OS/RIOT/blob/2018.01/drivers/periph_common/timer.c#L24.

[cladmi]

I have not tested that the board still works. And I will let Murdock check the builds.

[cladmi]

Another option to solve this could be to define PERIPH_TIMER_PROVIDES_SET for mips32r2_generic.

[jnohlgard]

Looks OK to me, but I don't have any MIPS board to test on.

Also, the return value of timer_set_absolute and timer_clear are wrong compared to the API documentation, should be return 1. Feel free to fix it or leave it

[neiljay]

Is the generic timer_set always called with interrupts disabled ?

int timer_set(tim_t dev, int channel, unsigned int timeout)
{
return timer_set_absolute(dev, channel, timer_read(dev) + timeout);
}

if you took a high latency interrupt (or even worse rescheduled) between the timer_read and the timer_set_absolute your timer is not going to be very accurate ?

[cladmi]

@neiljay I did not think about that, I just copied what was done for other cpus. I could change it to defining PERIPH_TIMER_PROVIDES_SET in mips32r2_generic periph_conf.h if you prefer.

[neiljay]

@cladmi maybe fix the generic implementation to disable interrupts as well first ?

int timer_set(tim_t dev, int channel, unsigned int timeout)
{
uint32_t status = irq_disable();
int ret = timer_set_absolute(dev, channel, timer_read(dev) + timeout);
irq_restore(status);
return ret
}

[jnohlgard]

I agree, it's better to fix the generic implementation

[cladmi]

The general fix should be done in another PR then. This one can wait until its done.
I could do something next week.

[cladmi]

Thinking again about this. Would it really help to put this irq_disable ?
I think, it does not really change the final behavior.
Let me explain my thoughts with a timer_set(5 seconds) and a 3 seconds re-schedule (to give numbers).

Without irq_disable

• if re-scheduled between timer_get and timer_set_absolute, timer_get is evaluated before re-scheduling, so the callback is triggered after 5 seconds since the function call.
• If re-scheduled before timer_get, timer_get will be done 3 seconds after and the callback is after 8 seconds in total.

With irq_disable

• In the irq_disable section, it cannot be re-scheduled so its a 5 seconds in total.
• If re-scheduled before irq_disable, timer_get will also be done 3 seconds after and the callback is after 8 seconds in total.

So adding irq_disable only protects a few cycles more. Your precision depend on how long can a high priority task take the cpu anyway.

[neiljay]

If the de-sceduled / high latency period is bigger than the set period, the timer may never fire at all (or atleast until it wraps). swapping you example so we want a timer to fire in 3s but he reschedule takes 5. time(s) fn 0 timer_get() <reschedule> 5 time_set_absolute(0+3) 5 > 3 so time never fires until the timer wraps. Neil

…

On 03/05/18 17:41, Gaëtan Harter wrote: Thinking again about this. Would it really help to put this |irq_disable| ? I think, it does not really change the final behavior. Let me explain my thoughts with a |timer_set(5 seconds)| and a 3 seconds re-schedule (to give numbers). Without |irq_disable| * if re-scheduled between |timer_get| and |timer_set_absolute|, |timer| get is evaluated before re-scheduling, so the callback is triggered after 5 seconds since the function call. * If re-scheduled before |timer_get|, |timer_get| will be done 3 seconds after and the callback is after 8 seconds in total. With |irq_disable| * In the |irq_disable| section, it cannot be re-scheduled so its a 5 seconds in total. * If re-scheduled before |irq_disable|, |timer_get| will also be done 3 seconds after and the callback is after 8 seconds in total. So adding |irq_disable| only protects a few cycles more. Your precision depend on how long can a high priority task take the cpu anyway. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8720 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AUEU6gtmF39ur33nlROeZRI3UrB2PIHMks5tbXi5gaJpZM4SYqAR>.

[jnohlgard]

I am working on a benchmark for periph_timer which will be useful for giving some statistics on latency and target accuracy,. It will not however be able to interrupt the timer_set function in the precise place you are discussing for triggering that particular race condition. See #8531 for the WIP state.

[cladmi]

I proposed another PR that just fixes the double definition to allow merging it now: #8817
This way it keeps the current behavior but disables defining timer_set in periph_common.

[cladmi]

Closed in favor of #8817