sdcard: solve backup bug when sd is re-inserted

[asi345]

This issue was existent for a long time. While using the BitBox02, when the sdcard is plugged into the device and the user unplugs and replugs it, backup operations (e.g., list backups, restore from backup) failed and throwed error from the firmware side.

Sdcard backup operations first check if the sdcard is inserted before calling sdcard interface funtions. This commit fixes the re-insertion problem by reinitializing the sdcard whenever it is checked if it is inserted.

The problem earlier most probably stems from sdcard being in an unexpected state when it is re-inserted. The forced initialization step fixes the broken states.

[asi345]

Also, I don't understand why CI fails (I haven't touched anything related to the error). And can you please check if the commit successfully addresses problem by trying out these steps:

- Have microSD inserted
- Plug in BitBox02
- Click on choose "recover from microSD"
- Go back to main menu (just hit "Enter")
- Take out MicroSD
- Plug MicroSD back in
- Click on "recover from microSD"

This flow was reproducing the error. If that works now, it means it should be fixed.

[benma]

Also, I don't understand why CI fails

It's because our clang-tidy check runs only on the files that are in the diff, which in this case is sdcard.c. Please try to figure out the error/fix. Could just be casting to (void*) on first glance.

[NickeZ]

I took an even closer look at this today. We are a bit sloppy with the return value of f_mount. So we missed that it returns an error message after reinserting the sd card. Since @asi345 pointed out that re-initializing the driver fixes the issue, I found another way to also solve the issue:

diff --git a/src/sd.c b/src/sd.c
index 9ad53f47..836962df 100644
--- a/src/sd.c
+++ b/src/sd.c
@@ -18,6 +18,7 @@
 #include <string.h>
 
 #ifndef TESTING
+#include "sd_mmc/sd_mmc_start.h"
 #include "driver_init.h"
 #include "sd_mmc.h"
 #endif
@@ -107,7 +108,12 @@ static bool _mount(void)
     sd_mmc_resume_clock();
 #endif
     memset(&fs, 0, sizeof(FATFS));
-    if (f_mount(&fs, "SD", 1) == FR_INVALID_DRIVE) {
+    int res = f_mount(&fs, "", 1);
+    if (res == FR_DISK_ERR) {
+        sd_mmc_start();
+        res = f_mount(&fs, "", 1);
+    }
+    if (res == FR_INVALID_DRIVE) {
 #ifndef TESTING
         sd_mmc_pause_clock();
 #endif
@@ -121,7 +127,8 @@ static bool _mount(void)
  */
 static void _unmount(void)
 {
-    f_mount(NULL, "SD", 1);
+    f_unmount("");
+
 #ifndef TESTING
     sd_mmc_pause_clock();
 #endif

• Our unmount function is also incorrectly implemented. There is a helper macro that gets it right. I suggest we change to that.
• I see now that I also removed SD from the argument to mount/umount. Empty string should mount the "default" volume. We anyway set FF_STR_VOLUME_ID to 0, so we don't have support for string volume names. I guess the correct "logical drive number" in our case is "0".

[asi345]

okay, I'm implementing this version that you suggested, and removing the extra rust-c function calls that I have added.

[asi345]

okay, I'm updating the code according to these reviews, but I am not sure whether I can reopen this pr. at the worst case I'll open a new pr.

[NickeZ]

okay, I'm updating the code according to these reviews, but I am not sure whether I can reopen this pr. at the worst case I'll open a new pr.

Just make a new branch and a new PR with the fixes!

[asi345]

Please check : https://github.com/BitBoxSwiss/bitbox02-firmware/pull/1320 for these fixes