Skip to content

drivers/at86rf215: make use of packet queue if available#14954

Merged
benpicco merged 1 commit intoRIOT-OS:masterfrom
benpicco:drivers/at86rf215-netif_pktq
Dec 1, 2020
Merged

drivers/at86rf215: make use of packet queue if available#14954
benpicco merged 1 commit intoRIOT-OS:masterfrom
benpicco:drivers/at86rf215-netif_pktq

Conversation

@benpicco
Copy link
Copy Markdown
Contributor

@benpicco benpicco commented Sep 4, 2020 •
edited
Loading

Contribution description

Instead of making the driver block until the hardware finishes TXing, we can now simply queue the packet and rely on it getting re-send when the hardware is ready.

This allows to remove an ugly hack where we call the ISR in a loop to achieve blocking send while still servicing radio events.

Testing procedure

Use the netif_pktq module and send fragmented packets.

master

2020-09-04 18:39:16,406 #  ping6 2001:db8::204:2519:1801:c8c5 -s 512 -c 10
2020-09-04 18:39:16,533 # 520 bytes from 2001:db8::204:2519:1801:c8c5: icmp_seq=0 ttl=64 rssi=-54 dBm time=113.710 ms
2020-09-04 18:39:17,525 # 520 bytes from 2001:db8::204:2519:1801:c8c5: icmp_seq=1 ttl=64 rssi=-57 dBm time=109.456 ms
2020-09-04 18:39:18,534 # 520 bytes from 2001:db8::204:2519:1801:c8c5: icmp_seq=2 ttl=64 rssi=-56 dBm time=120.383 ms
2020-09-04 18:39:19,540 # 520 bytes from 2001:db8::204:2519:1801:c8c5: icmp_seq=3 ttl=64 rssi=-62 dBm time=118.382 ms
2020-09-04 18:39:20,533 # 520 bytes from 2001:db8::204:2519:1801:c8c5: icmp_seq=4 ttl=64 rssi=-62 dBm time=110.219 ms
2020-09-04 18:39:22,533 # 520 bytes from 2001:db8::204:2519:1801:c8c5: icmp_seq=6 ttl=64 rssi=-65 dBm time=112.776 ms
2020-09-04 18:39:23,539 # 520 bytes from 2001:db8::204:2519:1801:c8c5: icmp_seq=7 ttl=64 rssi=-61 dBm time=113.988 ms
2020-09-04 18:39:24,548 # 520 bytes from 2001:db8::204:2519:1801:c8c5: icmp_seq=8 ttl=64 rssi=-61 dBm time=123.613 ms
2020-09-04 18:39:26,401 # 
2020-09-04 18:39:26,420 # --- 2001:db8::204:2519:1801:c8c5 PING statistics ---
2020-09-04 18:39:26,425 # 10 packets transmitted, 8 packets received, 20% packet loss
2020-09-04 18:39:26,428 # round-trip min/avg/max = 109.456/115.315/123.613 ms

netif_pktq

