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

feat: ledger stax + flex support #6588

Merged
merged 8 commits into from
Oct 4, 2024

Conversation

SWvheerden
Copy link
Collaborator

Description

Upgrading ledger library for stax and flex support

@ghpbot-tari-project ghpbot-tari-project added the CR-insufficient_context Your PRs commit messages don't provide enough context to justify accepting the change. label Sep 26, 2024
Copy link

github-actions bot commented Sep 26, 2024

Test Results (Integration tests)

36 tests  +36   36 ✅ +36   14m 56s ⏱️ + 14m 56s
11 suites +11    0 💤 ± 0 
 2 files   + 2    0 ❌ ± 0 

Results for commit 22db1ff. ± Comparison against base commit 7f282d9.

♻️ This comment has been updated with latest results.

@ghpbot-tari-project ghpbot-tari-project added P-acks_required Process - Requires more ACKs or utACKs P-reviews_required Process - Requires a review from a lead maintainer to be merged labels Sep 26, 2024
Copy link

github-actions bot commented Sep 26, 2024

Test Results (CI)

    3 files    129 suites   35m 46s ⏱️
1 325 tests 1 324 ✅ 0 💤 1 ❌
3 973 runs  3 972 ✅ 0 💤 1 ❌

For more details on these failures, see this check.

Results for commit 22db1ff.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@brianp brianp left a comment

Choose a reason for hiding this comment

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

Some changes, and questions.

I don't love that this is how they want us to write apps. I'd almost be more inclined to write two different ones, and modularize all the functional bits so we just wrap it in two different UX's.

but the work is done now, and it is how they suggest doing it so 🤷🏻

#[cfg(any(target_os = "stax", target_os = "flex"))]
{
// Load glyph from 64x64 4bpp gif file with include_gif macro. Creates an NBGL compatible glyph.
const FERRIS: NbglGlyph = NbglGlyph::from_include(include_gif!("key_64x64.gif", NBGL));
Copy link
Contributor

Choose a reason for hiding this comment

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

Same with FERRIS

What's an NBGL campatible glyph anyway? What's NBGL?

@@ -199,6 +202,7 @@ pub fn handler_get_script_offset(
comm.append(&[RESPONSE_VERSION]); // version
comm.append(&script_offset.to_vec());
offset_ctx.reset();
offset_ctx.finished = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this to do with your state issue?
What exactly is the bool solving, I think I'm overlooking its purpose.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

an attempt at getting the UI working, good catch this needs to be removed

Comment on lines -61 to -94
/// Allocator heap size
const HEAP_SIZE: usize = 1024 * 26;

/// Statically allocated heap memory
static mut HEAP_MEM: [MaybeUninit<u8>; HEAP_SIZE] = [MaybeUninit::uninit(); HEAP_SIZE];

/// Bind global allocator
#[global_allocator]
static HEAP: embedded_alloc::Heap = embedded_alloc::Heap::empty();

/// Error handler for allocation
#[alloc_error_handler]
fn alloc_error(_: core::alloc::Layout) -> ! {
SingleMessage::new("allocation error!").show_and_wait();
ledger_device_sdk::exit_app(250)
}

/// Initialise allocator
pub fn init() {
unsafe { HEAP.init(HEAP_MEM.as_ptr() as usize, HEAP_SIZE) }
}

struct MyCriticalSection;
critical_section::set_impl!(MyCriticalSection);

unsafe impl critical_section::Impl for MyCriticalSection {
unsafe fn acquire() -> RawRestoreState {
// nothing, it's all good, don't worry bout it
}

unsafe fn release(_token: RawRestoreState) {
// nothing, it's all good, don't worry bout it
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this tested on the nanos+ ?

I was under the distinct impression without defining the allocator we lose the ability to well... Allocate anything. Essentially it's memory-less

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes it was,
Ledger realised that they need one, so the rust_sdk lib now includes one and it clashes as you can only have one defined.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahhh neat.

Copy link
Contributor

@brianp brianp left a comment

Choose a reason for hiding this comment

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

Ack

@ghpbot-tari-project ghpbot-tari-project removed the P-reviews_required Process - Requires a review from a lead maintainer to be merged label Oct 4, 2024
@SWvheerden SWvheerden merged commit fa7611d into tari-project:development Oct 4, 2024
16 of 17 checks passed
@SWvheerden SWvheerden deleted the ledgerupgrade branch October 4, 2024 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CR-insufficient_context Your PRs commit messages don't provide enough context to justify accepting the change. P-acks_required Process - Requires more ACKs or utACKs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants