Adding support for ESP32C3

esp-rs / esp-flasher-stub

Rust implementation of flasher stub located in esptool

Apache License 2.0

17 stars 10 forks source link

Adding support for ESP32C3 #2

Closed MaValki closed 1 year ago

MaValki commented 2 years ago

Added support for ESP32C3 though UART. Unit tests are to be updated.

MaValki commented 2 years ago

I Updated MR, with support for ESP32C3. @MabezDev @jessebraham PTAL again.

dobairoland commented 2 years ago

FYI in case you are interested @radimkarnis @dnedic

DNedic commented 2 years ago

Amazing work @MaValki , reading this was a joy and it's really high quality work. I have left a few minor suggestions. @dobairoland Thank you for the ping.

MaValki commented 2 years ago

Thank you @MabezDev @jessebraham @DNedic @radimkarnis for great suggestions, I adjusted the code accordingly.

jessebraham commented 1 year ago

@MaValki just touching bases, would like to get this merged :) There's no real rush but i'd be nice to have something in the main branch.

If you would you like me to address the remaining changes I'm more than happy to do so, just let me know.

jessebraham commented 1 year ago

Alright, I've pushed a few more commits to this branch:

Updated all dependencies (now using published version of esp32c3_hal) and fixed all build/linker errors, cleaned up imports, etc.
Addressed the suggested changes from the previous review
Miscellaneous cleanup and formatting, fix a bunch of warnings, etc.

I haven't tested this on hardware yet but will report back when I have.

@MabezDev @DNedic if you could take another look when you get a chance I'd appreciate it. I think we're pretty close to merging this.

EDIT: oops, I think I screwed up the linker script. I had copy-pasted from esp-hal to resolve some errors and didn't mean to commit all the changes 😁

MabezDev commented 1 year ago

@jessebraham will test soon! How do we test it with esptool (or espflash, I think we support stubs in there now too?)

jessebraham commented 1 year ago

Martin did include a patch file in this PR for esptool which modifies it to use these. Probably needs some updates for paths and what not, but you could try that. I have not used this before.

We do support stubs in espflash now but don't presently have a method of providing a custom one. The stubs are just JSON files and I think the binary has just been base64'd and tossed in there, so we should be able to update the JSON file and rebuild espflash to use it.

MabezDev commented 1 year ago

Seems to work with esptool! Haven't tried with espflash yet.

By the way, I didn't need to apply the patch to esptool, if the binary has the correct name, esptools wrap_elf.py utility will correctly embed the stub in esptool:

[[bin]]
name = "stub_flasher_32c3"
path = "src/main.rs"

./wrap_stub.py esp-flasher-stub/target/riscv32imc-unknown-none-elf/release/stub_flasher_32c3

So maybe we want to rename the binary before merging? Hopefully there is a way to rename per target, else we'll need a post build step.

jessebraham commented 1 year ago

@MabezDev I'm not sure there's a way to dynamically rename the binaries without some post-processing step. This is something we can look in to, but for the time being I'm not sure we should block merging on this. We can rename it to stub-flasher or something closer if you think that makes sense, though.