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

Adds "screen saver" mode to signer app #190

Merged
merged 8 commits into from
Aug 21, 2024

Conversation

italo-sampaio
Copy link
Collaborator

The screen saver screen is just a completely black screen with no active pixels. This
PR introduces the logic to switch to this "screen saver" screen after 30 seconds
of inactivity in the signer app. When any of the physical buttons is pressed, the usual
Signer running... screen is presented again.

Also, I took the chance to remove some unused states and their corresponding logic, since they were not being used.

Copy link
Collaborator

@amendelzon amendelzon left a comment

Choose a reason for hiding this comment

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

Works like a charm!

A few things I think we should try to improve. I'll write them in order from most to least important. Optional items marked with a (*):

  • There are no tests. I'd probably separate some of the logic and write some minimal unit tests so that we can catch potential issues in the future.
  • There are no comments whatsoever.
  • The name ui_idle is misleading, since it's actually the only state in which the screen shows something (and is activated in response to user actions). So maybe rename it to something more meaningful like ui_info?
  • (*) Functionality wise: does it make sense resetting the timer if the user keeps pressing the button? There's not that much info that a user can't finish reading it in 30 seconds. What do you think?
  • (*) This is out of scope, but if it's not too hard, we might as well seize the momentum: how hard would it be to have the signer version displayed underneath the "Signer running..." message? As in: Version: 5.2.0.

firmware/src/ledger/signer/src/main.c Outdated Show resolved Hide resolved
firmware/src/ledger/signer/src/main.c Outdated Show resolved Hide resolved
@italo-sampaio
Copy link
Collaborator Author

Works like a charm!

A few things I think we should try to improve. I'll write them in order from most to least important. Optional items marked with a (*):

  • There are no tests. I'd probably separate some of the logic and write some minimal unit tests so that we can catch potential issues in the future.
  • There are no comments whatsoever.
  • The name ui_idle is misleading, since it's actually the only state in which the screen shows something (and is activated in response to user actions). So maybe rename it to something more meaningful like ui_info?

Thanks for the comments, I'll tackle them.

  • (*) Functionality wise: does it make sense resetting the timer if the user keeps pressing the button? There's not that much info that a user can't finish reading it in 30 seconds. What do you think?

Good point. This is the usual behavior of a screensaver, that's why I went that way, I think it's more intuitive for the user. I agree there's no point in keeping pressing the buttons since their only side effect is showing the same static screen, but if for some reason you keep pressing, doesn't it look strange if the screen goes off on you even though you just pressed it? IDK, I don't have a strong opinion on that, what do you think?

  • (*) This is out of scope, but if it's not too hard, we might as well seize the momentum: how hard would it be to have the signer version displayed underneath the "Signer running..." message? As in: Version: 5.2.0.

Sounds good, will give it a try. Doesn't seem to be hard to implement and surely adds value.

@amendelzon
Copy link
Collaborator

Thanks for the comments, I'll tackle them.

Thanks!

Good point. This is the usual behavior of a screensaver, that's why I went that way, I think it's more intuitive for the user. I agree there's no point in keeping pressing the buttons since their only side effect is showing the same static screen, but if for some reason you keep pressing, doesn't it look strange if the screen goes off on you even though you just pressed it? IDK, I don't have a strong opinion on that, what do you think?

Fair enough, that makes sense. Let's leave it as is.

Sounds good, will give it a try. Doesn't seem to be hard to implement and surely adds value.

Great! I wouldn't spend a lot of time working on this, so only if it turns out to be pretty straightforward.

The screen saver screen is just a completely black screen with no active pixels. This
commit introduces the logic to switch to this "screen saver" screen after 30 seconds
of inactivity in the Signer app. When any of the physical buttons is pressed, the usual
"Signer running..." screen is presented again.
The states UI_TEXT and UI_APPROVAL were never used. This commit removes
them, since there's no good reason to keep these additional states around, along with some elements that are specific to these states.
Added comments and replaced the magic number with a constant
@italo-sampaio italo-sampaio force-pushed the enhancement/add-screen-saver branch from f8ca1e1 to b51008e Compare August 19, 2024 21:22
The next commit will introduce some unit tests for the UX logic. This commit factors out that logic to make it testable.
Adds unit tests to the signer UX logic
@italo-sampaio italo-sampaio force-pushed the enhancement/add-screen-saver branch from b51008e to 50a63e0 Compare August 19, 2024 21:35
@italo-sampaio
Copy link
Collaborator Author

Ready for review!

Tackled all suggestions except for displaying the current signer version. Although feasible, this is not so simple since our current versioning scheme uses hexadecimal constants for the version, and our screens are declared as static const, meaning we need to know string that is shown in that screen at compile time.

To add this feature we would need to either modify the version definition to decimal values or change how our screens are declared (or even add fancy macros to do the conversion). Since this is out of scope and we agreed not to spend too much time on this suggestion, I left this for another time. In any case, let me know if you think it's worth proceeding with it anyways.

Copy link
Collaborator

@amendelzon amendelzon left a comment

Choose a reason for hiding this comment

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

Thanks for tackling all these comments! Made a small comment regarding the unit tests, let's see what we decide on that before moving forwards.

firmware/src/ledger/signer/test/signer_ux/test_signer_ux.c Outdated Show resolved Hide resolved
Copy link
Collaborator

@amendelzon amendelzon left a comment

Choose a reason for hiding this comment

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

LGTM!

Thanks for the changes!

@italo-sampaio italo-sampaio merged commit 6bbebfa into master Aug 21, 2024
6 checks passed
@italo-sampaio italo-sampaio deleted the enhancement/add-screen-saver branch August 21, 2024 12:21
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.

2 participants