Sd auto detect for insert and remove #1270
Conversation
src/ui/components/sdcard.c
Outdated
| #include <touch/gestures.h> | ||
| #include <ui/screen_stack.h> | ||
|
|
||
| // TODO: use a TIMER interrupt to get a more accurate timer. | ||
| // 250 is ~0.5 sec. Unit: rendering rate. | ||
| #define ACTIVE_AFTER 250 |
There was a problem hiding this comment.
ACTIVE_AFTER is a bit misnamed., it's more like CHECK_INTERVAL?
src/ui/components/sdcard.c
Outdated
| // 250 is ~0.5 sec. Unit: rendering rate. | ||
| #define ACTIVE_AFTER 250 | ||
|
|
||
| static int _count = 0; |
There was a problem hiding this comment.
move this to the data_t struct to avoid globals and so it is initialized to 0 when the component is created.
| static void _continue_callback(component_t* component) | ||
| { | ||
| data_t* data = (data_t*)component->parent->data; | ||
| if (!data->insert || sd_card_inserted()) { |
There was a problem hiding this comment.
For removing the card, the continue button served to exit the workflow without an error. You changed it to show a cancel button only, which errors with UserAbort when pressed, which is a breaking change.
Please leave the semantics as it was - leave the continue button/callback and hide the cancel button/callback for removal. Otherwise we have to worry about how it will behave in the app 😅
asi345
force-pushed
the
sd-auto-detect
branch
2 times, most recently
from
0621b40 to
0e7f3b8
Compare
| @@ -648,16 +648,6 @@ def insert_sdcard(self) -> None: | |||
| ) | |||
| self._msg_query(request, expected_response="success") | |||
|
|
|||
| def remove_sdcard(self) -> None: | |||
There was a problem hiding this comment.
This implies a major version bump of the bitbox02 Python library (7.0.0 => 8.0.0) and a CHANGELOG entry to py/bitbox02/CHANGELOG.md
src/ui/components/sdcard.c
Outdated
| if (sd_card_inserted()) { | ||
| if (data->callback) { |
There was a problem hiding this comment.
if(data->callback && sd_card_inserted()) ? not sure if sd_card_inserted() is cheap or expensive, but if expensive, it's better to call it only when needed.
CHANGELOG.md
Outdated
| @@ -16,6 +16,8 @@ customers cannot upgrade their bootloader, its changes are recorded separately. | |||
| - Ethereum: allow signing EIP-712 messages containing multi-line strings | |||
| - Allow exiting the screen asking to insert the microSD card | |||
| - HWW: add initialized status byte to _api_info response | |||
| - SD card: delete remove sd card api | |||
There was a problem hiding this comment.
confusing wording, maybe better like "Remove API to prompt removal of the microSD card"
It seems that big share of the sd card support tickets are customers not clicking the upper right corner after inserting their sd card. That's why auto-detection of sd card in the device is critical. With this commit, when 'insert sd card' option is called from api, BitBox02 device will start listening its sd card port and automatically recognize if an sd card is inserted. Furthermore, we noticed that 'remove sd card' feature is not used in any of our libraries. That's why we removed it from api and Python library. Signed-off-by: asi345 <inanata15@gmail.com>
| @@ -2,6 +2,9 @@ | |||
|
|
|||
| ## [Unreleased] | |||
|
|
|||
| # 8.0.0 | |||
There was a problem hiding this comment.
Also bump the actual package version please:
So far we usually bump versions as needed in each PR that makes a change, but keep the changelog in "UNRELEASED" until shortly before an actual release. This is not ideal I guess as it needs another commit to fix the changelog before a release. Maybe we can revisit how we do this for all our repos consistently at some point (cc @NickeZ).
There was a problem hiding this comment.
We can take a look at best practices. I think there also are some changelog tooling, maybe that helps.
Check commit messages for detailed explanation.
Let me know if you have a better suggestion for polling interval. You can test and let me know if it takes too long.