2020-09-04 18:38:11,040 #  ping6 2001:db8::204:2519:1801:c8c5 -s 512 -c 10
2020-09-04 18:38:11,133 # 520 bytes from 2001:db8::204:2519:1801:c8c5: icmp_seq=0 ttl=64 rssi=-57 dBm time=77.002 ms
2020-09-04 18:38:12,128 # 520 bytes from 2001:db8::204:2519:1801:c8c5: icmp_seq=1 ttl=64 rssi=-55 dBm time=73.160 ms
2020-09-04 18:38:13,121 # 520 bytes from 2001:db8::204:2519:1801:c8c5: icmp_seq=2 ttl=64 rssi=-56 dBm time=75.721 ms
2020-09-04 18:38:14,129 # 520 bytes from 2001:db8::204:2519:1801:c8c5: icmp_seq=3 ttl=64 rssi=-56 dBm time=74.772 ms
2020-09-04 18:38:15,120 # 520 bytes from 2001:db8::204:2519:1801:c8c5: icmp_seq=4 ttl=64 rssi=-55 dBm time=73.810 ms
2020-09-04 18:38:16,127 # 520 bytes from 2001:db8::204:2519:1801:c8c5: icmp_seq=5 ttl=64 rssi=-56 dBm time=72.549 ms
2020-09-04 18:38:17,134 # 520 bytes from 2001:db8::204:2519:1801:c8c5: icmp_seq=6 ttl=64 rssi=-56 dBm time=77.309 ms
2020-09-04 18:38:18,130 # 520 bytes from 2001:db8::204:2519:1801:c8c5: icmp_seq=7 ttl=64 rssi=-57 dBm time=83.665 ms
2020-09-04 18:38:19,133 # 520 bytes from 2001:db8::204:2519:1801:c8c5: icmp_seq=8 ttl=64 rssi=-58 dBm time=75.729 ms
2020-09-04 18:38:20,128 # 520 bytes from 2001:db8::204:2519:1801:c8c5: icmp_seq=9 ttl=64 rssi=-59 dBm time=76.030 ms
2020-09-04 18:38:20,129 # 
2020-09-04 18:38:20,131 # --- 2001:db8::204:2519:1801:c8c5 PING statistics ---
2020-09-04 18:38:20,142 # 10 packets transmitted, 10 packets received, 0% packet loss
2020-09-04 18:38:20,147 # round-trip min/avg/max = 72.549/75.974/83.665 ms

Issues/PRs references

uses feature from #11263

@benpicco benpicco added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation State: waiting for other PR State: The PR requires another PR to be merged first Area: drivers Area: Device drivers labels Sep 4, 2020
@benpicco benpicco requested review from jia200x and miri64 September 4, 2020 16:59
@benpicco benpicco force-pushed the drivers/at86rf215-netif_pktq branch from 30a423b to 55c5a72 Compare September 4, 2020 19:40
@benpicco benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed State: waiting for other PR State: The PR requires another PR to be merged first CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Sep 7, 2020
@benpicco benpicco force-pushed the drivers/at86rf215-netif_pktq branch from 55c5a72 to bb2f88e Compare September 8, 2020 08:09
@benpicco benpicco requested a review from fjmolinas September 8, 2020 09:14
miri64
miri64 reviewed Sep 8, 2020
View reviewed changes
Makefile.dep Outdated
ifneq (,$(filter gnrc_lorawan,$(USEMODULE)))
USEMODULE += gnrc_netif_lorawan
endif
ifneq (,$(filter netif_pktq,$(USEMODULE)))
Copy link
Copy Markdown
Member

@miri64 miri64 Sep 8, 2020

Choose a reason for hiding this comment

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

Not sure how this would be expressed in the build system: but shouldn't netif_pktq rather be a feature that the driver may request? If a stack does not support this for whatever reason you would still be able to use it without netif_pktq and the user wouldn't be bothered with such a low-level decision.

Copy link
Copy Markdown
Contributor Author

@benpicco benpicco Sep 8, 2020

Choose a reason for hiding this comment

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

Are you suggesting something like FEATURE_OPTIONAL?
IIRC modules can't provide features with the current build system.

Or should I just add USEMODULE += netif_pktq to the driver's Makefile.dep and drop the blocking code entirely?

Copy link
Copy Markdown
Member

@miri64 miri64 Sep 8, 2020

Choose a reason for hiding this comment

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

I meant the first, but did not know, that it is not easily realizable, currently. I would not enforce the packet queue, as one might want to optimize it out.

Copy link
Copy Markdown
Contributor

@fjmolinas fjmolinas Sep 8, 2020

Choose a reason for hiding this comment

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

I meant the first, but did not know, that it is not easily realizable, currently. I would not enforce the packet queue, as one might want to optimize it out.

For now FEATURES_OPTIONAL does not apply to this use case. The only thing that kind of resembles this is DEFAULT_MODULE but that has its own issues, would only work if gnrc_netif_pktq has no dependencies of its own.

AFAIK this seems like the right way for the use to include netif_pktq if solicited, its an opt-in policy, the only opt-out option for modules is DEFAULT_MODULE.

Copy link
Copy Markdown
Contributor

@fjmolinas fjmolinas Sep 21, 2020

Choose a reason for hiding this comment

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

@miri64 I think the way it is modeled currently is OK considered our current buildystem, should we go ahead with this one? Are there any other showstoppers?

Copy link
Copy Markdown
Contributor Author

@benpicco benpicco Sep 29, 2020

Choose a reason for hiding this comment

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

So many options which one should I take? 😄

Copy link
Copy Markdown
Member

@miri64 miri64 Sep 29, 2020

Choose a reason for hiding this comment

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

I would prefer the last one :-).

Copy link
Copy Markdown
Contributor Author

@benpicco benpicco Sep 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Pulling in at86rf215_blocking_send when gnrc_netif_pktq is not selected sounds like the best approach.

Copy link
Copy Markdown
Contributor Author

@benpicco benpicco Sep 29, 2020

Choose a reason for hiding this comment

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

The only problem I see is that now drivers can't depend on having a netif_pktq available and still have to implement a fall-back blocking send :\

Copy link
Copy Markdown
Member

@miri64 miri64 Sep 30, 2020

Choose a reason for hiding this comment

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

That will be addressed by @jia200x's new API though, right?

@miri64 miri64 mentioned this pull request Sep 23, 2020
Closed
@benpicco benpicco force-pushed the drivers/at86rf215-netif_pktq branch from bb2f88e to 5b19c7b Compare September 29, 2020 21:53
@benpicco benpicco force-pushed the drivers/at86rf215-netif_pktq branch from 5b19c7b to b543a34 Compare October 24, 2020 19:36
@benpicco benpicco added Area: drivers Area: Device drivers and removed Area: drivers Area: Device drivers labels Oct 29, 2020
@fjmolinas
Copy link
Copy Markdown
Contributor

fjmolinas commented Nov 10, 2020

Is everything good on your side here @miri64 ?

@benpicco benpicco requested a review from maribu December 1, 2020 09:18
@maribu
Copy link
Copy Markdown
Member

maribu commented Dec 1, 2020

Looks good to me. To me it sounded like @miri64 is fine with the PR. I will test and (depending on the test result) ACK, but I would like to wait for @miri64 to confirm her comments are addressed before the merge button is hit.

@maribu
Copy link
Copy Markdown
Member

maribu commented Dec 1, 2020

Works as advertised. On the nucleo-f767zi I only get a small reduction of the RTT (124.234/124.345/124.441 ms vs 124.675/124.735/124.811 ms for ping6 -s 1024). I blindly assume this is due to the CPU not being a bottleneck on the fast nucleo-f767zi anyway.

@maribu maribu added 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 labels Dec 1, 2020
maribu
maribu approved these changes Dec 1, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

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

ACK

@maribu maribu removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Dec 1, 2020
@maribu maribu added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Dec 1, 2020
miri64
miri64 reviewed Dec 1, 2020
View reviewed changes
@benpicco benpicco force-pushed the drivers/at86rf215-netif_pktq branch from 3d8d4ed to 57211c8 Compare December 1, 2020 14:24
@miri64
Copy link
Copy Markdown
Member

miri64 commented Dec 1, 2020

Murdock is very unhappy :-/

@benpicco benpicco force-pushed the drivers/at86rf215-netif_pktq branch from 57211c8 to 7fc042e Compare December 1, 2020 15:04
@benpicco
Copy link
Copy Markdown
Contributor Author

benpicco commented Dec 1, 2020

image

@benpicco benpicco merged commit ef0eb90 into RIOT-OS:master Dec 1, 2020
@benpicco benpicco deleted the drivers/at86rf215-netif_pktq branch December 1, 2020 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@miri64 miri64 miri64 left review comments

@maribu maribu maribu approved these changes

@jia200x jia200x Awaiting requested review from jia200x

@fjmolinas fjmolinas Awaiting requested review from fjmolinas

@leandrolanzieri leandrolanzieri Awaiting requested review from leandrolanzieri

Assignees

No one assigned

Labels

Area: drivers Area: Device drivers 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: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@benpicco @fjmolinas @maribu @miri64