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

chore(core): enable reboot-to-bootloader without experimental features #2841

Merged
merged 1 commit into from
Feb 22, 2023

Conversation

grdddj
Copy link
Contributor

@grdddj grdddj commented Feb 21, 2023

Fixes #2316:

  • not needing experimental features to trigger reboot-to-bootloader functionality

@grdddj grdddj self-assigned this Feb 21, 2023
@@ -0,0 +1 @@
Reboot to bootloader does not require experimental features
Copy link
Contributor

Choose a reason for hiding this comment

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

changelog-wise, this is a "new" feature. so please move this to added and modify the text accordingly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in the force-push

@grdddj grdddj force-pushed the grdddj/enable_reboot_to_bootloader branch from 6c50166 to 92fbdcc Compare February 21, 2023 14:54
@grdddj grdddj merged commit 107a7c9 into master Feb 22, 2023
@grdddj grdddj deleted the grdddj/enable_reboot_to_bootloader branch February 22, 2023 09:21
@STew790
Copy link

STew790 commented Mar 1, 2023

QA STATUS
I can use trezorctl device reboot-to-bootloader without enabling the experimental features and the device correctly shows the prompt but after the reboot the device boots to normal state and not to bootloader. If I hold my finger over the screen though, it boots to bootloader. Is this the correct behaviour? cc @matejcik probably

Info:

  • Suite version: desktop 23.3.1 (8f0a5a67bd1cd0cf95724ee1468ee6ad70aead60)
  • Browser: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) TrezorSuite/23.3.1 Chrome/104.0.5112.124 Electron/20.3.12 Safari/537.36
  • OS: Linux x86_64
  • Screen: 1920x1080
  • Device: model T 2.5.4 Universal (revision 557e297)
  • Transport: BridgeTransport 2.0.32

image

@Hannsek
Copy link
Contributor

Hannsek commented Mar 2, 2023

Can we change the copy on that screen? 😅

@Hannsek
Copy link
Contributor

Hannsek commented Mar 23, 2023

@STew790 You'll need to wait a bit longer, we are waiting for another pr to merge.

Answer to myself: Yes, we can change it. 😄

@Hannsek
Copy link
Contributor

Hannsek commented Mar 26, 2023

After releasing this, when users will want to update fw, they click on "update fw" in suite and they will see this screen. Is that right @grdddj? Do we need the confirmation from the user?

@grdddj
Copy link
Contributor Author

grdddj commented Mar 26, 2023

After releasing this, when users will want to update fw, they click on "update fw" in suite and they will see this screen. Is that right @grdddj? Do we need the confirmation from the user?

No idea, I do not know anything about reboot-to-bootloader functionality, but your suggestion makes sense

@Hannsek
Copy link
Contributor

Hannsek commented Mar 26, 2023

@matejcik maybe?

@matejcik
Copy link
Contributor

After releasing this, when users will want to update fw, they click on "update fw" in suite and they will see this screen. Is that right @grdddj? Do we need the confirmation from the user?

not sure what Suite does, but presumably there will be something about "confirm rebooting to bootloader on your Trezor", and Trezor will show this screen.

and yes, we need user confirmation.

@Hannsek
Copy link
Contributor

Hannsek commented Mar 27, 2023

Why do we need it? I don't like the fact that the process of updating takes at least 3 clicks on Trezor screen.

+ users don't know what firmware is (and they don't need to know), let alone bootloader…

@prusnak
Copy link
Member

prusnak commented Mar 27, 2023

Why do we need it? I

Hannsek has a good point here. If every operation in the bootloader requires active user interaction, then we might as well go to bootloader with no confirmation (maybe just a countdown?), because nothing can be done there without user confirming it first. Or are there reasons I don't see?

@matejcik
Copy link
Contributor

matejcik commented Mar 27, 2023

In that case i'd much rather remove the initial confirmation from bootloader side than before executing the jump. I don't have a specific argument, but dropping the user to bootloader without any interaction reeks of badness.

it would make the UX flow make more sense too, IMHO: change wording to something like "proceed to firmware installation? y/n", then jump to bootloader blue screen "waiting for host", and then install.

while we're thinking about this, we could allow interaction-less upgrade, i.e., skip the "really install firmware by SatoshiLabs version 2.99" dialog, if (a) the vendor is unchanged, and (b) it is a strict upgrade, and (c) click flag is not set in the vendor header
i.e.:

  • reinstall of same version still gets a confirmation
  • downgrade gets confirmation
  • vendor change gets confirmation, obviously
  • any operation with devel firmware gets confirmation

then you would, in firmware, click Yes on "proceed to installing firmware?", Trezor screen goes blue, waits for host, straight to progress bar -> reboot -> you're done

@Hannsek
Copy link
Contributor

Hannsek commented Mar 27, 2023

Hmm good idea, the less click for user the better.
What is your time estimate?

In that case, we don’t have to do the happy path in bootloader blue. So the user won’t even see that he is in boot loader. I.e. waiting for host screen and progress bar would be black.

@matejcik
Copy link
Contributor

Hmm good idea, the less click for user the better. What is your time estimate?

It's pretty easy actually. Skipping the "install firmware" button when coming from firmware is a matter of two or so lines. Finding the upgrade happy path is also easy to code, but I'd like to go through the implications better, so I'd defer that to the next bootloader release (along with #2794 hopefully)

In that case, we don’t have to do the happy path in bootloader blue. So the user won’t even see that he is in boot loader. I.e. waiting for host screen and progress bar would be black.

Doable, but no point in doing it imho. The firmware upgrade process is special.

@Hannsek
Copy link
Contributor

Hannsek commented Mar 28, 2023

I agree. I will create another issue after thinking it through.

@bosomt
Copy link

bosomt commented Mar 30, 2023

Tested yesterday with bootloader_T2T1_qa-2.1.0.bin via trezorctl device reboot-to-bootloader and it worked as expected 🎉

I will retest with final version

@Hannsek
Copy link
Contributor

Hannsek commented Mar 30, 2023

Can we test this also with Suite?

@bosomt
Copy link

bosomt commented Mar 30, 2023

I think that we need to ask Suite developers to implement reboot to bootloader feature for TT.
@mroz22 right ? Do you want me to send Trezor Suite issue/feature request or its already covered?

@Hannsek
Copy link
Contributor

Hannsek commented Mar 30, 2023

@mroz22 is not here this week. So probably @marekrjpolak. cc @matejkriz

@matejkriz
Copy link
Member

@bosomt Please test with Suite that nothing has changed and user is able to do the FW upgrade old way (with unplugging).

Feature request on Suite to be specified and prioritized trezor/trezor-suite#7961

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Enable jump-to-bootloader on TT with release of new bootloader
7 participants