stub support (ESF-126)

[higaski]

Add stub loader support as requested by #92 and #84.

The stubs are pulled from https://github.com/espressif/esptool/tree/master/esptool/targets/stub_flasher. A CMake target called gen_esp_stubs pulls the json files from a specified esptool version and converts them to .h/.c files. That target does not depend on ALL and does not get created if Python isn't installed (a warning is issued though).

~~New Kconfig option~~ ~~- SERIAL_FLASHER_STUB_ENABLED ~~ ~~ defaults to y~~

Additions to public API:

• esp_loader_error_t esp_loader_connect_to_stub(esp_loader_connect_args_t *connect_args)
  Upload stub loader, then connect to it instead of the internal ROM loader.
• esp_loader_change_transmission_rate_stub(uint32_t new_transmission_rate, uint32_t old_transmission_rate)
  Like esp_loader_change_transmission_rate but has a second parameter for the stub version of the CHANGE_BAUDRATE command.

Additions to private API: ~~-esp_loader_error_t loader_no_stub(bool no_stub) ~~ ~~ Actual implementation of esp_loader_no_stub.~~

• esp_loader_error_t loader_run_stub(target_chip_t target)
  Upload and run the stub loader.

~~New definition~~ ~~- STUB_ENABLED~~

[DNedic]

Hello @higaski thanks for the contribution! I will investigate the issue you are having as soon as I can.

Currently, our intent is to use the Rust flasher stub for esp-serial-flasher for stub support, however for regular serial flashing it should be 100% compatible with the C flasher stub.

[higaski]

Currently, our intent is to use the Rust flasher stub for esp-serial-flasher for stub support, however for regular serial flashing it should be 100% compatible with the C flasher stub.

Sure, feel free to change the way the stub loader gets generated (and or included) to whatever way you prefer. I already thought that the JSON conversion would be more of a temporary thing.

[DNedic]

I'm not sure we should do both compile and run-time stub toggling this way.

In the case of run-time toggling, if the compiler has visibility into the fact that stubs are not used at compile-time, it will still mark all unused symbols for removal by the linker.

Conversely, we could just leave the compile-time toggle and simplify the code. Cases where we would try stub flashing and then fall back if something doesn't work should be extreme.

Personally, I would add a boolean flag use_stub to esp_loader_connect_args_t to toggle stub use or create a new esp_loader_connect_stub function. None of these would be breaking changes either.

[DNedic]

Regarding the SPI_ATTACH logic for ESP8266, in esptool, ESPLoader is only a base class and the behavior for ESP8266 is overriden by its concrete class here.

Edit: I realize this is not what you are reffering to, according to the code, esptool should be sending SPI_ATTACH when using the stub only when a custom SPI config is passed with the commandline. We can skip it entirely apparently until we decide to support that.

[DNedic]

Stub support should be gated behind #if (defined SERIAL_FLASHER_INTERFACE_UART) || (defined SERIAL_FLASHER_INTERFACE_USB) for now, as the stub does not support uploading over SPI.

[higaski]

I'm not sure we should do both compile and run-time stub toggling this way.

The reason I've added both, compile- and run-time toggling is that embedded systems might not want to add the stubs if they don't need them. I assume adding all stubs takes a reasonable amount of flash memory. Run-time toggling on the other hand is useful for things like x86 applications where the footprint of all stubs isn't an issue...

In the case of run-time toggling, if the compiler has visibility into the fact that stubs are not used at compile-time, it will still mark all unused symbols for removal by the linker.

I don't see how this could happen. The compiler can't optimize away the stubs if their usage depends on another run-time value.

Personally, I would add a boolean flag use_stub to esp_loader_connect_args_t to toggle stub use or create a new esp_loader_connect_stub function. None of these would be breaking changes either.

I like that, it sounds like a clean solution. It would also make esp_loader_no_stub obsolete.

Stub support should be gated behind #if (defined SERIAL_FLASHER_INTERFACE_UART) || (defined SERIAL_FLASHER_INTERFACE_USB) for now, as the stub does not support uploading over SPI.

