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

SD_MMC update #8294

Closed
wants to merge 5 commits into from
Closed

SD_MMC update #8294

wants to merge 5 commits into from

Conversation

PilnyTomas
Copy link
Contributor

@PilnyTomas PilnyTomas commented Jun 7, 2023

Added getter methods for pins
both examples:

  • Added usage of those getters
  • Added example for changing pins
  • Added ASCII art pin definition

@PilnyTomas PilnyTomas requested a review from P-R-O-C-H-Y June 7, 2023 09:02
@PilnyTomas PilnyTomas self-assigned this Jun 7, 2023
@PilnyTomas PilnyTomas marked this pull request as draft June 7, 2023 09:03
@P-R-O-C-H-Y
Copy link
Member

@PilnyTomas If you postpone this PR for 3.0.0 you can simplify the getPins function to just return _pin_clk for example. Look at the 5.1-libs branch for latest changes or PR #8289.

@PilnyTomas
Copy link
Contributor Author

@PilnyTomas If you postpone this PR for 3.0.0 you can simplify the getPins function to just return _pin_clk for example. Look at the 5.1-libs branch for latest changes or PR #8289.

This should be easy change for the 3.0.0

@PilnyTomas PilnyTomas marked this pull request as ready for review June 8, 2023 10:25
@PilnyTomas PilnyTomas changed the title Draft: SD_MMC update SD_MMC update Jun 8, 2023
Copy link
Member

@P-R-O-C-H-Y P-R-O-C-H-Y left a comment

Choose a reason for hiding this comment

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

@PilnyTomas PTAL.

Comment on lines +37 to +38
const char* ssid = "Viks";
const char* password = "Mordornumber1";
Copy link
Member

Choose a reason for hiding this comment

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

You should replace the Wifi ssid and password with the place-holders.

// If you are using any other ESP32-S3 board than ESP32-S3-USB-OTG which has preset default pins, you will
// need to specify the pins with the following example of SD_MMC.setPins()
// If you want to use only 1-bit mode, you can use the line with only one data pin (d0) begin changed.
// Please note that ESP32 does not allow pin change and will always fail.
Copy link
Member

Choose a reason for hiding this comment

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

As its not that true that the setPins() will always fail, because if you use the right pins its not gonna fail, but it don't make sense to call it as it won't make any difference. I would rather write that ESP32 has dedicated pins for SDMMC so there is no need to call setPin as it won't have any effect. Something like that :)

*/

#include "FS.h"
#include "SD_MMC.h"

// Default pins for ESP-S3
// Warning: ESP32-S3-WROOM-2 is using most of the default GPIOs (33-37) to interface with on-board OPI flash.
Copy link
Member

Choose a reason for hiding this comment

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

Add SDMMC to avoid misunderstanding.
ESP32-S3-WROOM-2 is using most of the default SDMMC GPIOs (33-37).

* D0 2 (add 1K pull up after flashing)
* D1 4
* For more info see file README.md in this library or on URL:
* https://github.com/espressif/arduino-esp32/tree/master/libraries/SD_MMC
Copy link
Member

Choose a reason for hiding this comment

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

There is no README.md file in the SDMMC library.

// 3.3V pin - SD pin 4
// GND pin - SD pin 3
int cmd = 40; // SD pin 2 - add 10k Pullup
int d3 = 41; // SD pin 1 - add 10k Pullup to card's pin even when using 1-bit mode
Copy link
Member

Choose a reason for hiding this comment

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

Is this tested? That it is necessary to pull up this pin when you won't use it? Just to be sure :)


// If you are using any other ESP32-S3 board than ESP32-S3-USB-OTG which has preset default pins, you will
// need to specify the pins with the following example of SD_MMC.setPins()
// If you want to use only 1-bit mode, you can use the line with only one data pin (d0) begin changed.
Copy link
Member

Choose a reason for hiding this comment

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

... one data pin (d0) being changed.

// If you are using any other ESP32-S3 board than ESP32-S3-USB-OTG which has preset default pins, you will
// need to specify the pins with the following example of SD_MMC.setPins()
// If you want to use only 1-bit mode, you can use the line with only one data pin (d0) begin changed.
// Please note that ESP32 does not allow pin change and will always fail.
Copy link
Member

Choose a reason for hiding this comment

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

As its not that true that the setPins() will always fail, because if you use the right pins its not gonna fail, but it don't make sense to call it as it won't make any difference. I would rather write that ESP32 has dedicated pins for SDMMC so there is no need to call setPin as it won't have any effect. Something like that :)

Comment on lines +221 to +225
//if(! SD_MMC.setPins(clk, cmd, d0)){ // 1-bit line version
if(! SD_MMC.setPins(clk, cmd, d0, d1, d2, d3)){ // 4-bit line version
Serial.println("Pin change failed!");
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

Why not to use use_1_bit_mode so no need for user to comment/uncomment code :)

if(use_1_bit_mode) {
    if(!SD_MMC.setPins(clk, cmd, d0)) { 
         Serial.println("Pin change failed!");
         return;
    }
}
else {
    if(!SD_MMC.setPins(clk, cmd, d0, d1, d2, d3)) { 
         Serial.println("Pin change failed!");
         return;
    }
}

Comment on lines +218 to +222
//if(! SD_MMC.setPins(clk, cmd, d0)){ // 1-bit line version
if(! SD_MMC.setPins(clk, cmd, d0, d1, d2, d3)){ // 4-bit line version
Serial.println("Pin change failed!");
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

Same as previous, if(use_1_bit_mode) can be used there.

Comment on lines +27 to +28
* For more info see file README.md in this library or on URL:
* https://github.com/espressif/arduino-esp32/tree/master/libraries/SD_MMC
Copy link
Member

Choose a reason for hiding this comment

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

The same, there is no README.md file in the SDMMC library.

Copy link
Contributor Author

@PilnyTomas PilnyTomas Jun 8, 2023

Choose a reason for hiding this comment

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

There will be

@PilnyTomas PilnyTomas closed this Jun 8, 2023
@PilnyTomas PilnyTomas deleted the SD_update branch June 8, 2023 13:38
@PilnyTomas PilnyTomas restored the SD_update branch June 8, 2023 13:48
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.

2 participants