posix_semaphore: make API POSIX compliant and port to xtimer (+ fixes) by miri64 · Pull Request #4110 · RIOT-OS/RIOT

miri64 · 2015-10-19T14:44:32Z

Fixes #4109 by renaming sem to sema and fix its usage by posix_semaphore accordingly.

Kijewski · 2015-10-19T14:49:10Z

sys/posix/semaphore/posix_semaphore.c

I guess I typed in :x a little bit hasty here and forgot the : ;-)

jnohlgard · 2015-10-20T04:43:12Z

While you're updating the POSIX semaphores, could you change the #define sem_xxx() (-1) stub implementations to inline static int sem_xxx(int abc) { return -1; }? (to get a proper function declaration)

miri64 · 2015-10-20T08:10:46Z

Done

jnohlgard · 2015-10-20T08:47:26Z

Quedtion: should we set errno to ENOMEM on sem_open, EINVAL on sem_close, ENOENT on sem_unlink?

miri64 · 2015-10-20T08:56:14Z

Yes and done.

jnohlgard · 2015-10-25T11:52:18Z

missing #include "msg.h" in sema.h

edit:

In file included from /data/riotbuild/riotbase/sys/posix/include/semaphore.h:29:0,
                 from /data/riotbuild/riotproject/tests/posix_semaphore/main.c:26:
/data/riotbuild/riotbase/sys/include/sema.h:84:57: error: unknown type name 'msg_t'
 int sema_wait_timed_msg(sema_t *sema, timex_t *timeout, msg_t *msg);
                                                         ^
/data/riotbuild/riotbase/sys/include/sema.h:97:47: error: unknown type name 'msg_t'
 static inline int sema_wait_msg(sema_t *sema, msg_t *msg)
                                               ^
/data/riotbuild/riotbase/Makefile.base:60: recipe for target '/data/riotbuild/riotproject/tests/posix_semaphore/bin/mulle/posix_semaphore/main.o' failed

miri64 · 2015-10-25T14:53:52Z

Done. As a bonus I ported both sema and posix_semaphore to xtimer ;-)

miri64 · 2015-10-25T15:06:12Z

Wait… something went wrong. The tests aren't working anymore -.-

miri64 · 2015-10-25T15:15:41Z

Bug seems to exist in master too. Will investigate.

miri64 · 2015-10-25T15:47:22Z

According to the debugger TEST4 fails on msg_receive with a SIGALARM on native and on other boards the timer just never fires. in the other tests (timeout == 0) it works just fine. Ideas?

miri64 · 2015-10-25T18:29:13Z

Herpderp, if you change an API you might want to change the tests testing that API, too. Now everything should be in order :-)

jnohlgard · 2015-10-26T06:30:03Z

ACK, please squash

I'm getting runtime failure of TEST4 first: waited only 999058 usec => FAILED but I get that in master as well. I am guessing this is a problem with the current xtimer configuration for mulle.

jnohlgard · 2015-10-26T06:33:54Z

I also see that native closes with alarm on master, I guess this is the same as the SIGALRM problem described above. This PR fixes that error, native works again.

miri64 · 2015-10-26T09:30:14Z

Squashed

miri64 · 2015-10-26T09:32:12Z

I'm getting runtime failure of TEST4 first: waited only 999058 usec => FAILED but I get that in master as well. I am guessing this is a problem with the current xtimer configuration for mulle.

Maybe I should put the abstime generation into the timed bracket. Might be that xtimer on mulle is "too precise" ;-).

jnohlgard · 2015-10-26T10:18:12Z

I have seen that xtimer seem to be firing a bit too early in general on Mulle so don't worry about it. I am working on improvements to xtimer (#4040, #3990) and kinetis timer periph (#4065)

jnohlgard · 2015-10-26T10:20:33Z

@authmillenon https://travis-ci.org/RIOT-OS/RIOT/jobs/87419969 Travis fails on msp430.. ~~Something about const correctness it seems.~~
Missing struct timespec declaration.

posix_semaphore:msb-430 (no linking):
Building application "posix_semaphore" for "msb-430" with MCU "msp430fxyz".
In file included from /home/travis/build/RIOT-OS/RIOT/tests/posix_semaphore/main.c:27:0:
/home/travis/build/RIOT-OS/RIOT/sys/posix/include/semaphore.h:246:44: error: 'struct timespec' declared inside parameter list [-Werror]
/home/travis/build/RIOT-OS/RIOT/sys/posix/include/semaphore.h:246:44: error: its scope is only this definition or declaration, which is probably not what you want [-Werror]
/home/travis/build/RIOT-OS/RIOT/tests/posix_semaphore/main.c: In function 'test4':
/home/travis/build/RIOT-OS/RIOT/tests/posix_semaphore/main.c:251:5: error: passing argument 2 of 'sem_timedwait' from incompatible pointer type [-Werror]
/home/travis/build/RIOT-OS/RIOT/sys/posix/include/semaphore.h:246:5: note: expected 'const struct timespec *' but argument is of type 'struct timespec *'
cc1: all warnings being treated as errors
make[1]: *** [/home/travis/build/RIOT-OS/RIOT/tests/posix_semaphore/bin/msb-430/posix_semaphore/main.o] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [all] Error 2

miri64 · 2015-10-28T13:03:06Z

#4182 should fix that.

miri64 · 2015-10-28T13:03:22Z

Based PR on that PR.

miri64 · 2015-11-09T13:36:48Z

if sema_post is called before sema_wait, then the value of the semaphore will already be > 0 and no message is needed. The implementation in sema_wait_timed_msg doesn't check for any message if the semaphore value is > 0 when entering the loop.

Of course it does. And yes: in that case it exits early with return value equal to 0.

jnohlgard · 2015-11-10T06:21:17Z

@authmillenon do you agree that the xtimer should move outside the message waiting loop? (#4110 (comment))

jnohlgard · 2015-11-11T08:08:35Z

There is still the issue of the timeout not being obeyed if the semaphore is posted and signalled to this thread, but another, higher priority, thread manages to snatch it before this thread has a chance to take it. Then the timeout in this thread will be reset to its original value again before waiting for another signal message. If this repeats we may overrun the timeout indefinitely. If the xtimer is moved to outside of the main waiting loop the timeout will be total waiting time instead of incremental per iteration.

miri64 · 2015-11-11T10:30:50Z

I'm only to 95% sure I got what you were saying so please check 8c49e19 =).

miri64 · 2015-11-12T14:30:39Z

Ping?

jnohlgard · 2015-11-12T14:55:58Z

yes, that was what I meant.
The code is a bit difficult to read though with the disableIRQ call coming after the restoreIRQ call in the revised loop.
From a readability perspective it would make sense to add a restoreIRQ call right after the xtimer_set call and move the second disableIRQ to right before unsigned value = sema->value;

miri64 · 2015-11-12T15:05:16Z

Fixed. This is a lot of discussion for an standardized API change ;-P

jnohlgard · 2015-11-13T07:17:44Z

sys/sema/sema.c

remove this leftover

jnohlgard · 2015-11-13T07:18:54Z

https://github.com/RIOT-OS/RIOT/pull/4110/files#diff-4af11e61eb26bcdd3eed69b819563f52R109
please address or add a comment why it is not necessary to disable IRQs there.

jnohlgard · 2015-11-13T07:22:15Z

@authmillenon The reason for this long discussion is mostly because the original semaphore module (before your refactoring in the previous PR) had some bit rot that we are addressing here as well.

miri64 · 2015-11-24T08:45:59Z

@authmillenon The reason for this long discussion is mostly because the original semaphore module (before your refactoring in the previous PR) had some bit rot that we are addressing here as well.

True, but that could have been done in a follow-up ;-)

https://github.com/RIOT-OS/RIOT/pull/4110/files#diff-4af11e61eb26bcdd3eed69b819563f52R109
please address or add a comment why it is not necessary to disable IRQs there.

Addressed

jnohlgard · 2015-11-24T09:08:01Z

ACK, please squash

miri64 · 2015-11-24T09:46:18Z

Squashed

jnohlgard · 2015-11-24T11:50:43Z

Travis fails:

/home/travis/build/RIOT-OS/RIOT/sys/posix/semaphore/posix_semaphore.c:45:42: error: passing argument 2 of 'sema_wait_timed' makes integer from pointer without a cast [-Werror]
     res = sema_wait_timed((sema_t *)sem, &timeout);

miri64 · 2015-11-24T13:07:12Z

Some commits got lost on rebase. Cherry-picked 'em back into place.

posix_semaphore: make API POSIX compliant and port to xtimer (+ fixes)

miri64 · 2015-11-24T20:12:48Z

Thanks for the review btw ^^"

jnohlgard · 2015-11-24T21:39:55Z

np, sorry for the long-winded review process.

miri64 added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: POSIX Area: POSIX API wrapper Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. labels Oct 19, 2015

miri64 assigned jnohlgard Oct 19, 2015

miri64 added this to the Release NEXT MAJOR milestone Oct 19, 2015

Kijewski reviewed Oct 19, 2015
View reviewed changes

