Skip to content

clarifying the need to use the patched esp-idf #3487

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
Closed
wants to merge 2 commits into from
Closed

clarifying the need to use the patched esp-idf #3487

wants to merge 2 commits into from

Conversation

julianrendell
Copy link

No description provided.

microdev1
microdev1 reviewed Sep 29, 2020
View reviewed changes
ports/esp32s2/README.md
@@ -48,6 +48,9 @@ Building boards such as the Saola is typically done through `make flash`. The de
make BOARD=espressif_saola_1_wrover flash PORT=/dev/tty.usbserial-1421120
```

More information about the `esp-idf` can be found [here](https://docs.espressif.com/projects/esp-idf/en/latest/esp32/).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Small suggestion, I think a link to esp32s2 page in esp-idf would be more relevant.

Suggested change
More information about the `esp-idf` can be found [here](https://docs.espressif.com/projects/esp-idf/en/latest/esp32/).
More information about the `esp-idf` can be found [here](https://docs.espressif.com/projects/esp-idf/en/latest/esp32s2/).

jepler
jepler reviewed Sep 29, 2020
View reviewed changes
ports/esp32s2/README.md Outdated
@@ -30,7 +30,7 @@ Connect these pins using a [USB adapter](https://www.adafruit.com/product/4090)

## Building and flashing ##

Before building or flashing the ESP32-S2, you must [install the esp-idf](https://docs.espressif.com/projects/esp-idf/en/latest/esp32/get-started/index.html). This must be re-done ever time the esp-idf is updated, but not every time you build. Run `cd ports/esp32s2` from `circuitpython/` to move to the esp32s2 port root, and run:
Before building or flashing the ESP32-S2, you must install the patched version of the `esp-idf` included with CircuitPython. This must be re-done ever time the esp-idf is updated, but not every time you build. Run `cd ports/esp32s2` from `circuitpython/` to move to the esp32s2 port root, and run:
Copy link

Choose a reason for hiding this comment

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

"ever time" should say "every time".

Instead of "the patched version" consider saying "the specific version". We may go back to not having local modifications to esp-idf if @hierophect succeeds but it will likely fall behind official esp-idf.

@hierophect
Copy link
Collaborator

I'm almost done moving us back to the official IDF - the reason for needing the patched IDF had to do with undef preprocessor errors that are prevalent in the IDF itself and were conflicting with our general policy of using -Werror. By marking them as system files, we can get around this problem, it's just taking a little while as a lot of includes have to be renamed.

Making a note that reinstalling the IDF after version changes is probably a good idea though.

@julianrendell
Copy link
Author

julianrendell commented Sep 29, 2020 •
edited
Loading

@microdev1, @jedgarpark, @tannewt, @hierophect - thanks for the feedback.
I think I've got the picture now:

  • there will (always?) be a local copy of esp-idf as a module
  • as much as possible it will be an up-to-date reference to a point in the original esp-idf source history
  • but there may times when it has CircuitPython specific patches (bugs, incompatible updates, etc)
  • using the esp_idf version from CP is expected to work; using the official esp_idf should work- but if it doesn't, try the version with CP (and let the CP community know there may be upcoming issues with the latest upstream esp_idf.)

I'll try to edit that in succinctly.

@julianrendell
Update README.md
451c32b 
re-wording after feedback.
@julianrendell
Copy link
Author

Build is failing due to a missing step, or step that isn't triggering?

Failing step is Upload mpy-cross builds to S3, but the log shows no content for that step; ie the previous steps last output is the line before the step after the erring step...

2020-09-30T00:16:58.5222843Z Artifact mpy-cross.static-x64-windows has been successfully uploaded!
2020-09-30T00:16:58.5329121Z Post job cleanup.

@jepler
Copy link

jepler commented Sep 30, 2020

The build failure should not prevent this being merged, it was just network flakiness.

I'll leave approving the wording to someone else.

@tannewt
Copy link
Member

tannewt commented Sep 30, 2020

@julianrendell Did you see microdev's comment about the link?

hierophect
hierophect requested changes Oct 5, 2020
View reviewed changes
Copy link
Collaborator

@hierophect hierophect left a comment

Choose a reason for hiding this comment

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

I think this PR has understandably gotten mixed up a little bit by the upcoming update to the ESP-IDF. In general, I think it needs some reductions to make it more clear - the current wording does a good job of trying to include both possibilities of a forked and direct IDF submodule, but that shouldn't ultimately be necessary (we're not going to use a forked IDF again) and I'm afraid it just adds extra confusion.

I would suggest removing the set of changes altogether these changes down to the following updates:

  • add cd command to install.sh steps
  • rework export instruction to note it's needed after installation
  • include the general link to the esp-idf documentation
  • I would also request you change the first esp-idf link so that it correctly points to the s2 documentation, not the base esp32

ports/esp32s2/README.md
@@ -30,24 +30,36 @@ Connect these pins using a [USB adapter](https://www.adafruit.com/product/4090)

## Building and flashing ##

Before building or flashing the ESP32-S2, you must [install the esp-idf](https://docs.espressif.com/projects/esp-idf/en/latest/esp32/get-started/index.html). This must be re-done ever time the esp-idf is updated, but not every time you build. Run `cd ports/esp32s2` from `circuitpython/` to move to the esp32s2 port root, and run:
Before building or flashing the ESP32-S2 version of CircuitPython, you must install a version of the [ESP32-S2 esp-idf](https://docs.espressif.com/projects/esp-idf/en/latest/esp32s2/) that is compatible with it. Please note that the esp-idf must be re-installed everytime the compatible version is updated. It does not have to be re-installed every time you build.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know that this change adds clarity, as per my general comments.

@hierophect hierophect added the espressif applies to multiple Espressif chips label Oct 6, 2020
@hierophect hierophect mentioned this pull request Oct 26, 2020
Merged
@hierophect
Copy link
Collaborator

Superseded by #3607

@hierophect hierophect closed this Oct 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jepler jepler jepler left review comments

@microdev1 microdev1 microdev1 left review comments

@hierophect hierophect hierophect requested changes

Assignees
No one assigned
Labels
espressif applies to multiple Espressif chips
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@julianrendell @hierophect @jepler @tannewt @microdev1