socket_zep: only report size of single datagram #19121

benpicco · 2023-01-10T13:29:26Z

Contribution description

The use of ioctl(zepdev->sock_fd, FIONREAD, &size) would report the number of total bytes available on the socket. This could mean the cumulative sum of bytes of all datagrams, not just a single one.

Instead, use recv(zepdev->sock_fd, &hdr, sizeof(hdr), MSG_TRUNC | MSG_PEEK) to only read the ZEP header (without consuming the datagram) to get the real size of the ZEP message.

This prevents accidental concatenation of two ZEP packets into on, which with 6lo fragmentation, would cause the 2nd fragment to be lost as it is read together with the first fragment, but as part of the payload.

Testing procedure

Fragmented multicast datagrams should work now.

> ping -s 128 ff02::1
136 bytes from fe80::7c5e:5802:4449:e1b3%7: icmp_seq=0 ttl=64 rssi=0 dBm time=1.515 ms
136 bytes from fe80::7c5e:5802:4449:e1b3%7: icmp_seq=1 ttl=64 rssi=0 dBm time=1.644 ms
136 bytes from fe80::7c5e:5802:4449:e1b3%7: icmp_seq=2 ttl=64 rssi=0 dBm time=1.613 ms

--- ff02::1 PING statistics ---
3 packets transmitted, 3 packets received, 0% packet loss
round-trip min/avg/max = 1.515/1.590/1.644 ms

Issues/PRs references

fixes #19117

riot-ci · 2023-01-10T15:38:48Z

Murdock results

✔️ PASSED

913d72c socket_zep: don't discard frames if not in RX mode

Success	Failures	Total	Runtime
6770	0	6770	13m:39s

Artifacts

Documentation preview

maribu · 2023-01-10T15:41:52Z

bors merge

bors · 2023-01-10T15:41:55Z

👎 Rejected by too few approved reviews

fabian18 · 2023-01-10T15:59:23Z

A:

> ping -s 128 ff02::1
ping -s 128 ff02::1
socket_zep::write(113 bytes)
socket_zep::request_transmit(145 bytes)
socket_zep::write(70 bytes)
socket_zep::request_transmit(102 bytes)
socket_zep::write(113 bytes)
socket_zep::request_transmit(145 bytes)
socket_zep::write(70 bytes)
socket_zep::request_transmit(102 bytes)
socket_zep::write(113 bytes)
socket_zep::request_transmit(145 bytes)
socket_zep::write(70 bytes)
socket_zep::request_transmit(102 bytes)
socket_zep::_socket_isr: bytes on 8
socket_zep::len 118 bytes on 8
socket_zep::len 118 bytes on 8
socket_zep::read: reading up to 116 bytes into 0x809e4f8
socket_zep::read: got 150/150 bytes
socket_zep::send_ack: seq_no: 170
socket_zep::_socket_isr: bytes on 8
socket_zep::len 118 bytes on 8
socket_zep::len 118 bytes on 8
socket_zep::read: reading up to 116 bytes into 0x809e510
socket_zep::read: got 150/150 bytes
socket_zep::send_ack: seq_no: 170
socket_zep::_socket_isr: bytes on 8
socket_zep::len 118 bytes on 8
socket_zep::len 118 bytes on 8
socket_zep::read: reading up to 116 bytes into 0x809e510
socket_zep::read: got 150/150 bytes
socket_zep::send_ack: seq_no: 170
socket_zep::_socket_isr: bytes on 8
socket_zep::len 76 bytes on 8
socket_zep::len 76 bytes on 8
socket_zep::read: reading up to 74 bytes into 0x809e510
socket_zep::read: got 108/108 bytes
socket_zep::send_ack: seq_no: 172
136 bytes from fe80::a47b:d9:d217:fbf9%7: icmp_seq=2 ttl=64 rssi=0 dBm time=8.281 ms
socket_zep::_socket_isr: bytes on 8
socket_zep::len 76 bytes on 8
socket_zep::len 76 bytes on 8
socket_zep::read: reading up to 74 bytes into 0x809e4f8
socket_zep::read: got 108/108 bytes
socket_zep::send_ack: seq_no: 172

--- ff02::1 PING statistics ---
3 packets transmitted, 1 packets received, 66% packet loss
round-trip min/avg/max = 8.281/8.281/8.281 ms
> ping -s 128 ff02::1
ping -s 128 ff02::1
socket_zep::write(113 bytes)
socket_zep::request_transmit(145 bytes)
socket_zep::write(70 bytes)
socket_zep::request_transmit(102 bytes)
socket_zep::write(113 bytes)
socket_zep::request_transmit(145 bytes)
socket_zep::write(70 bytes)
socket_zep::request_transmit(102 bytes)
socket_zep::write(113 bytes)
socket_zep::request_transmit(145 bytes)
socket_zep::write(70 bytes)
socket_zep::request_transmit(102 bytes)

--- ff02::1 PING statistics ---
3 packets transmitted, 0 packets received, 100% packet loss
> socket_zep::write(45 bytes)
socket_zep::request_transmit(77 bytes)
socket_zep::_socket_isr: bytes on 8
socket_zep::len 45 bytes on 8
socket_zep::len 45 bytes on 8
socket_zep::read: reading up to 43 bytes into 0x809e4f8
socket_zep::read: got 77/77 bytes

B:

socket_zep::len 113 bytes on 8
socket_zep::len 113 bytes on 8
socket_zep::_socket_isr: bytes on 8
socket_zep::_socket_isr: bytes on 8
socket_zep::read: reading up to 111 bytes into 0x809e4f8
socket_zep::read: got -1/145 bytes
socket_zep::_socket_isr: bytes on 8
socket_zep::len 113 bytes on 8
socket_zep::len 113 bytes on 8
socket_zep::read: reading up to 111 bytes into 0x809e4f8
socket_zep::_socket_isr: bytes on 8
socket_zep::read: got 145/145 bytes
socket_zep::_socket_isr: bytes on 8
socket_zep::len 113 bytes on 8
socket_zep::len 113 bytes on 8
socket_zep::read: reading up to 111 bytes into 0x809e510
socket_zep::read: got 145/145 bytes
socket_zep::_socket_isr: bytes on 8
socket_zep::len 70 bytes on 8
socket_zep::len 70 bytes on 8
socket_zep::read: reading up to 68 bytes into 0x809e528
socket_zep::read: got 102/102 bytes
socket_zep::write(118 bytes)
socket_zep::request_transmit(150 bytes)
socket_zep::_socket_isr: bytes on 8
socket_zep::read: reading up to 0 bytes into (nil)
socket_zep::read: got 37/34 bytes
socket_zep::request_transmit(150 bytes)
socket_zep::_socket_isr: bytes on 8
socket_zep::read: reading up to 0 bytes into (nil)
socket_zep::read: got 37/34 bytes
socket_zep::request_transmit(150 bytes)
socket_zep::_socket_isr: bytes on 8
socket_zep::read: reading up to 3 bytes into 0x809e159
socket_zep::read: got 37/37 bytes
socket_zep::dst_not_me: got ACK
socket_zep::write(76 bytes)
socket_zep::request_transmit(108 bytes)
socket_zep::_socket_isr: bytes on 8
socket_zep::read: reading up to 0 bytes into (nil)
socket_zep::read: got 37/34 bytes
socket_zep::request_transmit(108 bytes)
socket_zep::_socket_isr: bytes on 8
socket_zep::read: reading up to 3 bytes into 0x809e159
socket_zep::read: got 37/37 bytes
socket_zep::dst_not_me: got ACK
socket_zep::_socket_isr: bytes on 8
socket_zep::_socket_isr: bytes on 8
socket_zep::_socket_isr: bytes on 8
socket_zep::len 113 bytes on 8
socket_zep::len: error reading FIONREAD: Resource temporarily unavailablesocket_zep::_socket_isr: bytes on 8
socket_zep::len 113 bytes on 8
socket_zep::len 113 bytes on 8
socket_zep::read: reading up to 111 bytes into 0x809e4f8
socket_zep::read: got 145/145 bytes
socket_zep::_socket_isr: bytes on 8
socket_zep::_socket_isr: bytes on 8
socket_zep::len 113 bytes on 8
socket_zep::len 113 bytes on 8
socket_zep::read: reading up to 111 bytes into 0x809e510
socket_zep::read: got 145/145 bytes
socket_zep::_socket_isr: bytes on 8
socket_zep::_socket_isr: bytes on 8
socket_zep::len 45 bytes on 8
socket_zep::len 45 bytes on 8
socket_zep::read: reading up to 43 bytes into 0x809e4f8
socket_zep::read: got 77/77 bytes
socket_zep::write(45 bytes)
socket_zep::request_transmit(77 bytes)
socket_zep::_socket_isr: bytes on 8
socket_zep::len 45 bytes on 8
socket_zep::len 45 bytes on 8
socket_zep::read: reading up to 43 bytes into 0x809e4f8
socket_zep::read: got 77/77 bytes
socket_zep::write(45 bytes)
socket_zep::request_transmit(77 bytes)

