8000 rp2: Add MSC support. by iabdalkader · Pull Request #7402 · micropython/micropython · GitHub
[go: up one dir, main page]

Skip to content

rp2: Add MSC support. #7402

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

Conversation

iabdalkader
Copy link
Contributor
@iabdalkader iabdalkader commented Jun 15, 2021

Any interest in adding MSC support for RP2 ?

@iabdalkader iabdalkader force-pushed the rp2_msc branch 2 times, most recently from 6c1c727 to f68d994 Compare June 15, 2021 23:03
@kadamski
Copy link
Contributor

@iabdalkader how does this work? The same filesystem is shared between both micropython internally and the host filesystem, right? Usually operating systems do not expect the data to be changed on the medium by anybody else than themselves, so they take quite a lot of liberty with caching and delayed writes. Doesn't this corrupt data?

@robert-hh
Copy link
Contributor

Yes. That is a common problem since the first days of the STM32 port, meaning that people who do not like that trouble simply avoid like the plague the MSC mode.
The variant Cicruitpyhton uses is blocking internal writes as long as MSC is enabled, which is also no fun, and still leaves the risk of FS damage if the board disappears.

@iabdalkader
Copy link
Contributor Author

There is still a problem that if strncpy does not make sure the resulting string is null terminated, do we need that here?

No I don't think so, this is based on the tinyusb ramdisk msc example, they use "TinyUSB" for vid, which is 7 characters so seems there's no requirement to null terminate in this function at least.

@kadamski
Copy link
Contributor

No I don't think so, this is based on the tinyusb ramdisk msc example, they use "TinyUSB" for vid, which is 7 characters so seems there's no requirement to null terminate in this function at least.

Well, it is 7 chars so there it will actually be null terminated. But I have checked the msc_device.c in tinyusb code and they say:
// vendor_id, product_id, product_rev is space padded string
Space padded, not null terminated.

@iabdalkader
Copy link
Contributor Author
iabdalkader commented Jun 23, 2021

How about this ? will always leave 1 space in the buffers:

    strncpy((char *)vendor_id,   vid, 8-1);
    strncpy((char *)product_id,  pid, 16-1);
    strncpy((char *)product_rev, rev, 4-1);

@kadamski
Copy link
Contributor

How about this ? will always leave 1 space in the buffers:

    strncpy((char *)vendor_id,   vid, 8-1);
    strncpy((char *)product_id,  pid, 16-1);
    strncpy((char *)product_rev, rev, 4-1);

That should be with sizeof(). After checking the code of tinyusb I think that it is safe to fill the buffer without null termination, though.

@iabdalkader
Copy link
Contributor Author
iabdalkader commented Jun 23, 2021

That should be with sizeof(). After checking the code of tinyusb I think that it is safe to fill the buffer without null termination, though.

Can't use sizeof (gcc 10):

error: 'sizeof' on array function parameter 'vendor_id' will return size of 'uint8_t *' {aka 'unsigned char *'} [-Werror=sizeof-array-argument]
   46 |     strncpy((char *)vendor_id,   vid, sizeof(vendor_id)   -1);

I agree the comment clearly says to fill the buffers with a string up to 8, 16, 4 characters respectively, so will keep 8, 16, 4.

@kadamski
Copy link
Contributor

Can't use sizeof (gcc 10):

Ach, damn it, I forgot about this stupid rule in C. I know that the array is acctually passed to the function as a pointer but if we are allowed to specify the argument to be an array of fixed size, not a pointer, we should have a way to extract the size we have declared (so that we don't have to repeat ourselves).. but there's no way to do this in C. Bummer.

@iabdalkader iabdalkader force-pushed the rp2_msc branch 2 times, most recently from 6eed135 to 519245d Compare June 23, 2021 13:56 8000
@peterhinch
Copy link
Contributor

MSC mode is a support PITA in the forum. We are forever having to explain to users why they have trashed their filesystem, I'm very much -1 for this.

@iabdalkader
Copy link
Contributor Author

MSC mode is a support PITA in the forum. We are forever having to explain to users why they have trashed their filesystem, I'm very much -1 for this.

Nevertheless it's still valuable to have standard access to the filesystem, at least the option to enable it if it's needed without having to use some Micropython fork (note this is not enabled by default and it's configured per-board not for the whole rp2 port). That said maybe we can avoid some of these forums questions by disabling the internal filesystem access as long as the storage is mounted/used by the host (this may need to be done at higher levels) this way it can only be corrupted if the board is not safely removed (same as using a USB stick).

@kadamski
Copy link
Contributor

Fortunately I have no vote here because it is a hard choice. On one hand, having this feature disabled should keep surprised people out of the forums, as only somewhat more experienced users would build their own version with this option (but not having this problem documented may mean they will still be surprised), on the other hand not having this feature enabled by default may mean it will be harder to maintain in good shape.
On one hand I think it would be better to (somehow) disable internal access while the MSC is mounted, on the other hand I don't like when ports diverge in behaviour.

@iabdalkader
Copy link
Contributor Author
iabdalkader commented Jun 23, 2021

On one hand I think it would be better to (somehow) disable internal access while the MSC is mounted, on the other hand I don't like when ports diverge in behaviour.

This should be common behavior to all ports that support USB MSC in the same way (ex: the stm32 port MSC support is more or less the same, and has the same issues), so it needs to be supported somewhere in upper layers, but not sure where exactly and it's unlikely to be fixed in this PR.

@peterhinch
Copy link
Contributor

On one hand I think it would be better to (somehow) disable internal access while the MSC is mounted, on the other hand I don't like when ports diverge in behaviour.

Rather than disabling, it would be best to provide read-only access. This would fix the problem, but the r/o restriction would need to apply only to that filesystem. The user might have an SD card or an EEPROM chip attached and would expect full access.

As far as I know STM is the only port to allow MSC mode so my guess is that @dpgeorge thought better of it. It would be interesting to hear his view.

@iabdalkader
Copy link
Contributor Author

Unrelated to the MSC discussion, was wondering why does this port enable both FAT and LFS2 at the same time ? Are both filesystems needed at the same time ? If not I can merge the frozen modules that create the filesystems in one module.

@robert-hh
Copy link
Contributor

The default file system of the RP2 port is LFS2. For people who attach a SD card to RP2 or who prefer FAT, FAT was added as well. So you may have FAT and LFS2 being used at the same time, but not at the same media. No need to change that. Does using MSC require the file system of the RP2 to be FAT?

@iabdalkader
Copy link
Contributor Author

Does using MSC require the file system of the RP2 to be FAT?

No not really but it's the only filesystem that will work out of the box on all OSes.

@robert-hh
Copy link
Contributor

Will MSC work with Windows, OS X or Linux, if the RP2 file system is LFS2?

@kadamski
Copy link
Contributor

Depends on what you mean by "work". MSC will, in general, work, you can access the block device but you can't access the files unless you mount the filesystem.

There is a littlefs-fuse,a userspace driver for LFS2 filesystem, that could be used to mount the filesystem on Linux. I'm not sure if anything similar is available on Windows or Mac OS X. Technically there is no reason why it wouldn't be possible but stock installation for sure isn't going to work.

@robert-hh
Copy link
Contributor

With "work" I mean indeed: use files on the file system. A little search revealed already the littlefs-fuse driver. But without file access, MSC support is of little use. That brings it back to FAT on the RP2, with all the PITA, as Peter said. I used that in the beginning with PyBoard. And even if I tried to be very careful, I wrecked the Pyboards filesystem regularly. Once I stopped using MSC, everything is stable. Whether Littlfs behaves more stable is to be determined. Using mpremote and it's reverse mount is definitely robust.

@kadamski
Copy link
Contributor

Some of the problems could be mitigated slightly if we had two separate filesystems - the internal one and external one. Internal could be LFS and not accessible via MSC, the other one could be FAT, that would be read-only on the microcontroller (could be made writable, which would disable MSC, or it could become writable when MSC is not in use).

In this case, the exported FS could still be broken but at least that wouldn't brake the internal one. As long as the MSC is properly ejected before removal, the external FS should be able to survive for some time.

@peterhinch
Copy link
Contributor

That brings it back to FAT on the RP2, with all the PITA, as Peter said.

My understanding of the problem is that it's unrelated to the filesystem. It is a consequence of the USB MSC spec. The PC assumes that the MSC device is a dumb disk drive. If instead the device modifies the filesystem chaos results.

I really can't see any good reason for providing this. Most ports don't have MSC mode. In the one which does (STM) it is an endless source of grief until people learn how to disable it. I would question whether any experienced users actually employ it. Disabling write access if MSC is enabled would fix the problem, but then we'll have to field queries about why perfectly valid Python code throws exceptions...

@kadamski
Copy link
Contributor

It is a consequence of the USB MSC spec. The PC assumes that the MSC device is a dumb disk drive.

Not really. USB MSC has nothing to do with filesystems, as far as I understand. It just exposes the block device. The problem is with the filesystems in the Operating Systems. In order to achieve good performance, they cheat - they don't flush everything to the block device as soon as possible and they use cache instead of reading everything from the device itself. Basically, OS assumes it is the only one who modifies the content of a filesystem.

@robert-hh
Copy link
Contributor

Look like we all say the same: trouble is ahead. The operating systems behavior cannot be reliably controlled. Even splitting the board's file system in two partitions is not really an advantage. It will keep the board's own file system intact, but the one accessed by the PC will be corrupted regularly and therefore not useful. A better alternative would be to implement MTP or the like, which are robust with respect to sudden disconnects and a file system agnostic.

@chrismas9
Copy link
Contributor

MSC is a lifesaver for field maintenance. The way I use it is very easy and reliable. I design my systems so the MCU is powered from USB even when the system is off. Just turn the system off, plug in a USB cable and copy files to PYBOARD. I rely on whoever is available at remote sites, usually an electrician or technician, to do this. It's not their main job and I don't expect them to install drivers. In fact many corporate computers are so locked down they couldn't even if they wanted to.

If you want to make it really reliable you can sense when USB is connected and disable disk writes, and disable outputs when system power is down.

@iabdalkader
Copy link
Contributor Author
iabdalkader commented Aug 20, 2021

@iabdalkader iabdalkader force-pushed the rp2_msc branch 2 times, most recently from c7a2de5 to 7ae1017 Compare November 22, 2021 15:23
@iabdalkader
Copy link
Contributor Author

Hello @dpgeorge I pushed another commit: 7ae1017 which gives exclusive access to the filesystem to the host (if the board is connected to a host) or access to the device if it's not (if for example powered from battery). If the board is connected to a host, the storage must be ejected to access the filesystem from the device. This should make MSC as safe as possible to use, perhaps it will help with making a decision about this PR.

@dpgeorge
Copy link
Member

I'm still undecided about this. There are good arguments on both sides, for and against having MSC support. It does provide a very simple way to get started and develop code, but for anything beyond that it has too many limitations. And having USB MSC available puts a big dependence on the FAT filesystem (which is not great for embedded) and constraints on how block devices integrate with the rest of the system.

I would much rather see mpremote become the standard way to develop with a device. The downside of that is it requires installing something. MSC just works out of the box, and this simplicity of MSC was the reason to include it in the pyboard from the beginning.

@peterhinch
Copy link
Contributor

I would much rather see mpremote become the standard way to develop with a device.

YES! This needs to be promoted to new users (I'm not sure quite how).

@dpgeorge
Copy link
Member

Here are my current thoughts on the situation.

What it boils down to is not whether MSC is useful (it is) or whether ports with native USB should support it (IMO they should). But rather whether MSC is enabled by default and and encouraged for use by new users, and I don't think it should be.

Serial IO (stdio, input and output) is a far more fundamental feature than MSC. Serial IO really is the minimum common feature of all ports. Serial can be over UART, USB, BLE, telnet (WiFi, ETH), I2C, CAN, etc. So it seems like a good choice to make serial the main way to control and interact with a board for development purposes, to build the workflow around serial.

MSC as a workflow does have the advantage that it is zero install, that it just works on a PC without installing anything. And that is a big selling point, and the original selling point for developing with MicroPython and the pyboard.

But these days serial has a much lower barrier to entry, because on Windows 10 the USB CDC driver is there by default, and Chrome has WebSerial/WebUSB (Linux and Mac have USB CDC as well).

We've also seen that serial-only access has been just fine for esp8266 and esp32 (prior to S2, S3 variants). The micro:bit also doesn't have MSC, only serial (apart from BLE), and it is used in primary schools for teaching beginner programming. Mu (which has serial filesystem access) and the micro:bit online editor have shown that a lot can be done with just serial access, and that it can be made very accessible with a low barrier to entry. Similarly Espruino is based around serial access with a Chrome app (AFAIK) and is very beginner friendly.

From this, it makes sense to me to develop more tools around serial access, to improve mpremote, make a web-based mpremote, and maybe even a native mpremote GUI version (also lots of docs!). This is all with the aim to lower the barrier to using the serial port. A web-based mpremote could offer a zero-install workflow, and one that allows a pathway for the user to more powerful tools based on serial access.

MSC only works for deployment of .py and .mpy files to a FAT filesystem. Serial access allows much more flexibility, eg a range of filesystem types, deployment of apps by downloading an entire filesystem image (like a pre-built littlefs image, or a linear filesystem with memory mappable .mpy files) and many other things.

MSC is still a useful feature, and can definitely exist as another building block for the user, so it can be used when it makes sense, eg for easy configuration/editting of files in the field. But I don't think it should be promoted as the default way to develop (although it would still be possible to do that if so desired).

Summary: serial is more fundamental than USB MSC. MSC can be supported by a port, but as a feature, not a fundamental part of the workflow. Want to improve, extend and promote mpremote more, eg a web-based version.


Given the above, to make progress with this PR I suggest to keep the bits that add MSC support so it can be used if needed, and eventually make it possible to dynamically select MSC or not (so users don't need to rebuild the firmware to enable it).

@iabdalkader
Copy link
Contributor Author

I suggest to keep the bits that add MSC support so it can be used if needed

Would it be possible to enable it per board as it is right now ?

Should the exclusive access feature be kept ? If so, it would be much cleaner to add a mp_vfs_umount_protected function.

@dpgeorge
Copy link
Member

Would it be possible to enable it per board as it is right now ?

Ideally it would be dynamically selectable, eg like stm32's pyb.usb_mode(...) (but this needs to be re-thought to be more port agnostic), and by default would boot into CDC-only mode. But in the meantime I guess it's OK to enable it per board, eg on the Nano connect....

Should the exclusive access feature be kept ?

I don't think so. It adds more complexity and makes it harder to document what the behaviour is. If MSC is going to be an advanced feature, then exclusive access is not needed.

@iabdalkader
Copy link
Contributor Author

Is there something blocking this ?

@dpgeorge
Copy link
Member
dpgeorge commented Mar 8, 2022

The first commit here (that adds MSC support) was merged in 507ad03.

I didn't merge the exclusive access commit. As above, I think it adds too much complexity.

Next step would be to add a way to select the USB configuration at runtime (so the user can enable USB MSC without rebuilding the firmware).

@dpgeorge dpgeorge closed this Mar 8, 2022
@MichaelDu9226
Copy link

@iabdalkader MSC is a great feature for me, thanks for your great work.
I found a little bug in file sync when using MSC

  1. copy a new file to the u disk of PICO
  2. send os.listdir() form repl, but can't find the new file.
  3. send os.mkdir("xxx"), then I can see the new file and new dir xxx
  4. But when I copy files on openmv, listdir can find them immediately
  5. Any suggestions, thanks

@robert-hh
Copy link
Contributor

That's a good example why MSC support is first of all a source of trouble. The PC assumes it is the sole user of the file system. So it will update the file system on it's own behalf. If you modify the board's files and forget to synchronize (eject properly), then you end up with a corrupted files system. Also, never make change to the file system on the board itself while it is mounted at the PC. The PC will not notice the changes, and when doing own changes -> corrupted file system. The only way to avoid the latter is to make the file system exclusive. That's what adafruit is doing. But that is inconvenient.
Not to mention that with MSC the FAT file system has to be used on the board, lacking wear leveling being not fault tolerant.
So my personal suggestion based on a frustrating experience with PyBoard for 5: Do not use MSC.

@MichaelDu9226
Copy link

@robert-hh Thanks for your suggestion.
MSC may have many weaknesses. But it also has a lot of convenience for me. I will refer to the circuit Python and change the MSC to exclusive mode, writeable just for PC.
The problem I mentioned should only be the problem of file list synchronization because just a simple uos.mkdir can refresh the new file created by PC. I believe there should be a way to solve it. For example, after MSC write10 completes, call micropython file updates and so on.
I noticed that openmv already supports this feature, so I'm here for some advice.
Thanks.

@iabdalkader
Copy link
Contributor Author

@MichaelDu9226 Hi, this is a known limitation with MSC that you need to be aware of, you can't use the FS from the device/host at the same time, and you shouldn't need to just reset the board to sync the changes. Note I did not add exclusive access to OpenMV firmware for any port.

@MichaelDu9226
Copy link

Yes. I use a reset to sync the changes on PICO.
But I don't need reset to sync when I created new files on openMV. So I want to ask the difference between them.

  • copy a new file to the u disk of PICO
  • send os.listdir() form repl, but can't find the new file.

Another bug, I found that the MSC needs to be formatted every time Pico is powered off and restarted. I'm not sure if it's my compile problem.

@iabdalkader
Copy link
Contributor Author

But I don't need reset to sync when I created new files on openMV. So I want to ask the difference between them.

OpenMV boards use the stm32 port, there might be some minor differences with caching etc.. but this doesn't fix the sync issue as it also depends on the host flushing the changes.

Another bug, I found that the MSC needs to be formatted every time Pico is powered off and restarted

I've been using it on Nano RP2040 it's working very well, check if you have a boot.py or something that changes the FS to LFS.

@MichaelDu9226
Copy link

Hi, @iabdalkader Thanks for your reply.

  1. I will use reset to sync files.
  2. After make clean & make RP2040 firmware, There is no problem with formatted after powered off and restarted.
    Thanks.

@t35tB0t
Copy link
t35tB0t commented May 28, 2022

@dpgeorge - Your summary above has a lot of insights packed into it. Yet, I think several important factors haven't been highlighted sufficiently:

  1. Physical serial UART ports are archaic. Very few PC platforms actually have these anymore. PC hardware is USB, Ethernet, or WiFi. Supporting MSC on any target that has the native USB hardware is logical. My own experience is that MSC actually is an indispensable fundamental workflow tool for complex projects - one that presently has no comparable alternative. I have projects that would be too painful to work on using these slow and awkward REPL tools.

  2. A web-based mpremote will still have the serious issue in that this class of tools are using the REPL and injecting things that muck with the target. And this requires that running application code to be halted. Side-note: I've added SIG-QUIT interrupt and am able to halt/resume asyncio tasks. The REPL is a debug console that I am using - I don't want my file editors mucking with my console in the middle of complex asyncio execution. Having many hours of success with this workflow, I must say that the FUD over MSC on these forums is disappointing.

  3. What is mpremote doing that a good file sync over FTP can't do using e.g FileZilla? Instead of simply tunneling REPL tools over the network - perhaps enable the greater workflow power of the file sharing paradigm and consider perhaps using the network with e.g NFS, SAMBA or even just a good built-in FTP server (not using REPL).

  4. Meanwhile, for the sake of mpremote, please make it easy for folks to run the REPL at e.g. 1MBAUD. Out of the box, MP/REPL BAUD is so slow I'm having KERMIT flashbacks. Reading the hardware notes on ESP32, I will note that ESP32 and other targets may have issues with clock dividers ratios at high BAUD rates. If this is the case (and not some sort of PLL issues), then we should be able to find the right USB/FTDI BAUD to match Target/UART BAUD. We just need one high BAUD setting to run more like 10x faster than the 115.2K we're at today.

tannewt pushed a commit to tannewt/circuitpython that referenced this pull request Jan 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants
0