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

Adding merge_image functionality (ESPTOOL-79) #568

Closed
wants to merge 13 commits into from

Conversation

Barmaley13
Copy link

Description of change

Added command to merge multiple .bin files into one file for flashing.

Furthermore, you can create both unencrypted and encrypted images (-k is optional)
esptool.py --port PORT merge_image -k flash_key.bin @download.config

and flash those with 0x0 offset, for instance
esptool.py --port PORT -b 460800 write_flash --flash_mode dio --flash_freq 40m --flash_size detect 0x0 flash.bin

or similar (for the encrypted flash)
esptool.py --port PORT -b 460800 write_flash --flash_mode dio --flash_freq 40m --flash_size detect 0x0 flash.enc.bin

This change fixes the following bug(s):

#254

I have tested this change with the following hardware & software combinations:

Ubuntu 18.04
ESP32-WROOM-32D
ESP32

I have NOT run the esptool.py automated integration tests with this change and the above hardware.

This addition would not affect automated integration tests since this pull request does not modify existing functionality it appends it.

@github-actions github-actions bot changed the title Adding merge_image functionality Adding merge_image functionality (ESPTOOL-79) Dec 8, 2020
Copy link
Contributor

@projectgus projectgus left a comment

Choose a reason for hiding this comment

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

Hi @Barmaley13,

I'm sorry noone has gotten back to you for such a long time.

Thanks for contributing this, it's a useful function and something that's been obviously missing from esptool.py for some time.

This PR needs some work before we can accept it. If you're interested in reworking it then I can promise a < 2 month turnaround from now on - although if you'd rather not then that's OK as well.

As well as the inline comments, some other things:

  • The PR is showing a lot of unrelated changes (I just reviewed the merge_image function not the other code as it was mostly merge-related issues). Please rebase on the master branch to clean this up.
  • The PR is failing some of the automated Python checks (flake8, etc). Please check those out.
  • It would be great to have some unit tests that the output is correct, errors correctly detected, etc.
  • The --flash_size/--flash_mode options are actually not applied unless you flash a binary directly at the bootloader offset (so 0x1000 for ESP32). So this part won't quite work as described in the PR. It'd be great if these options could be supplied to this command as well, in the same way they can be supplied d to write_flash command, so that the output file is generated with the specified modes/etc.

if encrypt:
with open(unenc_name, 'rb') as input_file:
with open(enc_name, 'wb') as output_file:
espsecure._flash_encryption_operation(
Copy link
Contributor

Choose a reason for hiding this comment

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

the underscore prefix in python is for private methods. overall, as this command will probably be run as part of a script or a custom build process then I'd prefer to make encryption a separate step in the script - otherwise when we add ESP32-S2 flash encryption support (for example) this command will also need updating to support that.

Copy link
Author

Choose a reason for hiding this comment

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

You are probably right! I am going to leave it as it is for now. But maybe you can change it to serve esptool needs better

esptool.py Outdated Show resolved Hide resolved
esptool.py Outdated Show resolved Hide resolved
esptool.py Outdated
def check_if_possible(self):
for i in range(1, len(self.bin_array)):
if self.bin_array[i].addr <= (self.bin_array[i - 1].addr + self.bin_array[i - 1].size):
print(self.bin_array[i].addr, (self.bin_array[i - 1].addr + self.bin_array[i - 1].size))
Copy link
Contributor

Choose a reason for hiding this comment

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

debug statement left in by mistake?

It should be possible to use the AddrFilenamePairAction which is already used by write_flash command to test none of the binaries overlap.

Copy link
Author

Choose a reason for hiding this comment

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

Removed debug statement. I've also cleaned up overlap check. But by no means if we have this functionality else where we should use it rather than rewrite it.

esptool.py Outdated Show resolved Hide resolved
@Barmaley13
Copy link
Author

Barmaley13 commented Dec 15, 2020

Hi @projectgus,
Thank you for looking at this!
Yes, I took someone else's code that was not part of the esptool and glued things together to make it work since I needed that
functionality myself. I think I'd rather have someone else finish this effort since I know so much about esptool and still somewhat amateur at contributing to open source projects.

As well as the inline comments, some other things:

  • The PR is showing a lot of unrelated changes (I just reviewed the merge_image function not the other code as it was mostly merge-related issues). Please rebase on the master branch to clean this up.

Sorry, I have not used rebase much. I think I just did it though

  • The PR is failing some of the automated Python checks (flake8, etc). Please check those out.

Just finished cleaning up portion of the code. Will see what kind of suggestions travis will come up with

  • It would be great to have some unit tests that the output is correct, errors correctly detected, etc.

I would love to leave this for someone else. I am hoping this latest version is good enough for handing it over to someone else to polish it up to the finish line.

  • The --flash_size/--flash_mode options are actually not applied unless you flash a binary directly at the bootloader offset (so 0x1000 for ESP32). So this part won't quite work as described in the PR. It'd be great if these options could be supplied to this command as well, in the same way they can be supplied d to write_flash command, so that the output file is generated with the specified modes/etc.

Going to leave this as is. Since I am not sure if this is done as a binary file offset or dynamically at flash time

@positron96
Copy link

positron96 commented Jan 26, 2021

Hi guys. Is any work being done for this feature? This will be very useful in CI/CD pipelines that automatically build firmwares

@projectgus
Copy link
Contributor

projectgus commented Jan 26, 2021

Hi @positron96,

I'm working on this but I need to write some tests for it, hopefully it'll be in the master branch in the next week or two. This PR will be updated once that happens.

Angus

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.

4 participants