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

support for ESP32-S2 ESP32-S3 Flash 32MB..128MB (ESPTOOL-325) #675

Closed
wants to merge 6 commits into from
Closed

Conversation

ESP32DE
Copy link
Contributor

@ESP32DE ESP32DE commented Oct 13, 2021

ESP32-S2 and ESP32-S3
supports up to 1 GB of external flash and RAM
add support for

  • 32MB
  • 64MB
  • 128MB ( example NOR spiFLASH 1G-BIT W25Q01JVZEIQ Winbond 0xEF 0x40 0x21 )

( i removed the 256MB, 512MB, 1024MB in this pull request and changed title from 32MB..1024MB to 32MB..128MB
as soon we can test 256MB, 512MB and the 1024MB i update with a new Pull Request )

change small typo in helper --flash-size

I have tested this change with the following hardware & software combinations:
Winbond
NOR spiFLASH 1G-BIT W25Q01JVZEIQ 0x21 )

image

ESP32-S2-DevKitC-1
ESP32-S3-DevKitC-1
ESP-IDF v4.4-dev-3235-g3e370c4296-dirty

ESP32-S2 and ESP32-S3
supports up to 1 GB of external flash and RAM
add support for 
- 32MB
- 64MB
- 128MB ( example NOR spiFLASH 1G-BIT W25Q01JVZEIQ )
- 256MB
- 512MB
- 1024MB

change small typo in helper --flash-size
@github-actions github-actions bot changed the title support for ESP32-S2 ESP32-S3 Flash 32MB..1024MB support for ESP32-S2 ESP32-S3 Flash 32MB..1024MB (ESPTOOL-325) Oct 14, 2021
Copy link
Member

@igrr igrr left a comment

Choose a reason for hiding this comment

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

Hi @ESP32DE, thank you for the PR. It seems that a few things are missing from this patch, could you please check?

When running esptool.py in this branch like this:

esptool.py --chip esp32s3 elf2image --flash_mode dout --flash_freq 80m --flash_size 32MB -o bootloader.bin bootloader.elf

I get an error:

esptool elf2image: error: argument --flash_size/-fs: 32MB is not a known flash size. Known sizes: 512KB, 256KB, 1MB, 2MB, 4MB, 2MB-c1, 4MB-c1, 8MB, 16MB

One missing thing seems to be updating the FLASH_SIZES dictionary in ESP32ROM class.

Another thing, seems like DETECTED_FLASH_SIZES goes up to 128MB but --flash_size help text also includes 256MB and above. Should these two be kept consistent? Since you have tested this with 128MB flash chip, I'd recommend keeping the list of supported flash sizes up to 128MB, and not including larger sizes.

- updating the FLASH_SIZES dictionary in ESP32ROM class was missing ( 32MB, 64MB, 128MB )
- remove > 128MB in the helper since we have just in time only the possibles ( Market )  to test 128MB Version
@ESP32DE ESP32DE changed the title support for ESP32-S2 ESP32-S3 Flash 32MB..1024MB (ESPTOOL-325) support for ESP32-S2 ESP32-S3 Flash 32MB..128MB (ESPTOOL-325) Oct 15, 2021
- updating the FLASH_SIZES dictionary in ESP32ROM class was missing ( 32MB, 64MB, 128MB )
- remove > 128MB in the helper since we have just in time only the possibles ( Market )  to test 128MB Version
@ESP32DE
Copy link
Contributor Author

ESP32DE commented Oct 15, 2021

Hi @ESP32DE, thank you for the PR. It seems that a few things are missing from this patch, could you please check?

When running esptool.py in this branch like this:

esptool.py --chip esp32s3 elf2image --flash_mode dout --flash_freq 80m --flash_size 32MB -o bootloader.bin bootloader.elf

I get an error:

esptool elf2image: error: argument --flash_size/-fs: 32MB is not a known flash size. Known sizes: 512KB, 256KB, 1MB, 2MB, 4MB, 2MB-c1, 4MB-c1, 8MB, 16MB

One missing thing seems to be updating the FLASH_SIZES dictionary in ESP32ROM class.

Hi @igrr
uhh...
image

sry - looks like i did forget this part in the online version...
i have updated the FLASH_SIZES dictionary in ESP32ROM class with missing 32MB, 64MB, 128MB

Another thing, seems like DETECTED_FLASH_SIZES goes up to 128MB but --flash_size help text also includes 256MB and above. Should these two be kept consistent? Since you have tested this with 128MB flash chip, I'd recommend keeping the list of supported flash sizes up to 128MB, and not including larger sizes.

Yes - no problem @igrr
i removed the 256MB, 512MB and 1024MB in the helper and also changed the title for support 32MB..128MB
after i can test > 128MB i will do a new request.

Info:
@igrr i create the pull also for the ESP-IDF espressif/esp-idf#7688
i will change the pull not including larger sizes ( > 128MB ) then there too since i could only tested it with 128MB.
Ok?

If there is any to do more please let me know and i will try my best.

thank you
best wishes

ps:
@igrr
i did not change the FlashSizeAction class yet

  • Custom flash size parser class to support backwards compatibility with megabit size arguments

what is your thinking to expand this too?

class FlashSizeAction(argparse.Action):
    """ Custom flash size parser class to support backwards compatibility with megabit size arguments.
    (At next major relase, remove deprecated sizes and this can become a 'normal' choices= argument again.)
    """
    def __init__(self, option_strings, dest, nargs=1, auto_detect=False, **kwargs):
        super(FlashSizeAction, self).__init__(option_strings, dest, nargs, **kwargs)
        self._auto_detect = auto_detect

    def __call__(self, parser, namespace, values, option_string=None):
        try:
            value = {
                '2m': '256KB',
                '4m': '512KB',
                '8m': '1MB',
                '16m': '2MB',
                '32m': '4MB',
                '64m': '8MB',
                '128m': '16MB',
                '256m': '32MB',
                '512m':  '64MB',
                '1024m': '128MB',
                '16m-c1': '2MB-c1',
                '32m-c1': '4MB-c1',
            }[values[0]]
            print("WARNING: Flash size arguments in megabits like '%s' are deprecated." % (values[0]))
            print("Please use the equivalent size '%s'." % (value))
            print("Megabit arguments may be removed in a future release.")
        except KeyError:
            value = values[0]


let me know and i will expand/remove this in this request too.

- updated Custom flash size parser class (8MB..128MB) to support backwards compatibility with megabit size arguments
- remove whitespace and multiple spaces
- remove whitespace and multiple spaces
@ESP32DE ESP32DE requested a review from igrr October 15, 2021 12:39
@ESP32DE
Copy link
Contributor Author

ESP32DE commented Oct 18, 2021

@igrr
what you think to upset the FLASH_MAX_SIZE from 64MB to 128MB #678

@radimkarnis
Copy link
Collaborator

Hi @ESP32DE,
could you please squash the commits and rebase? I will then put the PR through our internal review queue and merge it. This way you will get a valid contribution on your profile.

@ESP32DE
Copy link
Contributor Author

ESP32DE commented Oct 21, 2021

Hi @ESP32DE, could you please squash the commits and rebase? I will then put the PR through our internal review queue and merge it. This way you will get a valid contribution on your profile.

hi @radimkarnis thank you for your help.
i did squash and rebased all in a new PR and closed this PR and the second PR
if any further work is need plz let me know.
txs

@ESP32DE ESP32DE closed this Oct 21, 2021
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.

None yet

3 participants