Skip to content

Y333/nbgl updates - #426

Open
yogh333 wants to merge 19 commits into
developfrom
y333/nbgl_updates
Open

yogh333 wants to merge 19 commits into
developfrom
y333/nbgl_updates

Conversation

@yogh333

@yogh333 yogh333 commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

No description provided.

Copilot AI lite review requested due to automatic review settings August 20, 2026 08:18
@yogh333
yogh333 requested a review from a team as a code owner August 20, 2026 08:18
@yogh333
yogh333 requested review from sportron-ledger and removed request for a team and sportron-ledger August 20, 2026 08:18

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the Ledger Rust SDK’s NBGL layer to support “value extensions” (aliases) on tag/value pairs (e.g., ENS resolution, address book entries, QR codes, nested lists), and improves the NBGL home/settings info list API to allow an arbitrary number of info fields. It also bumps crate versions and improves diagnostics in the ledger_secure_sdk_sys build script.

Changes:

  • Add TagValue + FieldExtension and extension-aware NBGL review entry points (*_ext variants) plus a new example demonstrating all alias kinds.
  • Refactor NBGL review/home-and-settings builders to share common C-interop plumbing and allow arbitrary home info fields.
  • Improve ledger_secure_sdk_sys build diagnostics for unreadable .defines files and bump versions/docs accordingly.

Reviewed changes

Copilot reviewed 18 out of 19 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
README.md Updates workspace-level Docker/Speculos example invocation to run from the crate dir with required flags.
ledger_secure_sdk_sys/CHANGELOG.md Adds release note for improved .defines-file error reporting.
ledger_secure_sdk_sys/Cargo.toml Bumps ledger_secure_sdk_sys version to 1.16.5.
ledger_secure_sdk_sys/build.rs Improves panic messages when opening/reading the per-device defines file fails.
ledger_device_sdk/src/nbgl/nbgl_tag_value.rs Introduces TagValue, FieldExtension, and CTagValueList to support NBGL value extensions and shared C buffer ownership.
ledger_device_sdk/src/nbgl/nbgl_streaming_review.rs Uses shared CTagValueList path and adds next_ext for extension-aware streaming pages.
ledger_device_sdk/src/nbgl/nbgl_review.rs Switches internal plumbing to TagValue and adds show_ext entry points.
ledger_device_sdk/src/nbgl/nbgl_review_extended.rs Switches internal plumbing to TagValue and adds show_ext entry points.
ledger_device_sdk/src/nbgl/nbgl_home_and_settings.rs Adds app_name/info_list, fixes info-list NULL handling, and refactors C-struct preparation into prepare().
ledger_device_sdk/src/nbgl/nbgl_generic_review.rs Refactors tag/value list backing storage to reuse CTagValueList and adds TagValueList::new_ext.
ledger_device_sdk/src/nbgl/nbgl_advance_review.rs Switches internal plumbing to TagValue and adds show_ext entry points.
ledger_device_sdk/src/nbgl/nbgl_address_review.rs Stores an owned CTagValueList (optionally extension-aware) instead of borrowed fields.
ledger_device_sdk/src/nbgl.rs Exposes the new nbgl_tag_value module and re-exports its public API.
ledger_device_sdk/README.md Updates example run commands to include io_new and clarifies required features.
ledger_device_sdk/examples/nbgl_tag_value_alias.rs New example showcasing all supported alias kinds on a review screen.
ledger_device_sdk/examples/nbgl_home_and_settings.rs Updates example to use app_name + info_list for multiple info fields.
ledger_device_sdk/CHANGELOG.md Documents new APIs (home info list + tag/value aliases) and behavior changes.
ledger_device_sdk/Cargo.toml Bumps ledger_device_sdk to 1.37.0, updates ledger_secure_sdk_sys dependency, adds new example metadata.
Cargo.lock Locks updated versions for the workspace crates.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread ledger_device_sdk/src/nbgl/nbgl_tag_value.rs
Comment thread ledger_device_sdk/src/nbgl/nbgl_tag_value.rs
Comment thread ledger_device_sdk/src/nbgl/nbgl_tag_value.rs
Comment thread ledger_device_sdk/src/nbgl/nbgl_home_and_settings.rs
Comment thread ledger_device_sdk/src/nbgl/nbgl_home_and_settings.rs
Comment thread ledger_device_sdk/examples/nbgl_home_and_settings.rs Fixed
yogh333 and others added 19 commits August 26, 2026 11:59
…t_t)

