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 hard reset to rfc2217 server (ESPTOOL-760) #929

Merged
merged 1 commit into from
Oct 31, 2023

Conversation

20162026
Copy link
Contributor

add hard reset functionality to rfc2217 server and modify bootloader reset to trigger on DTR_ON instead of RTS_ON. This modification allows to use a rfc2217 server with idf_monitor and other compatible serial monitoring tools.
I'm not a python or rfc2217 expert and was assuming the default reset sequence, so there could be some issues with custom sequences.

This change fixes the following bug(s):

#892

I have tested this change with the following hardware & software combinations:

I have tested esp_rfc2217_server.py on windows10 and Ubuntu 22.04.2 LTS using custom esp32-wroom-32 board with the usual automatic bootloader circuit.

I have run the esptool.py automated integration tests with this change and the above hardware:

NO TESTING

@github-actions
Copy link

github-actions bot commented Oct 26, 2023

Messages
📖 Good Job! All checks are passing!

👋 Welcome 20162026, thank you for your first contribution to espressif/esptool project!

📘 Please check project Contributions Guide of the project for the contribution checklist, information regarding code and documentation style, testing and other topics.

Pull request review and merge process you can expect

We do welcome contributions in the form of bug reports, feature requests and pull requests via this public GitHub repository.

  1. An internal issue has been created for the PR, we assign it to the relevant engineer
  2. They review the PR and either approve it or ask you for changes or clarifications
  3. Once the Github PR is approved we do the final review, collect approvals from core owners and make sure all the automated tests are passing
    • At this point we may do some adjustments to the proposed change, or extend it by adding tests or documentation.
  4. If the change is approved and passes the tests it is merged into the default branch

Generated by 🚫 dangerJS against d66de5c

@github-actions github-actions bot changed the title add hard reset to rfc2217 server add hard reset to rfc2217 server (ESPTOOL-760) Oct 26, 2023
@radimkarnis
Copy link
Collaborator

Hi @20162026,
thank you for your contribution! This is a very welcome addition.

I have synced the PR to our internal queue and will be testing the change soon. After a first look, the code LGTM!

I would like to ask you to do some stylistic changes:

  1. The flake8 linter is failing, please see the failing GH Actions checks below.
  2. Please change the commit message according to the conventional commits standard. Something like feat(rfc2217_server): Add hard reset sequence could work.

Thank you!

@20162026
Copy link
Contributor Author

hi @radimkarnis,
stylistic changes are done.

@radimkarnis
Copy link
Collaborator

radimkarnis commented Oct 30, 2023

@20162026 thank you!
Sorry to bother, but there is still a small change to make reported by the black formater.
Sorry for this pedantic styling, esptool follows a certain code style and the commits cannot be merged until the code complies. I would make the changes myself, but then you'd lose a GitHub contributor credit, because the commit would be altered by me.

@20162026
Copy link
Contributor Author

@radimkarnis fixed.
Im not quite sure why black did not catch error when i was commiting localy as I did use the precommit hook, however now I ran linters manually and it should be ok

@dobairoland
Copy link
Collaborator

@radimkarnis I've noticed that the link to the project contribution guide is invalid in #929 (comment).

@dobairoland
Copy link
Collaborator

Hi @20162026. Thank you for your contribution. We will be happy to receive any feedback regarding the contribution process. Maybe we can something improve in our guides?

@espressif-bot espressif-bot merged commit d66de5c into espressif:master Oct 31, 2023
19 checks passed
@radimkarnis
Copy link
Collaborator

@20162026 thank you again! Now merged to master. This will propagate to ESP-IDF for use with other tools with the next esptool release.

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