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

First attempt to adding hardware support for NRF52 SPI SD Card #5561

Draft
wants to merge 7 commits into
base: store-and-forward
Choose a base branch
from

Conversation

Woutvstk
Copy link

My first time contributing to an open source project so not very confident in what i'm doing.

Changes to
FSCommon:
initializing SD library for NRF52. Progress: No compile error, but SD card does not get initialized properly yet added ifdef ARCH_ESP32 conditions around esp32 SD library functions

memget: added ifdef conditional statements

StoreForwardModule.cpp: added ifdef conditional statements

Rak4631 platfromIO.ini and variant.h:
added arduino-libraries/SD@^1.3.0 library to libdeps defined HAS_SDCARD and SPI pins

Arduino SD library. Made changes to library because using namespace SDLIB in header file caused ambiguity problems Not sure this is the right way of adding a library, also, how do i implement changes to the library permanently to the project?

Am I going somewhat in the right direction with these changes? Tell me your thoughts, thanks

Woutvstk and others added 3 commits December 14, 2024 00:17
…arduino SD library

My first time contributing to an open source project so not very confident in what i'm doing.

Changes to
FSCommon:
initializing SD library for NRF52. Progress: No compile error, but SD card does not get initialized properly yet
added ifdef ARCH_ESP32 conditions around esp32 SD library functions

memget: added ifdef conditional statements

StoreForwardModule.cpp: added ifdef conditional statements

Rak4631 platfromIO.ini and variant.h:
added arduino-libraries/SD@^1.3.0 library to libdeps
defined HAS_SDCARD and SPI pins

Arduino SD library. Made changes to library because using namespace SDLIB in header file caused ambiguity problems
Not sure this is the right way of adding a library, also, how do i implement changes to the library permanently to the project?

Am I going somewhat in the right direction with these changes? Tell me your thoughts, thanks
A "using namespace" statement in the header file was to messy to work around.
NRF52 SD card initialisation added
@garthvh
Copy link
Member

garthvh commented Dec 15, 2024

There is already an open draft pull from one of our primary firmware developers for this

#5083

@garthvh garthvh requested a review from caveman99 December 15, 2024 16:45
@Woutvstk
Copy link
Author

There is already an open draft pull from one of our primary firmware developers for this

#5083

How can I add/propose my commits to this draft pull request?

added card size and type function to SD library
added populateSDCard for NRF52
Changed NRF52 SD object from SDFilesystem to SD for more compatibility with esp32 SD library. Some functions are still different but most used open, read, write and close are the same.
@Woutvstk Woutvstk closed this Dec 17, 2024
@caveman99 caveman99 reopened this Dec 29, 2024
Copy link
Member

@caveman99 caveman99 left a comment

Choose a reason for hiding this comment

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

There is a lot of duplicate code in there. what about instead defining a couple of wrapper functions to emulate the original ESP32 sdcard driver? Also the test file on init is not neccessary. Ihe way it is not it makes the code really problematic to maintain.

Mainly made changes to the custom arduino SD library to make it compatible with the esp32 SD library already used in the store and forward code.
With these compatible function names and return, I removed some duplicate code.
@Woutvstk Woutvstk requested a review from caveman99 January 2, 2025 23:30
@Woutvstk
Copy link
Author

Woutvstk commented Jan 2, 2025

Made changes to the SD library and removed (most) duplicate code. Checking how much space is already used is not implemented in the arduino SD library.

The SD.begin() function for esp32 gives the spi interface object as an argument. The arduino library uses a "#define SDCARD_SPI" statement to tell the underlying sd2fat library what spi interface to use. So now the SD.begin() function takes an SPI interface argument but does not use it. Not sure how this should be solved in a clean way.
It does work and run on my rak4631.

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.

3 participants