Tag/value pairs could only ever be {name, value}: nbgl_contentValueExt_t and
nbgl_contentValueAliasType_t were unreachable from Rust, so an app could not
expand a shortened value into a QR code, an ENS or Address Book entry, or a
nested list without dropping to raw FFI.

Field keeps its two public fields, so no existing struct literal breaks. The
extension lives on a new parallel type instead:

  TagValue { name, value, extension: Option<FieldExtension> }
  impl From<&Field> for TagValue

FieldExtension has one constructor per alias kind (full_value, ens,
address_book, qr_code, info_list, tag_value_list) plus chainable
alias_sub_name / explanation / title / back_text setters.

Each review use case gains an extension-aware sibling taking &[TagValue] where
the existing one takes &[Field]: NbglReview::show_ext,
NbglAdvanceReview::show_ext, NbglReviewExtended::show_ext,
NbglStreamingReview::next_ext, NbglAddressReview::set_tag_value_list_ext and
TagValueList::new_ext. The &[Field] methods now lift into extension-less
TagValues, so each use case keeps a single path down to C rather than two.

Two details that are load-bearing and not compiler-checked:

- nbgl_contentTagValue_t.extension shares a union with valueIcon. C only reads
  it as an extension when the aliasValue bit is set, so CTagValueList sets that
  bit on every pair carrying one; setting the pointer alone would have NBGL
  treat it as an icon.
- The Vec of nbgl_contentValueExt_t is built to completion before any pair
  stores a pointer into it, since a later push could reallocate and leave those
  pointers dangling. Nested info / tag-value lists are boxed so their addresses
  are pinned once an extension points at them.

Adds the nbgl_tag_value_alias example, exercising all six kinds on one review
screen. Verified on Speculos.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
predefinedSet was hard-coded to `1 << W3C_RISK_DETECTED_WARN` in two places,
each with its own copy of a four-string WarningDetailsType, so an app could not
raise a blind-signing, gated-signing, threat or no-threat warning at all, nor
combine several, nor configure the pages by hand.

Both use cases now share one NbglWarning builder:

  NbglWarning::new()
      .predefined(&[WarningType::BlindSigning, WarningType::W3cRiskDetected])
      .dapp_provider("Example DApp")

predefined() takes any combination of the six nbgl_warningType_t values. The
manual path is covered too: info, intro_details, review_details,
intro_top_right_icon, review_top_right_icon and prelude, backed by new
CenterInfo, QrCode, Prelude, WarningBar and WarningDetails types. Bar lists
nest to arbitrary depth, each bar's sub-page being another WarningDetails.

warning_details(...) keeps its signature and behaviour on both
NbglAdvanceReview and NbglStreamingReview: it delegates to warning() through a
shared build_legacy_warning() that raises exactly W3cRiskDetected, so existing
apps are unaffected.

Two constraints worth recording:

- QrCode and WarningDetails::QrCode are compiled only for stax/flex/apex_p.
  NBGL_QRCODE is absent from the Nano defines, so those bindings have neither
  nbgl_layoutQRCode_t nor the qrCode union member.
- On the manual path NBGL draws the intro page's top-right button only when
  introTopRightIcon is set (nbgl_use_case.c:2730), and that button is the only
  way into intro_details. Setting the details without the icon builds pages
  that can never be shown, so intro_top_right_icon is documented as required
  and the example sets it. reviewTopRightIcon is declared by the C SDK but read
  nowhere in lib_nbgl, so it currently has no effect; kept for struct parity
  and documented as such.

