-
Notifications
You must be signed in to change notification settings - Fork 3
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
Soft 468 jumping back to bootloader #459
base: master
Are you sure you want to change the base?
Conversation
…s://github.com/uw-midsun/firmware_xiv into soft_468_jumping_back_to_bootloader
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good, I'll test this in a bit, and I've left some action items in the comments.
|
||
// Reset the vector table to bootloader's vector table | ||
SYSCFG_MemoryRemapConfig( | ||
SYSCFG_MemoryRemap_SRAM); // change where the vector table is stored to beginning of flash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will set the vector table to the beginning of SRAM (which is where it is already since we're in the application) - you'll need to use SYSCFG_MemoryRemap_Flash
instead (see stm32f0xx_syscfg.h).
static noreturn __attribute__((naked)) void prv_perform_jump(uint32_t sp, uint32_t pc) { | ||
__asm( | ||
"msr msp, %[sp] \n" // reset the main stack pointer (msp) to sp | ||
"bx %[pc] \n" // jump to pc | ||
|
||
// this bizarre syntax associates the "sp" and "pc" in asm with the sp and pc parameters | ||
// see http://www.ethernut.de/en/documents/arm-inline-asm.html | ||
: | ||
: [sp] "r"(sp), [pc] "r"(pc)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might want to factor this function out from this and jump_to_application.c into an ms-common (or ms-bootloader) library called jump.h
since it just jumps to a particular location.
@@ -8,6 +8,7 @@ | |||
#include "flash.h" | |||
#include "interrupt.h" | |||
#include "jump_to_application.h" | |||
#include "jump_to_bootloader.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the next steps for this module is that we want to jump from the bootloader to the application when we receive a datagram message while in an application. This will be in can_fsm.c, in the if (rx_msg.source_id == SYSTEM_CAN_DEVICE_BOOTLOADER)
if statement. Essentially, when that if statement is reached:
- if we're in an application running on the bootloader, in which case we should jump to the bootloader so that the bootloader can receive subsequent datagram messages
- oherwise, we should call
bootloader_can_receive
there like we do now so that the bootloader can process the message (if necessary)
So there are a couple of parts to doing this:
- We'll need to move
jump_to_bootloader
to a library, perhaps calledms-bootloader
, so can_fsm.c can reach it. Perhaps move bootloader_mcu.h to that library too. - We'll need a way to determine which of the three scenarios above we're in. This is a bit tricky: we'll need a preprocessor symbol that's defined when we're compiling for an application running on the bootloader. You can do this by adding a
-D<symbolname>
argument to the C compiler flags when compiling for the application, which currently you can do in platform/stm32f0xx/platform.mk by addingCFLAGS += -D<symbolname>
to theifeq ($(MAKECMDGOALS),temp-bootloader-write)
if statement (replace<symbolname>
with the symbol you want defined when on the application only). Then you can test with#ifdef
in can_fsm.c.
…pps on bootloader
…s://github.com/uw-midsun/firmware_xiv into soft_468_jumping_back_to_bootloader
@@ -23,3 +25,5 @@ extern uint32_t _vector_table_size; | |||
#define BOOTLOADER_RAM_START ((void *)&_ram_start) | |||
#define BOOTLOADER_RAM_SIZE ((size_t)&_ram_size) | |||
#define BOOTLOADER_VECTOR_TABLE_SIZE ((size_t)&_vector_table_size) | |||
#define BOOTLOADER_DEFAULT_LOCATION ((void *)&_bootloader_start) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think BOOTLOADER_START
might be a clearer name for this, especially since we'll likely never have the bootloader at a different location (so not "default").
@@ -0,0 +1,30 @@ | |||
#include "jump_to_bootloader.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jump_to_application, jump_to_bootloader, and jump shouldn't be duplicated between the bootloader project and the ms-bootloader libraries, they should only appear in ms-bootloader. So remove the copies in the bootloader project / move all the functionality to ms-bootloader.
platform/stm32f0xx/platform.mk
Outdated
@@ -44,6 +44,7 @@ endif | |||
DEFAULT_LINKER_SCRIPT ?= stm32f0_default.ld | |||
ifeq ($(MAKECMDGOALS),temp-bootloader-write) | |||
DEFAULT_LINKER_SCRIPT := stm32f0_application.ld | |||
CFLAGS += -DAPP_COMP_ON_BOOT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does APP_COMP_ON_BOOT
stand for? BOOTLOADER_APPLICATION
might be clearer.
libraries/ms-bootloader/src/jump.c
Outdated
@@ -0,0 +1,12 @@ | |||
#include "jump.h" | |||
|
|||
noreturn void prv_perform_jump(uint32_t sp, uint32_t pc) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll need to add back the __attribute__((naked))
here - it tells the compiler not to generate the standard entry/exit code for functions and just to use the raw assembly. It also will stop CI from complaining about noreturn
.
Also, this is STM32-specific - you'll need to move it to an stm32f0xx
folder and add a (no-op) x86
implementation.
- How does it fit into the overall system? | ||
- How does it work? (architectural overview, e.g. what each module's purpose is or how data flows through the firmware) | ||
--> | ||
# ms-bootloader |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should write a brief description of what goes in ms-bootloader here.
…s://github.com/uw-midsun/firmware_xiv into soft_468_jumping_back_to_bootloader
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good (barring tiny changes)! I'll approve once you validate it onsite :)
The big thing to validate is if we can successful jump back to the bootloader from an application a) on demand (i.e. using a test project) and b) when we receive a bootloader CAN message. Use make temp-bootloader-write
as described in #476 to flash (you'll need that PR on your branch). Mitchell can show you how to use cansend
to send a bootloader CAN message manually.
libraries/ms-bootloader/README.md
Outdated
<!-- | ||
ms-bootloader currently contains the jump_to_bootloader project, which allows an application to jump back to the bootloader on command | ||
> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't comment it out! <!-- -->
is the syntax for a comment in Markdown (and also HTML), just write it in plain text.
@@ -57,6 +59,9 @@ static void prv_handle_rx(Fsm *fsm, const Event *e, void *context) { | |||
|
|||
// Process bootloader messages | |||
if (rx_msg.source_id == SYSTEM_CAN_DEVICE_BOOTLOADER) { | |||
#ifdef APP_COMP_ON_BOOT | |||
jump_to_bootloader(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: you'll want to add a comment saying that jump_to_bootloader
doesn't return since that's important for anyone reading the code. You also might want to add a return;
afterwards even though it technically doesn't do anything, just so it doesn't read like you're then going to call bootloader_can_receive
.
libraries/ms-common/src/can_fsm.c
Outdated
@@ -57,6 +59,9 @@ static void prv_handle_rx(Fsm *fsm, const Event *e, void *context) { | |||
|
|||
// Process bootloader messages | |||
if (rx_msg.source_id == SYSTEM_CAN_DEVICE_BOOTLOADER) { | |||
#ifdef APP_COMP_ON_BOOT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll need to change this to BOOTLOADER_APPLICATION
too.
#include "bootloader_can.h" | ||
#include "can.h" | ||
#include "can_hw.h" | ||
#include "can_msg_defs.h" | ||
#include "can_rx.h" | ||
#include "jump_to_bootloader.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll get a linker error here when the jump_to_bootloader
call actually gets compiled - you'll need to add ms-bootloader
to the $(T)_DEPS
list in ms-common's rules.mk to tell the build system to link the ms-bootloader object files along with the ms-common ones.
libraries/ms-bootloader/inc/jump.h
Outdated
@@ -0,0 +1,8 @@ | |||
#pragma once | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: add a comment here saying what the library is used for, i.e. it lets you jump to an arbitrary memory address.
No description provided.