produce merged binaries with injected wifi creds for web flashable firmware files#690
produce merged binaries with injected wifi creds for web flashable firmware files#690
Conversation
Summary of ChangesHello @tavdog, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the user experience for initial device setup by enabling the generation of web-flashable firmware files that come pre-configured with WiFi credentials. By producing "merged" binaries that include all necessary components (bootloader, partition table, and application), users can now flash their devices directly via web-based tools at address Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the capability to generate merged, web-flashable firmware binaries with injected credentials. The changes include:
- A new
GenerateMergedfunction to create full firmware images. - An update to the firmware generation logic to use a more efficient checksum delta calculation.
- UI changes to support selecting between OTA-only and full merged firmware generation, along with updated flashing instructions.
Overall, this is a great enhancement that simplifies the initial device flashing process for users. I've provided a couple of suggestions for improvement:
- An efficiency improvement in
GenerateMergedto avoid reading the entire firmware file into memory. - A small readability enhancement in the checksum calculation loop.
| mergedContent, err := os.ReadFile(mergedPath) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("merged firmware file not found (needed for bootloader/partition): %s", mergedPath) | ||
| } | ||
|
|
||
| if len(mergedContent) < MergedAppOffset { | ||
| return nil, fmt.Errorf("merged firmware file too short to contain preamble") | ||
| } | ||
|
|
||
| // Extract the preamble (first 0x10000 bytes containing bootloader + partition table) | ||
| preamble := mergedContent[:MergedAppOffset] |
There was a problem hiding this comment.
The current implementation reads the entire merged firmware file into memory, even though only the first 0x10000 bytes (the preamble) are needed. This can be inefficient for large firmware files. You can optimize this by opening the file and reading only the required number of bytes using io.ReadFull.
| mergedContent, err := os.ReadFile(mergedPath) | |
| if err != nil { | |
| return nil, fmt.Errorf("merged firmware file not found (needed for bootloader/partition): %s", mergedPath) | |
| } | |
| if len(mergedContent) < MergedAppOffset { | |
| return nil, fmt.Errorf("merged firmware file too short to contain preamble") | |
| } | |
| // Extract the preamble (first 0x10000 bytes containing bootloader + partition table) | |
| preamble := mergedContent[:MergedAppOffset] | |
| mergedContent, err := os.Open(mergedPath) | |
| if err != nil { | |
| return nil, fmt.Errorf("merged firmware file not found (needed for bootloader/partition): %s", mergedPath) | |
| } | |
| defer mergedContent.Close() | |
| // Extract the preamble (first 0x10000 bytes containing bootloader + partition table) | |
| preamble := make([]byte, MergedAppOffset) | |
| if _, err := io.ReadFull(mergedContent, preamble); err != nil { | |
| return nil, fmt.Errorf("merged firmware file too short to contain preamble: %w", err) | |
| } |
There was a problem hiding this comment.
yeah this could be a good idea. gotta go to bed now though so can't test it.
|
I've tested the firmware generation for flash at 0x0, wifi portal triggered OTA and server triggered OTA. Seems to be working. Only issue I ran into was that the the merged files don't get downloaded if the firmware version is already up to date. |
| @@ -648,23 +648,10 @@ func (dt DeviceType) FirmwareFilename(swapColors bool) string { | |||
|
|
|||
| func (dt DeviceType) MergedFilename(swapColors bool) string { | |||
There was a problem hiding this comment.
Is this intentionally mapping Tidbyt Gen1+2 to the same file (as well as S3, S3-Wide, and the Matrix Portals)?
There was a problem hiding this comment.
yes, the merged bins bootloader and partitions don't change between gen1/gen2 or between any S3 target.
There was a problem hiding this comment.
yes, the merged bins bootloader and partitions don't change between gen1/gen2 or between any S3 target.
There was a problem hiding this comment.
Ah, I see that you're only taking the bootloader and partition table from there and then add the patched app. I assumed that you're patching the app partition contained in the merged bin, in which case they would need to be device-specific.
…files and re-download firmware if not.
2 participants
Produce merged firmware files with injected firmware for initial webflash and later OTA capable device. Also change the firmware instructions to use webflashers instead of espflasher app