Skip to content
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.

Sign up for GitHub

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

Jump to bottom

posix: initial import of select() function (only support sockets for now) #12975

Merged
merged 6 commits into from
Jul 2, 2020
Merged

posix: initial import of select() function (only support sockets for now) #12975

merged 6 commits into from
Jul 2, 2020

Conversation

miri64
Copy link
Member

@miri64 miri64 commented Dec 17, 2019 •
edited
Loading

Contribution description

This adds an implementation of the POSIX select() function, utilizing sock_async. An example application is provided for how to use it.

Testing procedure

Follow the README for examples/posix_select.

Issues/PRs references

Requires #12921 (merged)

@miri64 miri64 added Area: POSIX Area: POSIX API wrapper Type: new feature The issue requests / The PR implemements a new feature for RIOT CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Dec 17, 2019
@miri64 miri64 added this to the Release 2020.01 milestone Dec 17, 2019
@miri64 miri64 added the State: waiting for other PR State: The PR requires another PR to be merged first label Dec 17, 2019
@miri64
Copy link
Member Author

miri64 commented Dec 17, 2019

I already can see... I opened another can of POSIX-header definition hell -.-

@miri64 miri64 mentioned this pull request Dec 18, 2019
Merged
@leandrolanzieri
Copy link
Contributor

Nice! This needs rebasing now

@miri64 miri64 removed the State: waiting for other PR State: The PR requires another PR to be merged first label Jan 9, 2020
@miri64
Copy link
Member Author

miri64 commented Jan 9, 2020

Rebased and squashed to current master. No longer has any dependencies.

@miri64 miri64 mentioned this pull request Feb 11, 2020
Open
2 tasks
@miri64
Copy link
Member Author

miri64 commented Jun 19, 2020

@leandrolanzieri care to review?

@leandrolanzieri leandrolanzieri self-assigned this Jun 23, 2020
leandrolanzieri
leandrolanzieri reviewed Jun 23, 2020
View reviewed changes
Copy link
Contributor

@leandrolanzieri leandrolanzieri left a comment

Choose a reason for hiding this comment

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

Some initial comments

sys/posix/sockets/posix_sockets.c Show resolved Hide resolved
sys/posix/sockets/posix_sockets.c Outdated Show resolved Hide resolved
sys/posix/sockets/posix_sockets.c Outdated Show resolved Hide resolved
sys/posix/select/posix_select.c Show resolved Hide resolved
@miri64 miri64 requested a review from jia200x as a code owner June 23, 2020 14:31
@leandrolanzieri leandrolanzieri added the Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines label Jun 30, 2020
@leandrolanzieri
Copy link
Contributor

May I squash?

Yes. Could you please update the README to include the interface when using link local in the posix sockets example?

@miri64
Copy link
Member Author

miri64 commented Jul 1, 2020

Just squashed for now. Will now amend the README update.

@miri64
Copy link
Member Author

miri64 commented Jul 1, 2020

And updated README.md

@miri64
Copy link
Member Author

miri64 commented Jul 1, 2020

As discussed offline, I hardened the select implementation against potential race-conditions:

  • check if there is a selecting thread before setting its thread flag
  • only return select() after thread_flags_wait_any() if readfds were set
  • report error in case of timeout (not discussed offline but noticed that it was not the case when fixing the above)

@cgundogan cgundogan added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jul 1, 2020
@miri64 miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jul 1, 2020
@leandrolanzieri
Copy link
Contributor

I rerun the test using an openmote-b as server and 3 samr21-xpro as clients. Everything looks good

@leandrolanzieri leandrolanzieri added Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels Jul 2, 2020
leandrolanzieri
leandrolanzieri approved these changes Jul 2, 2020
View reviewed changes
Copy link
Contributor

@leandrolanzieri leandrolanzieri left a comment

Choose a reason for hiding this comment

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

The implementation looks good. I tested in various platforms and the example application works well. ACK!

@leandrolanzieri leandrolanzieri merged commit 1927379 into RIOT-OS:master Jul 2, 2020
@miri64 miri64 deleted the posix/feat/select branch July 2, 2020 08:48
@miri64 miri64 mentioned this pull request Jul 2, 2020
Closed
@miri64
Copy link
Member Author

miri64 commented Jul 2, 2020

Thanks for the review and intensive testing btw :-)

@HendrikVE HendrikVE mentioned this pull request Sep 14, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leandrolanzieri leandrolanzieri leandrolanzieri approved these changes

@kaspar030 kaspar030 Awaiting requested review from kaspar030

@jia200x jia200x Awaiting requested review from jia200x jia200x is a code owner

Assignees

@leandrolanzieri leandrolanzieri

Labels
Area: POSIX Area: POSIX API wrapper CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: new feature The issue requests / The PR implemements a new feature for RIOT
Projects
None yet
Milestone
Release 2020.07
Development

Successfully merging this pull request may close these issues.
4 participants
@miri64 @leandrolanzieri @cgundogan @fjmolinas