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

Add checksum-md5 command #536

Merged
merged 5 commits into from
Dec 28, 2023
Merged

Conversation

bjoernQ
Copy link
Contributor

@bjoernQ bjoernQ commented Dec 27, 2023

This adds a checksum-md5 command to espflash.

e.g.

❯ espflash checksum-md5 --address 0x10000 --length 0x1000 --no-stub
[2023-12-27T11:39:27Z INFO ] Serial port: 'COM22'
[2023-12-27T11:39:27Z INFO ] Connecting...
[2023-12-27T11:39:27Z WARN ] Setting baud rate higher than 115,200 can cause issues
0x95d0df6456de4ac114251a67a5f85

While this is not too exciting on its own, it's the ground work for #259 and #479

It's very unfortunate that the Rust flasher-stub currently doesn't implement the required command while the ROM code and the C based flasher stub does. (Verified by temporarily reverting the stubs locally). Maybe we should revert the change to use the Rust flasher-stub (but use it based on a feature at build time) until it's supported there

I checked the result with reading a flash region with esptool and running md5sum on it.

@bjoernQ bjoernQ marked this pull request as ready for review December 27, 2023 11:59
Copy link
Member

@SergioGasquez SergioGasquez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding this feature! We should also add the subcommand for cargo-espflash

espflash/src/flasher/mod.rs Show resolved Hide resolved
@SergioGasquez
Copy link
Member

It's very unfortunate that the Rust flasher-stub currently doesn't implement the required command while the ROM code and the C based flasher stub does. (Verified by temporarily reverting the stubs locally). Maybe we should revert the change to use the Rust flasher-stub (but use it based on a feature at build time) until it's supported there:

The initial idea of the Rust stub, was to put them behind a feature, but we decided not to do that because nobody will use the feature. That being said, we now have an argument to do so or to go back to the C stubs or just force to use --no-stub for this subcommand until esp-rs/esp-flasher-stub#43 is implemented

@bjoernQ
Copy link
Contributor Author

bjoernQ commented Dec 27, 2023

It's very unfortunate that the Rust flasher-stub currently doesn't implement the required command while the ROM code and the C based flasher stub does. (Verified by temporarily reverting the stubs locally). Maybe we should revert the change to use the Rust flasher-stub (but use it based on a feature at build time) until it's supported there:

The initial idea of the Rust stub, was to put them behind a feature, but we decided not to do that because nobody will use the feature. That being said, we now have an argument to do so or to go back to the C stubs or just force to use --no-stub for this subcommand until esp-rs/esp-flasher-stub#43 is implemented

BTW I also currently cannot flash the H2 with the current stub - we really need to be very confident that the stub works for us (including MD5) before we force every user to use it (while I agree that no average user will use such a feature we could agree to use it in our team - it's kind of a chicken and egg problem)

Update: same for C6

@SergioGasquez
Copy link
Member

Current main of esp-flasher-stub should be able to flash all targets, we should probably cut a release and update them in espflash. But I agree, current state of espflash is not good as you can't flash many targets, I'm happy to add the Rust stubs behind a feature.

@SergioGasquez
Copy link
Member

In any case, that behind the scope of this PR, I'll do some testing tomorrow morning and update with the results!

@bjoernQ
Copy link
Contributor Author

bjoernQ commented Dec 28, 2023

I locally updated the flasher stubs ( commit 2b0b0f0de5020e6d5e2bf39e8950d49737fd5edb ) and it works for all chips - also flashing H2 works now for me

I'm still not sure if we should add a feature to choose the old vs the new stub. e.g. the new stub doesn't support 26 MHz xtals - maybe having at least a feature to force using the old stubs might be helpful for those users (but certainly not scope of this PR)

Copy link
Member

@SergioGasquez SergioGasquez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Tested it on all the targets but S2 and ESP32 and worked fine with the new stubs, will create a PR updating them!

Edit: PR updating the stubs: #537

@SergioGasquez
Copy link
Member

I'm still not sure if we should add a feature to choose the old vs the new stub. e.g. the new stub doesn't support 26 MHz xtals - maybe having at least a feature to force using the old stubs might be helpful for those users (but certainly not scope of this PR)

Is the 26 MHz a stub issue? I don't have any 26 MHz board so I cant test it. We have #526, but that's a bootloader issue. This definitely needs some investigation, so I'll try to get some 26 MHz devkit

@bjoernQ
Copy link
Contributor Author

bjoernQ commented Dec 28, 2023

I'm still not sure if we should add a feature to choose the old vs the new stub. e.g. the new stub doesn't support 26 MHz xtals - maybe having at least a feature to force using the old stubs might be helpful for those users (but certainly not scope of this PR)

Is the 26 MHz a stub issue? I don't have any 26 MHz board so I cant test it. We have #526, but that's a bootloader issue. This definitely needs some investigation, so I'll try to get some 26 MHz devkit

Yes it works with --no-stub ... so probably we don't need that feature ... I forgot there is --no-stub 😆

@SergioGasquez SergioGasquez merged commit 5123911 into esp-rs:main Dec 28, 2023
19 checks passed
@bjoernQ bjoernQ deleted the feature/md5-checksum branch December 28, 2023 09:36
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.

2 participants