search

esp-rs / esp-flasher-stub

Rust implementation of flasher stub located in esptool
Apache License 2.0
18 stars 10 forks source link

Change the ChangeBaudrate command delay to match the esptool stub #62

Closed DNedic closed 2 months ago

DNedic commented 2 months ago

While evaluating the use of esp-flasher-stub as a drop-in replacement for stub support in esp-serial-flasher, I encountered issues tracing back to different delay intervals before changing the UART baudrate.

This changes the delay so that it is in line with the esptool stub in order ensure the drop-in nature of esp-flasher-stub.

In the future, we might want to consider adding io.flush() in order to remove all or almost all delays of this nature.

SergioGasquez commented 2 months ago

We made that change in https://github.com/esp-rs/esp-flasher-stub/pull/50 because it was causing some issues (https://github.com/esp-rs/esp-flasher-stub/issues/49), I think I wasn't able to reproduce the issue, but the changes with the delay solved it for the user.

DNedic commented 2 months ago

Thanks for the insight @SergioGasquez . Are we sure that it is the delay that fixed issues for the user and not other changes in that PR?

dobairoland commented 2 months ago

@radimkarnis PTAL as well

SergioGasquez commented 2 months ago

Thanks for the insight @SergioGasquez . Are we sure that it is the delay that fixed issues for the user and not other changes in that PR?

Not really, as we couldn't reproduce it on our end, I'd say it would be worth to merge this change. It should work with the same delay as C flasher stub, and we know where to look if this fails in the future.

DNedic commented 2 months ago

Thanks for taking a look @SergioGasquez @dobairoland!

radimkarnis commented 2 months ago

Late to the party, but LGTM! Better to have these aligned with the C stub.