M-Labs
/
thermostat
9
0
Fork 5
Code Issues 9 Pull Requests 5 Releases Wiki Activity

software_dfu #46

Merged
sb10q merged 8 commits from software_dfu into master 2021-01-13 11:59:06 +08:00
Conversation 5 Commits 8 Files Changed 5 +68 -3
topquark12 commented 2021-01-12 15:39:22 +08:00
Contributor
Copy Link

DFU can be triggered by command over network.

DFU can be triggered by command over network.
🎉 1
topquark12 added 4 commits 2021-01-12 15:39:23 +08:00
topquark12 added 1 commit 2021-01-12 15:47:00 +08:00
sb10q reviewed 2021-01-12 16:34:25 +08:00
README.md Outdated
@ -124,6 +124,7 @@ formatted as line-delimited JSON.
| `load [0/1]` | Restore configuration for channel all/0/1 from flash |
| `save [0/1]` | Save configuration for channel all/0/1 to flash |
| `reset` | Reset the device |
| `dfu` | Reset device and enters USB device firmware update (DFU) mode |
sb10q commented 2021-01-12 16:34:25 +08:00
Owner
Copy Link

Table formatting does not seem OK.

Table formatting does not seem OK.
sb10q reviewed 2021-01-12 16:35:52 +08:00
src/dfu.rs Outdated
@ -0,0 +48,4 @@
// Jump to bootloader
asm!("LDR R0,[R0, #4]");
asm!("BX R0");
sb10q commented 2021-01-12 16:35:52 +08:00
Owner
Copy Link

You can put all these lines into a single asm! macro, see https://doc.rust-lang.org/nightly/unstable-book/library-features/asm.html

You can put all these lines into a single ``asm!`` macro, see https://doc.rust-lang.org/nightly/unstable-book/library-features/asm.html
sb10q reviewed 2021-01-12 16:36:16 +08:00
src/dfu.rs Outdated
@ -0,0 +15,4 @@
/// Called by reset handler in lib.rs immediately after reset, checks if booting into dfu is needed
#[pre_init]
unsafe fn __pre_init() {
sb10q commented 2021-01-12 16:36:16 +08:00
Owner
Copy Link

remove blank line

remove blank line
sb10q reviewed 2021-01-12 16:36:38 +08:00
src/dfu.rs Outdated
@ -0,0 +19,4 @@
let dfu_msg_addr = DFU_MSG_ADDR as *mut usize;
// Check DFU trigger message
if *dfu_msg_addr == DFU_TRIG_MSG{
sb10q commented 2021-01-12 16:36:38 +08:00
Owner
Copy Link

space before {

space before {
sb10q reviewed 2021-01-12 16:36:44 +08:00
src/dfu.rs
@ -0,0 +20,4 @@
// Check DFU trigger message
if *dfu_msg_addr == DFU_TRIG_MSG{
sb10q commented 2021-01-12 16:36:44 +08:00
Owner
Copy Link

remove blank line

remove blank line
sb10q reviewed 2021-01-12 16:39:28 +08:00
src/dfu.rs Outdated
@ -0,0 +1,54 @@
use cortex_m_rt::{pre_init};
/// RAM location used to store DFU trigger message
const DFU_MSG_ADDR: usize = 0x2001BC00;
sb10q commented 2021-01-12 16:39:28 +08:00
Owner
Copy Link

This can and should be automatically obtained via a linker symbol. See this example:
https://git.m-labs.hk/M-Labs/artiq-zynq/src/branch/master/src/runtime/src/kernel/core1.rs#L31-L36
https://git.m-labs.hk/M-Labs/artiq-zynq/src/branch/master/src/runtime/link.x#L10

This can and should be automatically obtained via a linker symbol. See this example: https://git.m-labs.hk/M-Labs/artiq-zynq/src/branch/master/src/runtime/src/kernel/core1.rs#L31-L36 https://git.m-labs.hk/M-Labs/artiq-zynq/src/branch/master/src/runtime/link.x#L10
topquark12 commented 2021-01-12 16:59:47 +08:00
Author
Contributor
Copy Link

The linker currently does not actually do anything to the ram carved out for the dfu trigger message, and I beleive there's no symbol generated for that chunk of memory.

I think I'll have to find the linker script the project is using, fork a copy, generate a symbol for that addr and use it in our project somehow.

The linker currently does not actually do anything to the ram carved out for the dfu trigger message, and I beleive there's no symbol generated for that chunk of memory. I think I'll have to find the linker script the project is using, fork a copy, generate a symbol for that addr and use it in our project somehow.
sb10q commented 2021-01-12 17:02:53 +08:00
Owner
Copy Link

Indeed, but the linker script can be modified so that such a symbol is generated. This way there isn't any value that needs to be manually synchronized in two places. You may not even need to hardcode the address; the linker could place that variable automatically.

Indeed, but the linker script can be modified so that such a symbol is generated. This way there isn't any value that needs to be manually synchronized in two places. You may not even need to hardcode the address; the linker could place that variable automatically.
sb10q commented 2021-01-12 17:04:05 +08:00
Owner
Copy Link

Though hardcoding is actually more reliable since after a firmware update the address could have changed otherwise... add a comment that explains that.

Though hardcoding is actually more reliable since after a firmware update the address could have changed otherwise... add a comment that explains that.
sb10q commented 2021-01-12 17:04:35 +08:00
Owner
Copy Link

The linker script is the second link I gave you.

The linker script is the second link I gave you.
pca006132 commented 2021-01-12 17:09:07 +08:00
First-time contributor
Copy Link

I think specifying a region that only contains the DFU msg would be reasonable. This way the address would not be changed after update.

I think specifying a region that only contains the DFU msg would be reasonable. This way the address would not be changed after update.
topquark12 commented 2021-01-12 17:10:19 +08:00
Author
Contributor
Copy Link

I thought that linker script is for a zynq? I am not sure where the linker script currently being used is placed, think it is packaged with one of the rust modules.

I thought that linker script is for a zynq? I am not sure where the linker script currently being used is placed, think it is packaged with one of the rust modules.
sb10q commented 2021-01-12 17:11:57 +08:00
Owner
Copy Link

Okay, you mean the thermostat linker script.
That memory.x you modified can be used, as you can see it already defines the _stack_start symbol.

Okay, you mean the thermostat linker script. That ``memory.x`` you modified can be used, as you can see it already defines the ``_stack_start`` symbol.
topquark12 commented 2021-01-12 17:19:12 +08:00
Author
Contributor
Copy Link

The stack is on CCM, the message is placed in RAM. I can try placing the msg in CCM, not sure what the reset behavior of CCM is yet.

The stack is on CCM, the message is placed in RAM. I can try placing the msg in CCM, not sure what the reset behavior of CCM is yet.
topquark12 commented 2021-01-12 17:22:56 +08:00
Author
Contributor
Copy Link

@pca006132 There's already a dedicated region specified in memory.x . Though the linker does not do anything with that region, and I don't think the linker generates a symbol for that space.

I also haven't found the linker script of the project just yet.

@pca006132 There's already a dedicated region specified in memory.x . Though the linker does not do anything with that region, and I don't think the linker generates a symbol for that space. I also haven't found the linker script of the project just yet.
pca006132 commented 2021-01-12 17:35:08 +08:00
First-time contributor
Copy Link

The linker script is defined in cortex-m-rt, https://github.com/rust-embedded/cortex-m-rt/blob/master/link.x.in
But I'm not familiar with that, not sure how to add new symbols.

The linker script is defined in `cortex-m-rt`, https://github.com/rust-embedded/cortex-m-rt/blob/master/link.x.in But I'm not familiar with that, not sure how to add new symbols.
sb10q commented 2021-01-12 17:37:22 +08:00
Owner
Copy Link

@topquark12 what's wrong with simply adding

_dfu_msg = ORIGIN(DFU_MSG)

at the end of thermostat's memory.x?

@topquark12 what's wrong with simply adding ``` _dfu_msg = ORIGIN(DFU_MSG) ``` at the end of thermostat's ``memory.x``?
sb10q reviewed 2021-01-12 16:41:34 +08:00
src/dfu.rs Outdated
@ -0,0 +50,4 @@
asm!("LDR R0,[R0, #4]");
asm!("BX R0");
}
sb10q commented 2021-01-12 16:41:34 +08:00
Owner
Copy Link

remove blank line

remove blank line
sb10q commented 2021-01-12 16:43:14 +08:00
Owner
Copy Link

What happens to the TCP connection after a dfu command is sent?

What happens to the TCP connection after a ``dfu`` command is sent?
sb10q commented 2021-01-12 17:06:23 +08:00
Owner
Copy Link

Why do we need a reset? What happens if we jump to the bootloader directly from the command handler in main.rs instead?

Why do we need a reset? What happens if we jump to the bootloader directly from the command handler in ``main.rs`` instead?
topquark12 commented 2021-01-12 17:12:31 +08:00
Author
Contributor
Copy Link

Why do we need a reset? What happens if we jump to the bootloader directly from the command handler in main.rs instead?

The STM32 bootloader expects the chip to be in its default state when ran. Some of the peripherials the application uses will mess with the bootloader communication protocol selection process.

> Why do we need a reset? What happens if we jump to the bootloader directly from the command handler in ``main.rs`` instead? The STM32 bootloader expects the chip to be in its default state when ran. Some of the peripherials the application uses will mess with the bootloader communication protocol selection process.
topquark12 commented 2021-01-12 17:17:31 +08:00
Author
Contributor
Copy Link

What happens to the TCP connection after a dfu command is sent?

Currently the other end will have to terminate the connection. I've just copied what the reset command does.

I can look into having the thermostat cut the connection.

> What happens to the TCP connection after a ``dfu`` command is sent? Currently the other end will have to terminate the connection. I've just copied what the reset command does. I can look into having the thermostat cut the connection.
sb10q commented 2021-01-12 17:32:29 +08:00
Owner
Copy Link

The STM32 bootloader expects the chip to be in its default state when ran. Some of the peripherials the application uses will mess with the bootloader communication protocol selection process.

Please add a source code comment that explains that. Such a comment (like the one about avoiding the address change after firmware update) is much more important and relevant than comments like e.g. "Set DFU trigger" which is easily guessed, and could be avoided by renaming the corresponding function set_dfu_trigger().

I can look into having the thermostat cut the connection.

Yes, the thermostat should at least send one RST before resetting. I'll open an issue.

> The STM32 bootloader expects the chip to be in its default state when ran. Some of the peripherials the application uses will mess with the bootloader communication protocol selection process. Please add a source code comment that explains that. Such a comment (like the one about avoiding the address change after firmware update) is much more important and relevant than comments like e.g. "Set DFU trigger" which is easily guessed, and could be avoided by renaming the corresponding function ``set_dfu_trigger()``. > I can look into having the thermostat cut the connection. Yes, the thermostat should at least send one RST before resetting. I'll open an issue.
sb10q reviewed 2021-01-12 17:37:57 +08:00
memory.x Outdated
@ -6,1 +6,3 @@
RAM (xrw) : ORIGIN = 0x20000000, LENGTH = 112K
RAM (xrw) : ORIGIN = 0x20000000, LENGTH = 111K
/* reserved for DFU trigger message */
DFU_MSG (wrx) : ORIGIN = 0x2001BC00, LENGTH = 1K
sb10q commented 2021-01-12 17:37:57 +08:00
Owner
Copy Link

The length should be 4 and not 1K AFAICT?

The length should be 4 and not 1K AFAICT?
topquark12 added 1 commit 2021-01-13 11:06:15 +08:00
sb10q reviewed 2021-01-13 11:07:37 +08:00
src/dfu.rs Outdated
@ -0,0 +3,4 @@
const DFU_TRIG_MSG: u32 = 0xDECAFBAD;
pub unsafe fn set_dfu_trigger() {
extern "C" {
sb10q commented 2021-01-13 11:07:37 +08:00
Owner
Copy Link

Why not put it outside the functions so this declaration can be shared?

Why not put it outside the functions so this declaration can be shared?
sb10q reviewed 2021-01-13 11:08:24 +08:00
src/dfu.rs Outdated
@ -0,0 +14,4 @@
/// bootloader expects MCU to be in reset state when called.
#[pre_init]
unsafe fn __pre_init() {
extern "C" {
sb10q commented 2021-01-13 11:08:24 +08:00
Owner
Copy Link

see comment above

see comment above
sb10q reviewed 2021-01-13 11:09:34 +08:00
src/main.rs Outdated
@ -427,0 +431,4 @@
unsafe {
dfu::set_dfu_trigger();
}
socket.close();
sb10q commented 2021-01-13 11:09:34 +08:00
Owner
Copy Link

Does this work or do we need to wait for the network driver/hardware to actually send the packet?
What if there are other open sockets? Maybe address this entire issue in a second PR.

Does this work or do we need to wait for the network driver/hardware to actually send the packet? What if there are other open sockets? Maybe address this entire issue in a second PR.
topquark12 commented 2021-01-13 11:13:48 +08:00
Author
Contributor
Copy Link

was just testing out the behavior of that, forgot to remove before committing.

was just testing out the behavior of that, forgot to remove before committing.
topquark12 added 1 commit 2021-01-13 11:17:05 +08:00
topquark12 added 1 commit 2021-01-13 11:50:26 +08:00
sb10q merged commit f6802635a4 into master 2021-01-13 11:59:06 +08:00
topquark12 deleted branch software_dfu 2021-01-13 12:12:43 +08:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No Label
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: M-Labs/thermostat#46
No description provided.
Delete Branch "software_dfu"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?