Skip to content

Conversation

@iabdalkader
Copy link
Contributor

@iabdalkader iabdalkader commented Jan 15, 2025

Summary

Add MSC support using internal flash storage or SD card. This has been sitting for a while in my fork so I figured I should send it upstream. Note this is disabled by default, and can be enabled by boards if needed.

Testing

Tested with:

  • MIXRT1020_EVK, Teensy 4.1 and MIMXRT1176_EVK
  • RT106x based board.

@github-actions
Copy link

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:    +0 +0.000% standard
      stm32:    +0 +0.000% PYBV10
     mimxrt:    +0 +0.000% TEENSY40
        rp2:    +0 +0.000% RPI_PICO_W
       samd:    +0 +0.000% ADAFRUIT_ITSYBITSY_M4_EXPRESS
  qemu rv32:    +0 +0.000% VIRT_RV32

@robert-hh
Copy link
Contributor

I could test that one. Besides that, I keep an branch for MIMXRT MSC support alive since quite a while. https://github.com/robert-hh/micropython/tree/mimxrt_msc
I had made a PR fro that a while ago, but closed it when USB device support was added to MicroPython in favor of a more general MSC support using Python code, which would be potable across several ports. But that is still to be done.

@robert-hh
Copy link
Contributor

Tried it. At the first run, it did not even compile with a Teensy 4.1 as target after setting MICROPY_HW_USB_MSC, complaining about the symbol MICROPY_HW_SDCARD_SDMMC not being set. Maybe you have that set in your environment. Having MICROPY_HW_SDCARD_SDMMC set, the board creates a new device block device at my PC. That cannot be mounted by littlefs-fuse. The Teensy's file system is littlefs, but a block device should be transparent. Is your PR by some means restricted to FAT?

@iabdalkader
Copy link
Contributor Author

complaining about the symbol MICROPY_HW_SDCARD_SDMMC not being set. Maybe you have that set in your environment

Yes, this just lets the msc disk know which SD is used for the block device if you have more than one, and since it's board-specific you need to define MICROPY_HW_SDCARD_SDMMC if you enable MSC.

The Teensy's file system is littlefs, but a block device should be transparent. Is your PR by some means restricted to FAT?

You probably need to match the flash sector size, maybe try progsize=4096?

@robert-hh
Copy link
Contributor

Not mounting LFS It was a problem with my PC. After reboot it worked. MICROPY_HW_SDCARD_SDMMC has to be set to 1. Setting it to 2 causes other compile error.

Maybe you get yourself MIMXRT boards and test the submission before posting a PR.

@iabdalkader
Copy link
Contributor Author

MICROPY_HW_SDCARD_SDMMC has to be set to 1. Setting it to 2 causes other compile error.

Is this unexpected? If you only have one SD card, you can't set the ID to 2.

Maybe you get yourself MIMXRT boards and test the submission before posting a PR.

As I've mentioned I have been using this code in production for year, so it's been tested, just never with an upstream board.

@robert-hh
Copy link
Contributor

I did a few more board tests after changing mpconfigport.h to:

// Enable USB Mass Storage with FatFS filesystem.
#ifndef MICROPY_HW_USB_MSC
#define MICROPY_HW_USB_MSC                  (1)
#if MICROPY_PY_MACHINE_SDCARD && !defined(MICROPY_HW_SDCARD_SDMMC)
#define MICROPY_HW_SDCARD_SDMMC             (1)
#endif
#endif

MIXRT1020_EVK, Teensy 4.1 and MIMXRT1176_EVK worked fine. It is a good idea to expose the inserted SD card.

MIMXRT1010 and MIMXRT1015 fail for two reasons:

  1. Overflow of the m_dtcm segment. by 2800 resp 1700 bytes. It may be possible to reorganize the loader file for that,
  2. Access to machine_sdcard_type in main.py. These devices do not support SD cards. I've changed that section to:
        #if MICROPY_HW_USB_MSC
        // Set the USB medium to flash block device.
        mimxrt_msc_medium = &mimxrt_flash_type;

        #if MICROPY_PY_MACHINE_SDCARD
        const char *path = "/sdcard";
        // If SD is mounted, set the USB medium to SD.
        if (mp_vfs_lookup_path(path, &path) != MP_VFS_NONE) {
            mimxrt_msc_medium = &machine_sdcard_type;
        }
        #endif
        #endif

@iabdalkader
Copy link
Contributor Author

@robert-hh I've incorporated both changes. As for the memory overflow, this should be fixed for individual boards if/when they enable this feature.

@robert-hh
Copy link
Contributor

robert-hh commented Jan 17, 2025

I see that you disable MSC by default.
The memory overflow at least for the 1011 is caused by a buffer with a need 2k alignment, which is placed by the loader just after a 2k boundary. And then there is a ~2k gap. On option would be to reduce the stack size, which is 20k at the moment. The other would be to re-arrange the objects in .bss manually.I know that I did that once, but I forgot how.

@iabdalkader
Copy link
Contributor Author

I see that you disable MSC by default.

Yes. If you like you can enable it by default or for certain boards if/when this PR gets merged.

As for the memory, I'd lower the stack, 16KBs is good probably good enough.

@robert-hh
Copy link
Contributor

I did both. Rearranging the BSS to reduce gaps and decreasing the stack size.

@iabdalkader iabdalkader force-pushed the mimxrt_msc_support branch 2 times, most recently from 8e640f9 to e2e0a50 Compare January 27, 2025 06:19
@dpgeorge
Copy link
Member

dpgeorge commented Feb 7, 2025

@robert-hh are you happy with this PR now?

@robert-hh
Copy link
Contributor

Actually the PR still does not build with MIMXRT1011 and MIMXRT1015 devices with MSC enabled. The stack has to be decreased, like from 20k to 16k. Same for MIMXRT1015
@iabdalkader will you do that in mimxrt1011.ld and mimxrt1015.ld? It may be useful when adding a new feature to at least build the firmware for all applicable boards with that feature enabled.

@iabdalkader
Copy link
Contributor Author

will you do that in mimxrt1011.ld and mimxrt1015.ld? It may be useful when adding a new feature to at least build the firmware for all applicable boards with that feature enabled.

Are you saying we should enable MSC for these boards by default?

@iabdalkader iabdalkader force-pushed the mimxrt_msc_support branch 2 times, most recently from 5cefb9c to cf8ed98 Compare February 7, 2025 12:49
@iabdalkader
Copy link
Contributor Author

@robert-hh Enabled for MIMXRT1011, MIMXRT1015 and MIMXRT1060. I've added 1060 to CI to have at least one board with MSC enabled.

Note I moved MICROPY_HW_SDCARD_SDMMC to msc_disk.c. MICROPY_PY_MACHINE_SDCARD is defined later in the mpconfigport.h so it failed to build if a board disables SD card.

@codecov
Copy link

codecov bot commented Feb 7, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.59%. Comparing base (0a433a0) to head (752c167).
Report is 3 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #16591   +/-   ##
=======================================
  Coverage   98.59%   98.59%           
=======================================
  Files         167      167           
  Lines       21599    21599           
=======================================
  Hits        21295    21295           
  Misses        304      304           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@robert-hh
Copy link
Contributor

Are you saying we should enable MSC for these boards by default?

Actually no. But if it's disabled, there should be no further hiccup. I enabled MSC by default and ran a build-all script with the 1011/1015 stack changed. All boards build.

The Teensy4.1 board has a MIMXRT1062 MCU, and it is already built in the CI. So you could use that one. But if you want to add a board, the MIMXRT1010 would be a good choice, because that one is always a candidate for trouble.

@dpgeorge
Copy link
Member

dpgeorge commented Feb 7, 2025

I don't want to enable MSC by default. It's just that it should build without error if a user enables it themselves.

@iabdalkader
Copy link
Contributor Author

@dpgeorge You can just drop commit number 3 which enables it for some boards, and commit number 4 which adds 1060 to CI. However, I'd leave 1060 in CI as it tests more things.

@iabdalkader
Copy link
Contributor Author

iabdalkader commented Feb 7, 2025

I don't want to enable MSC by default. It's just that it should build without error if a user enables it themselves.

@dpgeorge @robert-hh On second thought, if we don't add at least 1 board with MSC enabled, we'll never notice if this ever gets broken by any change. I only care about the core functionality, but this is worth mentioning.

@dpgeorge
Copy link
Member

dpgeorge commented Feb 7, 2025

On second thought, if we don't add at least 1 board with MSC enabled, we'll never notice if this ever gets broken by any change

You can build with MSC enabled just under CI using something like make BOARD=... CFLAGS_EXRTA=-DMICROPY_HW_USB_MSC=1.

@iabdalkader
Copy link
Contributor Author

Done. Seems to build fine.

Add MSC support using internal flash storage or SD card.

Note this is disabled by default, and can be enabled by boards if needed.

Signed-off-by: iabdalkader <i.abdalkader@gmail.com>
Reduced to 16KBs to allow enabling MSC.

Signed-off-by: iabdalkader <i.abdalkader@gmail.com>
Signed-off-by: iabdalkader <i.abdalkader@gmail.com>
Copy link
Member

@dpgeorge dpgeorge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested with TEENSY40 and enabling MSC. It works.

@dpgeorge dpgeorge merged commit 752c167 into micropython:master Feb 10, 2025
65 checks passed
@iabdalkader iabdalkader deleted the mimxrt_msc_support branch February 10, 2025 06:16
@robert-hh
Copy link
Contributor

As a note: This feature works also with the lfs file system at the board and littlefs-fuse at the PC.

@mattytrentini
Copy link
Contributor

As a note: This feature works also with the lfs file system at the board and littlefs-fuse at the PC.

Oh nice, I've been meaning to try this - that's great to know!

@iabdalkader
Copy link
Contributor Author

Unrelated question, but does anyone know what the minimum LFSV2 volume size is? Or is there no such limitation ? For example, for FatFS it's 512 * 128 = 64K.

@robert-hh
Copy link
Contributor

Lfs uses at least 5 blocks (2+2+file data), with the minimal block size being 104 bytes. So yes, there is a minimal size, but that is rather small.

@iabdalkader
Copy link
Contributor Author

Amazing! Thanks @robert-hh

@dpgeorge
Copy link
Member

With two blocks of 96 bytes each, it's possible to create a LFS2 filesystem that has one small file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants