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

[BLE] enable use of local APDU buffer #403

Merged
merged 3 commits into from
Sep 20, 2023
Merged

[BLE] enable use of local APDU buffer #403

merged 3 commits into from
Sep 20, 2023

Conversation

yhql
Copy link
Contributor

@yhql yhql commented Aug 9, 2023

Description

This PR primarily submits changes to match its use in the Rust SDK.

  • The use of HAVE_LOCAL_APDU_BUFFER means G_io_apdu_buffer does not exist anymore and a way to know where to copy received commands is put in place. An additional pointer is added to the main BLE context, and all copies to the global apdu buffer are replaced with copies to this new pointer.
    Its value and associated length can be set externally through a new function. The use of G_io_seproxyhal_spi_buffer is also replaced with locals where possible to ease tracking data moves.

  • To make sure bindings to the C APIs are correct, the use of const was enforced were necessary, sometimes requiring modifying the code a bit (see the DEF1 channel ID copy that was moved).

  • Functions that enable BLE pairing while in an app were gated so that they can be deactivated, which allows not having to link in snprintf and other functions from another module (ux)

Changes include

  • Bugfix (non-breaking change that solves an issue)

@yhql yhql marked this pull request as draft August 9, 2023 16:03
@yhql yhql marked this pull request as ready for review August 22, 2023 15:45
@yhql yhql changed the title WIP: [BLE] enable use of local APDU buffer [BLE] enable use of local APDU buffer Aug 22, 2023
@yhql
Copy link
Contributor Author

yhql commented Aug 22, 2023

Tested as applied on API_LEVEL_5 on a Nano X 2.2.2, on rust-app ("menu" and a "sign" command), and on the current app-boilerplate with a "get name and version" command.

@sportron-ledger
Copy link
Contributor

Hello,
What is the purpose of the HAVE_INAPP_BLE_PAIRING define?

@yhql
Copy link
Contributor Author

yhql commented Sep 14, 2023

@sportron-ledger it allows disabling parts of the code that require specific dependencies on printf and ux machinery that is not present (or not easy to integrate) in the Rust SDK.

@codecov-commenter
Copy link

codecov-commenter commented Sep 14, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (80b72e7) 69.33% compared to head (2a8c8c6) 69.33%.
Report is 3 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #403   +/-   ##
=======================================
  Coverage   69.33%   69.33%           
=======================================
  Files          11       11           
  Lines         874      874           
=======================================
  Hits          606      606           
  Misses        268      268           
Flag Coverage Δ
unittests 69.33% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@yhql yhql force-pushed the ble_local_buf branch 2 times, most recently from e45dc45 to c275867 Compare September 20, 2023 15:11
@yhql yhql merged commit c8363b3 into master Sep 20, 2023
159 of 160 checks passed
@yhql yhql deleted the ble_local_buf branch September 20, 2023 16:15
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