Use static SRAM buffer to fix USB host DMA problem #10636
Conversation
This modifies tannewt's DMA-safe buffer fix for usb.core.Device to use a single global static buffer instead of potentially many heap allocated buffers. Hopefully the shared buffer will avoid runtime allocation errors. CAUTION: The size of the buffer may not be big enough for some transferst that people might want to make. If the requested length of a transfer is too big, that length will be truncated to what can fit in the buffer. The actual amount of transferred data will be indicated by the length in the transfer result. The calling code _should definitely_ check to see if the result matched what they asked for.
|
I tested a local build of this code with my reproducer code from #10562 and with Fruit Jam Fruitris 1.0.0-alpha.5. Both worked fine. I did not try installing Fruit Jam OS, so I haven't actually seen the problem that RetiredWizard reported. I'm not sure if this code will fix that. But, since I removed the dynamic allocation stuff, it seems possible. |
|
What are the memory implications for programs that don't use USB (at least not USB-host)? Would it make sense to put the buffer-size constant in one of the global/port/board make-files to allow easy tweaking? At least you should check if |
As this stands now, RP2350 boards would have 256 bytes of internal SRAM dedicated to the usb dma-safe buffer. (see ports/raspberrypi/mpconfigport.mk in the changed files). Other ports and boards would have no change unless unless they opted into the dma-safe stuff by setting I am expecting that the suitability of the buffer size will be determined by the size of configuration descriptors for HID keyboards, some of which are kinda big (i.e. what's the biggest control transfer buffer size that should be possible). I don't think it is likely to make sense to tune the buffer size differently for individual CircuitPython boards or ports. If someone were to do that, it could mean some keyboards would work on some CircuitPython boards but not on others. That could get really confusing. If a board wants to disable USB host, that's already possible ( |
|
Also, it might work fine to set the default buffer size to something much lower, like 128 or perhaps 64. The supervisor uses TinyUSB's HID class driver in a way which probably (maybe? not sure) does not depend on anything in shared-module/usb/core/Device.c for fetching configuration descriptors. I'm not sure what other code might depend on being able to read large configuration descriptors in one control transfer call. I think it would probably work fine to write code that made a series of smaller control transfers with different starting indexes. But, I'd expect any existing code probably assumes it can make whatever size of control transfer it wants. |
|
I have to run this morning but I'll load this up this afternoon. Thanks! |
|
It sounds like we may not go this way but Fruitris does now start with the mouse option and the artifact from here. |
Do you mean this PR does successfully fix the memory allocation error that you were seeing on the other PR? |
Yes |
This is a revision to @tannewt's earlier PR that used dynamic allocation in the SRAM heap:
This is intended to fix #10562
Changes relative to tannewt's PR:
The point of this PR is to see if it resolves the dynamic allocation errors reported by @RetiredWizard with the Fruit Jam Fruitris game running on Fruit Jam OS.