sd: prompt user for formatting if format is not VFAT compatible #1330

asi345 · 2024-11-27T16:11:35Z

This commit inserts a check to validate whether the sdcard where the backup will be put is VFAT compatible or not. VFAT compatilibity is crucial for using the card and lack of it could result in errors in the firmware. Therefore, when setting up the device, this commit validates it and if it is not compatible, it asks user whether he/she is okay with formatting or not. When the user confirms, SD card is formatted accordingly and the fresh SD card is ready to be used by BitBox02.

asi345 · 2024-12-04T11:32:15Z

@NickeZ can you check if the format check is sufficient? I saw that fat32 is the common format from my search, but I also want to confirm it. with you.
Btw @benma I don't think the commit adds too much to the firmware size as you worried (if the code is correct ofc 😅)

asi345 · 2024-12-04T11:35:37Z

And this is how the prompting screen looks on the device. Let me know what you think!

NickeZ · 2024-12-04T13:24:40Z

Doesn't our code work with all fat file systems? Doesn't it make more sense to check if f_mount() == FR_NO_FILESYSTEM

benma · 2024-12-05T14:09:37Z

src/sd.c

    uint8_t work[FF_MAX_SS] = {0};
    return f_mkfs("SD", &params, work, sizeof(work)) == FR_OK;


What is FF_MAX_SS, and what does this param do in general?

Is this suitable for production or was it possibly only a good value for the tests/simulator? Asking b/c I am not familiar, but the sd_format function was originally added for unit tests only.

This set of options defines the extent of sector size used for the low level disk I/O interface, disk_read and disk_write function. Valid values are 512, 1024, 2048 and 4096. FF_MIN_SS defines minimum sector size and FF_MAX_SS defines the maximum sector size. Always set both 512 for memory card and harddisk.

Taken from FatFs configurations, http://elm-chan.org/fsw/ff/doc/config.html#max_ss. Only thing that boggled me was "SD" parameter there, but I did not encounter any issue while testing out. The card resetted and continued to work without an error.

benma · 2024-12-05T14:11:04Z

src/rust/bitbox02/src/sd.rs

+    true
+}
+
+pub fn sd_format() -> bool {


no need for the sd prefix, this is in the sd module and we don't need prefixes for namespacing like in C.

benma · 2024-12-05T14:12:10Z

src/rust/bitbox02/src/sd.rs

+    true
+}
+
+pub fn sd_format() -> bool {


-> Result<(), ()> is more idiotmatic Rust for error handling than bool. You can see other examples for converting C bools to Results in the bitbox02/src files.

Okay, converting that to Result object

benma · 2024-12-05T14:13:02Z

src/rust/bitbox02-rust/src/hww/api/backup.rs

+            ..Default::default()
+        })
+        .await?;
+        bitbox02::sd::sd_format();


one reason to make sd_format() return a Result<> is so that the compiler can warn you if you don't handle the result 😁

True, I intended to ask this in the pr message but forgot. Thanks, now it also handles when format fails 👍

asi345 · 2024-12-11T09:42:33Z

Doesn't our code work with all fat file systems? Doesn't it make more sense to check if f_mount() == FR_NO_FILESYSTEM

You meant f_mount() != FR_NO_FILESYSTEM ? But does this check mitigate other kinds of error that could be returned?

NickeZ · 2024-12-11T09:56:26Z

Doesn't our code work with all fat file systems? Doesn't it make more sense to check if f_mount() == FR_NO_FILESYSTEM

You meant f_mount() != FR_NO_FILESYSTEM ? But does this check mitigate other kinds of error that could be returned?

Not sure, what error messages do you get for a drive that is erased or a drive that has an incompatible file system? You can try with some apple fs or ntfs?

asi345 · 2024-12-12T12:14:22Z

Okay, I'll try that out with my old windows computer which I can plug the sdcard. Then I'll write the result here.

asi345 · 2024-12-18T10:32:01Z

Doesn't our code work with all fat file systems? Doesn't it make more sense to check if f_mount() == FR_NO_FILESYSTEM

You meant f_mount() != FR_NO_FILESYSTEM ? But does this check mitigate other kinds of error that could be returned?

Not sure, what error messages do you get for a drive that is erased or a drive that has an incompatible file system? You can try with some apple fs or ntfs?

I did a bit of research/trial and it seems the possible results for f_mount in that case would be:

FR_NO_FILESYSTEM
FR_DISK_ERR
FR_NOT_READY
FR_INVALID_DRIVE

In this case, I'm feeling like checking f_mount() == FR_OK could make sense.

NickeZ · 2024-12-18T11:17:57Z

Doesn't our code work with all fat file systems? Doesn't it make more sense to check if f_mount() == FR_NO_FILESYSTEM

You meant f_mount() != FR_NO_FILESYSTEM ? But does this check mitigate other kinds of error that could be returned?

Not sure, what error messages do you get for a drive that is erased or a drive that has an incompatible file system? You can try with some apple fs or ntfs?

I did a bit of research/trial and it seems the possible results for f_mount in that case would be:
* `FR_NO_FILESYSTEM`

* `FR_DISK_ERR`

* `FR_NOT_READY`

* `FR_INVALID_DRIVE`
In this case, I'm feeling like checking f_mount() == FR_OK could make sense.

FR_DISK_ERR could also mean that the sd-card was re-inserted right?

I think with the message "SD-card needs to be formatted", we can only check FR_NO_FILESYSTEM. Otherwise the message must be something like "Failed to read SD-card".

We really, really, don't want people to erase their backups.

There should probably also be two screens, first one confirm/cancel, and then one "pinch gesture" confirm with something like "PERMANENTELY ERASES SD-CARD".

asi345 · 2024-12-18T13:13:17Z

FR_DISK_ERR could also mean that the sd-card was re-inserted right?

yes, i just listed error possibilities that could occur at that stage. then I'll update the condition to check only FR_NO_FILESYSTEM, since you mentioned that is the purpose of this validation.

I'll also be adding second confirmation step as you mentioned, that's a good idea since I actually needed to cut off text to fit into one screen. I'll be adding screenshots again when I push.

jadzeidan · 2024-12-18T15:18:48Z

@asi345 @NickeZ

Have you thought of how to avoid false positives? For example, if the the user plugs in a working microSD card with a backup, but it is not detected correctly (we have had many "error detecting mSD card" issues in the past that is usually solved by replugging the device).

Does this prompt only appear explicitly if the BitBox02 detects that the microSD card is in a known incompatible format? Or does the pompt appear anytime there is a mSD card detection error?

jadzeidan · 2024-12-18T15:31:27Z

Regarding the UI. Would be good to use the "hold" gesture instead of the "check mark" since it's a permutant change.

This commit inserts a check to validate whether the sdcard where the backup will be put is VFAT compatible or not. VFAT compatilibity is crucial for using the card and lack of it could result in errors in the firmware. Therefore, when setting up the device, this commit validates it and if it is not compatible, it asks user whether he/she is okay with formatting or not. When the user confirms, SD card is formatted accordingly and the fresh SD card is ready to be used by BitBox02. Signed-off-by: asi345 <inanata15@gmail.com>

asi345 · 2024-12-19T10:31:42Z

I pushed a new commit:

@NickeZ I changed the condition to be f_mount() != FR_NO_FILESYSTEM, which only extracts the condition where file system is corrupted/erased.

And this is how the prompting screen looks on the device. Let me know what you think!

The first confirmation still looks like the above. Additionally I added this second confirmation screen:

If you can, I also suggest you trying out the code and check that you're not getting this prompt 😅

asi345 self-assigned this Nov 27, 2024

asi345 marked this pull request as draft November 27, 2024 16:13

asi345 force-pushed the prompt-sd-format branch 4 times, most recently from d01ec43 to 93a3b8f Compare December 4, 2024 11:04

asi345 marked this pull request as ready for review December 4, 2024 11:26

asi345 requested review from benma and NickeZ December 4, 2024 11:26

benma reviewed Dec 5, 2024

View reviewed changes

asi345 force-pushed the prompt-sd-format branch from 93a3b8f to 2f770bc Compare December 18, 2024 10:58

asi345 force-pushed the prompt-sd-format branch from 2f770bc to 8fdab28 Compare December 19, 2024 10:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sd: prompt user for formatting if format is not VFAT compatible #1330

sd: prompt user for formatting if format is not VFAT compatible #1330

asi345 commented Nov 27, 2024

asi345 commented Dec 4, 2024

asi345 commented Dec 4, 2024

NickeZ commented Dec 4, 2024

benma Dec 5, 2024

asi345 Dec 12, 2024

benma Dec 5, 2024

benma Dec 5, 2024 •

edited

Loading

asi345 Dec 11, 2024

benma Dec 5, 2024

asi345 Dec 11, 2024

asi345 commented Dec 11, 2024

NickeZ commented Dec 11, 2024 •

edited

Loading

asi345 commented Dec 12, 2024

asi345 commented Dec 18, 2024

NickeZ commented Dec 18, 2024

asi345 commented Dec 18, 2024

jadzeidan commented Dec 18, 2024

jadzeidan commented Dec 18, 2024

asi345 commented Dec 19, 2024

		uint8_t work[FF_MAX_SS] = {0};
		return f_mkfs("SD", &params, work, sizeof(work)) == FR_OK;

sd: prompt user for formatting if format is not VFAT compatible #1330

Are you sure you want to change the base?

sd: prompt user for formatting if format is not VFAT compatible #1330

Conversation

asi345 commented Nov 27, 2024

asi345 commented Dec 4, 2024

asi345 commented Dec 4, 2024

NickeZ commented Dec 4, 2024

benma Dec 5, 2024

Choose a reason for hiding this comment

asi345 Dec 12, 2024

Choose a reason for hiding this comment

benma Dec 5, 2024

Choose a reason for hiding this comment

benma Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

asi345 Dec 11, 2024

Choose a reason for hiding this comment

benma Dec 5, 2024

Choose a reason for hiding this comment

asi345 Dec 11, 2024

Choose a reason for hiding this comment

asi345 commented Dec 11, 2024

NickeZ commented Dec 11, 2024 • edited Loading

asi345 commented Dec 12, 2024

asi345 commented Dec 18, 2024

NickeZ commented Dec 18, 2024

asi345 commented Dec 18, 2024

jadzeidan commented Dec 18, 2024

jadzeidan commented Dec 18, 2024

asi345 commented Dec 19, 2024

benma Dec 5, 2024 •

edited

Loading

NickeZ commented Dec 11, 2024 •

edited

Loading