tests/pkg/fatfs*: cleanup MTD default dependencies #21478

gschorcht · 2025-05-09T14:05:04Z

Contribution description

To prevent mtd_sdcard from being used by default for boards that use a different default MTD, which can lead to module conflicts, mtd_sdcard is now only enabled in tests/pkg/fatfs and tests/pkg/fatfs_vfat if no other MTD is enabled by default with the corresponding mtd_*_default module.

With this PR, MTD dependencies for esp32-wrover-kit could be fixed.

Testing procedure

Compilation in CI should succeed.

Use any board definition with SDMMC as SD Card interface und try the following command with and w/o this PR:

BOARD=stm32f746g-disco make -C tests/pkg/fatfs info-modules | grep mtd

Without the PR, the list of MTD modules is:

mtd
mtd_sdcard
mtd_sdmmc
mtd_sdmmc_default

With the PR the command should give:

mtd
mtd_sdmmc
mtd_sdmmc_default

Issues/PRs references

Prerequisite for PR #21471

riot-ci · 2025-05-09T14:21:10Z

Murdock results

✔️ PASSED

0908b86 boards/esp32-wrover-kit: cleanup of MTD dependencies

Success	Failures	Total	Runtime
591	0	591	03m:17s

Artifacts

Documentation preview

mguetschow · 2025-05-09T16:09:31Z

tests/pkg/fatfs/Makefile


 include $(RIOTBASE)/Makefile.include
+
+ifneq (,$(filter native native32 native64,$(BOARD)))


Suggested change

ifneq (,$(filter native native32 native64,$(BOARD)))

ifneq (,$(filter native32 native64,$(BOARD)))

When checked after including Makefile.include, the board alias will already have been replaced by either native32 or native64. Same below

gschorcht · 2025-05-09T16:52:23Z

Hm, it doesn't seem to be as easy as I though. Enabling mtd_sdcard after including Makefile.include doesn't pull in sdcard_spi automatically 🤔 It seems to be a chicken-egg problem. Testing for any enabled mtd_*_default module before including Makefile.include doesn't work because boards make dependencies are then resolved afterwards.

gschorcht · 2025-05-11T09:19:57Z

I had to change the approach to clean up the dependencies. Now, the makefile variable BOARD_WITH_MTD_SDMMC defines the boards that have the SD Card connected to a SD/MMC Controller and for which the MTD driver module mtd_sdmmc should be used for the tests/pkg/fatfs* test applications.

crasbe · 2025-05-12T10:51:12Z

The same functionality is already implemented in drivers/mtd:

RIOT/drivers/mtd/Makefile.dep

Lines 9 to 23 in e0a6628

    
           ifneq (,$(filter mtd_sdcard_default,$(USEMODULE))) 
        
             USEMODULE += mtd_sdcard 
        
           endif 
        
           ifneq (,$(filter mtd_sdcard,$(USEMODULE))) 
        
             USEMODULE += sdcard_spi 
        
           endif 
        
           ifneq (,$(filter mtd_sdmmc_default,$(USEMODULE))) 
        
             USEMODULE += mtd_sdmmc 
        
           endif 
        
           ifneq (,$(filter mtd_sdmmc,$(USEMODULE))) 
        
             USEMODULE += sdmmc 
        
           endif

Just removing the following lines from the tests/pkg/fatfs/Makefile fixed the issue for me:

ifneq (,$(filter native native32 native64,$(BOARD)))
#overwrite default mtd_native-config to use fat image as flash device
CFLAGS += -DMTD_NATIVE_FILENAME=\"./bin/riot_fatfs_disk.img\"
CFLAGS += -DFATFS_IMAGE_FILE_SIZE_MIB=$(FATFS_IMAGE_FILE_SIZE_MIB)
CFLAGS += -DMTD_SECTOR_NUM=\(\(\(FATFS_IMAGE_FILE_SIZE_MIB\)*1024*1024\)/MTD_SECTOR_SIZE\)
- else
- # for actual hardware use mtd_sdcard as storage device
- USEMODULE += mtd_sdcard
endif

This is the console log (also works for devices with mtd_sdcard):

chris@W11nMate:~/RIOTstuff/riot-gettingstarted/RIOT/tests/pkg/fatfs$ BOARD=stm32f746g-disco make info-modules | grep  mtd
fatfs_diskio_mtd
mtd
mtd_sdmmc
mtd_sdmmc_default

chris@W11nMate:~/RIOTstuff/riot-gettingstarted/RIOT/tests/pkg/fatfs$ BOARD=sensebox_samd21 make info-modules | grep mtd
fatfs_diskio_mtd
mtd
mtd_sdcard
mtd_sdcard_default

gschorcht · 2025-05-12T11:13:07Z

Just removing the following lines from the tests/pkg/fatfs/Makefile fixed the issue for me:

🤔 that's interesting but makes the life much more easier.

gschorcht · 2025-05-12T11:20:43Z

Just removing the following lines from the tests/pkg/fatfs/Makefile fixed the issue for me:

Dependencies for MTDs are just bit tricky. Some years ago I had a discussion with @benpicco whether it would make sense to define the used SD Card interface as a feature of the board. At that time, we decided not to define according features to be able to wire any SD Card breakout module for testing anytime.

benpicco · 2025-05-12T11:39:12Z

In hindsight it would make sense to define the feature by the board. Even when wiring the SD card manually, that requires a change to the board configuration, so adding the feature would be no big hassle.

Also back then we didn't have many boards with proper / native SD card interfaces, so manual wiring was the way to go.

We should probably also remove the sam0_sdhc driver, not much point in having two drivers for the same hardware.

gschorcht · 2025-05-20T09:06:16Z

Just removing the following lines from the tests/pkg/fatfs/Makefile fixed the issue for me:

ifneq (,$(filter native native32 native64,$(BOARD)))
#overwrite default mtd_native-config to use fat image as flash device
CFLAGS += -DMTD_NATIVE_FILENAME=\"./bin/riot_fatfs_disk.img\"
CFLAGS += -DFATFS_IMAGE_FILE_SIZE_MIB=$(FATFS_IMAGE_FILE_SIZE_MIB)
CFLAGS += -DMTD_SECTOR_NUM=\(\(\(FATFS_IMAGE_FILE_SIZE_MIB\)*1024*1024\)/MTD_SECTOR_SIZE\)
- else
- # for actual hardware use mtd_sdcard as storage device
- USEMODULE += mtd_sdcard
endif

Not really. It works for boards that have an SDMMC or SPI SD Card interfac and enable mtd_sdmmc_default or mtd_sdcard_default, respectively. But it doesn't work for other boards that should use mtd_sdcard by default even though they don't have it:

BOARD=atmega256rfr2-xpro make -j4 -C tests/pkg/fatfs info-modules | grep -E "mtd|sdcard"
fatfs_diskio_mtd
mtd

Therefore, I dropped the last two fixups.

crasbe · 2025-05-20T09:21:28Z

We could use a tests/pkg/fatfs/Makefile.board.dep to handle the board specific dependencies. This file is (re-)evaluated during the dependency resolution, unlike the tests/pkg/fatfs/Makefile or tests/pkg/fatfs/Makefile.include, which are only evaluated once.

ifeq (,$(filter mtd_sdcard mtd_sdmmc,$(USEMODULE)))
  # if no SD interface is specified, use mtd_sdcard by default
  USEMODULE += mtd_sdcard
endif

This results in the following output:

cbuec@W11nMate:~/RIOTstuff/riot-gettingstarted/RIOT$ BOARD=atmega256rfr2-xpro make -j4 -C tests/pkg/fatfs info-modules | grep -E "mtd|sdcard"
shell fatfs_diskio_mtd mtd
fatfs_diskio_mtd
mtd
mtd_sdcard
sdcard_spi

gschorcht · 2025-05-20T09:42:37Z

But for boards with SD Card connected to SDMMC, we have the problem as before:

BOARD=stm32f746g-disco make -j4 -C tests/pkg/fatfs_vfs info-modules | grep -E "mtd|sdcard"
fatfs_diskio_mtd
mtd
mtd_sdcard
mtd_sdmmc
mtd_sdmmc_default
sdcard_spi

Makefile.board.dep is evaluated at the beginning, even before the Makefile.dep of the board is included, so that mtd_sdmm_default is not yet set in the first pass of dependency resolution and thus mtd_sdcard is set in any case.

crasbe · 2025-05-20T10:40:15Z

Yes indeed. While I generally consider this behavior a bug of #12755, I'm not sure if changing that behavior won't introduce other issues in other applications...

Consider this Makefile.board.dep, which tracks the dependency resolution rounds and only evaluates the USEMODULE after the second round.

Note that Make does not support incrementing a counter variable, therefore I used a string. Not the prettiest thing, but more elegant than a load of shell calls IMO.

# The dependency resolution is called multiple times, but not all the
# dependencies are resolved in the first round, such as the board deps.
# To make sure that the board deps are resolved before running, we
# create a temporary variable that stores the rounds and adds a `+` to
# the string (make does not support integer incrementation).
TEST_FATFS_DEP_ROUND := $(TEST_FATFS_DEP_ROUND)+

ifeq (++,$(TEST_FATFS_DEP_ROUND))
  ifeq (,$(filter mtd_sdcard mtd_sdmmc,$(USEMODULE)))
    # if no SD interface is specified, use mtd_sdcard by default
    USEMODULE += mtd_sdcard
  endif
endif

This leads to this output:

cbuec@W11nMate:~/RIOTstuff/riot-gettingstarted/RIOT$ BOARD=stm32f746g-disco make -j4 -C tests/pkg/fatfs info-modules | grep -E "mtd
|sdcard"
fatfs_diskio_mtd
mtd
mtd_sdmmc
mtd_sdmmc_default
cbuec@W11nMate:~/RIOTstuff/riot-gettingstarted/RIOT$ BOARD=atmega256rfr2-xpro make -j4 -C tests/pkg/fatfs info-modules | grep -E "mtd|sdcard"
fatfs_diskio_mtd
mtd
mtd_sdcard
sdcard_spi

gschorcht · 2025-05-20T11:00:04Z

Consider this Makefile.board.dep, which tracks the dependency resolution rounds and only evaluates the USEMODULE after the second round.

Nice hack 😉

gschorcht · 2025-05-20T11:07:29Z

Murdock seems to be happy 👍

crasbe

I can't really test it locally on real hardware, but if you're happy and CI is happy, I'm happy too.

Please squash.

To prevent `mtd_sdcard` from being used by default for boards that use a different default MTD, which can lead to module conflicts, `mtd_sdcard` is now only enabled if no other MTD is enabled by default with the corresponding `mtd_*_default` module.

gschorcht · 2025-05-21T10:39:07Z

@crasbe Thanks for reviewing and the test application "hack".

gschorcht requested review from MichelRottleuthner, aabadie and leandrolanzieri as code owners May 9, 2025 14:05

github-actions bot added Area: doc Area: Documentation Area: tests Area: tests and testing framework Area: boards Area: Board ports labels May 9, 2025

gschorcht mentioned this pull request May 9, 2025

boards/esp32s3-usb-otg: enable SDMMC peripheral #21471

Merged

mguetschow reviewed May 9, 2025

View reviewed changes

gschorcht mentioned this pull request May 12, 2025

boards/seeedstudio-xiao-esp32c3: fix pin conflict #21463

Merged

gschorcht force-pushed the tests/pkg/fatfs_cleanup_mtd_default branch from 5bd7fb5 to a401f80 Compare May 20, 2025 08:57

crasbe approved these changes May 20, 2025

View reviewed changes

gschorcht added 3 commits May 20, 2025 15:35

boards/esp32-wrover-kit: cleanup of MTD dependencies

0908b86

gschorcht force-pushed the tests/pkg/fatfs_cleanup_mtd_default branch from 494bbc1 to 0908b86 Compare May 20, 2025 13:54

gschorcht added this pull request to the merge queue May 20, 2025

Merged via the queue into RIOT-OS:master with commit 1f0f3f6 May 20, 2025
25 checks passed

gschorcht deleted the tests/pkg/fatfs_cleanup_mtd_default branch May 21, 2025 10:39

Teufelchen1 added this to the Release 2025.07 milestone Jul 14, 2025


		include $(RIOTBASE)/Makefile.include

		ifneq (,$(filter native native32 native64,$(BOARD)))

	ifneq (,$(filter native native32 native64,$(BOARD)))
	ifneq (,$(filter native32 native64,$(BOARD)))

tests/pkg/fatfs*: cleanup MTD default dependencies #21478

tests/pkg/fatfs*: cleanup MTD default dependencies #21478

Uh oh!

Conversation

gschorcht commented May 9, 2025

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

riot-ci commented May 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Murdock results

Artifacts

Uh oh!

mguetschow May 9, 2025

Choose a reason for hiding this comment

Uh oh!

gschorcht commented May 9, 2025

Uh oh!

gschorcht commented May 11, 2025

Uh oh!

crasbe commented May 12, 2025

Uh oh!

gschorcht commented May 12, 2025

Uh oh!

gschorcht commented May 12, 2025

Uh oh!

benpicco commented May 12, 2025

Uh oh!

gschorcht commented May 20, 2025

Uh oh!

crasbe commented May 20, 2025

Uh oh!

gschorcht commented May 20, 2025

Uh oh!

crasbe commented May 20, 2025

Uh oh!

gschorcht commented May 20, 2025

Uh oh!

gschorcht commented May 20, 2025

Uh oh!

crasbe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gschorcht commented May 21, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

riot-ci commented May 9, 2025 •

edited

Loading