STUB_ENABLED implies SERIAL_FLASHER_INTERFACE_UART. But I guess no harm in being more explicit.

I have another issue with the stub code. When I try to switch the transmission rate like done in the examples https://github.com/espressif/esp-serial-flasher/blob/285159162050decb156b9c8d9d55c54f58f92eff/examples/common/example_common.c#L257-L273

then detecting the flash size fails.

    if (esp_loader_flash_detect_size(&flash_size) == ESP_LOADER_SUCCESS) {
        if (image_size + offset > flash_size) {
            return ESP_LOADER_ERROR_IMAGE_SIZE;
        }
        loader_port_start_timer(DEFAULT_TIMEOUT);
        RETURN_ON_ERROR( loader_spi_parameters(flash_size) );
    } else {
        loader_port_debug_print("Flash size detection failed, falling back to default");
    }

More specifically this call here https://github.com/espressif/esp-serial-flasher/blob/285159162050decb156b9c8d9d55c54f58f92eff/src/esp_loader.c#L265-L265

I can't find documentation of an 0x9F command? Is this a ROM loader only thing?

[DNedic]

In the case of run-time toggling, if the compiler has visibility into the fact that stubs are not used at compile-time, it will still mark all unused symbols for removal by the linker.

I don't see how this could happen. The compiler can't optimize away the stubs if their usage depends on another run-time value.

It will happen if the value is known at compile time and the effect is visible in that translation unit. In the case of using the config struct, it will not happen without link-time optimization, however if we have a separate connect function, and that function never gets called, the compiler will mark all symbols that have a sole dependency on that function for removal by the linker, stubs included.

[higaski]

In the case of run-time toggling, if the compiler has visibility into the fact that stubs are not used at compile-time, it will still mark all unused symbols for removal by the linker.

I don't see how this could happen. The compiler can't optimize away the stubs if their usage depends on another run-time value.

It will happen if the value is known at compile time and the effect is visible in that translation unit. In the case of using the config struct, it will not happen without link-time optimization, however if we have a separate connect function, and that function never gets called, it will discard all symbols that have a sole dependency on that function, stubs included.

So that I understand you correctly. Let's drop compile-time support in favor of a separate connect function and rely on the compiler to remove the stubs if not needed?

[DNedic]

In the case of run-time toggling, if the compiler has visibility into the fact that stubs are not used at compile-time, it will still mark all unused symbols for removal by the linker.

I don't see how this could happen. The compiler can't optimize away the stubs if their usage depends on another run-time value.

It will happen if the value is known at compile time and the effect is visible in that translation unit. In the case of using the config struct, it will not happen without link-time optimization, however if we have a separate connect function, and that function never gets called, it will discard all symbols that have a sole dependency on that function, stubs included.

So that I understand you correctly. Let's drop compile-time support in favor of a separate connect function and rely on the compiler to remove the stubs if not needed?

Yes, either that or only use compile-time toggling. Two toggles complicate code, and over time will create bugs when people forget to check both.

If you're concerned about the approach, try mocking up a quick test and see if the stubs are dropped by looking at the mapfile.

[DNedic]

I have another issue with the stub code. When I try to switch the transmission rate then detecting the flash size fails.

I assume that is again because of this line:

RETURN_ON_ERROR( loader_spi_parameters(flash_size) );

Reading flash ID is also done by esptool: https://github.com/espressif/esptool/blob/master/esptool/loader.py#L937

Edit: looks like there is a difference in the command for changing the baudrate: https://github.com/espressif/esptool/blob/master/esptool/loader.py#L1138

[DNedic]

When writing to RAM, we need to check if we are overwriting the stub.

[higaski]

Edit: looks like there is a difference in the command for changing the baudrate: https://github.com/espressif/esptool/blob/master/esptool/loader.py#L1138

Yes, this works. Now I'm starting to miss function overloads... Any preferences how this should be handled with the function

esp_loader_error_t esp_loader_change_transmission_rate(uint32_t transmission_rate)

