feat: add usb msc support for nrf52840

[mikesmitty]

I don't have an nrf52840 on hand to test yet (should have one tomorrow), but I figured I'd push this up in case @aykevl or anyone else wanted to give it a whirl
#4844 (comment)

[aykevl]

I tried this, but sadly it doesn't work.
I haven't investigated why it doesn't work though.

[mikesmitty]

No worries, my nrf52840 just showed up so I'll investigate

[deadprogram]

@mikesmitty any update on this? I would really like to have it for something... 😸

[mikesmitty]

Oh yeah, this fell off my radar. I've got a debugger just about set up with an nrf52840 now so I can dig into this again

[mikesmitty]

Ok, at least in my testing it looks like it's just a problem with m.buf being too small. My test device has a write block size of 4 bytes, which I didn't think would ever happen. The m.buf buffer is set, perhaps a bit naively, to use the write block size as a capacity, but it needs to be 64 bytes at a minimum to be able to store a full usb packet and it's panicking. #5017 also modifies these buffer sizes but in a more expansive way so this will likely end up just being a temporary patch. I just saw Ayke commented again on that one so I'll swap back to that one next

[deadprogram]

Pinging Dr. @mikesmitty 😸

[mikesmitty]

Sorry, been getting distracted a lot recently haha. I'll start working on this again this weekend

[deadprogram]

Thank you!

[ysoldak]

I gave it a try today on XIAO BLE board.

"usb-storage" example compiles and flashes to the board.

Mac can see the device, but can't really use it. I've tried to "Erase" it with Disk Utility, that just hangs.

See screenshots.

[ysoldak]

Same happens with xiao-rp2040 target though, which as I understood is supposed to work, so either nrf version already works as expected OR we need a better example to try with OR need an instruction how to initialize the storage device.

[mikesmitty]

Ok, finally getting back into this now. Let me see where I left off

[ysoldak]

Just in case it helps in any way, here is UF2 bootloader for nRF52 chips.
Boards with UF2 bootloader show up as mass storage devices.

https://github.com/adafruit/Adafruit_nRF52_Bootloader/blob/master/src/usb/usb.c

[mikesmitty]

Woof, that was an adventure. The usb endpoint handling logic for nrf52840 was unconditionally accepting packets and I ended up having to rework it a little bit in order to allow returning a NAK when needed, but it seems to be working for me now if you guys want to give it a shot

[deadprogram]

@ysoldak please give this PR another try if you get a chance.

[ysoldak]

Still can't see Mass Storage Device when flashing usb-storage example to XIAO BLE board.
tinygo flash -target=xiao-ble -opt=z ./src/examples/usb-storage

There is no popup with Eject, Ignore and Initialize... buttons anymore, but nothing else either.

Is there any other (better) way to test?

[mikesmitty]

Sorry, I've been playing whackamole with bugs in this one a little bit. I started putting together a branch with tests to try and iron those out properly. I'll trim this back and put those in the other branch

[deadprogram]

@mikesmitty #5109 has been merged, perhaps this PR can now be rebased and retested?

[mikesmitty]

I rebased if you want to give it a go, but I haven't tested it yet. I spent a bunch of time arguing with both the jlink and the nrf52 board, but I've finally got a reliable workflow going again

[mikesmitty]

Ok, nailed down the bug I was stuck on for the longest time where endpoint stalls were hanging indefinitely and the CSW was not being returned. In order to reset the DATA0/DATA1 in setEPDataPID like is done after an endpoint stall you have to actually write the register twice. Once with just the endpoint address, then again with the address combined the new DATA0/DATA1 value at the appropriate offsets. Getting a new hang now, but making progress

[mikesmitty]

Alrighty, I fixed the last bug I was running into and pushed the changes up on #5110 since that's where I was doing my testing. I tried porting the change back over to this branch, but it's running into some fatal bug on this branch and I don't have time to sort it out this morning. I'll sort out/split up the changes a bit later, but for now you can try out the changes on that other PR

Edit: Oh, actually there's another new bug, but it enumerates properly now. I'll take a look into it a bit later

[deadprogram]

Alrighty, I fixed the last bug I was running into and pushed the changes up on #5110 since that's where I was doing my testing. I tried porting the change back over to this branch, but it's running into some fatal bug on this branch and I don't have time to sort it out this morning. I'll sort out/split up the changes a bit later, but for now you can try out the changes on that other PR

Edit: Oh, actually there's another new bug, but it enumerates properly now. I'll take a look into it a bit later

Thanks for working on this @mikesmitty ❤️

[mikesmitty]

I'm still working on this btw. I'm beginning to suspect that easyDMA was named in the same way greenland was (iceland being green, greenland being icy). Getting delayed ACKs to work properly with it is anything but easy

[mikesmitty]

Sorry, dumb joke. Anyway, this now finally tests out on my msc.RamDisk test device as well as machine.Flash on an nrf52840. I need to go over it with a fine-tooth comb, but I've spent so many hours on this over the last several weeks my noodle is cooked. There's a few random fixes for bodges I added at the end of adding it for the rp2s, but I haven't yet tested this on the rp2s, just the nrf so far. It should work just fine, but wanted to call it out as untested.

[deadprogram]

Hello @mikesmitty finally got a chance to test this out on my xiao-ble:

First I flashed it:

$ tinygo flash -target=xiao-ble -size short ./src/examples/usb-storage
   code    data     bss |   flash     ram
  14760     584    7800 |   15344    8384

Then I tried the test script you provided previously:

$ sudo ./test_msc.sh /dev/sda
Target Device: /dev/sda
Device Size:   776 KB (794624 bytes)
Limit:         16 MB
-------------------------------------------------------------
WARNING: ALL DATA ON /dev/sda WILL BE OVERWRITTEN!
-------------------------------------------------------------
Type 'YES' to continue: YES
[22:21:41] Running Test: SinglePacket_64B (Size: 64 bytes, Offset: 0 blocks)
[22:21:41]   Writing data...
1+0 records in
1+0 records out
64 bytes copied, 0,155933 s, 0,4 kB/s
[22:21:42]   Reading data back...
1+0 records in
1+0 records out
64 bytes copied, 0,000133493 s, 479 kB/s
[22:21:42]   FAILURE: Checksum mismatch!
[22:21:42]     Expected: 693e5f0f347a5d70acbb7baaab9beb988301b3e9588e32c73d7dcdfb7b2c4604
[22:21:42]     Got:      24016a4ae339cfff64a6840fa7e2999edd8af6fbf6dd321acc17e0590fc1430c
--- Expected Data (First 64 bytes) ---
0000000 aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa aa
*
0000100
--- Got Data (First 64 bytes) ---
0000000 a8 a8 a8 8a 82 a8 02 02 20 28 00 aa 80 00 20 28
0000020 02 aa 00 00 00 a0 08 00 08 02 a8 a2 28 a8 20 28
0000040 00 20 20 20 00 20 a8 a8 a8 8a aa a2 a0 aa 28 a8
0000060 aa 0a 88 a8 00 20 08 02 0a aa 02 2a a2 a0 80 2a
0000100
--- First 10 Mismatches (Byte Offset | Expected | Got) ---
 1 252 250
 2 252 250
 3 252 250
 4 252 212
 5 252 202
 6 252 250
 7 252   2
 8 252   2
 9 252  40
10 252  50
-------------------------------------------------------------
TESTS FAILED.

So for sure progress, but either not working yet or user error on my part? What do you think?

[mikesmitty]

Oh, you most likely need to erase machine.Flash before running the script. Implementing erase before write is probably going to be necessary. I added a little function to my test app once I got the RamDisk variant working. Erasing at startup without a goroutine caused it to timeout iirc:

go func() {
		println("Erasing flash...")
		err := machine.Flash.EraseBlocks(0, machine.Flash.Size()/machine.Flash.EraseBlockSize())
		if err != nil {
			println("EraseBlocks error:", err)
		}
		println("Flash erased.")
	}()

Then with a freshly erased disk:

./test_msc.sh --force /dev/sda
Target Device: /dev/sda
Device Size:   1008 KB (1032192 bytes)
Limit:         16 MB
[20:32:37] Running Test: SinglePacket_64B (Size: 64 bytes, Offset: 0 blocks)
[20:32:37]   Writing data...
1+0 records in
1+0 records out
64 bytes copied, 0.160121 s, 0.4 kB/s
[20:32:37]   Reading data back...
1+0 records in
1+0 records out
64 bytes copied, 6.6284e-05 s, 966 kB/s
[20:32:37]   SUCCESS: Checksums match (693e5f0f347a5d70acbb7baaab9beb988301b3e9588e32c73d7dcdfb7b2c4604)
[20:32:37] Running Test: TwoPackets_128B (Size: 128 bytes, Offset: 0 blocks)
[20:32:37]   Writing data...
1+0 records in
1+0 records out
128 bytes copied, 3.158e-05 s, 4.1 MB/s
[20:32:37]   Reading data back...
1+0 records in
1+0 records out
128 bytes copied, 4.4691e-05 s, 2.9 MB/s
[20:32:37]   SUCCESS: Checksums match (55dbd20dff3ae84c9bc6bcd1546194d272793727ca6c03585a8804178b640342)
[20:32:37] Running Test: ThreePackets_192B (Size: 192 bytes, Offset: 0 blocks)
[20:32:37]   Writing data...
1+0 records in
1+0 records out
192 bytes copied, 0.116428 s, 1.6 kB/s
[20:32:38]   Reading data back...
1+0 records in
1+0 records out
192 bytes copied, 4.0001e-05 s, 4.8 MB/s
[20:32:38]   SUCCESS: Checksums match (1052ae7ed79c25c278caa51190d294ed118568138695679b4eef6ede6a0a75fd)
[20:32:38] Running Test: Sector_512B (Size: 512 bytes, Offset: 0 blocks)
[20:32:38]   Writing data...
1+0 records in
1+0 records out
512 bytes copied, 3.2649e-05 s, 15.7 MB/s
[20:32:38]   Reading data back...
1+0 records in
1+0 records out
512 bytes copied, 5.1078e-05 s, 10.0 MB/s
[20:32:38]   SUCCESS: Checksums match (799edf40e8115dc980109a64ff0a7ae2c6b62e20313c4a01f9871d0e189aa7c2)
[20:32:38] Running Test: Start_50KB (Size: 51200 bytes, Offset: 0 blocks)
[20:32:38]   Writing data...
100+0 records in
100+0 records out
51200 bytes (51 kB, 50 KiB) copied, 1.56468 s, 32.7 kB/s
[20:32:40]   Reading data back...
100+0 records in
100+0 records out
51200 bytes (51 kB, 50 KiB) copied, 0.114009 s, 449 kB/s
[20:32:40]   SUCCESS: Checksums match (b159c9e4d0e655216851d9ae94b6c27d8e54bd77d5c752aa3b6f3dc7be5db85d)
[20:32:40] Running Test: Offset_100KB (Size: 51200 bytes, Offset: 200 blocks)
[20:32:40]   Writing data...
100+0 records in
100+0 records out
51200 bytes (51 kB, 50 KiB) copied, 0.103678 s, 494 kB/s
[20:32:41]   Reading data back...
100+0 records in
100+0 records out
51200 bytes (51 kB, 50 KiB) copied, 0.423051 s, 121 kB/s
[20:32:42]   SUCCESS: Checksums match (b159c9e4d0e655216851d9ae94b6c27d8e54bd77d5c752aa3b6f3dc7be5db85d)
[20:32:42] Running Test: Mid_400KB (Size: 409600 bytes, Offset: 0 blocks)
[20:32:42]   Writing data...
800+0 records in
800+0 records out
409600 bytes (410 kB, 400 KiB) copied, 11.71 s, 35.0 kB/s
[20:32:54]   Reading data back...
800+0 records in
800+0 records out
409600 bytes (410 kB, 400 KiB) copied, 1.10338 s, 371 kB/s
[20:32:55]   SUCCESS: Checksums match (3e91e461d7ee64b2c712aea37ad508808b9da2bac68482ce18131b0e018814da)
-------------------------------------------------------------
ALL TESTS PASSED.

[deadprogram]

Oh, you most likely need to erase machine.Flash before running the script. Implementing erase before write is probably going to be necessary.

@mikesmitty you mean like this PR does? #5017

[mikesmitty]

I did some more poking at this. The issue with the rp2 compatibility was a combination of the usb packet handling order and something with the cores scheduler. Verified it works on an rp2350 with the tasks scheduler on the original setup from a few hundred commits ago and again with the latest dev base

[deadprogram]

Anyone who can help test this out please and thank you!

[deadprogram]

@digitalentity @sago35 anyone else have any comments here?

[digitalentity]

I'm a bit on the fence where this code should live. It's nice to have a generic implementation, but I'm not sure it should belong to the "machine" package.

[digitalentity]

Why this change is needed?