Skip to content
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

Open
wants to merge 33 commits into
base: master
Choose a base branch
from

Conversation

vaaranan-y
Copy link
Member

No description provided.

Copy link
Collaborator

@ryandancy ryandancy left a 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
Copy link
Collaborator

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

Comment on lines 11 to 20
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));
}
Copy link
Collaborator

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"
Copy link
Collaborator

@ryandancy ryandancy Oct 2, 2021

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:

  1. We'll need to move jump_to_bootloader to a library, perhaps called ms-bootloader, so can_fsm.c can reach it. Perhaps move bootloader_mcu.h to that library too.
  2. 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 adding CFLAGS += -D<symbolname> to the ifeq ($(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.

@@ -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)
Copy link
Collaborator

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"
Copy link
Collaborator

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.

@@ -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
Copy link
Collaborator

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.

@@ -0,0 +1,12 @@
#include "jump.h"

noreturn void prv_perform_jump(uint32_t sp, uint32_t pc) {
Copy link
Collaborator

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
Copy link
Collaborator

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.

Copy link
Collaborator

@ryandancy ryandancy left a 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.

Comment on lines 18 to 20
<!--
ms-bootloader currently contains the jump_to_bootloader project, which allows an application to jump back to the bootloader on command
>
Copy link
Collaborator

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();
Copy link
Collaborator

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.

@@ -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
Copy link
Collaborator

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"
Copy link
Collaborator

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.

@@ -0,0 +1,8 @@
#pragma once

Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants