selfcustody / krux

Open-source signing device firmware for Bitcoin
https://selfcustody.github.io/krux/
Other
175 stars 34 forks source link

Fix some issues with qrcode progress #441

Closed tadeubas closed 1 month ago

tadeubas commented 1 month ago

Description

What is the purpose of this pull request?

codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 80.55556% with 14 lines in your changes missing coverage. Please review.

Project coverage is 94.19%. Comparing base (19f99ec) to head (96983bf).

Files Patch % Lines
src/krux/display.py 44.44% 10 Missing :warning:
src/krux/pages/qr_capture.py 92.30% 2 Missing :warning:
src/krux/camera.py 66.66% 1 Missing :warning:
src/krux/input.py 88.88% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## develop #441 +/- ## =========================================== - Coverage 94.31% 94.19% -0.13% =========================================== Files 58 58 Lines 7146 7153 +7 =========================================== - Hits 6740 6738 -2 - Misses 406 415 +9 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

tadeubas commented 1 month ago

I am doing some final adjustments, I will place it as draft for now

Follow attached some QR code examples with edge cases for 240px width devices (e.g. yahboom):

https://github.com/user-attachments/assets/fcf49b4e-76ba-4c23-a793-5886cfc86e88

https://github.com/user-attachments/assets/2792a6fc-43b6-4d96-99e1-03332f0b0714

odudex commented 1 month ago

Great improvements! But it has adjustments to do, I saw you told you're aware off, but in case you missed one of these:

  1. The progress bars have random holes in it. image

  2. I did not observe any alteration in the entropy capture touch behavior; what could be the mechanism behind the change? Additionally, the display's clear() at the beginning is needed. image

tadeubas commented 1 month ago

Thanks for the review!

  1. The progress bars have random holes in it.

Yes, this is fixed now with https://github.com/selfcustody/krux/pull/441/commits/136b228bb816e895e716d0bc8c6330b55ce17db9

  1. I did not observe any alteration in the entropy capture touch behavior

I've noticed a change in the Yahboom tap behavior, before I would tap 5 times or more to get the entropy capture, now I tested it and the worst case was 3 taps, maybe it's just my impression...

Additionally, the display's clear() at the beginning is needed.

Sorry, I didn't test it on Amigo, so I didn't know about the text in the back. clear() returned with https://github.com/selfcustody/krux/pull/441/commits/3f9e6bea5d968549893198ba409c139781c753cc

odudex commented 1 month ago

Great improvements! Thank you!