evtimer: initial import by miri64 · Pull Request #6767 · RIOT-OS/RIOT

miri64 · 2017-03-20T14:33:29Z

Extends #5999 by doc, some helpers for message-based events and a test. Also rebases it to current master.

miri64 · 2017-03-20T14:34:21Z

Desperately needed for the new NDP ;-).

miri64 · 2017-03-20T14:36:05Z

I tested it on native and are just about to test it on samr21-xpro.

miri64 · 2017-03-20T14:59:20Z

Mhhhh I hit some race condition on samr21-xpro... If I just run it as is (i.e. BOARD=samr21-xpro make -C tests/evtimer/ flash term) the test hangs after "If yes, the tests were successful". However if I apply the following patch:

diff --git a/sys/evtimer/evtimer.c b/sys/evtimer/evtimer.c
index 797f146..d8da2e2 100644
--- a/sys/evtimer/evtimer.c
+++ b/sys/evtimer/evtimer.c
@@ -124,6 +124,7 @@ void evtimer_add(evtimer_t *evtimer, evtimer_event_t *event)
     _update_head_offset(evtimer);
     _add_event_to_list(evtimer, event);
 
+    xtimer_usleep(1);
     if (evtimer->events == event) {
         _set_timer(&evtimer->timer, event->offset);
     }

it runs through. Any idea why?

miri64 · 2017-03-20T15:00:02Z

Same goes for CFLAGS_OPT=-O0 or if I add some debug output btw: It runs through.

miri64 · 2017-03-20T15:08:42Z

Added a workaround for the race condition for now:

    /* XXX: next two lines fix known race condition on Cortex-M0 boards */
    volatile int i = 1;
    while (i--);

miri64 · 2017-03-20T15:10:33Z

(adds 14 byte to the text segment).

kaspar030 · 2017-03-20T17:21:43Z

tests/evtimer/main.c

+    { .event = { .offset = 1500 }, .msg = { .content = { .ptr = "supposed to be 1500" } } },
+    { .event = { .offset = 659 }, .msg = { .content = { .ptr = "supposed to be 659" } } },
+    { .event = { .offset = 3954 }, .msg = { .content = { .ptr = "supposed to be 3954" } } },
+    { .msg = { .content = { .ptr = NULL } }  },     /* sentinel */


