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

'Reboot to bootloader' boots to wrong bootloader. #11334

Closed
hydra opened this issue Jan 17, 2022 · 12 comments · Fixed by betaflight/betaflight-configurator#2904
Closed

'Reboot to bootloader' boots to wrong bootloader. #11334

hydra opened this issue Jan 17, 2022 · 12 comments · Fixed by betaflight/betaflight-configurator#2904
Labels
BUG Bugs are excluded from automatically being marked as stale

Comments

@hydra
Copy link
Contributor

hydra commented Jan 17, 2022

Describe the bug

Betaflight supports bootloaders, bootloaders are used to do the flashing of firmware. Two examples are a) the original OpenPilot bootloader, and b) the SPRacing EXST bootloader.

When the user clicks 'reboot to bootloader' they expect the firmware to reboot to the bootloader that allows for flashing new firmware via the tool they are currently using. i.e. use 'reboot to bootloader' in the configurator to flash via the configurator. Users don't do anything else other than flash firmware or unplug the FC from the configurator when the FC is in it's bootloader mode. Users in the SPRacing discord server will attest to this situation.

They do NOT expect 'reboot to bootloader' that takes them to a bootloader (the built-in ST ROM-based DFU bootloader) that can potentially require reflashing/recovery of the pre-installed bootloader.

The usual 'Flash Firmware' button reboots to the correct bootloader for flashing.

To Reproduce

Click 'Reboot to bootloader', flash wrong firmware to FC that has a bootloader.

Expected behavior

  1. The firmware should reboot to the firmware used for flashing BY DEFAULT when asked to go to the 'bootloader' (both via CLI 'bl' command and MSP).
  2. The configurator should reboot to the firmware used for flashing when 'reboot to bootloader' is pressed.
  3. The firmware and configurator should NEVER boot to the ROM bootloader unless specifically requested, as in the case of an FC with a bootloader that is for developers and manufacturers only. e.g. they should use 'bl rom' in the CLI or jump the boot pads if they really need to, e.g. for connecting a debugger or flashing bootloader firmware, not BF firmware.

To summarize:
cli: bl should reboot to the 'default' bootloader. i.e. flash-based bootloader, if one exists.
cli: bl flash should reboot to flash-based bootloader, if one exists.
cli: bl rom should reboot to ROM-based bootloader.
gui: 'reboot to bootloader' should reboot to the 'default' bootloader, i.e. flash-based bootloader, if one exists.
gui: 'flash firmware' should reboot to the 'default' bootloader, i.e. flash-based bootloader, if one exists.

gui: additionally, if there were 'reboot' and 'reboot to ROM bootloader' expert-user-only buttons in the GUI that would be helpful too.

Setup

  • Configurator version: nightly
N/A

Additional information

Way back when the EXST bootloader existed the original behavior was to reboot to the DEFAULT bootloader, the target knows what it's default is. In the case of normal targets it was ROM, in the case of EXST targets it was FLASH.

Then this PR was created 'Add booting into the flash boot loader as an option' which changed the default behavior to always be the ROM bootloader. I disagreed strongly via slack at the time that this would cause support issues, additional documentation and a bad user experience.
#8429

Due to that change more code had to be added to the configurator to allow it to reboot to the correct bootloader when flashing - previously it 'just worked' by rebooting to the 'default' bootloader, i.e. the one that allows flashing.
#8680

@NikoN-FPV
Copy link

yes i agree

@haslinghuis
Copy link
Member

In common_post.h in firmware on line 392 we have:

#if defined(USE_EXST) && !defined(RAMBASED)
#define USE_FLASH_BOOT_LOADER
#endif

Unless I'm missing something USE_EXST or RAMBASED are not defined anywhere?

@hydra
Copy link
Contributor Author

hydra commented Jan 23, 2022

the targets define USE_EXST. that setting is already enabled. It's mostly the choice of the default bootloader in the firmware that is wrong. The logic in the cli 'reboot' and msp 'reboot' commands is where the problem needs to be fixed.

@haslinghuis
Copy link
Member

Then we should transfer this issue to firmware first? Would you suggest reverting the PR's in question?

@haslinghuis haslinghuis transferred this issue from betaflight/betaflight-configurator Jan 24, 2022
@hydra
Copy link
Contributor Author

hydra commented Jan 26, 2022

I don't think any PR's will revert cleanly now, especially because more code was added which is now used by the configurator and is now required to be kept until the configurator drops support for the oldest firmware that needed the MSP support.

A new one should be created that fixes the issues mentioned, then later we could schedule the changes to MSP for deprecation and finally another PR to delete them if you want to reclaim the previous MSP behavior and reduce the code-size and complexity of the firmware.

@github-actions
Copy link

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs within a week.

@github-actions github-actions bot added the Inactive Automatically detected and labeled, will be closed after another week of inactivity. label Feb 26, 2022
@haslinghuis haslinghuis added BUG Bugs are excluded from automatically being marked as stale and removed Inactive Automatically detected and labeled, will be closed after another week of inactivity. labels Feb 26, 2022
@ctzsnooze
Copy link
Member

@hydra please confirm if this issue is still active in current master?
If not, maybe make a PR to address it?

@hydra
Copy link
Contributor Author

hydra commented Apr 15, 2022

@ctzsnooze confirmed it's still an issue in master. Yes, I can make a PR to address it. I'm currently unwell and haven't been able to work on any of my PRs recently.

@haslinghuis haslinghuis added this to Bug Tracker in Finalizing Firmware 4.3 Release via automation Apr 15, 2022
@klutvott123
Copy link
Member

If the options to reboot to bootloader in flash or rom had been added in addition to just regular reboot to default bootloader this wouldn't have been an issue.
The simple solution to would be to add the missing code in the configurator to make the button reboot to bootloader in flash if one exists, and change the default behaviour of bl in cli

@klutvott123
Copy link
Member

I made a PR for discussion betaflight/betaflight-configurator#2904

@klutvott123
Copy link
Member

Logic in cli looks sound to me. It will use the bootloader in flash if available and no arguments are given

betaflight/src/main/cli/cli.c

Lines 3568 to 3592 in a0ed6b0

static void cliBootloader(const char *cmdName, char *cmdline)
{
rebootTarget_e rebootTarget;
if (
#if !defined(USE_FLASH_BOOT_LOADER)
isEmpty(cmdline) ||
#endif
strncasecmp(cmdline, "rom", 3) == 0) {
rebootTarget = REBOOT_TARGET_BOOTLOADER_ROM;
cliPrintHashLine("restarting in ROM bootloader mode");
#if defined(USE_FLASH_BOOT_LOADER)
} else if (isEmpty(cmdline) || strncasecmp(cmdline, "flash", 5) == 0) {
rebootTarget = REBOOT_TARGET_BOOTLOADER_FLASH;
cliPrintHashLine("restarting in flash bootloader mode");
#endif
} else {
cliPrintErrorLinef(cmdName, "Invalid option");
return;
}
cliRebootEx(rebootTarget);
}

Finalizing Firmware 4.3 Release automation moved this from Bug Tracker to Finished (Merged) Apr 24, 2022
@hydra
Copy link
Contributor Author

hydra commented May 2, 2022

@klutvott123 confirmed that the firmware logic does appear to be correct. I just tested bl with no arguments on the H7RF and it reboots to the SP Racing bootloader. Not sure exactly when that changed. I'll check out the configurator side of things and comment/update as appropriate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BUG Bugs are excluded from automatically being marked as stale
Projects
No open projects
5 participants