Skip to content

refactor: single source for both stacks' interface text - #152

Merged
aido merged 2 commits into
aido:bip85from
buzzromain:refactor/single-source-for-interface-strings-bip85
Aug 4, 2026
Merged

aido merged 2 commits into
aido:bip85from
buzzromain:refactor/single-source-for-interface-strings-bip85

Conversation

@buzzromain

Copy link
Copy Markdown

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:

  • on a malformed recovery phrase, BAGL's invalid-flow steps advise "Check length," / "order and spelling"; NBGL's equivalent screen gives no advice at all (its invalid-result body text has no third line);
  • BAGL has two distinct flows and titles for "phrase doesn't match the device seed" vs. "phrase matches" (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:

  • all six device targets (nanos, nanox, nanos+, stax, flex, apex_p) were built before and after this change; text/data/bss are byte-identical on every target (nanos: 41224/0/3712, unchanged);
  • strings was 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, Done doubles 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 costs

nanos has the least flash of the six targets, and a static const char * const table 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) or extern 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's text/data/bss.

The one thing a table buys — enumerating every string in a test — is recovered without that cost: tests/unit/tests/ui_strings.c declares 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.c nor anything under src/bagl/ is compiled into any unit target, so ui_strings.h — holding only constants — is the first piece of interface text this repository can test outside Speculos. The new test asserts two things:

  1. every macro is non-empty;
  2. every BAGL fragment destined for a fixed, non-wrapping Nano layout (NN/NNN/PB/PBB/PNN/BN — never BNNN_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

  • Six targets compiled, nanos built by hand (CI's matrix does not build it — removed deliberately in an earlier commit): sizes identical before/after on all six, strings diff empty on all six.
  • Six unit-test-matrix configurations (ASan, UBSan, -O2, -Os, -fsigned-char, -funsigned-char): 80/80 passing on each (79/79 before this PR's test was added).
  • Functional tests under Speculos, both stacks: 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 -Werror clean on every touched file under src/.
  • Not covered by this repository's CI on a fork PR: functional-test workflows do not run on pull requests from forks (read-only workflow permissions there), so the Speculos runs above are local and are the only evidence for this PR; noting this as a limitation, not a defect.

…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).
@aido
aido merged commit 4e5a688 into aido:bip85 Aug 4, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants