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

Update QR-code-generator #1639

Closed
matejcik opened this issue Jun 1, 2021 · 7 comments · Fixed by #1894
Closed

Update QR-code-generator #1639

matejcik opened this issue Jun 1, 2021 · 7 comments · Fixed by #1894
Assignees
Labels
code Code improvements core Trezor Core firmware. Runs on Trezor Model T and T2B1. low hanging fruit Simple, quick task. T1B1 legacy Trezor One

Comments

@matejcik
Copy link
Contributor

matejcik commented Jun 1, 2021

https://github.com/nayuki/QR-Code-generator

couldn't find a changelog, reviewing all commits shouldn't be too difficult though

I also notice they have a Rust version! and it's available as a crate too. Maybe investigate switching to that?

@nayuki
Copy link

nayuki commented Jun 3, 2021

I see that the current version of this Trezor repository has pinned my QR lib at a version from 2019-02-14.

Trezor is using the C language version of my library. What has changed between then and now in the C port:

  • Slightly different algorithm for detecting finder patterns for penalty calculation.
  • Improved the correctness of handling integers - types, bit widths, signedness, arithmetic, etc.
  • Revamp some private/internal functions and variables.

For the C lib, when moving from the 2019-02-14 version to the 2020-09-12 version, the impact on callers should be small.

@nayuki
Copy link

nayuki commented Jun 3, 2021

My C library completely avoids heap allocations; it expects the caller to pass in input/output buffers and scratch space. This is not true for my other language ports; all of them do heap allocations and deallocations. So neither my C++ lib nor Rust lib match this feature of my C lib. Also, my Rust lib doesn't define a C foreign function interface (FFI) or do anything to help users coming from other languages.

@prusnak
Copy link
Member

prusnak commented Jun 3, 2021

@nayuki Thanks for the summary! It seems like it makes sense to stick to the C version for now.

Maybe in the future when we rewrite the whole rendering stack to Rust (so no FFI is needed) and when qrcodegen crate works with #![no_std] option and has no allocations, it would make sense to switch.

@tsusanka tsusanka added MEDIUM core Trezor Core firmware. Runs on Trezor Model T and T2B1. T1B1 legacy Trezor One code Code improvements and removed enhancement labels Oct 7, 2021
@matejcik matejcik added the low hanging fruit Simple, quick task. label Oct 22, 2021
@bosomt
Copy link

bosomt commented Nov 16, 2021

@mmilata any special testing needed on our side or just test/validate/compare QR code generated by T1 and TT in current dev master ?

@nayuki
Copy link

nayuki commented Nov 16, 2021

Maybe in the future [...] when qrcodegen crate works with #![no_std] option and has no allocations

FYI I made this a reality last week: https://github.com/nayuki/QR-Code-generator/tree/master/rust-no-heap

@mmilata
Copy link
Member

mmilata commented Nov 16, 2021

@bosomt generating and validating a QR code on T1 and TT should be enough

@nayuki neat! we might switch to that eventually

@bosomt
Copy link

bosomt commented Nov 16, 2021

QA OK trezor-fw-regular-2.4.3-221977ad.bin
QA OK trezor-fw-regular-1.10.4-221977ad.bin

BTC receive QR code OK

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code Code improvements core Trezor Core firmware. Runs on Trezor Model T and T2B1. low hanging fruit Simple, quick task. T1B1 legacy Trezor One
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants