usb: add USB mass storage class support

[mikesmitty]

I've only tested it at a very basic level so far, but it's a somewhat faithful port of TinyUSB's mass storage class support and it shows up as a USB disk without errors in linux. There are still a number of things to be cleaned up, a few bits ironed out (clearing stalls doesn't work yet, for example), and I'll soon be implementing a FAT12 emulation package to go with it, but I figured I'd push it up as a draft for any comments

This is a basic program that runs it:

package main

import (
	"machine/usb/msc"
	"time"
)

func main() {
	m := msc.Port()
	for {
		println(m.State())
		time.Sleep(2 * time.Second)
	}
}

Edit: Oh, and tests will be coming as well

[b0ch3nski]

@mikesmitty cool stuff!

Regarding FAT emulation, have you seen @soypat repo? https://github.com/soypat/fat :)

[deadprogram]

I would suggest looking at https://github.com/tinygo-org/tinyfs/tree/release/fatfs since it is already being supported in tinyfs, which would probably be an important user of this feature.

[mikesmitty]

@b0ch3nski @deadprogram Thanks for the info, I should have probably explained a bit more. My goal is to emulate FAT12 with a TinyFS backend. My personal desired use case is to use LittleFS on disk, but present an emulated FAT12 over USB

[mikesmitty]

As I'm thinking about it, adding the FAT12 emulation bits probably make more sense in the TinyFS repo itself. This is the interface I put together for interfacing with the USB SCSI commands. It seems fine to me at first glance, but since it'd be a hard-to-change interface I want to make sure it makes sense:

type Disk interface {
	Ready() bool
	ReadOnly() bool
	BlockCount() uint32
	BlockSize() uint32
	Read(offset uint32, buffer []byte) (uint32, error)
	Write(offset uint32, buffer []byte) (uint32, error)
}

The big potential issue that I can foresee is that because I'm using a uint32 for the offset in Read/Write any emulated disks will be limited to a max of ~4GB in size. I originally was trying to allow it to use io.ReaderAt/io.WriterAt which would require converting the sector + offset used internally into a plain offset, but in hindsight I think that isn't a useful goal given the other required parts of the interface. Since it's already more than just throwing a simple reader/writer at it, abstracting away the concept of sectors and as a result limiting potential max disk size probably isn't very useful

[soypat]

I arrived at the following abstraction for a block device. I'd omit Ready and ReadOnly methods for now. Maybe there's a better way of doing things leveraging abstractions in other Go standard library packages such as the fs package for permissions.

type BlockReader interface {
	ReadBlocks(dst []byte, startBlock int64) (int, error)
}

type BlockWriter interface {
	WriteBlocks(data []byte, startBlock int64) (int, error)
}

// BlockDevice is the basic interface that must be implemented for a device to function properly.
type BlockDevice interface {
	BlockReader
	BlockWriter
	EraseBlocks(startBlock, numBlocks int64) error
	BlockSize() int
	BlockCount() int64
}

[mikesmitty]

Thanks for weighing in @soypat, I could roll with that interface and those suggestions. I think I'll need to implement the UNMAP SCSI command in order for EraseBlocks() to be usable, but given that just about everything this will write to is probably going to be flash storage that's probably for the best anyway. I realized afterwards that it's a pretty likely that this would at some point be used to provide access to an SD card over USB so artificially restricting the interface to low-GB volume sizes instead of TB volume sizes is definitely sub-optimal

[mikesmitty]

So I completely forgot that machine.BlockDevice exists while thinking I needed to create another blockdevice-type interface, and that the erase block size vs write block size is also super important for discard operations. I'm super flexible about whatever shape the interface ends up taking

[soypat]

I guess do with BlockDevice for now since its in the machine package

[deadprogram]

Once #4850 has had the merge conflicts resolved, and also merged, this PR will need the same.

It should, at that point, be a lot easier to review.

Is there some related code that you are using to test out the full file system mount etc @mikesmitty ?

Can I just say now very excited I am about this work, and how much I want/need to use it! 😻

[mikesmitty]

Is there some related code that you are using to test out the full file system mount etc @mikesmitty ?

I'm still working through my port (though it's turning into a bit of a rewrite as it goes on) of the FAT12 emulation layer in https://github.com/oyama/pico-littlefs-usb, but for testing in the mean time I've been using sg_vpd from the sg3-utils tool suite to verify the VPD communication fields:

• Supported VPD pages: sg_vpd -p sv /dev/sda
• Block Device Characteristics: sg_vpd -p bdc /dev/sda
• Block Limits: sg_vpd -p bl /dev/sda
• Logical Block Provsioning: sg_vpd -p lbpv /dev/sda
• Show all VPD page data: sg_vpd -a /dev/sda
  There's also a -H flag to provide a hex output of the returned data if that's useful for CI/CD later

And for testing the UNMAP/discard operations I used the blkdiscard tool, but found that I needed to manually enable UNMAP for the disk through sysfs first:

sudo sh -c 'echo "unmap" >/sys/block/sda/device/scsi_disk/0:0:0:0/provisioning_mode'

Because of the small size of the emulated disk I was only able to get blkdiscard to test single-pass, full-device discard operations so multi-packet UNMAP commands are as of yet untested (the else block on line 62 here: https://github.com/tinygo-org/tinygo/pull/4844/files#diff-28a9c9ce5c40a794d3a2b9f2db9aff7efae6a0f21d4420534cbc8a7243d41dfbR62), but single-packet commands worked fine. If I can't find a way of testing multi-packet UNMAP commands we can also just simplify it down to accept only single-packet UNMAP commands and set the max unmap descriptor block count here to 3: https://github.com/tinygo-org/tinygo/pull/4844/files#diff-7229572bc6cfb2b3f9b13656ea876d7612ec27a3c20e255e8c632bd9436b95aeR31

For read/write commands I've just been using dd to/from the emulated disk, though the emulated disk I'm using doesn't properly verify if writes are occurring, just that it's not throwing errors. Since it's using machine.BlockDevice now I'll have to put together some code to do some read/write/verification to the flash on a pico to make sure the writes are happening as intended

All that said, of course after giving it another read through I noticed a number of comments and little issues that need to be updated/fixed. Once I have a chance to look over the review in #4850 and that gets merged I'll rebase and fix those

[deadprogram]

Now that #4850 has been merged into dev this PR will need to be rebased @mikesmitty if you please.

Getting closer! 😅

[mikesmitty]

Doing some follow-up tests with a real backing flash device I'm seeing some errors. I'll need to do some diagnosing and cleanup before it's ready to be reviewed

[mikesmitty]

After testing it on actual flash hardware I realized that it was trying to perform writes in interrupt context and that needed to be handled differently. Needed to make another change to the RxHandler in order to allow for waiting until a write goes through before ACKing the message, otherwise the USB host will just keep sending data faster than we can write it.

I still need to do some proper testing, but since it's been a hot minute and I finally got it in a stable place I figured I'd push an update

[deadprogram]

I did notice that this PR increased the size of some binaries, such as this failing test: https://github.com/tinygo-org/tinygo/actions/runs/14986938840/job/42102543852#step:17:28

We should either change the size of the expected if we consider this increase acceptable, or else look into what might be able to avoid it, since the example that increased size did not use the features exposed by the new code in this PR.

[mikesmitty]

Ok, got the bugs with using machine.Flash sorted out for linux hosts. Now with a simple msc.Port(machine.Flash) command it will show up as a usb flash drive on linux and I was able to format it/copy files to it/etc. I also fixed one bug preventing macos from recognizing the drive, but there's still at least one more left I'm looking at.

We should either change the size of the expected if we consider this increase acceptable, or else look into what might be able to avoid it, since the example that increased size did not use the features exposed by the new code in this PR.

Sure, I'll look into it once I've got the macos bug fixed

[sago35]

I tried the following source code on Windows 11. Unfortunately, enumeration fails. I'll take a closer look this weekend.

However, I was able to mount it on Android.

package main

import (
	"machine"
	"machine/usb/msc"
	"time"
)

func main() {
	m := msc.Port(machine.Flash)
	for {
		println(m.State())
		time.Sleep(2 * time.Second)
	}
}

[mikesmitty]

I tried the following source code on Windows 11. Unfortunately, enumeration fails. I'll take a closer look this weekend.

Awesome, this will print out the commands it's receiving from the host:

m := msc.Port(machine.Flash)
m.SetMsgLogSize(1024)
for {
	for _, msg := range m.MsgLog() {
		if msg.Valid {
			cmd := msg.CBW.SCSICmd()
			println(cmd.String())
		}
	}
	m.ClearMsgLog()
	println(m.State())
	time.Sleep(2 * time.Second)
}

[sago35]

I also tried the code you provided.

On Windows, enumeration fails, so I can't check the println() output on the PC. In this case, it's likely difficult to confirm even with Wireshark, so I'll use a logic analyzer to inspect the USB bus directly.

Alternatively, I’ll check via the debugger. Lately, I’ve mostly been using the RP2040 Zero, so I’ll prepare a different microcontroller that can connect to a debugger.

[mikesmitty]

Oh, interesting that it doesn't enumerate at all. I've been using -serial=uart for debug output, but the logic analyzer output could be helpful

[mikesmitty]

Ok, finally fixed that bug on macos that was preventing it from doing a successful read, and I bet it will likely fix the windows issue also. It was essentially double-sending a signal message at the end of each read request that linux would ignore, but macos was choking on.

As far as tests go, the only method that has come to mind so far is scripting around dd and sg-utils against real hardware to verify inputs/outputs, but I'm open to suggestions

[deadprogram]

This PR is making amazing progress @mikesmitty

I would suggest adding an example/smoke test to at least ensure that the code builds, and also to give implementers some idea of where to start.

As far as real hardware tests, that would be an interesting thing to add to the TinyHCI.

[mikesmitty]

As far as I can tell this PR seems to be complete enough. @mikesmitty some of the commits probably could be squashed together instead of just squashing them all.

Sure I can do that, or would you prefer that I break out the non-mass storage changes into another PR?

Also, I realized after looking back at it again that in the MSC descriptor only endpoints 1, 2, 3, and 8/9 were being used so I switched to re-using 6/7 instead. I want to say there's still another change that is needed with the samd and nrf chips in order for them to support it, but I forget off the top of my head. I want to say the delayed ack support still needs to be implemented

[deadprogram]

would you prefer that I break out the non-mass storage changes into another PR?

That would make this oh so much easier to review! Please and thank you.

Also, I realized after looking back at it again that in the MSC descriptor only endpoints 1, 2, 3, and 8/9 were being used so I switched to re-using 6/7 instead.

Very good idea, IMO. We need to conserve where we can.

I want to say there's still another change that is needed with the samd and nrf chips in order for them to support it, but I forget off the top of my head. I want to say the delayed ack support still needs to be implemented.

🤔 maybe someone else can spot any missing parts?

[sago35]

My view is that, at a minimum, we should avoid breaking existing features like USBCDC on samd21/51 or nrf52840. So for this PR, I think it’s sufficient to just handle endPoints. It makes sense to position this PR as MSC support for rp2. I don’t think it’s a problem if MSC support isn’t yet available on samd21/51 or nrf52840.

[deadprogram]

My view is that, at a minimum, we should avoid breaking existing features like USBCDC on samd21/51 or nrf52840.

Which existing features are broken in this PR? I did not run into them myself yet.

It makes sense to position this PR as MSC support for rp2. I don’t think it’s a problem if MSC support isn’t yet available on samd21/51 or nrf52840.

I would very much like to get MSC support on all of these platforms. 😃

[sago35]

Which existing features are broken in this PR? I did not run into them myself yet.

Originally, all USB functionality on atsamd21/51 and nrf52840 had stopped working, but it was fixed by the following commit. I’ve confirmed that everything is working correctly as of now, so I believe this PR is ready to be merged.

e9f9a39

I would very much like to get MSC support on all of these platforms. 😃

Of course, I think so too. Once this PR is merged, we can try again.

[sago35]

LGTM

[deadprogram]

@mikesmitty I am very eager to get this PR merged in this week in time for the next release. Will you have time to work on the commit squashing? Thanks!

[mikesmitty]

@deadprogram Sure thing. Sorry for the delay, been a bit hectic recently

[deadprogram]

@mikesmitty I was able to do some testing with this PR and TinyFS. I was able to get a LittleFS file system created on a Pico to mount on my Linux machine using https://github.com/littlefs-project/littlefs-fuse

I probably did not have all of the correct values to mount it properly, but I was able to read small text files located on the Pico's flash. Trying to copy a file from my machine to the mounted volume crashed, but that might have been due to not having the correct block size/etc on the Linux side.

When I tried to do the same thing using TinyFS fatfs, it failed pretty much right away, so perhaps that is still broken on the TinyFS side.

Anyhow, it certainly feels like progress!

[mikesmitty]

Nice! Possibly related (and something I didn't realize, having not dealt with flash extensively at this low a level before), one issue I ran into a lot with testing is not erasing the disk before attempting to reuse sectors. The unmap/discard/trim operations set all bytes in a sector to 0xFF because flash writes can only remove bits from a sector, so writing to the same sector twice without erasing can cause what looks like data corruption. I haven't worked out how to get the unmap/discard operations to be automatically recognized yet. In the mean time I've been manually setting the provisioning_mode for the disk to unmap when needed:

sudo sh -c 'echo "unmap" >/sys/block/sdX/device/scsi_disk/0:0:0:0/provisioning_mode'
sudo blkdiscard /dev/sdX

[aykevl]

The unmap/discard/trim operations set all bytes in a sector to 0xFF because flash writes can only remove bits from a sector, so writing to the same sector twice without erasing can cause what looks like data corruption.

Correct, though note that some chips (such as the STM32L series) erase to 0x00 and write the one bits. All the bits are the inverse compared to the usual flash behavior. So you can't assume that an erase operation always erases to all ones.

[deadprogram]

Did more testing, and things appear to be working as expected, and hardware integration tests still passing.

There is still more work to be done to fully implement the feature of mounting a volume with a full file system, but this PR sets the stage for the further work to come.

Thank you very much @mikesmitty for all your work on this, and to @b0ch3nski @sago35 @soypat @aykevl @mateusznowakdev and anyone else who helped review.

Now merging!!

[aykevl]

What's the status of nrf support? I'm not sure whether it's supported or not from the discussion above.

At least for me, it doesn't seem to work:

$ tinygo flash -target=nicenano
# machine/usb/msc
/home/ayke/src/tinygo/tinygo/src/machine/usb/msc/setup.go:120:18: machine.USBDev.SetStallEPIn undefined (type *machine.USBDevice has no field or method SetStallEPIn)
/home/ayke/src/tinygo/tinygo/src/machine/usb/msc/setup.go:123:18: machine.USBDev.SetStallEPOut undefined (type *machine.USBDevice has no field or method SetStallEPOut)
/home/ayke/src/tinygo/tinygo/src/machine/usb/msc/setup.go:125:18: machine.USBDev.SetStallEPIn undefined (type *machine.USBDevice has no field or method SetStallEPIn)
/home/ayke/src/tinygo/tinygo/src/machine/usb/msc/setup.go:131:18: machine.USBDev.ClearStallEPIn undefined (type *machine.USBDevice has no field or method ClearStallEPIn)
/home/ayke/src/tinygo/tinygo/src/machine/usb/msc/setup.go:134:18: machine.USBDev.ClearStallEPOut undefined (type *machine.USBDevice has no field or method ClearStallEPOut)

[aykevl]

I tested this support but unfortunately it does not work for flash devices. Flash devices need to have their data erased before they can be written. However, this MSD implementation will write data without erasing the pages first.
This is not spelled out specifically in machine.BlockDevice, though it should have been since that interface is mostly used for flash storage. See #5010 for a documentation fix.

(This erase-before-write is a bit complicated though since the USB block size appears to be 256 bytes (?) and many flash devices use 4096-byte erase block sizes. So to rewrite just a part of that, the block would need to be loaded in memory, erased in flash, the relevant part rewritten, and written back to flash - causing write amplification and needing a large chunk of RAM).

EDIT: oh you already found this @mikesmitty

Possibly related (and something I didn't realize, having not dealt with flash extensively at this low a level before), one issue I ran into a lot with testing is not erasing the disk before attempting to reuse sectors. The unmap/discard/trim operations set all bytes in a sector to 0xFF because flash writes can only remove bits from a sector, so writing to the same sector twice without erasing can cause what looks like data corruption.

[mikesmitty]

What's the status of nrf support? I'm not sure whether it's supported or not from the discussion above.

At least for me, it doesn't seem to work:

Oh, I forgot nrf wasn't supported yet. I'll start taking a look into getting support for those

[aykevl]

I did some experimenting, and the following seems to work more or less:

• Change blockSizeUSB to the erase page size (but note that erase page sizes of 128 bytes also exist).
• When writing a block, check whether it starts at the start of an erase page size (blockStart % EraseBlockSize == 0 essentially) and if so erase that flash page before writing. (This assumes the OS will always write whole pages in order - I'm not sure that is actually true, haven't read the MSD specification).
• If using FAT, use a sector size at least the size of an erase page otherwise Linux will refuse to mount the filesystem.

I'm still having issues, but this seems to work at least partially. (Some of my issues might be a wonky USB implementation on Asahi Linux).

EDIT: after a reboot (to fix USB errors) things seem to be working!

[aykevl]

Yes, things seem to work correctly with my hacky patches :D
Test code: https://codeberg.org/maaike328p/fat12/src/branch/main/examples/pico.go
I've now turned my Raspberry Pi Pico into a very small USB stick, with ~20kB of free space (with 4096 byte blocks, filesystem overhead is 12kB and I limited flash size to 32kB). Works on Linux, and a previous version also worked on MacOS (didn't test this version).

[aykevl]

#5017 <- this fixes the erase problem for me, though I'm not 100% sure it is correct and would love to have a review of it.

[aykevl]

@mikesmitty did you test USB MSC support on Windows? I can't seem to get it to work, at least not in a VM (while a USB stick works).