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

esp*/README.md: Updates and clarifications. #5713

Closed
wants to merge 2 commits into from

Conversation

jimmo
Copy link
Member

@jimmo jimmo commented Mar 2, 2020

Mostly around obtaining and using the toolchain

  • Docker instructions for ESP8266
  • IDF 4.x instructions for ESP32

@jimmo
Copy link
Member Author

jimmo commented Mar 2, 2020

This comes up on the forum/irc/slack often enough that I thought it was worthwhile fixing the docs.

Copy link
Contributor

@tve tve left a comment

Choose a reason for hiding this comment

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

Great initiative! I added some comments...

ports/esp32/README.md Outdated Show resolved Hide resolved
ports/esp32/README.md Outdated Show resolved Hide resolved
ports/esp32/README.md Show resolved Hide resolved
ports/esp32/README.md Outdated Show resolved Hide resolved
$ ./install.sh # (or install.bat on Windows)
```

Then the `ports/esp32` directory, source the `export.sh` script to set up the
Copy link
Contributor

Choose a reason for hiding this comment

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

"The the"?
Also, I'm not following, why does one need to run export.sh from within the ports/esp32 directory?

Copy link
Member Author

Choose a reason for hiding this comment

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

You source it, not run it. It sets PATH.
But you're right of course that it can be run from anywhere. Have clarified this slightly.

ports/esp32/README.md Outdated Show resolved Hide resolved
ports/esp32/README.md Outdated Show resolved Hide resolved
ports/esp32/README.md Outdated Show resolved Hide resolved
@jimmo
Copy link
Member Author

jimmo commented Mar 2, 2020

Thanks for the feedback @tve & @mcauser -- I've addressed all your comments pretty much exactly as you suggested and pushed an update.

SPIRAM, but prefer to use a specific board target (or define your own as
necessary).

Note the use of `?=` in the `makefile` which allows them to be overriden on
Copy link
Contributor

Choose a reason for hiding this comment

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

s/overriden/overridden

Also fix Espressif links to the specific version they apply to.

- Use a Docker image with a pre-built toolchain (**recommended**).
To use this, install Docker, then prepend
`docker run --rm -v $HOME:$HOME -u $UID -w $PWD larsks/esp-open-sdk ` to the start
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest really spelling this out step by step for newer users.
(you'd think newer users wouldn't be trying to build from scratch, but a surprising number of people do)

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, will do.

Yes it's really useful for people who want to freeze modules, so definitely worth spelling it out in detail.

Copy link
Member

Choose a reason for hiding this comment

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

this was left as-is; please open a new PR for additional changes (if any)

Choose a reason for hiding this comment

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

Yes, please - more detail here would be very helpful. It seems like several steps are skipped here.

@@ -146,7 +165,7 @@ Documentation
-------------

More detailed documentation and instructions can be found at
http://docs.micropython.org/en/latest/esp8266/ , which includes Quick
http://docs.micropython.org/en/latest/esp8266/, which includes Quick
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't the space necessary to prevent markdown from including the , in the URL?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah makes sense. Will fix. Thanks

Copy link
Member

Choose a reason for hiding this comment

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

fixed during merge

@dpgeorge
Copy link
Member

Thanks for this! Merged in 0cd1308 and 9715905

@dpgeorge dpgeorge closed this Mar 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants