Skip to content

pkg/lwip: add support for HAL radios that require IRQ offloading#18465

Merged
benpicco merged 3 commits intoRIOT-OS:masterfrom
jia200x:pr/lwip_bhp
Aug 25, 2022
Merged

pkg/lwip: add support for HAL radios that require IRQ offloading#18465
benpicco merged 3 commits intoRIOT-OS:masterfrom
jia200x:pr/lwip_bhp

Conversation

@jia200x
Copy link
Copy Markdown
Member

@jia200x jia200x commented Aug 17, 2022 •
edited
Loading

Contribution description

This PR utilizes #18464 to run radio HAL based devices with LWIP. This also allows the usage of other non-netdev devices that require ISR offloading.

As a result of this PR, I also push support for KW2XRF radios on LWIP.

Testing procedure

Murdock compile output should be fine
Test LWIP with any KW2XRF based radio.

Issues/PRs references

Depends on #18464
Now it depends on #18479

@jia200x jia200x added the State: waiting for other PR State: The PR requires another PR to be merged first label Aug 17, 2022
@jia200x jia200x requested review from benpicco and maribu August 17, 2022 15:29
@github-actions github-actions bot added Area: Kconfig Area: Kconfig integration Area: network Area: Networking Area: pkg Area: External package ports Area: sys Area: System Area: tests Area: tests and testing framework labels Aug 17, 2022
@jia200x jia200x changed the title pkg/lwip: add support for HAL radios with IRQ offload pkg/lwip: add support for HAL radios that require IRQ offloading Aug 17, 2022
@benpicco
Copy link
Copy Markdown
Contributor

benpicco commented Aug 17, 2022

Why can't LWIP use bhp_event?

@jia200x
Copy link
Copy Markdown
Member Author

jia200x commented Aug 17, 2022

It definitely can, I just wanted to speed up the migration process of the HAL. I was planning anyway to migrate all pending IPC based IRQ offloaders at some point.

@jia200x jia200x mentioned this pull request Aug 18, 2022
Merged
@benpicco benpicco removed the State: waiting for other PR State: The PR requires another PR to be merged first label Aug 19, 2022
@benpicco
Copy link
Copy Markdown
Contributor

benpicco commented Aug 19, 2022

This needs a rebase

@jia200x
Copy link
Copy Markdown
Member Author

jia200x commented Aug 19, 2022

rebased!

@github-actions github-actions bot removed Area: tests Area: tests and testing framework Area: sys Area: System Area: Kconfig Area: Kconfig integration labels Aug 19, 2022
benpicco
benpicco reviewed Aug 19, 2022
View reviewed changes
benpicco
benpicco reviewed Aug 19, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

Looks good to me - there is now a leftover kw2xrf_setup() in kw2xrf.h - want to get rid of that while you're at it?

benpicco
benpicco reviewed Aug 19, 2022
View reviewed changes
pkg/lwip/init_devs/auto_init_kw2xrf.c
kw2xrf_init(&kw2xrf_devs[i], &kw2xrf_params[i], &kw2xrf_netdev[i].submac.dev,
bhp_msg_isr_cb, &netif[i].bhp);

netdev_register(&kw2xrf_netdev[i].dev.netdev, NETDEV_KW2XRF, i);
Copy link
Copy Markdown
Contributor

@benpicco benpicco Aug 19, 2022

Choose a reason for hiding this comment

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

Why is this not done by kw2xrf_init()?

Copy link
Copy Markdown
Member Author

@jia200x jia200x Aug 19, 2022

Choose a reason for hiding this comment

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

because that would add a dependency to netdev that does not exist in the current Radio HAL drivers.

@jia200x
Copy link
Copy Markdown
Member Author

jia200x commented Aug 19, 2022

Looks good to me - there is now a leftover kw2xrf_setup() in kw2xrf.h - want to get rid of that while you're at it?

sure, good catch. I can remove it

@jia200x
Copy link
Copy Markdown
Member Author

jia200x commented Aug 19, 2022

done!

@github-actions github-actions bot added the Area: drivers Area: Device drivers label Aug 19, 2022
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Aug 19, 2022
@jia200x
Copy link
Copy Markdown
Member Author

jia200x commented Aug 19, 2022

Probably a mutex can do the trick, but still, this should probably be handled in a separate PR, as it affects essentially all network devices

@benpicco
Copy link
Copy Markdown
Contributor

benpicco commented Aug 19, 2022

@maribu just recently stumbled across this too.
Is there a reason for LWIP to use two threads? Most netdev drivers will have been build with the assumption that access only happens from a single thread (as is with the default GNRC), so they will likely fail in subtle ways when paired with LWIP.

@jia200x
Copy link
Copy Markdown
Member Author

jia200x commented Aug 19, 2022

It uses only only uses "one thread" (with quotes because it actually allocates one for every network device), everything else runs in the context of the caller (for tests/lwip this would be main).
Due to the nature of the sock API we need "at least" an extra thread, similar to GNRC. The problem lies on concurrency between netdev calls from different threads.

I just implemented a version with rmutex (although it's not my favourite sync mechanism) and it seems to work fine.

IMO the solutions are either:

  1. We stick to these recursive mutexes.
  2. We ensure all netdev calls occur on the same IRQ offloader thread. Since the IRQ offload thread always runs with the highest priority, we could just send a message to the thread with the right context.

Any preference?

@maribu
Copy link
Copy Markdown
Member

maribu commented Aug 19, 2022

I think it is possible to use a single thread to handle both lwip events as well as ISRs. With sys/event, we can event wait on multiple queues to implement priorities. Likely it does make sense to boost the priority of ISR events, as some drives have to deadlines to fullfil (e.g. timeouts for ACKs, or the CC110x that is writing frames of up to 255 bytes into a 64 byte fifo under the assumption the MCU can drain the FIFO faster than the transceiver can fill it).

But having an rmutex until then sounds good to me.

@jia200x jia200x mentioned this pull request Aug 19, 2022
Merged
@benpicco
Copy link
Copy Markdown
Contributor

benpicco commented Aug 24, 2022

This needs a rebase, but then it should be good to merge, right?

@jia200x
Copy link
Copy Markdown
Member Author

jia200x commented Aug 24, 2022

rebased! and yes, it should be ready to merge

@benpicco benpicco enabled auto-merge August 24, 2022 11:01
@benpicco benpicco disabled auto-merge August 24, 2022 11:02
@benpicco
Copy link
Copy Markdown
Contributor

benpicco commented Aug 24, 2022

CI is unhappy

@jia200x
Copy link
Copy Markdown
Member Author

jia200x commented Aug 24, 2022

CI is unhappy

Unrelated error

@jia200x jia200x 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 Aug 24, 2022
@benpicco
Copy link
Copy Markdown
Contributor

benpicco commented Aug 24, 2022

Unrelated error

I don't think so

@MrKevinWeiss
Copy link
Copy Markdown
Contributor

MrKevinWeiss commented Aug 25, 2022

I really hope I fixed all the unrelated error issues...

@MrKevinWeiss
Copy link
Copy Markdown
Contributor

MrKevinWeiss commented Aug 25, 2022

try building locally examples/paho-mqtt with arduino-mkrwan1300 or other boards

@benpicco
Copy link
Copy Markdown
Contributor

benpicco commented Aug 25, 2022

If you look at the error output it is clearly related to this PR.

@jia200x
Copy link
Copy Markdown
Member Author

jia200x commented Aug 25, 2022

I think I saw the wrong output, because sure, it's definitely a problem of this PR

@jia200x
Copy link
Copy Markdown
Member Author

jia200x commented Aug 25, 2022

it was a rebase error. Now it should be fixed

@benpicco benpicco enabled auto-merge August 25, 2022 15:32
@benpicco benpicco merged commit 3ae973a into RIOT-OS:master Aug 25, 2022
@maribu maribu mentioned this pull request Sep 19, 2022
Merged
@maribu maribu added this to the Release 2022.10 milestone Oct 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benpicco benpicco benpicco approved these changes

@maribu maribu Awaiting requested review from maribu

@leandrolanzieri leandrolanzieri Awaiting requested review from leandrolanzieri

@MrKevinWeiss MrKevinWeiss Awaiting requested review from MrKevinWeiss

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

Assignees

No one assigned

Labels

Area: drivers Area: Device drivers Area: network Area: Networking Area: pkg Area: External package ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR

Projects

None yet

Milestone

Release 2022.10

Development

Successfully merging this pull request may close these issues.

4 participants

@jia200x @benpicco @maribu @MrKevinWeiss