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.

Sign up for GitHub

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

Jump to bottom

ref: use esptool main instead other func #236

Merged
merged 2 commits into from
Feb 23, 2024
Merged

ref: use esptool main instead other func #236

merged 2 commits into from
Feb 23, 2024

Conversation

horw
Copy link
Member

@horw horw commented Nov 7, 2023 •
edited
Loading

  • This PR replaced the use of the esptool functions with esptool.main.
  • Added the flag '--esp-flash-force' for 'force mode for esptool'.

closes #235

@horw horw force-pushed the ref/revise-the-code-flashing-binaries-to-target branch 3 times, most recently from e166a45 to 5684ea1 Compare December 20, 2023 02:21
@horw horw force-pushed the ref/revise-the-code-flashing-binaries-to-target branch 2 times, most recently from e573fe2 to 1a194de Compare December 24, 2023 13:19
@horw
Copy link
Member Author

horw commented Dec 29, 2023 •
edited
Loading

Actually, Arduino means Arduino application. I initially thought it was a chip..... 😊

@horw
Copy link
Member Author

horw commented Dec 29, 2023

Finally passed...

hfudev
hfudev approved these changes Dec 29, 2023
View reviewed changes
Copy link
Member

@hfudev hfudev left a comment

Choose a reason for hiding this comment

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

LGTM! @radimkarnis PTAL! Thank you

pytest-embedded-idf/pytest_embedded_idf/serial.py Show resolved Hide resolved
pytest-embedded-idf/pytest_embedded_idf/serial.py Outdated Show resolved Hide resolved
pytest-embedded-idf/pytest_embedded_idf/serial.py Show resolved Hide resolved
radimkarnis
radimkarnis approved these changes Jan 2, 2024
View reviewed changes
Copy link

@radimkarnis radimkarnis left a comment

Choose a reason for hiding this comment

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

The approach of using esptool.main with esp=esptool.get_default_connected_device seems like the correct approach to me. From the point of esptool this LGTM!

@hfudev
Copy link
Member

hfudev commented Jan 2, 2024

btw, before merge, please cleanup the commit history.

@hfudev hfudev force-pushed the ref/revise-the-code-flashing-binaries-to-target branch from 8147ee5 to 6040b38 Compare January 2, 2024 12:51
@hfudev
Copy link
Member

hfudev commented Jan 2, 2024

rebased to run the tests

@horw horw force-pushed the ref/revise-the-code-flashing-binaries-to-target branch from 9ef242c to d59682f Compare January 8, 2024 04:25
@horw horw requested a review from radimkarnis January 8, 2024 07:01
@horw horw force-pushed the ref/revise-the-code-flashing-binaries-to-target branch 2 times, most recently from 793710b to edfbcc0 Compare January 9, 2024 06:41
hfudev
hfudev requested changes Jan 10, 2024
View reviewed changes
Copy link
Member

@hfudev hfudev left a comment

Choose a reason for hiding this comment

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

Got a few more questions. Thank you for the cleanup. Now it looks much better.

pytest-embedded-arduino/pytest_embedded_arduino/serial.py Outdated Show resolved Hide resolved
pytest-embedded-idf/pytest_embedded_idf/serial.py Outdated Show resolved Hide resolved
pytest-embedded-idf/pytest_embedded_idf/serial.py Show resolved Hide resolved
pytest-embedded-serial-esp/pytest_embedded_serial_esp/serial.py Outdated Show resolved Hide resolved
pytest-embedded-serial-esp/pytest_embedded_serial_esp/serial.py Outdated Show resolved Hide resolved
pytest-embedded-serial-esp/pytest_embedded_serial_esp/serial.py Show resolved Hide resolved
@horw horw force-pushed the ref/revise-the-code-flashing-binaries-to-target branch 2 times, most recently from 22ae602 to f4141b2 Compare January 11, 2024 03:50
@hfudev hfudev force-pushed the ref/revise-the-code-flashing-binaries-to-target branch from f4141b2 to c1ebd5b Compare January 15, 2024 08:32
@horw horw changed the title ref: use esptool main instead other func (init commit) ref: use esptool main instead other func Jan 16, 2024
@horw horw force-pushed the ref/revise-the-code-flashing-binaries-to-target branch 4 times, most recently from 8b82053 to 13496fb Compare February 4, 2024 09:16
hfudev
hfudev approved these changes Feb 5, 2024
View reviewed changes
Copy link
Member

@hfudev hfudev left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you @horw for this nice improvement.

pytest-embedded-arduino/pytest_embedded_arduino/serial.py Outdated Show resolved Hide resolved
pytest-embedded-idf/pytest_embedded_idf/serial.py Outdated Show resolved Hide resolved
pytest-embedded-serial-esp/pytest_embedded_serial_esp/serial.py Outdated Show resolved Hide resolved
@horw horw force-pushed the ref/revise-the-code-flashing-binaries-to-target branch 5 times, most recently from f3a0570 to 53022b9 Compare February 7, 2024 06:55
@horw horw force-pushed the ref/revise-the-code-flashing-binaries-to-target branch from 6ebe641 to f137ce0 Compare February 9, 2024 03:10
@horw horw force-pushed the ref/revise-the-code-flashing-binaries-to-target branch from cf8a17b to 51b3086 Compare February 22, 2024 09:05
hfudev
hfudev requested changes Feb 22, 2024
View reviewed changes
Copy link
Member

@hfudev hfudev left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Nice. Final round. One major comment and a few nitpicks.

besides, regarding the commit message:

let's update it into

  • refactor: call esptool.main() instead of implementing on our own (combining your first two commits)
  • feat: support --esp-flash-force to run esptool.flash with the force flag

then the user could trace the changelog better

pytest-embedded-idf/pytest_embedded_idf/serial.py Outdated Show resolved Hide resolved
pytest-embedded-idf/pytest_embedded_idf/serial.py Outdated Show resolved Hide resolved
pytest-embedded-idf/pytest_embedded_idf/serial.py Outdated Show resolved Hide resolved
pytest-embedded-serial-esp/pytest_embedded_serial_esp/serial.py Show resolved Hide resolved
pytest-embedded-idf/pytest_embedded_idf/serial.py Outdated Show resolved Hide resolved
pytest-embedded-idf/pytest_embedded_idf/serial.py Outdated Show resolved Hide resolved
pytest-embedded-idf/pytest_embedded_idf/serial.py Outdated Show resolved Hide resolved
@horw horw force-pushed the ref/revise-the-code-flashing-binaries-to-target branch 2 times, most recently from ebec9ce to 8379718 Compare February 23, 2024 04:14
@horw
Copy link
Member Author

horw commented Feb 23, 2024

@hfudev Thanks for your advice on commit messages! The nitpick has already been fixed.

@horw horw force-pushed the ref/revise-the-code-flashing-binaries-to-target branch 2 times, most recently from 6f02f0c to 2e02374 Compare February 23, 2024 06:45
@horw horw force-pushed the ref/revise-the-code-flashing-binaries-to-target branch from 2e02374 to 9f39de7 Compare February 23, 2024 08:34
@hfudev hfudev merged commit 4b0d191 into espressif:main Feb 23, 2024
5 checks passed
@hfudev
Copy link
Member

hfudev commented Feb 23, 2024

LGTM! let's have a new release!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hfudev hfudev hfudev requested changes

@radimkarnis radimkarnis radimkarnis approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Revise the Code Flashing Binaries to Target (RDT-576)
3 participants
@horw @hfudev @radimkarnis