The stub version would require the old baud rate as second parameter. I've thought about adding a baudrate variable to

typedef struct {
    uint32_t sync_timeout;  /*!< Maximum time to wait for response from serial interface. */
    int32_t trials;         /*!< Number of trials to connect to target. If greater than 1,
                               100 millisecond delay is inserted after each try. */
    uint32_t baudrate;  /*!< Like this maybe? */
} esp_loader_connect_args_t;

and then store a local copy of it. This would then allow setting the old baud rate for the CHANGE_BAUDRATE command depending on whether the stub loader is used or not.

/edit Or maybe add another function

esp_loader_error_t esp_loader_change_transmission_rate_from_to(uint32_t from, uint32_t to)

but then the user would have to know those subtle differences between stub and ROM loader.

[DNedic]

The stub version would require the old baud rate as second parameter. I've thought about adding a baudrate variable to

typedef struct {
    uint32_t sync_timeout;  /*!< Maximum time to wait for response from serial interface. */
    int32_t trials;         /*!< Number of trials to connect to target. If greater than 1,
                               100 millisecond delay is inserted after each try. */
    uint32_t baudrate;  /*!< Like this maybe? */
} esp_loader_connect_args_t;
and then store a local copy of it. This would then allow setting the old baud rate for the CHANGE_BAUDRATE command depending on whether the stub loader is used or not.

/edit Or maybe add another function

esp_loader_error_t esp_loader_change_transmission_rate_from_to(uint32_t from, uint32_t to)

but then the user would have to know those subtle differences between stub and ROM loader.

For this reason, the fact that we now have to track if the stub is running for some things (like checking whether we are overwriting it), and the fact that stubs are now a part of the build process, whether users use them or not (along with attempting to pull from esptool repo) I am leaning more and more to a preprocessor only toggle without the possibility of deciding on stub use at runtime at all.

@dobairoland WDYT?

[higaski]

For this reason, the fact that we now have to track if the stub is running for some things (like checking whether we are overwriting it), and the fact that stubs are now a part of the build process, whether users use them or not (along with attempting to pull from esptool repo) I am leaning more and more to a preprocessor only toggle without the possibility of deciding on stub use at runtime at all.

I'd like to keep the runtime toggle if possible. I've already built it into my application.

/edit Even with a compile time only switch you'd need to check if the stub is running for the RAM command.

[DNedic]

Even with a compile time only switch you'd need to check if the stub is running for the RAM command.

Unless I am missing something, if we always upload the stub with esp_loader_connect() when the compile-time switch is enabled, we don't have to check anything at runtime.

[higaski]

Even with a compile time only switch you'd need to check if the stub is running for the RAM command.

Unless I am missing something, if we always upload the stub with esp_loader_connect() when the compile-time switch is enabled, we don't have to check anything at runtime.

When uploading the bootloader (mem* functions) you can't check for RAM being overwritten. Of course the stub loader gets written to the stub loader destination. So, even with a compile time switch you'd need a flag for this special case to make sure that check does not run before the stub is in place.

Maybe we can find a compromise and have both again, compile- and run time switch?

/edit Pulling the stubs from the repo is not part of the build process. This is an optional target.

[DNedic]

When uploading the bootloader (mem* functions) you can't check for RAM being overwritten. Of course the stub loader gets written to the stub loader destination. So, even with a compile time switch you'd need a flag for this special case to make sure that check does not run before the stub is in place.

Sorry, maybe I didn't elaborate well enough. To upload anything using mem*() functions you first need to connect using esp_loader_connect(). If the stub is always uploaded during esp_loader_connect(), and we know the stub size, we can safely know where the user is not allowed to write to RAM.

[higaski]

When uploading the bootloader (mem* functions) you can't check for RAM being overwritten. Of course the stub loader gets written to the stub loader destination. So, even with a compile time switch you'd need a flag for this special case to make sure that check does not run before the stub is in place.

Sorry, maybe I didn't elaborate well enough. To upload anything using mem*() functions you first need to connect using esp_loader_connect(). If the stub is always uploaded during esp_loader_connect(), and we know the stub size, we can safely know where the user is not allowed to write to RAM.

Then the stub loader would require its own esp_loader_mem_start (and friends) functions. Currently I use it to upload the stub and those functions don't know if the stub is already in place or not.

[DNedic]

Then the stub loader would require its own esp_loader_mem_start (and friends) functions. Currently I use it to upload the stub and those functions don't know if the stub is already in place or not.

You're correct, my apologies.

[DNedic]

Overall, the PR seems in a solid state now, I will dedicate some time to test it on hardware. @radimkarnis If you have the time please take a look as well.

[DNedic]

I'm still not sure how we would handle the baudrate change command requirement when using the stub, breaking changes are not an option so we can't change the function itself.

We also can't track the baudrate from the start as initialization never touches the esp_loader layer, only the port layer (and we don't want circular dependencies).

We can require a new port function to report current baudrate, but that would require all current user ports to be updated in order to use the stub functionality, so it isn't a perfect solution either.

Finally, we can add a new function esp_loader_change_transmission_rate_stub(), to work around this and check whether the stub is running in both esp_loader_change_transmission_rate() and the new function.

[higaski]

If C11 is an option we use the _Generic operator to "overload" the existing esp_loader_change_transmission_rate function and add a second parameter.

See https://stackoverflow.com/questions/479207/how-to-achieve-function-overloading-in-c#25026358

I'm not terrible familiar with modern C but apparently such things are possible nowadays.

[DNedic]

I would not go that route personally for the following reasons:

1. This is a public API function and this would force people to understand a very esoteric language feature
2. We're not sure everyone using the library has access to C11, this would be a breaking change as we currently don't use any C11 features

[higaski]

I'm still not sure how we would handle the baudrate change command requirement when using the stub, breaking changes are not an option so we can't change the function itself.

We also can't track the baudrate from the start as initialization never touches the esp_loader layer, only the port layer (and we don't want circular dependencies).

We can require a new port function to report current baudrate, but that would require all current user ports to be updated in order to use the stub functionality, so it isn't a perfect solution either.

Finally, we can add a new function esp_loader_change_transmission_rate_stub(), to work around this and check whether the stub is running in both esp_loader_change_transmission_rate() and the new function.

Added proposed changes

https://github.com/higaski/esp-serial-flasher/commit/928dd73850c2c2cd689ecdeb854afe7b2b159622

[DNedic]

Thanks for rebasing on top of master! Please try disabling autoformatters/doing any manual formatting on code that's not part of the PR functionality. If something is inconsistent it can be fixed later, but the diffs need to be reviewable and related to the functional changes.

[higaski]

Thanks for rebasing on top of master! Please try disabling autoformatters/doing any manual formatting on code that's not part of the PR functionality. If something is inconsistent it can be fixed later, but the diffs need to be reviewable and related to the functional changes.

Hm weird, I've my IDEs auto formatting explicitly disabled for the project. Not sure where this suddenly comes from... maybe the pre-commit hook? I'll fix that monday, it's really just 3 lines of CMake.

/edit Had to change my VSCode workspace settings for formatting as well... Looks like they introduced a bug for me. Disabling auto formatting on the projects folder alone did not work anymore.

[higaski]

Any updates on when this might merge?

[DNedic]

Any updates on when this might merge?

I still haven't had the time to fully test this, but I should get around to it soon. After I test it manually and we approve it for merging, you should squash all commits into a single conventional commit and then we have to add documentation and create examples. Of course, you can help with that as well if you wish.

[DNedic]

I'm trying to solve timing issues manifesting themselves as flash size not being detected correctly. When logging is enabled, or the firmware is compiled with O0, everything works fine, but not when logging is disabled or Og or higher optimisation levels are used. Changing timeouts doesn't help either, so this might take time to trace down.

[higaski]

On which port and/or architecture? I'm compiling the x86 flasher tool I've written with -O2 (or whatever the default CMake Release build is) and had no such issues.