software_dfu #46

Merged
sb10q merged 8 commits from software_dfu into master 2021-01-13 11:59:06 +08:00
4 changed files with 29 additions and 30 deletions
Showing only changes of commit 2e6116016a - Show all commits

View File

@ -124,7 +124,7 @@ formatted as line-delimited JSON.
| `load [0/1]` | Restore configuration for channel all/0/1 from flash | | `load [0/1]` | Restore configuration for channel all/0/1 from flash |
| `save [0/1]` | Save configuration for channel all/0/1 to flash | | `save [0/1]` | Save configuration for channel all/0/1 to flash |
| `reset` | Reset the device | | `reset` | Reset the device |
| `dfu` | Reset device and enters USB device firmware update (DFU) mode | | `dfu` | Reset device and enters USB device firmware update (DFU) mode |
Outdated
Review

Table formatting does not seem OK.

Table formatting does not seem OK.
| `ipv4 <X.X.X.X/L> [Y.Y.Y.Y]` | Configure IPv4 address, netmask length, and optional default gateway | | `ipv4 <X.X.X.X/L> [Y.Y.Y.Y]` | Configure IPv4 address, netmask length, and optional default gateway |

View File

@ -3,12 +3,13 @@ MEMORY
FLASH (rx) : ORIGIN = 0x8000000, LENGTH = 1024K FLASH (rx) : ORIGIN = 0x8000000, LENGTH = 1024K
/* reserved for config data */ /* reserved for config data */
CONFIG (rx) : ORIGIN = 0x8100000, LENGTH = 16K CONFIG (rx) : ORIGIN = 0x8100000, LENGTH = 16K
RAM (xrw) : ORIGIN = 0x20000000, LENGTH = 111K RAM (xrw) : ORIGIN = 0x20000000, LENGTH = 112K - 4
/* reserved for DFU trigger message */ /* reserved for DFU trigger message */
DFU_MSG (wrx) : ORIGIN = 0x2001BC00, LENGTH = 1K DFU_MSG (wrx) : ORIGIN = 0x2001BFFC, LENGTH = 4
Outdated
Review

The length should be 4 and not 1K AFAICT?

The length should be 4 and not 1K AFAICT?
RAM2 (xrw) : ORIGIN = 0x2001C000, LENGTH = 16K RAM2 (xrw) : ORIGIN = 0x2001C000, LENGTH = 16K
RAM3 (xrw) : ORIGIN = 0x20020000, LENGTH = 64K RAM3 (xrw) : ORIGIN = 0x20020000, LENGTH = 64K
CCMRAM (rw) : ORIGIN = 0x10000000, LENGTH = 64K CCMRAM (rw) : ORIGIN = 0x10000000, LENGTH = 64K
} }
_stack_start = ORIGIN(CCMRAM) + LENGTH(CCMRAM); _stack_start = ORIGIN(CCMRAM) + LENGTH(CCMRAM);
_dfu_msg = ORIGIN(DFU_MSG);

View File

@ -1,28 +1,25 @@
use cortex_m_rt::{pre_init}; use cortex_m_rt::{pre_init};
/// RAM location used to store DFU trigger message const DFU_TRIG_MSG: u32 = 0xDECAFBAD;
const DFU_MSG_ADDR: usize = 0x2001BC00;
Outdated
Review
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

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.
Outdated
Review

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.
Outdated
Review

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.
Outdated
Review

The linker script is the second link I gave you.

The linker script is the second link I gave you.

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.

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.
Outdated
Review

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.

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.

@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.

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.
Outdated
Review

@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``?
/// DFU trigger message pub unsafe fn set_dfu_trigger() {
const DFU_TRIG_MSG: usize = 0xDECAFBAD; extern "C" {
Outdated
Review

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?
static mut _dfu_msg: u32;
/// Set DFU trigger }
pub unsafe fn trig_dfu() { _dfu_msg = DFU_TRIG_MSG;
let dfu_msg_addr = DFU_MSG_ADDR as *mut usize;
*dfu_msg_addr = DFU_TRIG_MSG;
} }
/// Called by reset handler in lib.rs immediately after reset, checks if booting into dfu is needed /// Called by reset handler in lib.rs immediately after reset.
/// This function should not be called outside of reset handler as
/// bootloader expects MCU to be in reset state when called.
#[pre_init] #[pre_init]
unsafe fn __pre_init() { unsafe fn __pre_init() {
extern "C" {
Outdated
Review

see comment above

see comment above
static mut _dfu_msg: u32;
Outdated
Review

remove blank line

remove blank line
}
let dfu_msg_addr = DFU_MSG_ADDR as *mut usize; if _dfu_msg == DFU_TRIG_MSG {
_dfu_msg = 0x00000000;
Outdated
Review

space before {

space before {
// Check DFU trigger message
if *dfu_msg_addr == DFU_TRIG_MSG{
// Reset message
*dfu_msg_addr = 0x00000000;
Review

remove blank line

remove blank line
// Enable system config controller clock // Enable system config controller clock
const RCC_APB2ENR: *mut u32 = 0xE000_ED88 as *mut u32; const RCC_APB2ENR: *mut u32 = 0xE000_ED88 as *mut u32;
@ -41,14 +38,14 @@ unsafe fn __pre_init() {
SYSCFG_MEMRMP, SYSCFG_MEMRMP,
*SYSCFG_MEMRMP | SYSCFG_MEMRMP_MAP_ROM, *SYSCFG_MEMRMP | SYSCFG_MEMRMP_MAP_ROM,
); );
// Set stack pointer to bootloader location asm!(
asm!("LDR R0, =0x1FFF0000"); // Set stack pointer to bootloader location
asm!("LDR SP,[R0, #0]"); "LDR R0, =0x1FFF0000",
"LDR SP,[R0, #0]",
// Jump to bootloader // Jump to bootloader
asm!("LDR R0,[R0, #4]"); "LDR R0,[R0, #4]",
asm!("BX R0"); "BX R0",
);
} }
} }
Outdated
Review

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

View File

@ -429,8 +429,9 @@ fn main() -> ! {
channels.power_down(i); channels.power_down(i);
} }
unsafe { unsafe {
dfu::trig_dfu(); dfu::set_dfu_trigger();
} }
socket.close();
Outdated
Review

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.

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.
SCB::sys_reset(); SCB::sys_reset();
} }
} }