Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for NBGL on LNS #70

Closed
wants to merge 14 commits into from
Closed

Add support for NBGL on LNS #70

wants to merge 14 commits into from

Conversation

xchapron-ledger
Copy link
Contributor

@xchapron-ledger xchapron-ledger commented Apr 26, 2024

Demo available here: LedgerHQ/app-boilerplate#123

Xavier Chapron and others added 11 commits April 25, 2024 16:28
(cherry picked from commit 2b4d131)
(cherry picked from commit cbe3ba5343d7ae0c64b8f80aa929acdf15dd1ae0)
(cherry picked from commit c951b1c0b8948e2c215fa79fa140d42c5a165329)
…ontentTagValueList_t

nbgl_layoutTagValueList_t was already an alias of
nbgl_contentTagValueList_t. Now with choose to use this directly.

(cherry picked from commit 2e7edf5300c24e69a3a8a1e8eed37798ca85c521)
// Consider the resulting text fit in screen
char buffer[SCREEN_LINE_MAX_STRING_LEN];

#if 1

Choose a reason for hiding this comment

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

Which option will we keep ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm considering exposing u8toa somewhere apps could use it too. Though this is useful only for LNS, sparing quite a lot of flash if the app code doesn't need snprintf...
What do you think about it?

Choose a reason for hiding this comment

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

Yes, could be good!


if (left) {
if (screen_content.vertical_nav) {
display_glyph(&C_icon_up, 2, 15, 7, 4);

Choose a reason for hiding this comment

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

Would be helpful to have defines, in order to follow easier the coce, and adapt if needed later?

@xchapron-ledger xchapron-ledger force-pushed the xch/nbgl-lns branch 2 times, most recently from 4bcc5c2 to b204a92 Compare April 29, 2024 07:54
uint8_t button_mask;
uint8_t button_same_mask_counter;

// enable speeded up long push (continuous)
Copy link
Contributor

@fbeutin-ledger fbeutin-ledger May 2, 2024

Choose a reason for hiding this comment

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

Keeping the button pressed to review faster ?
Has it been validated by product or something ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is there only because it's needed on LNX and LNSP and I kept the file really similar to the one in the ledger-secure-sdk branch.
Apps that only use the NBGL use case API won't use it. But some apps can still use it if they use the lower API.

@@ -58,9 +58,7 @@ WEAK uint8_t io_event(uint8_t channel)

switch (G_io_seproxyhal_spi_buffer[0]) {
case SEPROXYHAL_TAG_BUTTON_PUSH_EVENT:
#ifdef HAVE_BAGL
UX_BUTTON_PUSH_EVENT(G_io_seproxyhal_spi_buffer);
Copy link
Contributor

Choose a reason for hiding this comment

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

i would have expected us to use the ! HAVE_BAGL in the standard app
I'm probably missing something 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I simplified to only support the LNS case which events are handled the same if we use BAGL or NBGL.
It's a divergence with the version in ledger-secure-sdk master branch, but to keep it compatible, I would have to add so many #ifdef TARGET_NANOS that it won't be better IMO.

@xchapron-ledger
Copy link
Contributor Author

Replaced by LedgerHQ/ledger-secure-sdk#651

@xchapron-ledger xchapron-ledger deleted the xch/nbgl-lns branch May 13, 2024 12:21
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.

4 participants