Hmm, not really working for me. Might be an Arch issue.
Tested with two native gnrc_metworking as shown in the issue.

benpicco · 2023-01-10T16:29:16Z

Uh turns out we end up here: https://github.com/RIOT-OS/RIOT/blob/master/cpu/native/socket_zep/socket_zep.c#L244

I guess discarding the frame if the radio is not in RX mode is not ideal, although I wonder why it's not in RX mode in the first place.

benpicco · 2023-01-10T16:37:23Z

913d72c fixes it, but it may be against the radio HAL interface.

The question is why is the radio not in RX mode?
Maybe @jia200x has an idea, is there some automatic state transition to RX that the virtual driver is missing?

fabian18 · 2023-01-10T17:05:06Z

it works now

> ping -s 128 ff02::2
ping -s 128 ff02::2
136 bytes from fe80::b8b0:7191:5734:365e%7: icmp_seq=0 ttl=64 rssi=0 dBm time=4.841 ms
136 bytes from fe80::b8b0:7191:5734:365e%7: icmp_seq=1 ttl=64 rssi=0 dBm time=0.816 ms
136 bytes from fe80::b8b0:7191:5734:365e%7: icmp_seq=2 ttl=64 rssi=0 dBm time=0.885 ms

kaspar030 · 2023-01-16T07:39:56Z

What's the state here? I'm waiting on this for the next RC. @jia200x could you take a look?

jia200x · 2023-01-16T13:14:11Z

Does this still guarantee that each received frame triggers its corresponding RX Done event? If so, everything should be fine.

kaspar030 · 2023-01-17T09:54:55Z

Does this still guarantee that each received frame triggers its corresponding RX Done event? If so, everything should be fine.

Looks like the RX Done indication might be called more often, not less, so this should be good?

miri64 · 2023-01-18T11:56:52Z

bors merge

jia200x · 2023-01-18T13:35:00Z

913d72c fixes it, but it may be against the radio HAL interface.

It should be fine. As per Radio HAL documentation, the user of the HAL must call ieee802154_radio_set_idle before calling ieee802154_radio_read.

The question is why is the radio not in RX mode?
Maybe @jia200x has an idea, is there some automatic state transition to RX that the virtual driver is missing?

No, there shouldn't be any automatic transition. But the user should call ieee802154_radio_set_rx manually after reading the frame (in case the radio should continue listening). The SubMAC will handle that automatically.

benpicco · 2023-01-18T13:44:10Z

Hm maybe socket_zep is sending too fast?

I had hoped socket_zep would serve as a good automatic test for the SubMAC, so I don't like that it needs some hacks now to not drop frames - the "RX off" state was simulated to be more like a real radio, removing it feels a bit like cheating.

18100: core/assert: print backtrace on failed assertion r=miri64 a=benpicco 19121: socket_zep: only report size of single datagram r=miri64 a=benpicco Co-authored-by: Benjamin Valentin <benjamin.valentin@ml-pa.com> Co-authored-by: Benjamin Valentin <benjamin.valentin@bht-berlin.de>

bors · 2023-01-18T14:36:01Z

This PR was included in a batch that was canceled, it will be automatically retried

18100: core/assert: print backtrace on failed assertion r=benpicco a=benpicco 19121: socket_zep: only report size of single datagram r=miri64 a=benpicco 19164: cpu/sam0_common: move adc_res_t to common code r=dylad a=benpicco 19169: tests/driver_ws281x: don't overwrite board definition r=benpicco a=benpicco Co-authored-by: Benjamin Valentin <benjamin.valentin@ml-pa.com> Co-authored-by: Benjamin Valentin <benjamin.valentin@bht-berlin.de>

bors · 2023-01-18T21:38:57Z

This PR was included in a batch that was canceled, it will be automatically retried

18100: core/assert: print backtrace on failed assertion r=kaspar030 a=benpicco 19121: socket_zep: only report size of single datagram r=miri64 a=benpicco 19164: cpu/sam0_common: move adc_res_t to common code r=dylad a=benpicco 19169: tests/driver_ws281x: don't overwrite board definition r=benpicco a=benpicco Co-authored-by: Benjamin Valentin <benjamin.valentin@ml-pa.com> Co-authored-by: Benjamin Valentin <benjamin.valentin@bht-berlin.de>

kaspar030 · 2023-01-18T21:42:12Z

bors cancel
bors merge

bors · 2023-01-18T21:42:14Z

Canceled.

bors · 2023-01-19T02:56:19Z

Build succeeded:

