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

gui: animated qr #55

Merged
merged 4 commits into from
Apr 30, 2020
Merged

Conversation

gorazdko
Copy link
Contributor

@gorazdko gorazdko commented Mar 14, 2020

close #48

Status

Ready for review

Edit

New implementation force pushed which resembles cryptoadvance/specter-desktop#104

@stepansnigirev
Copy link
Collaborator

I think it would be better to extend QRCode class defined in
https://github.com/cryptoadvance/specter-diy/blob/master/src/gui/components.py#L15

We could add a method to set max size, and if QR code text is larger than that size - add two arrows on the right and left sides of the qr code. When you click on the arrow - next / prev part of the QR code is shown.
Label under the QR code could show current and total number of qr codes.

What do you think?

@stepansnigirev
Copy link
Collaborator

Maybe something like this:

Screenshot 2020-03-14 at 14 22 12

Copy link
Contributor Author

@gorazdko gorazdko left a comment

Choose a reason for hiding this comment

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

I changed the approach as you suggested.

src/gui/components.py Outdated Show resolved Hide resolved
src/gui/components.py Outdated Show resolved Hide resolved
src/gui/components.py Outdated Show resolved Hide resolved
src/gui/components.py Outdated Show resolved Hide resolved
src/gui/components.py Outdated Show resolved Hide resolved
@gorazdko gorazdko force-pushed the display_animated_qrs branch 3 times, most recently from 4869799 to 3234473 Compare March 23, 2020 17:38
@gorazdko
Copy link
Contributor Author

I force pushed the implementation which now works the same as cryptoadvance/specter-desktop#104

What should we do with animations in simulator mode? It doesnt look to have a timer.

self.set_event_cb(QrA.qr_cb)

def is_qr_big(self, message):
QrA.isQRtoobig = len(message) >= 850
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I set the limit at 850 which may be even lower. It needs more testing. The thing is drawing the qr takes alot of RAM, more than 10 times of the qr payload. One thing i noticed also, we can draw a 1KB qrcode in a 300x300 label most of the times but not into a 480x480 label because it requires that much more RAM.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think to solve it we could move lvgl buffer to SDRAM in the display driver.
I opened a corresponding issue: diybitcoinhardware/f469-disco#8

For now let's just live with this limitation.

@stepansnigirev
Copy link
Collaborator

stepansnigirev commented Apr 26, 2020

What should we do with animations in simulator mode? It doesnt look to have a timer.

We can write a simple scheduler where we can register / unregister callbacks. Maybe even look at asyncio.
I think we can do the same both in simulator and on the board itself.
At the moment we have something very similar to asyncio in ioloop() function - we constantly call .update() function on everything that requires updates.

Here is a pure python implementation:
https://github.com/micropython/micropython-lib/blob/master/asyncio_slow/asyncio_slow.py

Recently uasyncio was added to micropython:
micropython/micropython#5332

We can rebase to the latest version of micropython and check that nothing breaks, but I think we should start with pure python implementation.

@gorazdko
Copy link
Contributor Author

Maybe even look at asyncio

For now I just used ticks.diff for simulator and for stm32 with the .update function, so the code is the same. Is that ok?

In simulator i notice this qr width glitching, it doesnt happen on hardware though:

qrA

Its a consequence of the fact that i have to adapt the qr width after i draw it. Will ponder on this one a bit...

@stepansnigirev
Copy link
Collaborator

stepansnigirev commented Apr 27, 2020

Looks good! I will test a bit and merge :)

@stepansnigirev
Copy link
Collaborator

stepansnigirev commented Apr 29, 2020

Tested works great, I didn't see any width glitches in the simulator.
On hardware the close button becomes a bit laggy but I think it's ok, we can live with it.

With cryptoadvance/specter-desktop#118 we can use it in specter-desktop.

Now we can sign and display large transactions! Awesome!

@gorazdko
Copy link
Contributor Author

On hardware the close button becomes a bit laggy but I think it's ok, we can live with it

You mean pressing OK button while animation is running? Weird, I havent noticed that at all. Mine has never lagged and works blazingly fast.

@k9ert
Copy link
Contributor

k9ert commented Apr 30, 2020

This should also go into the release, i guess? I'd suggest to quickly merge it and then test everything together?!

@stepansnigirev
Copy link
Collaborator

I agree, works great, I don't see a reason not to merge :)

@stepansnigirev stepansnigirev merged commit 60120c2 into cryptoadvance:master Apr 30, 2020
@gorazdko gorazdko deleted the display_animated_qrs branch April 30, 2020 09:09
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.

Implement displaying animated QR codes
3 participants