Skip to content

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

Merged
leandrolanzieri merged 6 commits intoRIOT-OS:masterfrom
miri64:posix/feat/select
Jul 2, 2020
Merged

posix: initial import of select() function (only support sockets for now)#12975
leandrolanzieri merged 6 commits intoRIOT-OS:masterfrom
miri64:posix/feat/select

Conversation

@miri64
Copy link
Member

@miri64 miri64 commented Dec 17, 2019

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 requested a review from kaspar030 December 17, 2019 16:08
@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 -.-

@leandrolanzieri
Copy link
Contributor

Nice! This needs rebasing now

@miri64 miri64 force-pushed the posix/feat/select branch from 38cdba1 to 4d7e055 Compare January 9, 2020 11:28
@miri64 miri64 removed the State: waiting for other PR State: The PR requires another PR to be merged first label Jan 9, 2020
@miri64 miri64 force-pushed the posix/feat/select branch from 4d7e055 to 78c4772 Compare January 9, 2020 11:29
@miri64
Copy link
Member Author

miri64 commented Jan 9, 2020

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

@miri64
Copy link
Member Author

miri64 commented Jun 19, 2020

@leandrolanzieri care to review?

@leandrolanzieri leandrolanzieri self-assigned this Jun 23, 2020
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

@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
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 minor comments. Also, why are you adding an empty doc.txt? I'll be running the test applications now on different platforms

2019-12-17 16:47:01,796 # Hello World!
```

Alternatively, with `native` or if your host also can connect to board, you can
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Alternatively, with `native` or if your host also can connect to board, you can
Alternatively, with `native` or if your host also can connect to the board, you can

* @{
* @file
* @author Martine S. Lenders <m.lenders@fu-berlin.de>
* @todo
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing doc?

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed.

@leandrolanzieri leandrolanzieri added the Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines label Jun 30, 2020
@leandrolanzieri
Copy link
Contributor

leandrolanzieri commented Jun 30, 2020

Testing looks mostly good. Just one thing I noticed in (2).

1. Native using netcat

Linux
 echo -ne "RIOT! 4973" | nc -6u "fe80::12:62ff:fe63:29d5%tapbr0" 4973 & \
>     echo -ne "RIOT! 1350" | nc -6u "fe80::12:62ff:fe63:29d5%tapbr0" 1350 & \
>     echo -ne "RIOT! 6717" | nc -6u "fe80::12:62ff:fe63:29d5%tapbr0" 6717 & \
>     echo -ne "RIOT! 9673" | nc -6u "fe80::12:62ff:fe63:29d5%tapbr0" 9673
Native node
Received data from [fe80::12:62ff:fe63:29d4]:12000:
RIOT! 4973
Received data from [fe80::12:62ff:fe63:29d4]:9935:
RIOT! 1350
Received data from [fe80::12:62ff:fe63:29d4]:18660:
RIOT! 6717
Received data from [fe80::12:62ff:fe63:29d4]:37341:
RIOT! 9673

2. Multiple samr21-xpros

During this test I sent packets from 2 clients to different ports. I noticed sometimes what it seemed in the server as duplicate packets being printed. I'll try to find a way to reproduce this every time.

Client 1
> udp send fe80::5c07:9e53:4649:7979 1350 RIOT!  5 200000
2020-06-30 15:33:54,411 #  udp send fe80::5c07:9e53:4649:7979 1350 RIOT!  5 200000
2020-06-30 15:33:54,417 # Success: send 5 byte to fe80::5c07:9e53:4649:7979:1350
2020-06-30 15:33:54,624 # Success: send 5 byte to fe80::5c07:9e53:4649:7979:1350
2020-06-30 15:33:54,832 # Success: send 5 byte to fe80::5c07:9e53:4649:7979:1350
2020-06-30 15:33:55,039 # Success: send 5 byte to fe80::5c07:9e53:4649:7979:1350
2020-06-30 15:33:55,246 # Success: send 5 byte to fe80::5c07:9e53:4649:7979:1350
> 
Client 2
> udp send fe80::5c07:9e53:4649:7979 4973 RIOT! 5 200000
2020-06-30 15:33:54,411 #  udp send fe80::5c07:9e53:4649:7979 4973 RIOT! 5 200000
2020-06-30 15:33:54,418 # Success: send 5 byte to fe80::5c07:9e53:4649:7979:4973
2020-06-30 15:33:54,625 # Success: send 5 byte to fe80::5c07:9e53:4649:7979:4973
2020-06-30 15:33:54,832 # Success: send 5 byte to fe80::5c07:9e53:4649:7979:4973
2020-06-30 15:33:55,038 # Success: send 5 byte to fe80::5c07:9e53:4649:7979:4973
2020-06-30 15:33:55,245 # Success: send 5 byte to fe80::5c07:9e53:4649:7979:4973
> 
Server
2020-06-30 15:33:52,967 # main(): This is RIOT! (Version: 2020.01-devel-1638-g8fbd8-posix/feat/select)
2020-06-30 15:33:52,970 # RIOT select example application
2020-06-30 15:33:52,975 # Started UDP server at [fe80::5c07:9e53:4649:7979]:1350
2020-06-30 15:33:52,980 # Started UDP server at [fe80::5c07:9e53:4649:7979]:4973
2020-06-30 15:33:52,985 # Started UDP server at [fe80::5c07:9e53:4649:7979]:6717
2020-06-30 15:33:52,990 # Started UDP server at [fe80::5c07:9e53:4649:7979]:9673
2020-06-30 15:33:54,422 # Received data from [fe80::e011:20b5:6249:7679]:21952:
2020-06-30 15:33:54,422 # RIOT!
2020-06-30 15:33:54,427 # Received data from [fe80::f830:5330:4426:6879]:21952:
2020-06-30 15:33:54,428 # RIOT!
2020-06-30 15:33:54,630 # Received data from [fe80::f830:5330:4426:6879]:21952:
2020-06-30 15:33:54,630 # RIOT!
2020-06-30 15:33:54,635 # Received data from [fe80::e011:20b5:6249:7679]:21952:
2020-06-30 15:33:54,636 # RIOT!
2020-06-30 15:33:54,836 # Received data from [fe80::f830:5330:4426:6879]:21952:
2020-06-30 15:33:54,836 # RIOT!
2020-06-30 15:33:54,841 # Received data from [fe80::e011:20b5:6249:7679]:21952:
2020-06-30 15:33:54,842 # RIOT!
2020-06-30 15:33:55,043 # Received data from [fe80::f830:5330:4426:6879]:21952:
2020-06-30 15:33:55,043 # RIOT!
2020-06-30 15:33:55,048 # Received data from [fe80::e011:20b5:6249:7679]:21952:
2020-06-30 15:33:55,049 # RIOT!
2020-06-30 15:33:55,250 # Received data from [fe80::f830:5330:4426:6879]:21952:
2020-06-30 15:33:55,250 # RIOT!
2020-06-30 15:33:55,257 # Received data from [fe80::e011:20b5:6249:7679]:21952:
2020-06-30 15:33:55,258 # RIOT!

@miri64
Copy link
Member Author

miri64 commented Jun 30, 2020

Also, why are you adding an empty doc.txt?

Not sure. Probably because posix_sockets contains a doc.txt I added it as a skeleton but than I forgot to remove it.

@miri64
Copy link
Member Author

miri64 commented Jun 30, 2020

I noticed sometimes what it seemed in the server as duplicate packets being printed. I'll try to find a way to reproduce this every time.

Just to be clear: in the logs you provided this isn't the case, right? Because I count 10 message prints.

@leandrolanzieri leandrolanzieri added the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Jul 1, 2020
@miri64
Copy link
Member Author

miri64 commented Jul 1, 2020

May I squash?

@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 miri64 force-pushed the posix/feat/select branch from 1e4ae21 to f0a66b0 Compare July 1, 2020 11:37
@miri64
Copy link
Member Author

miri64 commented Jul 1, 2020

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

@miri64 miri64 force-pushed the posix/feat/select branch from f0a66b0 to 91430bd Compare July 1, 2020 11:41
@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 force-pushed the posix/feat/select branch from f3282f8 to fa9371d Compare July 1, 2020 16:10
@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
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
Copy link
Member Author

miri64 commented Jul 2, 2020

Thanks for the review and intensive testing btw :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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

Development

Successfully merging this pull request may close these issues.

4 participants