bootloader refactoring #4572

TychoVrahe · 2025-02-04T09:42:46Z

This PR implements major bootloader refactoring - as preparation for adding BLE.

overall structure should be close to how things are done in firmware/micropython
cleaner separation of communication layers
introducing workflows, separate of communication
workflows run from menu use same code as workflows triggered by communication
introducing unified polling mechanism
some error handling in communication is improved/introduced

github-actions · 2025-02-04T09:44:12Z

core UI changes	device test	click test	persistence test
T2T1 Model T	test(screens) main(screens)	test(screens) main(screens)	test(screens) main(screens)
T3B1 Safe 3	test(screens) main(screens)	test(screens) main(screens)	test(screens) main(screens)
T3T1 Safe 5	test(screens) main(screens)	test(screens) main(screens)	test(screens) main(screens)
All	main(screens)

[no changelog]

core/embed/projects/bootloader/main.c

hiviah

Would be nicer without the hardcoded ui_result constants in workflow_bootloader, but they were there before he refactor.

cepetr · 2025-02-11T10:00:10Z

core/embed/projects/bootloader/main.c

@@ -64,27 +57,16 @@
 #include <sec/hash_processor.h>
 #endif

-#include <io/usb.h>
-#include "version.h"
+#include <workflow/workflow.h>


this should be included as "workflow/worfklow.h", since it's inside the bootloader folder

cepetr · 2025-02-11T10:15:52Z

core/embed/projects/bootloader/bootui.h

+  UI_RESULT_CANCEL = 1,
+  UI_RESULT_CONFIRM = 2,
+} ui_result_t;
+
 typedef enum {
  SCREEN_INTRO = 0,


This enum describes the states of the workflow in wf_bootloader.c and might belong in this file. Some of the enum entries are unused.

cepetr · 2025-02-11T10:37:46Z

core/embed/projects/bootloader/workflow/wf_auto_update.c

+                                       const image_header *const hdr) {
+  workflow_reset_jump();
+  ui_set_initial_setup(true);
+  ui_screen_connect();


If we called redraw_screen_wait() at the beginning of workflow_host_control(), we would not need to call ui_screen_connect() here. The same applies to workflow_bootloader and workflow_empty_device.

cepetr · 2025-02-11T10:43:17Z

core/embed/projects/bootloader/workflow/wf_auto_update.c

+
+workflow_result_t workflow_auto_update(const vendor_header *const vhdr,
+                                       const image_header *const hdr) {
+  workflow_reset_jump();


Maybe it would be better/safer to call workflow_reset_jump() outside this function - in the loop in main, just after do {…. Then we can remove this call from workflow_bootloader and workflow_empty_device as well.

cepetr · 2025-02-11T10:47:32Z

core/embed/projects/bootloader/workflow/workflow_internal.h

+
+#include "workflow.h"
+
+void workflow_allow_jump_1(void);


These names are a bit confusing to me since the logic inside isn’t a workflow.

cepetr · 2025-02-11T10:55:29Z

core/embed/projects/bootloader/poll.c

+                    poll_event_t* event, uint32_t timeout_ms) {
+  uint32_t start = systick_ms();
+
+  while (systick_ms() - start < timeout_ms) {


How about using the pattern below? It inherently solves the uint32 overflow problem.

uint32_t deadline = ticks_timeout(timeout_ms); while (! ticks_expired(deadline)) { ...

..

cepetr · 2025-02-11T14:45:06Z

core/embed/projects/bootloader/main.c

+        ensure(dont_optimize_out_true * (workflow_is_jump_allowed_1() ==
+                                         workflow_is_jump_allowed_2()),
+               NULL);
+#ifdef USE_RESET_TO_BOOT


Maybe we don’t need to set firmware_jump_fn here, as it is set a bit later, right after the switch statement.

cepetr · 2025-02-11T14:50:32Z

core/embed/projects/bootloader/main.c

-        firmware_jump_fn = real_jump_to_firmware;
-#endif
-        break;
+        return 1;


Instead of returning here, shouldn’t we call eboot_or_halt_after_rsod() or shutdown_error() ? I think this return will result in showing the RSOD with the “EXIT 1” text.

cepetr · 2025-02-11T15:12:28Z

core/embed/projects/bootloader/workflow/workflow.h

+  WF_SHUTDOWN = 0x11223344,
+  WF_CONTINUE_TO_FIRMWARE = 0xAABBCCDD,
+  WF_RETURN = 0x55667788,
+  WF_STAY = 0xEEFF0011,


The meaning of the result constants is a bit unclear to me. It would feel more natural to have results that describe what happened in that workflow, allowing the caller to decide what to do. For example:

WF_ERROR- Shutdown without overwriting the display.

WF_FATAL_ERROR - Show a fatal error message, then shut down.

WF_OK - Workflow successfully finished (can replace WF_RETURN and WF_CONTINUE_TO_FIRMWARE).

cepetr · 2025-02-11T15:16:51Z

core/embed/projects/bootloader/main.c

      }
+    } while (result == WF_STAY || result == WF_RETURN);


It seems to me that the do…while loop here is unnecessary since it is already incorporated into all workflows it calls. These workflows never return WF_STAY nor WF_RETURN.

cepetr · 2025-02-11T15:36:56Z

core/embed/projects/bootloader/workflow/wf_bootloader.c

+        break;
+      case SCREEN_MENU:
+        ui_result = ui_screen_menu(firmware_present);
+        if (ui_result == 0xAABBCCDD) {  // exit menu


These constants (0xAABBCCDD...) are the same as the workflow_result_t constants, which feels a bit strange. Shouldn’t we use WF_xxx here instead?

TychoVrahe self-assigned this Feb 4, 2025

TychoVrahe force-pushed the tychovrahe/bootloader/reorg branch 5 times, most recently from bedf3bb to 97363e2 Compare February 6, 2025 15:14

TychoVrahe added 4 commits February 7, 2025 21:51

refactor(core): complete bootloader refactoring

337d4da

fix(core): fix emulator usb deinitialization

8913d03

[no changelog]

fix(core): fix emulator usb polling

71863fc

[no changelog]

fix(core): handle communication errors in bootloader

8e4557e

[no changelog]

TychoVrahe force-pushed the tychovrahe/bootloader/reorg branch from f37dfa0 to 8e4557e Compare February 7, 2025 20:51

TychoVrahe marked this pull request as ready for review February 9, 2025 20:56

TychoVrahe requested a review from prusnak as a code owner February 9, 2025 20:56

TychoVrahe requested review from matejcik, hiviah and cepetr and removed request for prusnak February 9, 2025 20:56

hiviah reviewed Feb 10, 2025

View reviewed changes

core/embed/projects/bootloader/main.c Show resolved Hide resolved

hiviah approved these changes Feb 10, 2025

View reviewed changes

cepetr requested changes Feb 11, 2025

View reviewed changes

fixup! refactor(core): complete bootloader refactoring

d35437d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bootloader refactoring #4572

bootloader refactoring #4572

TychoVrahe commented Feb 4, 2025 •

edited

Loading

github-actions bot commented Feb 4, 2025 •

edited

Loading

hiviah left a comment

cepetr Feb 11, 2025

cepetr Feb 11, 2025

cepetr Feb 11, 2025

cepetr Feb 11, 2025

cepetr Feb 11, 2025

cepetr Feb 11, 2025

cepetr Feb 11, 2025

cepetr Feb 11, 2025

cepetr Feb 11, 2025

cepetr Feb 11, 2025

cepetr Feb 11, 2025

bootloader refactoring #4572

Are you sure you want to change the base?

bootloader refactoring #4572

Conversation

TychoVrahe commented Feb 4, 2025 • edited Loading

github-actions bot commented Feb 4, 2025 • edited Loading

hiviah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TychoVrahe commented Feb 4, 2025 •

edited

Loading

github-actions bot commented Feb 4, 2025 •

edited

Loading