Conversation
|
Very nice work! And thanks for the use case description. Do you know what the delta RAM usage is for this on, say, Also, on quick glance, I think this might have issues under FreeRTOS due to the async context threadsafe. I will dig a bit more to understand how you're using it in place of IRQ callbacks, and how it would interact with the existing network IRQ/packet injection which happens in a separate task. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Looks like you need to define a |
|
I have tested
|
|
The ROM's not too big a deal, I'd say, but losing 6KB of RAM for all sketches feels kind of painful. Let me see if there's a simple SDKOverride we can do to lazy allocate the network buffers (looking at the MAP, the FWIW I was able to test the net connection from the Pico to my Ubuntu 22.04 box (for some reason IP forwarding is not quite working, and I don't want to mess with things because I have several office VPN connections and Virtmgr/Docker rules). Just moving the fortune to the GW |
|
I'm getting lots of crashes when a USB serial connection is active and the network interface is line If I don't have a serial monitor/minicom active things are fine, but OTW I get something like the above almost every time. There may be some weird interaction between USB and Ethernet processing (which were never intertwined before) when this is actuve... |
|
Oh, I have not seen that before. I will try to replicate. Maybe the packet handoff is breaking some rules still |
|
I can't seem to find the magic incantation to make a PR against your PR here, but if you look at the last 3 commits to #3266 you'll see where I was able to make the ncd_device not use any RAM (and almost no ROM) when not used by making a dummy sdkoverride, using the original ncd_device.c inside your library folder (so only compiled when used), and removing the precompiled file from libpico. |
|
Wouldn't that cause a dependency headache when upgrading |
It's a very minor one. We have hand modified files from the SDK in If you have a better way, though, of not needing the extra unused 6K of RAM for most users, I'm happy to hear it. But OTW, while I think this is pretty neat. I don't think I could make all other apps lose that space. -edit- Now that I found what seems to be a reasonable way to do this, I might also do the same for MIDI and HID which takes 1/2 KB on every sketch. With RAM prices increasing 300% in the last 30 days every byte counts. 😆 -edit- |
earlephilhower
left a comment
earlephilhower
left a comment
There was a problem hiding this comment.
I get your hesitation on doing the TUSB hackage, but 6K lost on every sketch is a real problem.
I'll take this as-is, with the 6k, and then combine the TUSB weak override for this and the MIDI, HID, and MSD devices in a follow-up.
One request, though, because getting this working is a little complicated. Can you add a small bit of documentation in the docs/ folder with the instructions you've got for Win and Linux? That way there's a way for a user to discover how to set it up without going through a (closed) PR.
|
I understand 6k is problematic. I will take a look in the next couple days. I also tested freertos and it does indeed not work. I might fix that, too |
|
In #3272 I implemented a pretty simple and clean (enough) framework to avoid the extra RAM and flash when not needed. A fake local library that includes the xxx_device.c and a weak stub (cut-n-paste with a simple Let me test that out on all the cases it could affect, but when merged I think the same could be done here with almost no thought to get that 6K RAM and 1.2K flash back. It should also let me increase the MSC (USB drive) buffer size to something much larger and faster, the MIDI buffers to larger for lsysex strings, and enable things like the USB audio and video without eating up RAM on most sketches. |
|
Very cool. I managed to read up on freeRTOS and how it is used by this project in the meantime. Pretty sure I will get USB NCM working with it. |
|
And could you please add your new library path |
Would you like me to enable NCM and do the mini-fake-lib in that PR so on your side you don't need to? You'd just need a 1-line |
|
Sure, if it is easy for you to do |
|
I just merged the TUSB restructure. You can remove the tusb-config and all library changes from the PR and just add Darn...just realized that now I need to remove my own |
|
I have a question regarding deadlocking. The So what happens if a lower prio IRQ or the main loop already has the lock? My guess is we block and only a second higher priority IRQ can save us from deadlock. I must be missing something as that doesn't sound like a working ethernet implementation |
|
I'm traveling now but this is special cased or it would be bad, as you note. We defer the irq in that case and it gets reenabled after the main app finishes. In the main irq callback we check the LWIP in progress flag and if set just disable the irq and do nothing then. When the LWIP mutex is feed by the app it will just reenabled the irq and it'll retry. See comments in LWIP_wrap:42 (sorry, GitHub won't let me link from phone) |
|
I don't think that addresses my concern. Unless I am missing something, it seems like the more the user code calls LWIP functions, the more likely an interrupt-driven ethernet could run into this. This can't happen with polling ethernet because One way out would be to avoid blocking acquires in Sorry for the offtopic, I just stumbled over this because NCM calls |
|
I'm still traveling so it's painful to review code, but IIRC that flag is set as part of the wrapped calls. Writing this, I guess it's possible to just check the mutex state (but the mutex changes from cyw43 to all others so it's a pain). If you have a faulting case because of this one the W or some wired Ethernet or 3rd party WiFi we can definitely look into it, but as of now I've not had any big reports. If it was really an issue I would have thought someone would have run into it in the couple years we've had this setup. My own stress testing of LWIP had been on both Cyw43 and W5100 irq enabled and I've had days of runtime with multiple clients without deadlocks seen. I think this particular trouble is coming from the fact USB can IRQ whenever and wants to send in a packet (since we don't disable USB IRQs on LWIP calls). I guess it's possible, but I have a feeling there would be bad side effects of we didn't service then promptly (ie. starvation and disconnection from host). But that's just a guess right now |
|
I was having occasional panic's in my testing, so I have been debugging. I think I have found a race condition in arduino-pico/libraries/WiFi/src/include/ClientContext.h Lines 80 to 98 in 690fb4b Between all the If in-between those a TCP RST packet is processed, LWIP will free the So |
|
Interesting failure mode, nice job finding it! I think the solution would be a trivial instantiation of a --edit-- Actually I don't think that will help you here. It's already safe for devices which use LWIPMutex to gate any packet inject IRQs, but USB isn't one of them, it's free-running. You might take the |
|
Actually, it seems to me that the clearing of the callbacks is paranoia and not really needed anyway. |
USB packet injection is actually gated with LWIPMutex in my test, because i have improved the implementation. The USB mutex adds packets to a queue and sets a worker in LWIP's async_context pending. That worker fetches from the queue and processes the packets. Calling Between the As far as i can see the only real solution is |
Sounds good to me. Maybe a separate PR to get more mileage while you're debugging NCM?
|
|
On inspection it seems to me the same race could happen in the abort() method as well. I've thrown a PR with the mutex just to get it out there and tested by a bunch of users ASAP while your work here continues. Thx! |
This work in progress commit adds NULL checks in lwip_wrap.cpp to prevent double frees in ClientContext.h See earlephilhower#3265 and earlephilhower#3368 It is alternative to earlephilhower#3370
It empowers Ethernet drivers to put packets in a queue. This is in preparation of USB NCM (Network Control Model), which requires a transmit queue to prevent deadlocks. Additionally, the change may allow chained pbufs to be sent in the future.
This implementation in arduino-pico enables an Ethernet connection with a host PC over USB without any extra hardware. Other uses of USB like Serial, Mouse or Keyboard continue to work in parallel. It has been tested on Windows and Linux. MacOS should also work. It supports baremetal and freertos, both with and without IPv6.
functionpointer
force-pushed
the
usb_ncm_new
branch
from
008c582 to
5000ac2
Compare
|
I have finally managed to get this working, it turned out to be way more work than expected. Some key implementation details: It turns out receive queues are not viable, as tinyusb frees packet buffers immediately after The new implementation therefore processes the entire packet inside The solution i have implemented is to schedule a worker in the lwIP async_context. This required adding This only works because it turns out Transmitting is different, it needs a transmit queue. That's because packets can be generated at any time, so we can't assume |
|
Great work, sounds like a ton of effort! Is this going to be dependant on your other prs, or standalone? And what order would you suggest going thru them? I've not had much time recently to work on the repo, so I want to prioritize and get the order right when I do sit down and review these. |
|
The PRs are independent. The two double free PRs #3370 #3377 are mutually exclusive. #3372 is needed for integrity, especially with ESPHome. #3408 is entirely optional, but getting something like it merged may help users find lock issues earlier. I would suggest #3372 first, then this PR, the rest in any order. But they are independent, any order works. |
libraries/lwIP_USB_NCM/examples/WiFiClient-NCMEthernet-platformio/.gitignore
Show resolved
Hide resolved
libraries/lwIP_USB_NCM/examples/WiFiClient-NCMEthernet/WiFiClient-NCMEthernet.ino
Outdated
Show resolved
Hide resolved
libraries/lwIP_USB_NCM/examples/WiFiClient-NCMEthernet-platformio/src/main.cpp
Outdated
Show resolved
Hide resolved
earlephilhower
left a comment
earlephilhower
left a comment
There was a problem hiding this comment.
This looks very nice. A reading of the code looks good with minimal changes out of your new module, which is great. Let me do a few sanity checks on real HW with existing ENET and this USB NCM. If nothing pops up we're good to go!
They were mistakenly left in from testing
3 participants
Overview
NCM (Network Control Model) is the newest of three protocols for Ethernet over USB.
This implementation creates an Ethernet connection with a host PC over USB without any extra hardware.
Other uses of USB like Serial, Mouse or Keyboard continue to work in parallel.
Use case is for smart home devices that live close to a PC or server. NCM gives the reliability of a wired connection without extra hardware like an Ethernet NIC and without the downsides of classic solutions like a custom protocol over serial. Instead, existing software like ESPHome can be used.
Testing
I have tested on Windows and Linux. No driver installation is required. MacOS should also work.
The example sketch provides 1ms ping latency.
Usage
Compile the provided example sketch.
Modify DHCP and logging as desired.
Once running, the Pico shows up as a new network interface on the host PC.
It must be configured before use. Example for Linux:
On Windows, Internet Connection Sharing can be configured: https://superuser.com/a/1899168
That even includes a DHCP server. In my tests, it works with LWIP's built-in DHCP client.