Adds the nbgl_warning example, covering both paths. Verified on Speculos.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
nbgl_useCaseHomeAndSettings takes an nbgl_homeAction_t for an optional button
under the app name, and Rust always passed NULL, so the button was unreachable.

HomeAction carries the text, an optional icon, the style (STRONG_HOME_ACTION or
SOFT_HOME_ACTION) and the function to run when it is touched:

  let action = HomeAction::new("Start action", on_action).icon(&FERRIS);
  NbglHomeAndSettings::new()./* ... */.action(&action)

The callback is the app's own fn(), forwarded by a small shim because
nbgl_callback_t carries no user data. The SDK makes no assumption about what it
does: any use case can be started from it.

The example demonstrates one displaying a status page and then redrawing home.
Two details it has to work around, both worth knowing:

- The callback is a bare fn() and init_comm panics if called twice, so the Comm
  from sample_main is parked in a static for the callback to borrow.
- The home builder is reached through a raw pointer static rather than an
  Option<NbglHomeAndSettings>. A null pointer initialiser lands in .bss, while
  storing the struct puts its initialiser in .data, which the app linker script
  rejects outright ("`.data` section must be empty").

Verified on Speculos.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
nbgl_useCaseAdvancedReview takes an nbgl_tipBox_t for a touchable strip under
the review title, opening a modal of [type, content] rows. Rust always passed
NULL, so it was unreachable.

  let infos = [Field { name: "Contract", value: "0xA0b8...eB48" }];
  let tip_box = TipBox::new("Why can't this be decoded?", &infos)
      .modal_title("About this transaction");
  NbglAdvanceReview::new(..)./* ... */.tip_box(&tip_box)

Only INFOS_LIST is offered, being the sole member of the C union.

The tip box applies only to a review whose warning set raises no tip box of its
own. initWarningTipBox() substitutes its own wording for any W3C_* warning or
BLIND_SIGNING_WARN, and sets validWarningCtx, which makes TIP_BOX_TOKEN
dispatch to displaySecurityReport() instead of displayInfosListModal()
(nbgl_use_case.c:809). Both the text and the info list are then dropped.

That rules out an equivalent on NbglReview: the only use case it wraps that
accepts a tip box is nbgl_useCaseReviewBlindSigning, which forwards to
nbgl_useCaseAdvancedReview with blindSigningWarning and so always takes the
substituting path. A first version of this change did add NbglReview::tip_box();
testing on device showed it had no effect whatsoever, so it is deliberately
absent rather than shipped with a caveat.

CNestedInfoList in nbgl_tag_value.rs becomes pub(crate) CInfoList with an
as_c_type(), since a tip box carries the same nbgl_contentInfoList_t that value
extensions already stage — reused rather than copied a third time.

The nbgl_advance_review example gains a review with a tip box and deliberately
no warning, the configuration where it is visible. Verified on Speculos.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…inter'

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
79970ca changed HOME_REF from a raw pointer to Option<&'static mut ...> and
updated the callback to match, but left the assignment site handing it a raw
pointer, so the example stopped compiling:

  error[E0308]: mismatched types
   --> examples/nbgl_home_and_settings.rs:158:20
     expected `Option<&mut NbglHomeAndSettings>`,
        found `*mut NbglHomeAndSettings`

Complete the change instead of reverting it: the borrow of `home` is extended
to 'static through a raw pointer at the one assignment, which keeps the
callback free of raw-pointer dereferences as the CodeQL finding intended.
`home` lives until the app exits, since sample_main never returns.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
NbglGenericReview wrapped 6 of the 11 members of the C content union. Add the
four that an app can actually supply:

  SwitchesList    -> SWITCHES_LIST    nbgl_contentSwitchesList_t
  ChoicesList     -> CHOICES_LIST     nbgl_contentRadioChoice_t
  BarsList        -> BARS_LIST        nbgl_contentBarsList_t
  ExtendedCenter  -> EXTENDED_CENTER  nbgl_contentExtendedCenter_t

each with a matching NbglPageContent variant.

TAG_VALUE_DETAILS, the fifth type the ticket lists, is deliberately left out.
The dispatch that turns app-supplied content into a page handles ten types and
sends everything else to `default:`, which logs "Unsupported type" and returns
false, so the page renders empty — as testing on device confirmed. NBGL only
produces TAG_VALUE_DETAILS internally, when a tag/value pair is too long to fit
and needs a "More" button (nbgl_use_case.c:1101). The reachable target for this
use case is therefore 10 of 11, not 11 of 11.

ExtendedCenter embeds an nbgl_contentCenter_t, which the warning work already
wraps as CenterInfo, so CCenterInfo becomes pub(crate) and is reused rather
than the struct being wrapped a second time. The inline nbgl_contentTipBox_t it
carries is text and icon only — not the nbgl_tipBox_t of a review's first page,
which additionally owns the modal opened on touch.

The four types are display-only: contentActionCallback stays None, so touching
a switch, choice or bar does nothing. Routing tokens back to the app is a
separate design question, and NbglGenericReview is a review rather than a
settings screen.

The nbgl_generic_review example gains a page per type. Verified on Speculos.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The module existed but was commented out of nbgl.rs, and was scaffolding rather
than a disabled wrapper. Its navigation callback ignored the page index and
always returned the same four hard-coded choices, built from

  names: &[CHOICE1.as_ptr() as *const i8, ...] as *const *const i8

which points at a temporary array that dies at the end of that statement, and
from &str values that carry no NUL terminator. Un-commenting it would have
handed C a dangling pointer to unterminated strings.

Pages are now declared up front, one NbglPageContent each:

  NbglNavigableContent::new()
      .title("Settings")
      .on_control(on_control)
      .add_titled_page(NbglPageContent::CenteredInfo(..), "Overview", true, None)
      .add_page(NbglPageContent::SwitchesList(..))

That ordering is what makes it sound: content built inside the callback would be
dropped before NBGL drew it, so the instance owns every page and the callback
only serves them. nbgl_pageContent_t has its own union — the same members as
nbgl_content_u but a distinct type — so the mapping is written out rather than
shared with NbglGenericReview.

Nano is more restricted than the touchscreen devices: its nbgl_pageContent_t
has no title, touchable title or top-right icon, and its union has eight
members against eleven. So add_titled_page's extra arguments are accepted and
ignored there, and an ExtendedCenter or InfoLongPress page is rejected by show()
with a panic rather than left to draw an empty page.

Adds the nbgl_navigable_content example. Verified on Speculos.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three of the four use cases listed as unwrapped. The fourth,
nbgl_useCaseKeyboard, cannot be wrapped at all: it sits behind
#ifdef NBGL_KEYBOARD, which none of the five device configs define, so it is
absent from every target's bindings. Noted on the ticket.

NbglChoice gains show_with_details and show_advanced_with_details, wrapping
nbgl_useCaseChoiceWithDetails and nbgl_useCaseAdvancedChoiceWithDetails. Both
take a &WarningDetails — the details tree already built for NbglWarning, whose
CDetails becomes pub(crate) and is reused rather than wrapped a second time.
C declares that parameter as *mut, so each call materialises its own copy
instead of passing a temporary. Both block and return the choice, since
nbgl_choiceCallback_t reports either outcome.

NbglConfirm wraps nbgl_useCaseConfirm, until now reachable only indirectly
through NbglChoice::ask_confirmation. Its shape is dictated by the C semantics
rather than by the SDK's usual pattern:

- it draws a modal, so it needs a screen already up; showing one with nothing
  underneath leaves a blank screen,