please use helper variable for length (unsigned nevent = sizeof(events)/sizeof(events[0]);

kaspar030 · 2017-03-20T17:22:52Z

tests/evtimer/main.c

+        evtimer_add_msg(&evtimer, event, pid);
+        count++;
+    }
+    printf("Are the reception times of all %i msgs near to the supposed values?\n",


close instead of near?

kaspar030 · 2017-03-20T17:25:13Z

sys/include/evtimer.h

+    evtimer_event_t *events;    /**< Event queue */
+} evtimer_t;
+
+/**


What do you think about moving all msg related stuff into "evtimer_msg.h"?

Moved to evtimer/msg.h

There's still a #include "msg.h" in this file :-)

kaspar030 · 2017-03-20T17:25:59Z

the test hangs after "If yes, the tests were successful".

That is indeed weird.

miri64 · 2017-03-21T08:35:45Z

Addressed comments.

lebrush · 2017-03-21T09:10:04Z

sys/include/evtimer.h

+    evtimer_event_t *events;    /**< Event queue */
+} evtimer_t;
+
+/**


There's still a #include "msg.h" in this file :-)

lebrush · 2017-03-21T09:10:31Z

sys/include/evtimer/msg.h

+#ifndef EVTIMER_MSG_H
+#define EVTIMER_MSG_H
+
+#include "evtimer.h"


Missing #include "msg.h"

kaspar030 · 2017-03-21T09:51:10Z

@miri64 Please check out my original branch.

I've added a callback field to evtimer_t, in the hope of simplifying handlers in the future.
With only the msg handler, this increases code by 16b and memory by 4b, but I expect the code size increase to amortize quickly, probably already with the second event type. (The msg event handler is now basically just one line.)

I've tried to debug the race condition you've found. I think it is the optimizer not getting the link traversal hack in _add_event_to_list(). Just making that function non-static also solves the issue.

@miri64 How do we proceed? I'm fine with closing my initial PR. we could also continue there.

miri64 · 2017-03-21T10:06:56Z

I've added a callback field to evtimer_t, in the hope of simplifying handlers in the future.
With only the msg handler, this increases code by 16b and memory by 4b, but I expect the code size increase to amortize quickly, probably already with the second event type. (The msg event handler is now basically just one line.)

I basically started to do the same yesterday, but stopped because I thought you maybe wanted to save code size, but if you are okay we can leave it as is.

I've tried to debug the race condition you've found. I think it is the optimizer not getting the link traversal hack in _add_event_to_list(). Just making that function non-static also solves the issue.

If that's true then let's do it this way instead of the hacky volatile int decrementation I did ;-).

@miri64 How do we proceed? I'm fine with closing my initial PR. we could also continue there.

I'd prefer keeping this one open and closing yours, since this way the workload and pushing force to getting this is into master is on me ;-).

miri64 · 2017-03-21T10:23:34Z

(the pull-in of @kaspar030's changes addressed @lebrush's comments)

miri64 · 2017-03-21T10:25:28Z

I've tried to debug the race condition you've found. I think it is the optimizer not getting the link traversal hack in _add_event_to_list(). Just making that function non-static also solves the issue.

If that's true then let's do it this way instead of the hacky volatile int decrementation I did ;-).

Size-wise both workarounds don't make a difference.

kaspar030 · 2017-03-21T10:48:42Z

sys/include/evtimer/msg.h

+ * @extends evtimer_t
+ */
+typedef evtimer_t evtimer_msg_t;
+/**


uh, newline missing

kaspar030 · 2017-03-21T10:49:42Z

tests/evtimer_msg/main.c

+    { .event = { .offset = 3954 }, .msg = { .content = { .ptr = "supposed to be 3954" } } },
+};
+
+#define NEVENTS (sizeof(events) / sizeof(evtimer_msg_event_t))


static const unsigned int?

Why? I use it one time

Did not improve code-size either.

kaspar030 · 2017-03-21T10:51:24Z

sys/evtimer/evtimer.c

+
+    _update_timer(evtimer);
+
+    if (!irq_is_in()) {


isn't this always called from isr?

This is your code ;-)

kaspar030 · 2017-03-21T10:57:38Z

Could you do some underflow testing, e.g., what happens when adding events with offset = 0 or 1, in a loop? Not sure xtimer will handle that case correctly. ;)

miri64 · 2017-03-21T11:20:32Z

Could you do some underflow testing, e.g., what happens when adding events with offset = 0 or 1, in a loop? Not sure xtimer will handle that case correctly. ;)

Done, they fail ;-) [edit] for both 0 and 1 [/edit]

kaspar030 · 2017-03-21T12:10:56Z

Could you do some underflow testing, e.g., what happens when adding events with offset = 0 or 1, in a loop? Not sure xtimer will handle that case correctly. ;)

Done, they fail ;-) [edit] for both 0 and 1 [/edit]

Did you try with a message queue for the calling thread? If xtimer's failsafe kicked in (target too close), it would have triggered the timer right away, causing msg_send_int() to fail if there's no message queue.

miri64 · 2017-03-21T17:33:12Z

Did you try with a message queue for the calling thread? If xtimer's failsafe kicked in (target too close), it would have triggered the timer right away, causing msg_send_int() to fail if there's no message queue.

Did not, but have now (see last commit). Still failing.

miri64 · 2017-03-28T15:26:26Z

Let's try to merge this today.

miri64 · 2017-03-28T15:27:00Z

(timeouts of 0 and 1 are not planned for NDP)

miri64 · 2017-05-06T17:26:56Z

Honestly, I'm a little reluctant to merge this as is. Hauke and I are working on a new timer architecture, which might make this one obsolete.

Wait a minute.... isn't xtimer supposed to stay regardless? Or will the new timer architecture contain an event-queue from scratch? Otherwise, I don't see how the new timer architecture obsoletes this PR.

zhuoshuguo · 2017-05-23T14:23:09Z

Any decision on this PR (needed for the gnrc_mac also) ? :-)

miri64 · 2017-06-09T14:10:52Z

tests/evtimer_underflow/main.c

+        for (unsigned i = 0; i < NEVENTS; i++) {
+            evtimer_add_msg(&evtimer, &events[i], pid);
+        }
+        thread_yield();


Did you try with a message queue for the calling thread? If xtimer's failsafe kicked in (target too close), it would have triggered the timer right away, causing msg_send_int() to fail if there's no message queue.

Did not, but have now (see last commit). Still failing.

@kaspar030 if I add this thread_yield() here however, it works. Isn't the sender supposed to yield if the receivers message queue is full anyway?

Can you try replacing "msg_send_int()" in "_evtimer_msg_handler()" with "msg_try_send()"?

Then I get (with the thread_yield() here removed):

/home/mlenders/Repositories/RIOT-OS/RIOT/tests/evtimer_underflow/bin/native/tests_evtimer_msg.elf: thread_yield_higher: interrupts are disabled - this should not be received msg "1" /home/mlenders/Repositories/RIOT-OS/RIOT/tests/evtimer_underflow/bin/native/tests_evtimer_msg.elf: thread_yield_higher: interrupts are disabled - this should not be received msg "2" /home/mlenders/Repositories/RIOT-OS/RIOT/tests/evtimer_underflow/bin/native/tests_evtimer_msg.elf: thread_yield_higher: interrupts are disabled - this should not be received msg "3" /home/mlenders/Repositories/RIOT-OS/RIOT/tests/evtimer_underflow/bin/native/tests_evtimer_msg.elf: thread_yield_higher: interrupts are disabled - this should not be received msg "4" /home/mlenders/Repositories/RIOT-OS/RIOT/tests/evtimer_underflow/bin/native/tests_evtimer_msg.elf: thread_yield_higher: interrupts are disabled - this should not be received msg "5" /home/mlenders/Repositories/RIOT-OS/RIOT/tests/evtimer_underflow/bin/native/tests_evtimer_msg.elf: thread_yield_higher: interrupts are disabled - this should not be received msg "6" /home/mlenders/Repositories/RIOT-OS/RIOT/tests/evtimer_underflow/bin/native/tests_evtimer_msg.elf: thread_yield_higher: interrupts are disabled - this should not be received msg "7" /home/mlenders/Repositories/RIOT-OS/RIOT/tests/evtimer_underflow/bin/native/tests_evtimer_msg.elf: thread_yield_higher: interrupts are disabled - this should not be received msg "8" ...

Hmshit. Please try reverting to msg_send_int(), then add if (sched_context_switch_request) { thread_yield_higher(); } after the irq_disable() in evtimer_add().

After irq_disable()? Wouldn't that result in the same message as above? Or did you mean after irq_restore()?

(that works btw).

See my latest commits.

Yes, I meant restore...

cgundogan · 2017-06-11T22:57:49Z

@miri64 or @kaspar030 could you briefly summarize the current state of this PR? There are a lot of comments up there and I am currently too tired and too lazy to read them all (: Since this new feature is needed by the new NDP implementation, we should put more focus to get this into decent shape quickly.

kaspar030 · 2017-06-13T11:50:52Z

could you briefly summarize the current state of this PR?

Apart from possible formalities, there's a problem when setting very short (e.g., zero or 1) offsets.
I suspect xtimer to back-off, executing the callback right away in user context. With the msg helper, this breaks when using msg_send_int() as that doesn't issue a context switch when in user mode.
Replacing that function with msg_try_send() leads to the warning in @miri64's last comment.

kaspar030 · 2017-06-13T15:21:01Z

sys/evtimer/evtimer.c

+void evtimer_add(evtimer_t *evtimer, evtimer_event_t *event)
+{
+    unsigned state = irq_disable();
+    unsigned int context_switch_request;


why the copy?

To not access the volatile variable during a potential context change.

(don't know if that might be bad for this particular variable. You decide ;-))

it should work without.

miri64 · 2017-06-13T16:00:23Z

Fixed doc errors reported by Murdock

miri64 · 2017-06-15T11:36:30Z

Ping @kaspar030?

kaspar030 · 2017-06-16T14:09:22Z

please squash!

miri64 · 2017-06-16T14:52:53Z

Squashed... since this was a little bit more complicated than anticipated:

[mlenders@sarajevo RIOT]<3 git diff HEAD~4 origin/evtimer/feat/initial | sha1sum
e9b6e1467b308e7e8ea414254f085bdc5138e411  -
(aka)
[mlenders@sarajevo RIOT]<3 git diff HEAD~4 f7d885a | sha1sum
e9b6e1467b308e7e8ea414254f085bdc5138e411  -
[mlenders@sarajevo RIOT]<3 git diff HEAD~4 HEAD | sha1sum
e9b6e1467b308e7e8ea414254f085bdc5138e411  -

miri64 · 2017-06-16T16:16:13Z

Both Murdocks are happy :-)

sys: div: add div_u64_by_125()

8c0249d

miri64 assigned jnohlgard and kaspar030 Mar 20, 2017

miri64 added Type: new feature The issue requests / The PR implemements a new feature for RIOT Area: timers Area: timer subsystems labels Mar 20, 2017

miri64 added this to the Release 2017.04 milestone Mar 20, 2017

miri64 mentioned this pull request Mar 20, 2017

sys: add evtimer WIP #5999

Closed

kaspar030 requested changes Mar 20, 2017

View reviewed changes

lebrush reviewed Mar 21, 2017

View reviewed changes

lebrush approved these changes Mar 21, 2017

View reviewed changes

kaspar030 requested changes Mar 21, 2017

View reviewed changes

miri64 added the Community: Hack'n'ACK candidate This PR is a candidate for review and discussion during one of RIOT's monthly Hack'n'ACK parties label Mar 28, 2017

miri64 added a commit to miri64/RIOT that referenced this pull request May 1, 2017

Merge RIOT-OS#6767 into RIOT-OS#6988

7b5f0ad

miri64 added a commit to miri64/RIOT that referenced this pull request May 5, 2017

Merge branch 'evtimer/feat/initial' (RIOT-OS#6767) into RIOT-OS#6988

00c0e64

miri64 mentioned this pull request Jun 1, 2017

LWMAC: A simple duty cycling 802.15.4 MAC protocol (2nd try). #6554

Merged

miri64 commented Jun 9, 2017

View reviewed changes

miri64 added a commit to miri64/RIOT that referenced this pull request Jun 9, 2017

Merge branch 'evtimer/feat/initial' (RIOT-OS#6767) into RIOT-OS#6988

2f5d010

miri64 added CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jun 9, 2017

miri64 added a commit to miri64/RIOT that referenced this pull request Jun 9, 2017

Merge branch 'evtimer/feat/initial' (RIOT-OS#6767) into RIOT-OS#6988

37bcc22

kaspar030 requested changes Jun 13, 2017

View reviewed changes

miri64 added a commit to miri64/RIOT that referenced this pull request Jun 15, 2017

Merge branch 'evtimer/feat/initial' (RIOT-OS#6767) into RIOT-OS#6988

8e43afd

kaspar030 and others added 3 commits June 16, 2017 16:50

evtimer: initial import

fce919d

tests: provide tests for evtimer

15732be

tests: add test for evtimer underflow

0f39c55

miri64 force-pushed the evtimer/feat/initial branch from f7d885a to 0f39c55 Compare June 16, 2017 14:53

kaspar030 approved these changes Jun 16, 2017

View reviewed changes

kaspar030 merged commit 0318700 into RIOT-OS:master Jun 16, 2017

miri64 deleted the evtimer/feat/initial branch June 16, 2017 23:09

zhuoshuguo mentioned this pull request May 19, 2017

gnrc_mac: add timeout module. #5949

Merged

Conversation

miri64 commented Mar 20, 2017

Uh oh!

miri64 commented Mar 20, 2017

Uh oh!

miri64 commented Mar 20, 2017

Uh oh!

miri64 commented Mar 20, 2017

Uh oh!

miri64 commented Mar 20, 2017

Uh oh!

miri64 commented Mar 20, 2017

Uh oh!

miri64 commented Mar 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaspar030 commented Mar 20, 2017

Uh oh!

miri64 commented Mar 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaspar030 commented Mar 21, 2017

Uh oh!

miri64 commented Mar 21, 2017

Uh oh!

miri64 commented Mar 21, 2017

Uh oh!

miri64 commented Mar 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaspar030 commented Mar 21, 2017

Uh oh!

miri64 commented Mar 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kaspar030 commented Mar 21, 2017

Uh oh!

miri64 commented Mar 21, 2017

Uh oh!

miri64 commented Mar 28, 2017

Uh oh!

miri64 commented Mar 28, 2017

Uh oh!

miri64 commented May 6, 2017

Uh oh!

zhuoshuguo commented May 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

miri64 commented Mar 21, 2017 •

edited

Loading

zhuoshuguo commented May 23, 2017 •

edited

Loading