-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Fix typo in firmware image format documentation (ESPTOOL-1205) #1135
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
Conversation
👋 Hello tyeth, we appreciate your contribution to this project! 📘 Please review the project's Contributions Guide for key guidelines on code, documentation, testing, and more. Click to see more instructions ...
Review and merge process you can expect ...
|
|
There's also something weird in the next table, the three values are listed as '0xf', 0, 2 which might be another typo. |
| | 3 | High four bits - Flash size (``0`` = 1MB, ``1`` = 2MB, ``2`` = 4MB, ``3`` = 8MB, ``4`` = 16MB) | | ||
| | | | | ||
| | | Low four bits - Flash frequency (``0`` = 80MHz, ``0`` = 40MHz, ``2`` = 20MHz) | | ||
| | | Low four bits - Flash frequency (``0`` = 80MHz, ``1`` = 40MHz, ``2`` = 20MHz) | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that this is correct and explained a couple of lines later, after the table in the note: Flash frequency with value ``0`` can mean either 80MHz or 40MHz based on MSPI clock source mode. Basically, there is a clock divider that will be set in ESP-IDF, which defines the final clock frequency.
Also, please note that these values correspond to ones in the code: https://github.com/espressif/esptool/blob/master/esptool/targets/esp32c6.py#L82-L86
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this could be improved in a way so it won't raise suspicion about being a mistake. For example:
``0`` = 80MHz or 40MHz
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah wonderful, thanks folks for the info and suggestion
Unchecked, but seems a sensible change.