Skip to content

posix: fix pthread and socket example#8368

Merged
jnohlgard merged 2 commits intoRIOT-OS:masterfrom
smlng:examples/fix_posix
Feb 19, 2018
Merged

posix: fix pthread and socket example#8368
jnohlgard merged 2 commits intoRIOT-OS:masterfrom
smlng:examples/fix_posix

Conversation

@smlng
Copy link
Copy Markdown
Member

@smlng smlng commented Jan 16, 2018

Contribution description

Started with fixing an intentional fall through reported as error by newer GCC (>7.x) versions. But also found an issue with the usage of usleep, apparently POSIX deprecated it and suggests to use nanosleep instead? See here https://stackoverflow.com/questions/10053788/implicit-declaration-of-function-usleep

Issues/PRs references

#8265

@smlng smlng added the Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation label Jan 16, 2018
@smlng smlng added this to the Release 2018.01 milestone Jan 16, 2018
@smlng smlng requested review from jnohlgard and kaspar030 January 16, 2018 10:11
@smlng smlng added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 16, 2018
@smlng
Copy link
Copy Markdown
Member Author

smlng commented Jan 16, 2018

note: you need to use gcc 7.2 on master and this PR to verify the fix

@smlng
Copy link
Copy Markdown
Member Author

smlng commented Jan 16, 2018

error messages, as follows:

usleep:

/RIOT/examples/posix_sockets/udp.c:118:9: error: implicit declaration of function 'usleep'; did you mean 'sleep'? [-Werror=implicit-function-declaration]
         usleep(delay);
         ^~~~~~
         sleep
cc1: all warnings being treated as errors

@smlng
Copy link
Copy Markdown
Member Author

smlng commented Jan 16, 2018

and

/RIOT/sys/posix/pthread/pthread.c:239:13: error: this statement may fall through [-Werror=implicit-fallthrough=]
             thread_sleep();
             ^~~~~~~~~~~~~~
/RIOT/sys/posix/pthread/pthread.c:241:9: note: here
         case (PTS_ZOMBIE):

@smlng smlng force-pushed the examples/fix_posix branch from 126c088 to fbb3476 Compare January 16, 2018 12:24
Copy link
Copy Markdown
Member

@jnohlgard jnohlgard left a comment

Choose a reason for hiding this comment

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

I am aware that usleep had been deprecated since before I started writing C (it was marked obsolete in 2001 according to the above link), but I am not convinced that this is the way we want to solve this. Do we provide a posix usleep function implementation? Do we need it for anything or is it just bloat? Do we provide a nanosleep function implementation that can be used instead?

@miri64
Copy link
Copy Markdown
Member

miri64 commented Jan 16, 2018

Do we provide a posix usleep function implementation? Do we need it for anything or is it just bloat? Do we provide a nanosleep function implementation that can be used instead?

At least for msp430-libc and avr-libc we do (the implementation is in sys/posix/time/posix_time.c. It exists to be able to run without explicitly calling xtimer_usleep() (but it just wraps this function) so POSIX libraries can be run on RIOT. I did not know about the deprecation, but if it is, we should indeed find an alternative (but this is unrelated to this PR IMHO).

@miri64
Copy link
Copy Markdown
Member

miri64 commented Jan 16, 2018

I did not know about the deprecation,

(at least the Linux manpages do not mention that).

@smlng
Copy link
Copy Markdown
Member Author

smlng commented Jan 16, 2018

first and foremost this RP is about getting things to work with GCC 7.x. I didn't want to start kind of a philosophical discussion or question POSIX compatibility and so on.

@smlng
Copy link
Copy Markdown
Member Author

smlng commented Jan 16, 2018

getting things to work with GCC 7.x

POSIX stuff here, there are more fixes needed to actually compile all of RIOT with GCC 7.x

@kaspar030
Copy link
Copy Markdown
Contributor

usleep:

Why not fix to use xtimer_usleep?

@smlng
Copy link
Copy Markdown
Member Author

smlng commented Jan 16, 2018

this is about POSIX and portability (I would say), i.e., using standard headers without need for RIOT specific ones. Look at the examples, they don't see xtimer but just use standard header functions. Fixing this by using xtimer, renders this tests redundant.

@miri64
Copy link
Copy Markdown
Member

miri64 commented Jan 16, 2018

renders this tests redundant.

Maybe not redundant, but one wouldn't be able to run it on e.g. Linux anymore, which should be possible at the moment with this example.

@miri64
Copy link
Copy Markdown
Member

miri64 commented Jan 18, 2018

ping @gebart?

@smlng smlng added the Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch label Jan 25, 2018
@smlng
Copy link
Copy Markdown
Member Author

smlng commented Jan 25, 2018

ping, this bugfix could also go into the release (I'll provide a backport). It would be nice to have GCC 7 support, or not?

@smlng smlng removed the Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch label Jan 30, 2018
Copy link
Copy Markdown
Member

@jnohlgard jnohlgard left a comment

Choose a reason for hiding this comment

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

ACK

@jnohlgard
Copy link
Copy Markdown
Member

Sorry for the delay

@jnohlgard jnohlgard mentioned this pull request Feb 19, 2018
13 tasks
@jnohlgard jnohlgard merged commit a25c059 into RIOT-OS:master Feb 19, 2018
@smlng smlng deleted the examples/fix_posix branch June 29, 2018 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants