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.

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

drivers/atwinc15x0: don't disable interrupts #18800

Merged
merged 1 commit into from
Oct 26, 2022

Conversation

benpicco
Copy link
Contributor

@benpicco benpicco commented Oct 25, 2022

Contribution description

Globally disabling interrupts is very bad if done for an extended period of times.
Here atwinc15x0 would disable interrupts for up to 55µs which leads to lost bytes when used with UART based interfaces like SLIP or DOSE.

When looking at the code it's not clear why the interrupts were disabled in the first place. Only the atwinc15x0 thread itself will interact with the data, even if there is a GPIO interrupt from the module it will only call netdev_trigger_event_isr() which adds an entry to the event queue to be processed by the atwinc15x0 thread.

Testing procedure

I ran examples/gnrc_networking with

2022-10-25 14:51:26,075 # ping -c 10 -i 100 riot-os.org
2022-10-25 14:51:26,246 # 12 bytes from 2001:67c:254:b0b0::1: icmp_seq=0 ttl=60 time=156.320 ms
2022-10-25 14:51:26,257 # 12 bytes from 2001:67c:254:b0b0::1: icmp_seq=1 ttl=60 time=65.487 ms
2022-10-25 14:51:26,318 # 12 bytes from 2001:67c:254:b0b0::1: icmp_seq=2 ttl=60 time=28.961 ms
2022-10-25 14:51:26,419 # 12 bytes from 2001:67c:254:b0b0::1: icmp_seq=3 ttl=60 time=30.556 ms
2022-10-25 14:51:26,518 # 12 bytes from 2001:67c:254:b0b0::1: icmp_seq=4 ttl=60 time=30.501 ms
2022-10-25 14:51:26,618 # 12 bytes from 2001:67c:254:b0b0::1: icmp_seq=5 ttl=60 time=29.821 ms
2022-10-25 14:51:26,715 # 12 bytes from 2001:67c:254:b0b0::1: icmp_seq=6 ttl=60 time=26.097 ms
2022-10-25 14:51:26,817 # 12 bytes from 2001:67c:254:b0b0::1: icmp_seq=7 ttl=60 time=28.229 ms
2022-10-25 14:51:27,081 # 12 bytes from 2001:67c:254:b0b0::1: icmp_seq=8 ttl=60 time=190.449 ms
2022-10-25 14:51:27,091 # 12 bytes from 2001:67c:254:b0b0::1: icmp_seq=9 ttl=60 time=98.197 ms
2022-10-25 14:51:27,092 # 
2022-10-25 14:51:27,096 # --- riot-os.org PING statistics ---
2022-10-25 14:51:27,104 # 10 packets transmitted, 10 packets received, 0% packet loss
2022-10-25 14:51:27,110 # round-trip min/avg/max = 26.097/68.461/190.449 ms

2022-10-25 14:52:46,783 # > ifconfig
2022-10-25 14:52:46,821 # Iface  5  HWaddr: F8:F0:05:A9:EC:19  Channel: 11  RSSI: -41  Link: up 
2022-10-25 14:52:46,823 #            State: IDLE 
2022-10-25 14:52:46,829 #           L2-PDU:1500  MTU:1492  HL:255  Source address length: 6
2022-10-25 14:52:46,831 #           Link type: wireless
2022-10-25 14:52:46,837 #           inet6 addr: fe80::faf0:5ff:fea9:ec19  scope: link  VAL
2022-10-25 14:52:46,844 #           inet6 addr: 2001:9e8:140c:8100:faf0:5ff:fea9:ec19  scope: global  VAL
2022-10-25 14:52:46,847 #           inet6 group: ff02::1
2022-10-25 14:52:46,850 #           inet6 group: ff02::1:ffa9:ec19
2022-10-25 14:52:46,851 #           
2022-10-25 14:52:46,854 #           Statistics for Layer 2
2022-10-25 14:52:46,858 #             RX packets 4905  bytes 570535
2022-10-25 14:52:46,863 #             TX packets 4130 (Multicast: 5)  bytes 485006
2022-10-25 14:52:46,866 #             TX succeeded 4130 errors 0
2022-10-25 14:52:46,869 #           Statistics for IPv6
2022-10-25 14:52:46,873 #             RX packets 4131  bytes 427578
2022-10-25 14:52:46,878 #             TX packets 4130 (Multicast: 5)  bytes 427186
2022-10-25 14:52:46,881 #             TX succeeded 4130 errors 0

and

sudo ping -f 2001:9e8:140c:8100:faf0:5ff:fea9:ec19
PING 2001:9e8:140c:8100:faf0:5ff:fea9:ec19(2001:9e8:140c:8100:faf0:5ff:fea9:ec19) 56 data bytes
..^C   
--- 2001:9e8:140c:8100:faf0:5ff:fea9:ec19 ping statistics ---
1573 packets transmitted, 1571 received, 0.127146% packet loss, time 7451ms
rtt min/avg/max/mdev = 2.453/5.225/122.543/7.055 ms, pipe 7, ipg/ewma 4.739/4.738 ms

in parallel with no ill effects.

Issues/PRs references

@github-actions github-actions bot added the Area: drivers Area: Device drivers label Oct 25, 2022
@benpicco benpicco added the Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) label Oct 25, 2022
@gschorcht gschorcht added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 26, 2022
@riot-ci
Copy link

riot-ci commented Oct 26, 2022

Murdock results

✔️ PASSED

d0a1a76 drivers/atwinc15x0: don't disable interrupts

Success Failures Total Runtime
1991 0 1991 07m:13s

Artifacts

This only reflects a subset of all builds from https://ci-prod.riot-os.org. Please refer to https://ci.riot-os.org for a complete build for now.

@benpicco benpicco merged commit da3ce65 into RIOT-OS:master Oct 26, 2022
@benpicco benpicco deleted the drivers/atwinc15x0-irq branch October 26, 2022 19:49
@kaspar030 kaspar030 added this to the Release 2023.01 milestone Jan 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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 Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants