Conversation
…e text BAGL (the three Nano targets) and NBGL (the touch targets) each carry the application's screen text separately, and the two had already drifted apart without anyone noticing: on a malformed recovery phrase, BAGL's invalid-flow steps advise "Check length, / order and spelling" while NBGL's equivalent screen gives no advice at all; and where BAGL uses two distinct flows and titles for a phrase that doesn't match the device seed versus one that does (ux_bip39_match_flow / ux_bip39_nomatch_flow), NBGL shows the same title, "Valid Secret\nRecovery Phrase", for both. No wording changes here. Every literal moves into src/common/ui_strings.h as a macro, and both stacks reference it instead of an inline literal -- this is a relocation, not a rewrite, and the binaries prove it (six targets built before and after, strings extracted from each .elf and diffed: identical on every one). The two stacks do not cut their text the same way: NBGL carries a full sentence per screen, BAGL splits the same idea across 2-4 lines. Where a screen's wording is shared and identical between the two, one macro serves both call sites; where the concept is shared but the wording already diverges (exactly the two cases above), both forms are declared next to each other under one comment, so the drift stays visible instead of being resolved by a splitting algorithm that would hide it again. `#define` rather than a table of `extern const char *`: nanos has the least flash of the six targets, and a string literal behind a macro costs nothing beyond the literal itself once the linker merges duplicates, while a table of pointers would cost 4 bytes and a relocation per entry on every target that links it in. Measured rather than assumed: nanos's text/data/bss did not move by a single byte across this change.
src/nbgl/ui.c and the files under src/bagl/ are not compiled into any unit
target, so this is the only net any of these strings gets other than the
functional suite under Speculos, which does not assert every screen's text.
Two things are checked. First, that every macro is non-empty -- a value that
became "" by accident (a bad merge, a copy-paste that dropped the literal) is
otherwise invisible until someone runs the app. Second, that every BAGL
fragment destined for a fixed (non-wrapping) Nano layout -- NN, NNN, PB, PBB,
PNN, BN -- fits the real pixel budget of that layout, measured from the SDK's
own per-character width table (lib_ux/src/ux_layout_paging_compute.c) and
box geometry (lib_ux/src/ux_layout_{bb,pb,pbb,pnn,nnn}.c), not guessed at.
Text that does not fit is silently clipped at runtime on real hardware;
nothing else in this repository catches that.
Two existing strings fail that budget and are deliberately left unasserted,
each with a comment at its exclusion: "recovery phrase" (both BAGL idle-menu
entries) at 93px against an 87px budget, and "BIP39 Recovery" at 90px against
the same budget. Both predate this test and this header -- neither word
changed for it -- and fixing wording is out of scope for this change.
Every entry in the test's own table is `{"NAME", NAME}`, expanding the
header's macro rather than retyping its value, so a renamed or removed macro
is a compile error in this file and not a silent gap in coverage.
All six configurations of the unit test matrix pass with this test included
(80/80, up from 79/79). Functional tests pass unchanged on both stacks under
Speculos (nanox: 8 passed, 9 skipped by pre-existing device gating; stax: 11
passed, 6 skipped, same reason).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
BAGL (the three Nano targets) and NBGL (the touch targets) each carry the application's screen text separately, and the two have already drifted apart — found by comparing the two stacks line by line, not by design:
"Check length," / "order and spelling"; NBGL's equivalent screen gives no advice at all (its invalid-result body text has no third line);ux_bip39_match_flow/ux_bip39_nomatch_flow, titled"BIP39 Phrase" / "is correct"vs."BIP39 Phrase" / "doesn't match"); NBGL shows the same title,"Valid Secret\nRecovery Phrase", for both.Neither is a deliberate variant — nothing tied the two stacks together, so nobody saw them diverge. Eight further interface-refactor PRs are planned behind this one; without a single source for text, each becomes two implementations at risk of a ninth, silent variant.
What this PR does — and does not do
Moves every user-visible string into
src/common/ui_strings.h. Changes no wording. Not one word moves, no screen looks different, and the PR proves it rather than asserting it:nanos,nanox,nanos+,stax,flex,apex_p) were built before and after this change;text/data/bssare byte-identical on every target (nanos: 41224/0/3712, unchanged);stringswas extracted from every.elf, sorted, and diffed against a build from the base commit — empty diff on all six targets.Some of the current wording is bad — two verdicts share a title,
Donedoubles as a refusal, a screen tells the user to check a length they never typed. This PR does not touch any of that; fixing it is later, separate work, and mixing it in here would make "nothing changed" unprovable.How the two stacks' text cohabits
NBGL carries a full sentence per screen; BAGL splits the same idea across 2-4 lines of a
UX_STEP, often in different words than NBGL uses for the same concept. A single canonical string and a splitting algorithm would erase that divergence rather than surface it, so the header does the opposite: where wording is already shared and identical, one macro serves every call site (Quit,Version,SSKR Share #%d,BIP39 Phrase,12/18/24 words); where the concept is shared but the wording already differs — including the two divergences above — both forms are declared next to each other under one comment, so a future edit to one is not made without seeing the other.Why
#define, and what it costsnanoshas the least flash of the six targets, and astatic const char * consttable would create a separate copy of every string per translation unit — unacceptable there. Two real options:#define(cost is whatever the literal itself costs; the linker merges identical literals) orextern const char[](enumerable by a test, but a pointer and a relocation per entry on every target it links into). This PR uses#define, and the six-target size comparison above is the measurement that justifies it: moving ~108 strings behind macros changed nothing in any target'stext/data/bss.The one thing a table buys — enumerating every string in a test — is recovered without that cost:
tests/unit/tests/ui_strings.cdeclares its own table,{ "NAME", NAME }per macro (the macro's actual value, not a retyped copy), so nothing in the header needs to be enumerable from device code.The test
Neither
src/nbgl/ui.cnor anything undersrc/bagl/is compiled into any unit target, soui_strings.h— holding only constants — is the first piece of interface text this repository can test outside Speculos. The new test asserts two things:NN/NNN/PB/PBB/PNN/BN— neverBNNN_PAGING, which the dynamic word/share buffers use and which auto-wraps) fits the real pixel budget of that layout. The budget and per-character widths are read from the SDK itself (lib_ux/src/ux_layout_paging_compute.c's character-width table,lib_ux/src/ux_layout_{bb,pb,pbb,pnn,nnn}.c's box geometry), not guessed — a string sized for a touch screen silently clips on a 128x32/64 Nano at runtime, and this is the only thing in the repository that would catch it before a device does.Two existing strings fail that check and are deliberately left unasserted, each flagged in a comment rather than silently passed:
"recovery phrase"(both BAGL idle-menu entries, 93px against an 87px budget) and"BIP39 Recovery"(90px against the same budget). Both predate this header — this PR does not touch either word — and correcting wording is out of scope here.Verification
nanosbuilt by hand (CI's matrix does not build it — removed deliberately in an earlier commit): sizes identical before/after on all six,stringsdiff empty on all six.-O2,-Os,-fsigned-char,-funsigned-char): 80/80 passing on each (79/79 before this PR's test was added).nanox(BAGL) 8 passed / 9 skipped;stax(NBGL) 11 passed / 6 skipped. Skips are pre-existing device gating (skip("Skipping test for Nano S+ device"),skip("Two-button devices only"), etc.), unrelated to this change.clang-format --dry-run -Werrorclean on every touched file undersrc/.