Skip to content

fixes GitHub automated build#294

Closed
nanch wants to merge 1 commit intoInfiniTimeOrg:developfrom
nanch:nan_fix_build
Closed

fixes GitHub automated build#294
nanch wants to merge 1 commit intoInfiniTimeOrg:developfrom
nanch:nan_fix_build

Conversation

@nanch
Copy link

@nanch nanch commented Apr 18, 2021

#268

This resolves the build issues.

I added workflow_dispatch so that builds can be triggered manually on the workflow page. This is useful for debugging and monitoring the build process.

the cbor python module is not in the mcuboot/scripts/requirements.txt file, so I added that into the workflow. Interestingly, cbor is in the source, but not in the version that gets checked-out (perhaps a caching issue or checking out the wrong version?) Either way, installing it as a build step is trivially adequate.

The input to the imgtool.py was not correct because multiple files were being passed into the input. I used a grep to select the correct file to use as the input to imgtool.py

submodules are required in the checkout because lvgl is a submodule (I think). Without submodules, the lvgl.h wasn't being included properly.

ACTIONS_ALLOW_UNSECURE_COMMANDS is needed since github updated their workflow configuration.

@petterhs
Copy link
Contributor

Great work! Nice to see this building again.

I don't have any experience with github actions but it seems like there is still something wrong with the uploaded dfu artifact. The size is too small and it fails if I try to DFU with it. Optimally it should also upload the pinetime-mcuboot-app-image-x.y.z.bin file aswell.

@pfeerick
Copy link
Contributor

I can't reproduce that failure... I was able to flash the DFU zip present at https://github.com/JF002/InfiniTime/actions/runs/761649804 ... BUT it seems it was rebuild about 5 minutes ago, even though there was no change to this PR... so I can't discount variation in the file.

@pfeerick pfeerick mentioned this pull request Apr 23, 2021
@nanch
Copy link
Author

nanch commented Apr 27, 2021

@petterhs

Great work! Nice to see this building again.

I don't have any experience with github actions but it seems like there is still something wrong with the uploaded dfu artifact. The size is too small and it fails if I try to DFU with it. Optimally it should also upload the pinetime-mcuboot-app-image-x.y.z.bin file aswell.

I agree that there are improvements to be made. I didn't want to make too many changes at once without a successfully merged pull request first.

My goal here was to "get the build working" with minimal changes.

Once this pull request is merged I'm happy to define the exported artifacts optimally.

There are many artifacts that can/should be exported. Which artifact is useful at any given time depends on what you're trying to do. For most users who are simply flashing to update, artifact D (below) is all they need.

All possible artifacts (as far as I understand):

A: infinitime image without MCUBoot bootloader support (flash-write at offset 0x00000000) [No bootloader support]

B: MCUBoot bootloader image (flash-write with offset 0x00000000) [Bootloader support]
C: infinitime image with MCUBoot bootloader support [Bootloader support]

D: Device Firmware Upgrade (DFU) .zip file ([C] infinitime image with MCUBoot bootloader support) [Bootloader support]

E: raw pinetime-recovery-1.0.0.bin (flash-write with offset 0x00000000) [No bootloader support]
F: pinetime-mcuboot-recovery-loader-1.0.0.bin [Bootloader support]

For ease of the reader, the MCUboot bootloader images are most recently referenced in the releases at https://github.com/JF002/InfiniTime/releases/tag/0.14.1

@nanch
Copy link
Author

nanch commented Apr 27, 2021

@petterhs @pfeerick

something wrong with the uploaded dfu artifact

I can't reproduce that failure

I'll fork, define the artifacts, test each of the artifacts, and create a follow-up pull request.

@JF002 JF002 added this to the Version 1.1 milestone May 2, 2021
make pinetime-mcuboot-app
echo ""

- name: Create firmware image
Copy link
Contributor

Choose a reason for hiding this comment

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

this step should be deleted -- make pinetime-mcuboot-app already does this (except the file name is image instead of img)

Copy link
Collaborator

Choose a reason for hiding this comment

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

@DavidVentura which step are you talking about here ? Make pinetime-mcuboot-app or Create firmware image

${{ runner.temp }}/mcuboot/scripts/imgtool.py create --align 4 --version 1.0.0 --header-size 32 --slot-size 475136 --pad-header build/src/${IMGTOOL_INPUT_ARGUMENT} build/src/pinetime-mcuboot-app-img.bin
${{ runner.temp }}/mcuboot/scripts/imgtool.py verify build/src/pinetime-mcuboot-app-img.bin

- name: Create DFU package
Copy link
Contributor

Choose a reason for hiding this comment

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

this one also could be removed by enabling DFU in the Cmake step

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right! It was not the case before, but now, images and DFU files are generated directly by CMake so that you don't need to add custom step after the build. You can have a look at the docker file to see how it's done :

Previously, the post build script generated images and DFU, but now it just copies the binaries into the output folder.

Basically, all you have to do is add -DBUILD_DFU=1 to your cmake command line.

@JF002
Copy link
Collaborator

JF002 commented May 17, 2021

@nanch Could you have a look at the review @DavidVentura and myself did about this PR?

@JF002 JF002 removed this from the Version 1.1 milestone May 21, 2021
@JF002
Copy link
Collaborator

JF002 commented Sep 1, 2021

CI builds were (finally) fixed in #579. Feel free to contribute and improve Github Actions configuration in a new PR ;)

@JF002 JF002 closed this Sep 1, 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.

5 participants