- its callback runs only when the button is touched. Dismissal through the
  footer reports nothing, because it simply reveals what was underneath, so
  there is no signal a blocking show() could wait for.

Hence show_and_return(on_confirm), non-blocking and confirm-only. A first
attempt demonstrated it standalone at the end of the choice example, where it
was drawn and torn down by the following exit_app with nothing behind it, and so
never appeared. It is now raised from the home action button in
nbgl_home_and_settings, over the live home screen: Delete shows a status and
redraws home, Cancel needs no handling because home is already behind the modal.

Verified on Speculos.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…tent actions

NbglGenericConfiguration builds a configuration screen from arbitrary
NbglPageContent, paginated by NBGL and ended through the header. It is the
general form of NbglGenericSettings, which is fixed to a single switch list
backed by NVM. Because it takes the shared content types it inherits all ten of
them, so the wrapper itself is thin.

It also fixes a hard-coded NULL introduced by b4e2a52. nbgl_content_t carries a
per-content contentActionCallback, which NBGL installs when the displayed
content changes (nbgl_use_case.c:1009) and invokes on any control touch (:820) —
the mechanism NbglHomeAndSettings::settings already relies on. Every
NbglPageContent was built with that field NULL, so the SwitchesList, ChoicesList
and BarsList added earlier in this release were drawn but reported nothing back:
a switch toggled on screen and the app never heard about it. That is the same
"parameter hard-coded to null" pattern this ticket exists to remove.

Both containers now take a handler:

  NbglGenericConfiguration::new().on_action(f)
  NbglGenericReview::new().on_action(f)

f receives the token of the touched element and, for a choices list, the index
picked. The page index NBGL also supplies is dropped: the token already
identifies the element, and the signature then matches
NbglNavigableContent::on_control.

TagValueConfirm, InfoLongPress and InfoButton keep the SDK's own callback
whatever on_action is set to. That callback is how a review detects approval, so
overriding it would break the result of every existing NbglGenericReview::show.
From<&NbglPageContent> for nbgl_content_t is kept, delegating to
to_c_content(None), so no existing behaviour changes.

Adds the nbgl_generic_configuration example. Verified on Speculos.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
nbgl_operationType_t is a mask: the transaction type in the low bits, then five
flags above it. Rust set only the base type and SKIPPABLE_OPERATION, leaving
BLIND, RISKY, NO_THREAT and ADDRESS_BOOK unreachable. OperationFlag now covers
all five, folded into the mask the way WarningType::predefined already does,
and to_c_type takes that mask rather than a skippable bool.
NbglStreamingReview::skippable keeps working through c_operation_type.

Blind, Risky and NoThreat are what gate the warning button on a review's first
and last pages, so a warning set through warning_details or warning was until
now only reachable from the intro page — the button was never drawn during the
review itself.

They are offered only where NBGL has a warning to fill that button. The
WARNING_BUTTON_TOKEN handler reads reviewWithWarnCtx.warning->predefinedSet and
otherwise falls through to displayModalDetails, which reads details->title;
neither is NULL-checked (nbgl_use_case.c:796, :2532). Only
nbgl_useCaseAdvancedReview and nbgl_useCaseAdvancedReviewStreamingStart set that
context (:4298, :4489). A first version put operation_flags on NbglReview too:
on device the button appeared and touching it crashed the app, since a plain
review leaves that pointer unset. Hence:

- no operation_flags on NbglReview, no configuration there being safe,
- on the other two, show() panics when one of the three flags is set without a
  warning that can fill the button. An advance review with no warning content is
  equally unsafe: it passes a zeroed struct, so predefinedSet is 0 and the
  reviewDetails path dereferences NULL just the same.

The button reaching only the first page of a light review is NBGL's own rule,
not a gap here: isFirstOrLastPage matches INFO_LONG_PRESS, and a light review
finishes on INFO_BUTTON (:2977). Documented on operation_flags.

The nbgl_advance_review example sets Risky alongside its warning. Verified on
Speculos.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
TagValue gains force_page_start and centered_info, the two bitfields of
nbgl_contentTagValue_t that control where a pair is laid out: the first starts
a new review page at that pair, the second draws it as a centered block instead
of a tag/value row. TagValueList gains hide_end_of_last_line, alongside the
list-level options it already exposed.

TagValue now derives Default, so a literal setting only some fields needs
..Default::default(). It ships in the unreleased 1.37.0, so nothing outside this
repo is affected.

Two of the fields the ticket lists are deliberately still out:

- valueIcon. A touch on a value icon is reported through the list's token, and
  BACK_TOKEN is 0 (nbgl_use_case.c:69), so an icon with the token left at its
  default would be reported as "navigate back" rather than doing nothing. Making
  it safe means setting token and actionCallback and routing an app handler
  through the five review entry points that build a tag/value list — a unit of
  work rather than a field, and shipping the field alone would leave a trap.
- startIndex. It is the first index fetched through
  nbgl_contentTagValueList_t.callback, the pair-retrieval callback, which is not
  wrapped: the pairs are always supplied directly. It would do nothing.

The nbgl_tag_value_alias example gains a pair using both new flags. Verified on
Speculos.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
f960adb added hide_end_of_last_line without checking whether C reads it. It
does not. nbgl_use_case.c:1116-1148 runs for every TAG_VALUE_LIST page in a
generic or review flow and unconditionally overwrites the field:

    p_tagValueList->smallCaseForValue  = false;
    p_tagValueList->nbMaxLinesForValue = NB_MAX_LINES_IN_REVIEW;
    p_tagValueList->hideEndOfLastLine  = true;
    p_tagValueList->wrapping           = <app's value>;

so the setter could never change anything on screen. Removing it rather than
leaving a knob wired to nothing.

The same lines explain the P2 item asking for nbMaxLinesForValue to be exposed:
it cannot be. b01cc31 "Enforce 0 for nbMaxLinesForValue" was correct, since NBGL
substitutes its own value regardless. Both facts are now recorded on
TagValueList::new, together with the note that the long-standing
small_case_for_value argument is overridden the same way.

Of the whole list-level struct only wrapping and token survive the override.
token being honoured is what keeps the deferred valueIcon work viable.

The per-pair flags from f960adb, force_page_start and centered_info, are read
from the pair rather than the list and are unaffected.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
TagValue gains value_icon, drawing an icon at the right of a value and making
the row touchable, with NbglReview::on_value_icon and
NbglAdvanceReview::on_value_icon receiving the index of the touched pair. The
touch arrives through contentActionCallback, which nbgl_use_case.c:2973 copies
from the list, so it reuses the dispatch added in 6333f97 with a trampoline that
drops the token and keeps the index.

valueIcon could not ship alone, which is why it was held back from f960adb.
Three C constraints had to come with it:

- The list needs a token of its own. A touch is reported through
  nbgl_contentTagValueList_t.token and BACK_TOKEN is 0 (nbgl_use_case.c:69), so
  an icon on a list with the default token would read as "navigate back" rather
  than doing nothing. A list carrying icons now sets FIRST_USER_TOKEN and an
  actionCallback.
- A pair carries either an extension or an icon, never both: they share a union
  selected by the aliasValue bit. Panics.
- A whole list uses one or the other. NBGL forces a page's token to
  VALUE_ALIAS_TOKEN as soon as one pair on it is an alias (:1124), and that
  handler passes pair->extension to displayFullValuePage (:785) for whichever
  pair was touched. An icon pair sharing a page with an alias would have its
  icon pointer read as an nbgl_contentValueExt_t. NBGL chooses the pagination,
  so the invariant cannot be per-page. Panics.

NbglReviewExtended, NbglStreamingReview::next_ext and
NbglAddressReview::set_tag_value_list_ext take the same pairs but install no
handler, so an icon is drawn there without reporting; extending them is a
mechanical repeat of the same eight lines if wanted.

The nbgl_review example gains a review whose pairs carry icons, logging the pair
index through ledger_device_sdk::log::info!. That needs --features debug,log_info
to print, the macros being no-ops otherwise. Verified on Speculos.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
016c0da added TagValue::value_icon and put on_value_icon on NbglReview and
NbglAdvanceReview only. NbglReviewExtended, NbglStreamingReview and
NbglAddressReview take the same pairs, so an icon there was drawn without the
app being able to hear about it. They now have the setter too.

This closes more than a gap in coverage. The handler lives in a slot shared with
NbglGenericReview::on_action, and only the use cases that call
set_value_icon_handler write it — including writing None when no handler is set.
The three use cases left out of 016c0da never wrote it at all, so a value icon
touched there could have reached a handler installed by an earlier flow, with
that flow's pair indices. Every use case that builds a tag/value list now
installs its own handler, or clears the slot.

No new example: nbgl_review already exercises the mechanism end to end and the
wiring is identical in each of the five.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two things pinned settings to SETTINGS_SIZE, in each of the two modules that
draw them:

- a fixed-size `static mut SWITCH_ARRAY: [nbgl_contentSwitch_t; SETTINGS_SIZE]`
  holding the descriptors handed to C,
- `AtomicStorage<[u8; SETTINGS_SIZE]>` baked into the settings() signature,
  fixing the NVM array length.

The descriptors now live in a Vec owned by the builder, which the C callback
reaches through a pointer and length rather than indexing a static. settings()
is generic over the NVM array length.

A static cannot name that length, so the NVM pointer is type-erased to *mut ()
and paired with read and toggle functions monomorphised for the concrete N. The
callback goes through those instead of knowing the type, which is what lets the
length leave the signature.

The cap becomes the app's own: settings() panics only when asked to show more
switches than the NVM array has bytes, one byte per setting, which is a real
constraint rather than an arbitrary ten.

Source-compatible. Callers passing &mut AtomicStorage<[u8; 10]> have N inferred,
and SETTINGS_SIZE stays exported, redocumented as the size the examples use.
const_zero leaves nbgl.rs with the zeroed statics that needed it.

The struct update building each switch is redundant on touchscreen, where
nbgl_contentSwitch_t has exactly the fields set, and required on Nano, where
tuneId does not exist; hence the needless_update allow.

Verified on Speculos: toggling persists across leaving and re-entering the
settings page, exercising the erased NVM write and read-back.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…en limit

NbglReviewExtended wraps nbgl_useCaseReviewStart, nbgl_useCaseStaticReview and
nbgl_useCaseStaticReviewLight, all three declared inside #ifdef HAVE_SE_TOUCH
and absent from every Nano binding. The module is already gated to touchscreen
targets, but its example was not, so cargo build --examples failed on
nanox/nanosplus for the whole of this ticket's work.

The example now builds as an empty app on Nano, and --examples succeeds on all
five targets.

The library is deliberately unchanged. Supporting Nano would mean
reimplementing the use case on nbgl_useCaseReview, which cannot express what
NbglReviewExtended is for: it is a single call taking the tag/value list up
front, where NbglReviewExtended exists precisely to show an intro page, return,
and let the app decide what to review afterwards. An emulation would have
start() draw nothing and always report success, move rejection into show(), and
silently drop reject_text, longpress_text and the last-page glyph, none of which
nbgl_useCaseReview accepts. Rather than an API that quietly does something other
than what it says, the module documents the constraint and points at NbglReview
as the Nano equivalent.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
develop released 1.37.0 while this branch was open, so the epic's changelog
section and version bump moved to 1.38.0 during the rebase. The lockfile still
recorded 1.37.0 and is regenerated here.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.

3 participants