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

feat: Support hard_reset without stub (RDT-542) #228

Merged
merged 1 commit into from
Sep 13, 2023

Conversation

Harshal5
Copy link
Contributor

@Harshal5 Harshal5 commented Sep 5, 2023

The method hard_reset() of the class EspSerial used the default arguments of the decorator use_esptool(), due to which stub is required for performing a hard_reset().

This support for no_stub allows usage of the package with the newer chips for whom flasher_stub support has not been added yet.

hard_reset() used the default arguments of the decorator use_esptool(),
due to which stub is required for performing a hard_reset().
@hfudev
Copy link
Member

hfudev commented Sep 12, 2023

@dobairoland PTAL! I'm unaware of the side effects if we use no-stub mode for hard reset.

@github-actions github-actions bot changed the title feat: Support hard_reset without stub feat: Support hard_reset without stub (RDT-542) Sep 12, 2023
@dobairoland
Copy link

In esptool, the stub/no-stub option is linked to the given flashing instruction and not to the reset sequence. So I don't think uploading the stub would be necessary in any scenario for the reset.

@radimkarnis Please correct me if I've missed something.

@radimkarnis
Copy link

radimkarnis commented Sep 13, 2023

@dobairoland exactly. It is completely irrelevant for the hard reset if the stub is used or not.

hard_reset should just assert the DTR and RTS lines of the serial port, it doesn't communicate with the ESP in any way.

So essentially this just makes sure that the stub flasher doesn't get uploaded - which is not needed before invoking a hard reset. LGTM

@hfudev
Copy link
Member

hfudev commented Sep 13, 2023

@dobairoland @radimkarnis Thank you for your explanation!

@Harshal5 Thank you for the PR! I'm merging it.

@hfudev hfudev merged commit 606a2f4 into espressif:main Sep 13, 2023
5 checks passed
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.

4 participants