bootloader refactoring #4572
bootloader refactoring #4572
Conversation
|
TychoVrahe
force-pushed
the
tychovrahe/bootloader/reorg
branch
5 times, most recently
from
bedf3bb to
97363e2
Compare
[no changelog]
[no changelog]
[no changelog]
TychoVrahe
force-pushed
the
tychovrahe/bootloader/reorg
branch
from
f37dfa0 to
8e4557e
Compare
TychoVrahe
requested review from
matejcik,
hiviah and
cepetr
and removed request for
prusnak
| const image_header *const hdr) { | ||
| workflow_reset_jump(); | ||
| ui_set_initial_setup(true); | ||
| ui_screen_connect(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
It seems to me that we cannot call ui_screen_connect() at the beginning of workflow_host_control(), as it doesn’t work well for workflow_empty_device(). I think we can simply call redraw_screen_wait() that will work well for all workflows.
There was a problem hiding this comment.
fixed in 016ac04
| ensure(dont_optimize_out_true * (workflow_is_jump_allowed_1() == | ||
| workflow_is_jump_allowed_2()), | ||
| NULL); | ||
| #ifdef USE_RESET_TO_BOOT |
There was a problem hiding this comment.
Maybe we don’t need to set firmware_jump_fn here, as it is set a bit later, right after the switch statement.
There was a problem hiding this comment.
tried to simplify 387bba0
There was a problem hiding this comment.
It seems to me that now jump_to_fw_through_reset is never called.
There was a problem hiding this comment.
seems to me that we can actually always jump to fw through reset after installation.. which would look like this and is nice simplification: 016ac04
but please check my logic
| secbool send_msg_request_firmware(protob_iface_t *iface, uint32_t offset, | ||
| uint32_t length) { | ||
| if (iface == NULL) { | ||
| return sectrue; |
There was a problem hiding this comment.
The recv_msg_xxx() functions return secfalse if iface == NULL, while the send_msg_xxx() function returns sectrue for the same condition. Shouldn’t we unify this?
There was a problem hiding this comment.
unified at sectrue - as i am using NULL for workflow run from menu, where no communication is needed 3fecd5e
There was a problem hiding this comment.
As we discussed in person, we could remove all if (iface == NULL) conditions and introduce a “null/blackhole” interface instead.
There was a problem hiding this comment.
here: 372eae4
not entirely satisfied though. seems like wire is the wrong layer, the protob iface should actually be the null one, but its api isn't really ready for that.
| // read function pointer | ||
| int (*read)(uint8_t *buffer, size_t buffer_size); | ||
|
|
||
| void (*error)(void); |
There was a problem hiding this comment.
It looks like the error handler is called by the codec in the event of a fatal communication error. Shouldn’t the codec signal the error to a higher level instead? It seems to me that the error handler is compensating for something missing between the codec and the state machines above.
There was a problem hiding this comment.
basically we use this for RSOD when there is such communication error. We could signal this above but then we need to add the RSOD everywhere where there is receiving a message, which seems just work with little effect.
But i agree that we will need to rethink this - but lets postpone it until we add BLE to bootloader, as RSOD on BLE comm error might be a bit too much, so lets handle this in BLE context in separate PR.
This PR implements major bootloader refactoring - as preparation for adding BLE.