jnohlgard added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 25, 2015

miri64 changed the title ~~posix_semaphore: make API POSIX compliant~~ posix_semaphore: make API POSIX compliant and port to xtimer Oct 25, 2015

miri64 force-pushed the posix/api/i4109 branch from a2c4ebc to 636f684 Compare October 25, 2015 14:57

miri64 force-pushed the posix/api/i4109 branch from 636f684 to 22c1486 Compare October 25, 2015 15:16

miri64 force-pushed the posix/api/i4109 branch from 8f8c0e5 to 7ba8473 Compare October 26, 2015 09:30

miri64 force-pushed the posix/api/i4109 branch from 7ba8473 to 378727a Compare October 28, 2015 13:03

jnohlgard reviewed Nov 13, 2015
View reviewed changes

sys/sema/sema.c Outdated

Copy link
Copy Markdown

Member

jnohlgard Nov 13, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove this leftover

jnohlgard added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 24, 2015

sema: port to xtimer

d104cff

miri64 force-pushed the posix/api/i4109 branch from 1722501 to c1effdc Compare November 24, 2015 09:46

miri64 removed the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Nov 24, 2015

miri64 changed the title ~~posix_semaphore: make API POSIX compliant and port to xtimer~~ posix_semaphore: make API POSIX compliant and port to xtimer (+ fixes) Nov 24, 2015

miri64 and others added 4 commits November 24, 2015 14:06

posix_semaphore: port to xtimer

21ea7cc

tests: port semaphore tests to xtimer

aca1224

sema: fix race-condition that causes timeout reset

77547dd

sema: deactivate interrupts for queue manipulation

db01af3

miri64 force-pushed the posix/api/i4109 branch from c1effdc to db01af3 Compare November 24, 2015 13:06

miri64 added a commit that referenced this pull request Nov 24, 2015

Merge pull request #4110 from authmillenon/posix/api/i4109

7af2a62

posix_semaphore: make API POSIX compliant and port to xtimer (+ fixes)

miri64 merged commit 7af2a62 into RIOT-OS:master Nov 24, 2015

miri64 deleted the posix/api/i4109 branch November 24, 2015 14:04

Conversation

miri64 commented Oct 19, 2015

Uh oh!

Kijewski Oct 19, 2015

Choose a reason for hiding this comment

Uh oh!

miri64 Oct 19, 2015

Choose a reason for hiding this comment

Uh oh!

miri64 Oct 19, 2015

Choose a reason for hiding this comment

Uh oh!

jnohlgard commented Oct 20, 2015

Uh oh!

miri64 commented Oct 20, 2015

Uh oh!

jnohlgard commented Oct 20, 2015

Uh oh!

miri64 commented Oct 20, 2015

Uh oh!

jnohlgard commented Oct 25, 2015

Uh oh!

miri64 commented Oct 25, 2015

Uh oh!

miri64 commented Oct 25, 2015

Uh oh!

miri64 commented Oct 25, 2015

Uh oh!

miri64 commented Oct 25, 2015

Uh oh!

miri64 commented Oct 25, 2015

Uh oh!

jnohlgard commented Oct 26, 2015

Uh oh!

jnohlgard commented Oct 26, 2015

Uh oh!

miri64 commented Oct 26, 2015

Uh oh!

miri64 commented Oct 26, 2015

Uh oh!

jnohlgard commented Oct 26, 2015

Uh oh!

jnohlgard commented Oct 26, 2015

Uh oh!

miri64 commented Oct 28, 2015

Uh oh!

miri64 commented Oct 28, 2015

Uh oh!

miri64 commented Nov 9, 2015

Uh oh!

jnohlgard commented Nov 10, 2015

Uh oh!

jnohlgard commented Nov 11, 2015

Uh oh!

miri64 commented Nov 11, 2015

Uh oh!

miri64 commented Nov 12, 2015

Uh oh!

jnohlgard commented Nov 12, 2015

Uh oh!

miri64 commented Nov 12, 2015

Uh oh!

jnohlgard Nov 13, 2015

Choose a reason for hiding this comment

Uh oh!

jnohlgard commented Nov 13, 2015

Uh oh!

jnohlgard commented Nov 13, 2015

Uh oh!

miri64 commented Nov 24, 2015

Uh oh!

jnohlgard commented Nov 24, 2015

Uh oh!

miri64 commented Nov 24, 2015

Uh oh!

jnohlgard commented Nov 24, 2015

Uh oh!

miri64 commented Nov 24, 2015

Uh oh!

miri64 commented Nov 24, 2015