Skip to content
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

Fix SDFat conflict with Arduino-Pico BSP #31

Merged
merged 6 commits into from
Mar 4, 2025

Conversation

brentru
Copy link
Member

@brentru brentru commented Feb 28, 2025

@ladyada This PR resolves earlephilhower/arduino-pico#2828 (comment)

by implementing the following fix proposed by earlephilhower:

Can Adafruit_SdFat just bump its own version number up to, say, 100+real version number (i.e. 102.2.3) so that when installed it will override any unmodified official versions? It'll bust the core here using the normal SD library, but if the user installs add'l libraries through the manager then they can uninstall them as needed.

I will create a new release of Adafruit/SDFat after this PR is merged.

@brentru
Copy link
Member Author

brentru commented Feb 28, 2025

@ladyada Requesting a re-review. This adds a file and bumps the semver. It should not break anything that is not already failing. We'd want to eventually fix the lib. CI by updating the examples.

This looks good and builds for WipperSnapper after I changed the inclusion to reflect the fork header (build log)

@hathach I saw that you had the same issue with TinyUSB_Arduino and disabled the RP2040 build target too. You may want to implement the same thing we did in WipperSnapper ☝️

@hathach
Copy link
Member

hathach commented Mar 1, 2025

@ladyada @brentru yeah, indeed, arduino-pico now include official SdFat in the core/libraries as builtin, which has higher priority in library name matching. The way arduino ide/cli find matched library with the same header include i.e SdFat.h when having a few candidates is using name matching: SdFat is shorter and better match than our Sdfat - Adafruit's fork.

I will re-investigate find more solution, but we have a few approaches that I have in mind now:

  • the best way is submit PR to get official support FAT12, which is rejected previously sadly support filesystem with only 1 fat greiman/SdFat-beta#80
  • we may need to add a different library header e.g Adafruit_SdFat.h instead of current SdFat which cause the conflict
  • tell user to remove builtin official SdFat which isn't ideal at all.

Copy link
Member

@hathach hathach left a comment

Choose a reason for hiding this comment

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

oops, I commented in hurry and didn't realize you already create a new header. That I think is the great solution, it will probably take it some time to mirgate but that ensure it will work in the future.
Once thing to consider is to just name the new header as Adafruit_Sdfat.h with the includes field, the arduino-cli should be able to detect and know where to find the library. But it is up to you and @ladyada to decide :)

@brentru brentru requested a review from hathach March 3, 2025 14:46
@ladyada
Copy link
Member

ladyada commented Mar 3, 2025

yeah i think its better to keep this name, so its clear - in case some day in the future we have our own adafruit_sdfat library or something

Copy link
Member

@hathach hathach left a comment

Choose a reason for hiding this comment

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

I think this is good now, there is a sligth concern above. For CI, I think we can skip the failed test with rp2040, since that need TinyUSB to be update, and TinyUSB need this to be released first (mutual recusrive :) )

@hathach hathach merged commit 1d3b2a2 into adafruit:master Mar 4, 2025
5 of 6 checks passed
@hathach
Copy link
Member

hathach commented Mar 5, 2025

@brentru just merged and released, sorry for a bit hurry since this block other ci such as brain and tinyusb to run tests properly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SdFat Upstream Conflicts with Adafruit SDFat Fork
3 participants