-
-
Notifications
You must be signed in to change notification settings - Fork 60
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix hang when animations are called too quickly. #214
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add an explanation of why this fixes the bug?
Also we need to fix the test that broke because of this change.
And we should add a changelog entry.
lib/pbio/src/light/animation.c
Outdated
@@ -120,6 +120,7 @@ PROCESS_THREAD(pbio_light_animation_process, ev, data) { | |||
clock_time_t interval = animation->next(animation); | |||
if (pbio_light_animation_is_started(animation)) { | |||
etimer_reset_with_new_interval(&animation->timer, interval); | |||
etimer_restart(&animation->timer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are already calling etimer_reset_with_new_interval()
here, why do we also need etimer_restart()
? It seems redundant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've never done any Contiki programming before so this might be wrong, but according to their docs, etimer_reset()
sets the timer to expire from the previous expire time whereas etimer_restart()
restarts the timer from the current time. Looking at the implementation for etimer_reset_with_new_interval()
, it internally calls etimer_reset()
(as you'd expect). I suspect with a low interval, it is possible that the new timer is set to fire in the past, which seems to cause problems.
|
||
PROCESS_CONTEXT_BEGIN(&pbio_light_animation_process); | ||
etimer_set(&animation->timer, 0); | ||
PROCESS_CONTEXT_END(&pbio_light_animation_process); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is better than the old hack. 👍
Restart only when necessary.
I've made a slight change to the fix so that it doesn't introduce drift unless it is necessary. |
Fix hang from calling animations too quickly.
Fixes: pybricks/support#1295