Rework rp2xxx USB driver #658
Conversation
core/src/core/usb/cdc.zig
Outdated
| if (this.rx_buf) |rx| { | ||
| const len = @min(rx.len, dst.len); | ||
| // TODO: please fixme: https://github.com/ZigEmbeddedGroup/microzig/issues/452 | ||
| std.mem.copyForwards(u8, dst, rx[0..len]); |
There was a problem hiding this comment.
Regarding those "TODO #452" memcpy should be part of DeviceInterface I guess as it is in TinyUSB and CherryUSB projects
There was a problem hiding this comment.
I disagree, the @memcpy builtin is meant to provide a good implementation regardless of the target.
After checking on actual hardware, it seems like the issue is gone, maybe because of the recent rom changes?
There was a problem hiding this comment.
Here is issue description in:
- CherryUSB: Change all memcpy to usb_memcpy cherry-embedded/CherryUSB#297 memcpy implementation in newlib (arm toolchain) is wrong.
- TinyUSB: https://github.com/liamfraser/tinyusb/blob/31a979a6cccc2d4dc0a86474643710ae3b7f34fe/src/portable/raspberrypi/rp2040/rp2040_usb.c#L57
- PicoSDK: https://github.com/raspberrypi/pico-sdk/blob/ee68c78d0afae2b69c03ae1a72bf5cc267a2d94c/src/rp2_common/pico_mem_ops/CMakeLists.txt#L15 (beware commpiler memcpy cannot be used for unaligned copies in peripheral space)
Don't know what to think about it. At least I would extract it to some inline usb_memcpy function and use std.mem.copyForwards or @memcpy inside for now.
There was a problem hiding this comment.
The usb DPRAM behaves like regular memory unlike most peripheral memory.
As per zig documentation on @memcpy both source and destination can have any alignment.
I can't comment on how good the memcpy implementation is, but judging from the lack of HardFault exceptions raised it must be correctly handling unaligned data. I also disassembled the .elf file of the hid example and the implementation of __aeabi_memcpy uses ldrb and strb instructions, which work on unaligned data.
There was a problem hiding this comment.
Not sure how I got this mixed up, but the whole time I was thinking it was the RP2040 that had problems with memcpy. #452 of course clearly states the bug affects RP2350...
And unsurprisingly the datasheet documents this:
This is rather annoying. How are we supposed to guarantee that the optimizer does not use unaligned LDR/LDRH? I originally wanted to avoid needlessly copying the buffers but that seems like the simplest option now.
There was a problem hiding this comment.
So we must read/write data to some intermediate buffer (in main memory) to make sure that aligned nonoptimized memcpy operation is used right?
|
@piotrfila, what toolchain version are you using, and which boards were tested? I am using Zig 0.14.1 on Windows and tried to run this code on the RP2040 and RP2350 (Arm cores). For the RP2350, I get some Windows USB errors, and the RP2040 is somewhat unstable—sometimes it works, and sometimes it doesn't. |
|
I am also using 0.14.1, but I have done all the testing under Linux, where RP2040 seems to work fine. |
This PR is my shot at a better USB driver implementation.
Key changes:
I'm open to suggestions for changes