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

Allow heap feature to be disabled. #180

Merged
merged 4 commits into from
Aug 21, 2024

Conversation

ryankurte
Copy link
Contributor

A while back the SDK introduced a heap allocator for everything but the nanos, which uses a config trick to enable this for all other platforms, however, the large size chosen for this default heap may exceed the available memory for applications that are already near RAM limits, and the target specific inclusion trick used to select this does not allow the heap feature to be disabled.

This PR simplifies the behaviour of the heap feature so this has no effect on the unsupported nanos target and can be disabled for apps that need to control the size of the allocated heap.

Gates heap feature in source rather than using cargo include trickery so that this can be disabled for apps that need to control the size of the allocated heap.
Copy link
Contributor

@yogh333 yogh333 left a comment

Choose a reason for hiding this comment

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

LGTM but both ledger_device_sdk and ledger_secure_sdk_sys crates shall be version bumped (1.14.1 for ledger_device_sdk and 1.4.4 for ledger_secure_sdk_sys)

@agrojean-ledger
Copy link
Contributor

agrojean-ledger commented Aug 21, 2024

@ryankurte @yogh333 @kingofpayne we need to be careful in allowing the heap to be disabled : the NBGL module relies on it for the alloc crate usage.

Apps for Stax and Flex that use NBGL (so most likely all of them) will not compile without the heap allocator enabled.

@yogh333
Copy link
Contributor

yogh333 commented Aug 21, 2024

@agrojean-ledger Yes you are right. This PR makes the heap feature enabled by default except on nanos so there is no issue on my side. To be disabled the --no-default-features shall be used explicitly. And when compiled for Nano S (no NBGL support), the heap feature is disabled automatically. So it is ok for me.

@agrojean-ledger
Copy link
Contributor

@agrojean-ledger Yes you are right. This PR makes the heap feature enabled by default except on nanos so there is no issue on my side. To be disabled the --no-default-features shall be used explicitly. And when compiled for Nano S (no NBGL support), the heap feature is disabled automatically. So it is ok for me.

Ok then, up to developers to be careful when disabling the feature for a specific target.

@yogh333 yogh333 merged commit e18d34f into LedgerHQ:master Aug 21, 2024
40 checks passed
@ryankurte ryankurte deleted the fix/heap-feature branch August 21, 2024 22:13
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