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

Model R embedded implementation #2243

Merged
merged 7 commits into from
May 10, 2022
Merged

Model R embedded implementation #2243

merged 7 commits into from
May 10, 2022

Conversation

TychoVrahe
Copy link
Contributor

Implemented basic Model R functionality:

  • display is functional
  • buttons are working
  • USB comm is working, possible to connect to suite and install (custom) firmware
  • shrank loader so it fits the screen, but its not very pretty, probably round loader is not the way
  • backlight implemented using display contrast settings, does not work to zero though.
  • set colors for monochromatic display in bootloader
  • UI needs work, current icons do not fit the display, etc
  • using framebuffer in order to support current display functionality used in bootloader, firmware

(firmware is not implemented for R, works partially when fed model type as 'T' via QSTR, but still for example crashes when trying to create wallet, not sure why, maybe missing touch screen causes problems)

Built on top of #2230

@TychoVrahe TychoVrahe requested a review from matejcik May 2, 2022 09:59
@andrewkozlik andrewkozlik removed their request for review May 2, 2022 10:04
@TychoVrahe TychoVrahe force-pushed the tychovrahe/model_r branch 2 times, most recently from aac84f2 to 50c87a5 Compare May 2, 2022 17:41
@TychoVrahe
Copy link
Contributor Author

Fixed style issues & force pushed

@TychoVrahe
Copy link
Contributor Author

Implemented firmware install confirmation via buttons d60ecb2

@TychoVrahe TychoVrahe force-pushed the tychovrahe/model_r branch from d60ecb2 to 5ab2acc Compare May 5, 2022 13:11
@matejcik
Copy link
Contributor

matejcik commented May 6, 2022

please rebase on current master, merging the base branch did something bad to this PR

#include "touch.h"
#endif
#if defined TREZOR_MODEL_R || defined TREZOR_MODEL_1
Copy link
Contributor

@matejcik matejcik May 6, 2022

Choose a reason for hiding this comment

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

fwiw we don't need to check TREZOR_MODEL_1 here because this code will never run as T1 bootloader.

but it's not breaking anything so 🤷‍♀️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed 572cc5b and 2e8dd0f. Though we still have model 1 variant in bootloader SConscript.

However, this breaks the compilation of bootloader for model 1 variant. Since bootloader is part of build_all and build_embed, this also breaks. Which makes me wanna drop the commits. Or i can change these particular conditionals to not throw "Unknown Trezor model" errors.

Copy link
Contributor

Choose a reason for hiding this comment

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

can change these particular conditionals to not throw "Unknown Trezor model" errors.

that seems like the best option

Copy link
Contributor

Choose a reason for hiding this comment

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

but I'm wondering if we could somehow noop-ize bootloader build for T1. Like, Sconscript.bootloader would just print "not building for T1" instead of doing anything

Copy link
Contributor Author

@TychoVrahe TychoVrahe May 6, 2022

Choose a reason for hiding this comment

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

This can be achieved with this placed at the beginning of sconscript, seems like a better choice then running dummy compilation:

if TREZOR_MODEL in ('1', ):
    # skip bootloader build
    env = Environment()
    def build_bootloader(target,source,env):
        print(f'Bootloader: nothing to build for Model {TREZOR_MODEL}')
    program_bin = env.Command(
                 target='bootloader.bin',
                 source=None,
                 action=build_bootloader
             )
    Return()

We can include this also in boardloader and bootloader_ci to skip those too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed 2ba1b65

@TychoVrahe TychoVrahe force-pushed the tychovrahe/model_r branch from 5ab2acc to 9c00be3 Compare May 6, 2022 12:21
@TychoVrahe
Copy link
Contributor Author

rebased onto master & force pushed

Copy link
Contributor

@matejcik matejcik left a comment

Choose a reason for hiding this comment

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

LGTM but I did not review the display driver (display-stm32_R.h) in too much detail.

@hiviah could you please take a look on that part specifically?

@matejcik matejcik requested a review from hiviah May 6, 2022 13:13
@prusnak prusnak removed their request for review May 6, 2022 17:25
Copy link
Member

@prusnak prusnak left a comment

Choose a reason for hiding this comment

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

Left couple of questions / suggestions ...

@prusnak
Copy link
Member

prusnak commented May 7, 2022

Now thinking more about changes in display_loader ... I think the better approach is the following:

  1. revert changes done to the function
  2. rename core/embed/extmod/modtrezorui/loader.h to core/embed/extmod/modtrezorui/loader_T.h
  3. set outer = 20 and inner = 14 in core/tools/codegen/gen_loader.py and generate loader_R.h using the modified constants
  4. in display.c include either loader_T.hor loader_R.h depending on model
  5. update core/embed/extmod/modtrezorui/display.h so it defines LOADER_ICON_SIZE as 24 for Model R (and keep 64 for Model T)

This approach has the following advantages:

  • zero changes required to already complicated code in display_loader
  • saves some flash space on Model R because the img_loader structure is 9-times smaller
  • possibly nicer render of the loader

@TychoVrahe
Copy link
Contributor Author

Now thinking more about changes in display_loader ... I think the better approach is the following:

Implemented in 55b5a47

The render looks maybe a bit better, but its not a big margin and still likely not good enough for production

Copy link
Member

@prusnak prusnak left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

Copy link
Contributor

@hiviah hiviah left a comment

Choose a reason for hiding this comment

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

LGTM, looked at the display driver more in detail, looks sane, though I couldn't test it on HW.

@matejcik
Copy link
Contributor

matejcik commented May 9, 2022

@TychoVrahe please squash, rebase, fix style checks etc.

@matejcik
Copy link
Contributor

matejcik commented May 9, 2022

please add a changelog entry, something like Added: basic Trezor Model R hardware support.
(see misc/changelog.md for instructions)

@TychoVrahe TychoVrahe force-pushed the tychovrahe/model_r branch from 5c3c686 to 7985c2e Compare May 10, 2022 08:08
@TychoVrahe TychoVrahe force-pushed the tychovrahe/model_r branch from 7985c2e to 57c692f Compare May 10, 2022 08:22
@TychoVrahe TychoVrahe force-pushed the tychovrahe/model_r branch from 57c692f to 6c84fbf Compare May 10, 2022 08:33
@TychoVrahe
Copy link
Contributor Author

Added changelog entries, squashed (still some separate commits that make sense to me, let me know if further squashing is required), force pushed.

@matejcik
Copy link
Contributor

T1 core build is now broken, please fix

@TychoVrahe
Copy link
Contributor Author

T1 core build is now broken, please fix

fixed in 718be20

@matejcik
Copy link
Contributor

everything seems to pass now so please squash the last fixup and let's get this merged

@TychoVrahe TychoVrahe force-pushed the tychovrahe/model_r branch from 718be20 to 68994a5 Compare May 10, 2022 13:31
@TychoVrahe
Copy link
Contributor Author

squashed

@matejcik matejcik merged commit 55bfab3 into master May 10, 2022
@matejcik matejcik deleted the tychovrahe/model_r branch May 10, 2022 14:49
@matejcik
Copy link
Contributor

🎉

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.

4 participants