chore: include bootloader in release images #197
Conversation
Reviewer's Guide by SourceryThis pull request modifies the build process to include the bootloader in the release images. It clones the bootloader repository, builds the bootloader, downloads and installs the XC8 compiler, combines the bootloader and firmware hex files, and updates the publish build files step to upload the combined hex file. Sequence diagram for combining hex filessequenceDiagram
participant GitHub Actions
participant pslab-bootloader
GitHub Actions->>pslab-bootloader: mv pslab-firmware.hex
GitHub Actions->>pslab-bootloader: chmod +x combine_hex.sh
GitHub Actions->>pslab-bootloader: ./combine_hex.sh
pslab-bootloader-->>GitHub Actions: combined.hex
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @Tejasgarg - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider using a Docker image with the necessary toolchain pre-installed to avoid installing the XC8 compiler in the workflow.
- It would be cleaner to combine the hex files using a tool like
srec_catinstead of a custom script.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟡 Security: 1 issue found
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
.github/workflows/main-builder.yml
Outdated
| @@ -64,9 +64,41 @@ jobs: | |||
| cd build_${{ matrix.target }} | |||
| cmake .. ${{ matrix.cmake_flags}} | |||
| make | |||
|
|
|||
| - name: Build bootloader | |||
There was a problem hiding this comment.
suggestion: Pin bootloader repository version for reproducible builds
Consider cloning a specific commit, tag, or branch that is known to work rather than the default branch to ensure build reproducibility.
Suggested implementation:
- name: Build bootloader
run: |
git clone https://github.com/fossasia/pslab-bootloader.git pslab-bootloader
cd pslab-bootloader
git checkout tags/v1.0.0
git submodule update --init
mkdir build
cd build
cmake ..
make
Make sure that the tag "v1.0.0" exists in the bootloader repository, or replace it with the appropriate tag/commit hash that is known to work. Adjust the tag name to the desired version if necessary.
|
|
||
| - name: Download XC8 Compiler |
There was a problem hiding this comment.
🚨 suggestion (security): Consider verifying the downloaded installer
A checksum verification or similar integrity check after downloading the installer could help ensure that the file is not corrupted or tampered with.
Suggested implementation:
- name: Download XC8 Compiler
if: steps.cache-compiler.outputs.cache-hit != 'true'
run: |
# Download XC8 Compiler installer
wget https://example.com/path/to/xc8_installer.run -O xc8_installer.run
# Verify installer checksum to ensure integrity
expected_checksum="abc123def456ghi789jkl012mno345pq" # Replace with the actual expected checksum value
actual_checksum=$(sha256sum xc8_installer.run | awk '{print $1}')
if [ "$actual_checksum" != "$expected_checksum" ]; then
echo "Checksum verification failed! Expected: $expected_checksum, Got: $actual_checksum."
exit 1
fi
# Continue with installation or further steps
Please update the download URL (https://example.com/path/to/xc8_installer.run) and expected_checksum with the actual values as required by your project.
.github/workflows/main-builder.yml
Outdated
| - name: Combining hex files | ||
| run: | | ||
| mv ${{ matrix.branch }}/build_${{ matrix.target }}/pslab-firmware.hex pslab-bootloader/build/pslab-firmware.hex | ||
| cd pslab-bootloader | ||
| chmod +x combine_hex.sh | ||
| ./combine_hex.sh |
There was a problem hiding this comment.
suggestion: Improve error handling in hex file combination step
Adding error handling (for example, using 'set -e' to fail on errors) in the script block for moving files and executing combine_hex.sh would help catch issues earlier in the process.
| - name: Combining hex files | |
| run: | | |
| mv ${{ matrix.branch }}/build_${{ matrix.target }}/pslab-firmware.hex pslab-bootloader/build/pslab-firmware.hex | |
| cd pslab-bootloader | |
| chmod +x combine_hex.sh | |
| ./combine_hex.sh | |
| - name: Combining hex files | |
| run: | | |
| set -e | |
| mv ${{ matrix.branch }}/build_${{ matrix.target }}/pslab-firmware.hex pslab-bootloader/build/pslab-firmware.hex | |
| cd pslab-bootloader | |
| chmod +x combine_hex.sh | |
| ./combine_hex.sh |
There was a problem hiding this comment.
Hi @Tejasgarg, thanks for the pull request!
Since we already build the bootloader in the bootloader repo, I would prefer to not duplicate the build script here. Instead, use the release image from the bootloader repo (https://github.com/fossasia/pslab-bootloader/releases/latest).
Fixes #189
As @bessman mentioned in the issue:
The current release process automatically builds and attaches firmware images to the release page. These firmware images include only the application firmware, not the bootloader. If someone were to flash these images with a PICkit, the existing bootloader would be erased and the PSLab would no longer be able to enter bootloader mode.
So, the bootloader image is added to the application image.
Did it in the following steps-
After the download, move to the installation.
Successful action run:
https://github.com/Tejasgarg/pslab-firmware/actions/runs/14147713427/job/39637063463
Summary by Sourcery
Modify the release build process to include the bootloader in the firmware image, ensuring a complete firmware package that can be flashed to the PSLab board without losing bootloader functionality.
New Features:
CI:
Chores: