Nbgl migration for Nano devices#325
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #325 +/- ##
========================================
Coverage 84.69% 84.69%
========================================
Files 17 17
Lines 2228 2228
========================================
Hits 1887 1887
Misses 341 341
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
b73ef55 to
0ab928a
Compare
bigspider
left a comment
There was a problem hiding this comment.
LGTM for the general structure, commented elsewhere for specific UX fine-tuning.
6f99e38 to
999f31a
Compare
5b873fd to
5219e1d
Compare
|
Seems you have commit unwanted files: |
There was a problem hiding this comment.
Pull Request Overview
This PR migrates the application’s UI functionality from BAGL to NBGL for Nano devices while updating tests and build configurations accordingly.
- Migrates UI flows and string resources to the new NBGL API (e.g. “Sign transaction”, “Sign message”, “Register account”)
- Removes legacy BAGL code and updates test automations and build flags for NBGL support
- Updates documentation changelog and build settings to reflect the NBGL migration
Reviewed Changes
Copilot reviewed 2204 out of 2204 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/instructions.py | Updated instruction texts to reflect the new NBGL UI flows. |
| src/ui/menu_nbgl.c | Updated main menu flow and icon usage to conform to NBGL standards. |
| src/ui/menu_bagl.c | Removed legacy BAGL menu implementation. |
| src/ui/display_utils.c | Removed alternate formatting for Nano devices now using NBGL formatting. |
| src/ui/display_nbgl.c | Updated NBGL display flows with new string constants and conditional styling. |
| src/ui/display.h | Introduced new icon macros (ICON_APP_LOGO / ICON_APP_IMPORTANT) for NBGL UI consistency. |
| src/ui/display.c | Removed BAGL code paths and standardized UI process logic; replaced with NBGL functions. |
| src/ui/bagl_custom_streaming.c | Removed legacy BAGL streaming implementation. |
| src/main.c | Replaced BAGL UX initialization with NBGL spinner usage. |
| src/handler/*.c | Updated handler functions to use the UNUSED macro and revised NBGL UX flow functions. |
| src/boilerplate/io*.{h,c} | Removed BAGL-specific functions and updated event handling for NBGL. |
| src/boilerplate/dispatcher.c | Adjusted termination callback to reflect NBGL changes. |
| bitcoin_client_js automation tests | Updated JSON test rules to match new NBGL UI labels. |
| Makefile | Added flag for enabling NBGL support for Nano devices. |
| CHANGELOG.md | Documented NBGL migration and related UI changes. |
Comments suppressed due to low confidence (4)
src/ui/display_nbgl.c:133
- Confirm that the usage of GA_SIGN_TRANSACTION is consistent with other instances of transaction-signing labels across the UI to avoid confusing users.
nbgl_useCaseReviewStreamingFinish(GA_SIGN_TRANSACTION, start_processing_transaction_callback);
src/ui/menu_bagl.c:1
- [nitpick] Verify that the removal of the BAGL-based menu implementation is intentional and ensure that any legacy documentation or build references to BAGL are updated accordingly.
#ifdef HAVE_BAGL
src/boilerplate/dispatcher.c:173
- [nitpick] Review the removal of BAGL-specific termination callbacks to confirm that dispatcher cleanup operates correctly for NBGL devices.
G_dispatcher_state.termination_cb();
tests/instructions.py:14
- Verify that the updated instruction text "Sign message" aligns with the NBGL UI labels defined in display_nbgl.c to ensure consistency in user messaging.
instructions.same_request("Sign message", save_screenshot=save_screenshot)
bigspider
left a comment
There was a problem hiding this comment.
Only minor comments (except the graphical glitches, that are surely not caused by this PR).
tests/snapshots/nanox/test_get_extended_pubkey_standard_display_m/44'/1'/0'_0_0/00003.png
Show resolved
Hide resolved
5219e1d to
f1d1459
Compare
Fixed. |
f1d1459 to
8a5e07f
Compare
8a5e07f to
da82509
Compare
In this PR:
dispatcher_context_t *contextfromio_ui_process()and all the callers ? - will not do in this PR, the context may be useful later, for other needsapp-bitcoin-neware usually bumped in separate PRs on demandtests/test_sign_psbt.py::test_sign_psbt_singlesig_wpkh_512to256fails with timeout (a several minutes one), even before present changes. Did it work recently ?)Later:
nbgl_useCaseAdvancedReview()ornbgl_useCaseReviewLight()