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

esptool.py: use upstream version, switch to python3 #14033

Closed
wants to merge 1 commit into from

Conversation

benpicco
Copy link
Contributor

@benpicco benpicco commented May 6, 2020

Contribution description

Instead of maintaining our own fork of esptool.py, switch to the upstream version. (Also includes support for ESP32-S2 now)

But patch it to use Python3.

Testing procedure

esp* can still be flashed.

Issues/PRs references

I would suddenly get this error espressif/esptool#350 after upgrading to Ubuntu 20.04
Using Python3 fixed it.

@benpicco benpicco added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Area: tools Area: Supplementary tools CI: run tests If set, CI server will run tests on hardware for the labeled PR labels May 6, 2020
@benpicco benpicco requested a review from gschorcht May 6, 2020 14:38
@benpicco benpicco removed the Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer label May 6, 2020
@benpicco benpicco changed the title esptool.py: use python3 esptool.py: use upstream version, switch to python3 May 6, 2020
@benpicco
Copy link
Contributor Author

benpicco commented May 6, 2020

gen_esp32part.py can also be found in esp-idf/components/partition_table/gen_esp32part.py so we shouldn't have to keep a copy of that either.

@gschorcht
Copy link
Contributor

(Also includes support for ESP32-S2 now)

I don't think that we will be able to support ESP32-S because it requires a much newer ESP-IDF version, I guess 4.1. We still use version 3.1 and it would not be easy to port the current RIOT implementation for ESP32 to this new ESP-IDF version. The reason is, that the RIOT port for ESP32 can't use the FreeRTOS based ESP-IDF. Rather it is uses a mixture of vendor code extracted from the ESP-IDF and changed vendor code to make it working with RIOT. I guess that a change from 3.1 to 4.1 would require to reimplement large parts of the RIOT port for ESP32 😟

I have tried such a version upgrade for the ESP8266. I have been working on it for 4 months and it is still not finished. So for now I would prefer to leave the ESP32 port as it is and continue using ESP-IDF version 3.1. In this respect, support for the ESP32-S is not a problem.

@gschorcht
Copy link
Contributor

gen_esp32part.py can also be found in esp-idf/components/partition_table/gen_esp32part.py so we shouldn't have to keep a copy of that either.

gen_esp32part.py is required for preparing the image for flashing. But flashing should be possible without having ESP-IDF installed. That's why we have our own copies of esptool.py and gen_esp32part.py.

@benpicco
Copy link
Contributor Author

benpicco commented May 7, 2020

I don't think that we will be able to support ESP32-S because it requires a much newer ESP-IDF version

No pressure 😉
I just thought that would be a nice bonus.

In the first version of this PR I just replaced #!/usr/bin/env python with #!/usr/bin/env python3 because of the error I got (missing Python 2 libraries) and all the other tools in RIOT use Python 3.

I then started to wonder why we keep a copy of esptool.py in the RIOT repo, as this encourages modifying it, which makes upgrades harder.

So I replaced it with a pkg like it's done with other tools that have an upstream.
Upstream still didn't switch to Python3, so I added the patch, but now to the pkg.

@benpicco benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jun 24, 2020
Use the upstream version instead of maintaining our own fork.
Also explicitely use Python3.
@gschorcht
Copy link
Contributor

gschorcht commented Jun 29, 2020

Sorry for the delay. I was completely busy in last three months and starting resume my work on RIOT now.

Our esptool.py version was extracted from the ESP8266_RTOS_SDK. The reason was that the upstream version can't be used to generate ESP8266 images using the ESP8266_RTOS_SDK (firmware V3).

While our esptool.py allows to specify generate an ESP8266 image for firmware V3

if args.chip == 'esp32':
image = ESP32FirmwareImage()
elif args.version == '1': # ESP8266
image = ESP8266ROMFirmwareImage()
elif args.version == '2': # ESP8266
image = ESP8266V2FirmwareImage()
else:
image = ESP8266V3FirmwareImage()
the upstream repository doesn't https://github.com/espressif/esptool/blob/master/esptool.py#L2837-L2852.

That is, the upstream version will not work for ESP8266 😟

@benpicco
Copy link
Contributor Author

Huh that's weird - I would have expected the upstream to version to support everything.
Could it be that they folded both v2 and v3 into ESP8266V2FirmwareImage()?

@gschorcht
Copy link
Contributor

gschorcht commented Jun 30, 2020

Huh that's weird - I would have expected the upstream to version to support everything.

Indeed. That is what someone would expect, especially if the same guys who are developing esptool.py are involved in the development of the ESP8266_RTOS_SDK. But as I said, the current master of ESP8266_RTOS_SDK uses still its own version of esptool.py which know a firmware version 3.

Could it be that they folded both v2 and v3 into ESP8266V2FirmwareImage()?

I don't think so. I have tried to use the upstream version of esptool.py. But with --version 2 here

FLASH_OPTS = --version 3
the bootloader says:

E (48) esp_image: image at 0x10000 has invalid magic byte
E (48) boot: Factory app partition is not bootable
E (48) boot: No bootable app partitions in the partition table
user code done

That is, the bootloader expects another magic byte. Using --version 3 as option for the right firmware version was determined from the ESP8266_RTOS_SDK make system.

Finally, we should keep our own version as long as ESP8266_RTOS_SDK also uses its own version. Therefore we should close this PR in favor of PR #14392.

@benpicco benpicco closed this Jun 30, 2020
@benpicco benpicco deleted the esptool-python3 branch June 30, 2020 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: tools Area: Supplementary tools CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants