Skip to content

sd-card: Increase usb processing timeout if sd card presence is checked. #1624

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

sd-card: Increase usb processing timeout if sd card presence is checked. #1624

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ customers cannot upgrade their bootloader, its changes are recorded separately.
- simulator: simulate a Nova device
- Add API call to fetch multiple xpubs at once
- Add the option for the simulator to write its memory to file.
- Attempt to fix "backup creation failed" while backup creation actually succeeded

### 9.23.2
- Improve touch sensor reliability when the sdcard is inserted
Expand Down
8 changes: 6 additions & 2 deletions src/sd.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
#include "screen.h"
#include "sd.h"
#include "util.h"
#include <usb/usb_processing.h>

#include <ff.h>

Expand Down Expand Up @@ -311,14 +312,17 @@ void sd_free_list(sd_list_t* list)

bool sd_card_inserted(void)
{
// The sd card is allowed up to 1s to initialize, therefore the usb processing timeout must also
// be increased by about 1s.
usb_processing_timeout_reset(-10);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you check if we actually sleep in case of backup create, assuming the app checked sd card presence beforehand once (which it does)? So we can more confidently release a hotfix knowing it's the right fix. Maybe one of the other operations in backup creation (copy_seed, writing backup) also causes delays and we should give more time there?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I checked now and during backup creation it only sleeps once, either if the sd-card was inserted before the device was started or after a replug of the sd-card.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When I create a backup from the app it calls sd_card_inserted() 88 times

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

during the device initialization it didn't sleep during the backup creation for me. But I don't think that is conclusive either way.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe sleeping in sd_card_inserted() isn't the culprit, but the timeout_reset still solves the issue because sd_card_inserted() is called from many places.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If the culprit is not sd_card_inserted (which it probably is not if there is no sleep and the check is fast), then the timeout reset should not be here right?

How about measuring how long backup.rs:create() takes, roughly?

When I create a backup from the app it calls sd_card_inserted() 88 times

That is crazy and clearly a bug, but can you specify when it is called? The app can't make API calls during another API call, so if it's the app making all these calls, it should technically not interfere with backup creation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can dig a little, but we don't call it from so many places and in one place we call it in the render function of a component. Probably that is why?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah yeah, if you run into the blocking insert prompt, that must be it. But that should not interfere with the backup creation api call 🤔

#ifdef TESTING
return true;
#else
sd_mmc_err_t err = sd_mmc_check(0);
/* If initialization is ongoing, wait up to 1 second for it to initialize */
if (err == SD_MMC_INIT_ONGOING) {
for (int i = 0; i < 10; ++i) {
delay_ms(100);
for (int i = 0; i < 100; ++i) {
delay_ms(10);
err = sd_mmc_check(0);
if (err != SD_MMC_INIT_ONGOING) {
break;
Expand Down