19173: socket_zep: only report size of single datagram [backport 2023.01] r=bergzand a=kaspar030 # Backport of #19121 Co-authored-by: Benjamin Valentin <benjamin.valentin@bht-berlin.de>

19173: socket_zep: only report size of single datagram [backport 2023.01] r=kaspar030 a=kaspar030 # Backport of #19121 Co-authored-by: Benjamin Valentin <benjamin.valentin@bht-berlin.de>

cpu/native: syscalls: provide real_recv()

8a80e70

benpicco added the Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) label Jan 10, 2023

github-actions bot added Area: cpu Area: CPU/MCU ports Platform: native Platform: This PR/issue effects the native platform labels Jan 10, 2023

benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 10, 2023

benpicco requested review from fabian18 and miri64 January 10, 2023 13:29

benpicco added the Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch label Jan 10, 2023

benpicco requested a review from maribu January 10, 2023 13:31

benpicco added 2 commits January 10, 2023 17:36

socket_zep: only report size of single datagram

3dedfb7

socket_zep: don't discard frames if not in RX mode

913d72c

benpicco force-pushed the cpu/native-zep_frag branch from f051baa to 913d72c Compare January 10, 2023 16:36

benpicco requested a review from jia200x January 13, 2023 10:12

maribu approved these changes Jan 18, 2023

View reviewed changes

bors bot merged commit d4d9149 into RIOT-OS:master Jan 19, 2023

kaspar030 mentioned this pull request Jan 19, 2023

socket_zep: only report size of single datagram [backport 2023.01] #19173

Merged

bors bot added a commit that referenced this pull request Jan 19, 2023

Merge #19173

24b0f45

19173: socket_zep: only report size of single datagram [backport 2023.01] r=bergzand a=kaspar030 # Backport of #19121 Co-authored-by: Benjamin Valentin <benjamin.valentin@bht-berlin.de>

bors bot added a commit that referenced this pull request Jan 19, 2023

Merge #19173

429506c

19173: socket_zep: only report size of single datagram [backport 2023.01] r=kaspar030 a=kaspar030 # Backport of #19121 Co-authored-by: Benjamin Valentin <benjamin.valentin@bht-berlin.de>

benpicco deleted the cpu/native-zep_frag branch January 19, 2023 11:04

MrKevinWeiss added this to the Release 2023.04 milestone Apr 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

socket_zep: only report size of single datagram #19121

socket_zep: only report size of single datagram #19121

benpicco commented Jan 10, 2023 •

edited

Loading

riot-ci commented Jan 10, 2023 •

edited

Loading

maribu commented Jan 10, 2023

bors bot commented Jan 10, 2023

fabian18 commented Jan 10, 2023

benpicco commented Jan 10, 2023

benpicco commented Jan 10, 2023 •

edited

Loading

fabian18 commented Jan 10, 2023

kaspar030 commented Jan 16, 2023

jia200x commented Jan 16, 2023

kaspar030 commented Jan 17, 2023

miri64 commented Jan 18, 2023

jia200x commented Jan 18, 2023 •

edited

Loading

benpicco commented Jan 18, 2023

bors bot commented Jan 18, 2023

bors bot commented Jan 18, 2023

kaspar030 commented Jan 18, 2023

bors bot commented Jan 18, 2023

bors bot commented Jan 19, 2023

socket_zep: only report size of single datagram #19121

socket_zep: only report size of single datagram #19121

Conversation

benpicco commented Jan 10, 2023 • edited Loading

Contribution description

Testing procedure

Issues/PRs references

riot-ci commented Jan 10, 2023 • edited Loading

Murdock results

Artifacts

maribu commented Jan 10, 2023

bors bot commented Jan 10, 2023

fabian18 commented Jan 10, 2023

benpicco commented Jan 10, 2023

benpicco commented Jan 10, 2023 • edited Loading

fabian18 commented Jan 10, 2023

kaspar030 commented Jan 16, 2023

jia200x commented Jan 16, 2023

kaspar030 commented Jan 17, 2023

miri64 commented Jan 18, 2023

jia200x commented Jan 18, 2023 • edited Loading

benpicco commented Jan 18, 2023

bors bot commented Jan 18, 2023

bors bot commented Jan 18, 2023

kaspar030 commented Jan 18, 2023

bors bot commented Jan 18, 2023

bors bot commented Jan 19, 2023

benpicco commented Jan 10, 2023 •

edited

Loading

riot-ci commented Jan 10, 2023 •

edited

Loading

benpicco commented Jan 10, 2023 •

edited

Loading

jia200x commented Jan 18, 2023 •

edited

Loading