analogdevicesinc / precision-converters-firmware

Precision Converters Embedded Firmware Repository
https://analogdevicesinc.github.io/precision-converters-firmware/
Apache License 2.0
15 stars 18 forks source link

Initial Github build support #16

Closed pamolloy closed 1 year ago

pamolloy commented 1 year ago

Fixes #14

mphalke commented 1 year ago

@pamolloy Thanks for your contribution. Changes look good to me. Good to see that makefile is renamed to Makefile, it was something I was also planning to do.

One question- We do build all our projects using the Jenkins CI/CD infrastructure and artifacts are uploaded on the ADI server. Each project consists of multiple matrix combinations (basically different macros enabled for generating different binaries). E.g. https://github.com/pamolloy/precision-converters-firmware/blob/philip/linux-build-support/projects/ad4130_iio/ci_build.groovy The git CI support that you have added, does it take care of all matrix combinations, or does it just build the default firmware combination?

pamolloy commented 1 year ago

One question- We do build all our projects using the Jenkins CI/CD infrastructure and artifacts are uploaded on the ADI server. Each project consists of multiple matrix combinations (basically different macros enabled for generating different binaries). E.g. https://github.com/pamolloy/precision-converters-firmware/blob/philip/linux-build-support/projects/ad4130_iio/ci_build.groovy The git CI support that you have added, does it take care of all matrix combinations, or does it just build the default firmware combination?

Wow! I thought there were already a lot of binaries. The Github CI/CD is currently building without defining any macros, but it should be pretty easy to add. Github has support for a matrix feature that I already included, but only as a one dimensional matrix. It should be simple to add, but I just created #17 to keep this PR from getting too big.

mphalke commented 1 year ago

One question- We do build all our projects using the Jenkins CI/CD infrastructure and artifacts are uploaded on the ADI server. Each project consists of multiple matrix combinations (basically different macros enabled for generating different binaries). E.g. https://github.com/pamolloy/precision-converters-firmware/blob/philip/linux-build-support/projects/ad4130_iio/ci_build.groovy The git CI support that you have added, does it take care of all matrix combinations, or does it just build the default firmware combination?

Wow! I thought there were already a lot of binaries. The Github CI/CD is currently building without defining any macros, but it should be pretty easy to add. Github has support for a matrix feature that I already included, but only as a one dimensional matrix. It should be simple to add, but I just created #17 to keep this PR from getting too big.

Got it. Let me discuss internally on this. We usually build our projects using Jenkins CI/CD and that is why we have all our CI scripts present in respective projects. It's good to have git based CI build but then we need to decide if can either use git based builds or Jenkins based builds. @mike-bradley do you have any suggestions here?

mphalke commented 1 year ago

@pamolloy Are you generating artifacts for the builds as well through CI scripts? e.g. this is how daily build is creating artifacts on the no-os repo. https://github.com/analogdevicesinc/no-OS/releases/tag/last_commit

pamolloy commented 1 year ago

@pamolloy Are you generating artifacts for the builds as well through CI scripts? e.g. this is how daily build is creating artifacts on the no-os repo. https://github.com/analogdevicesinc/no-OS/releases/tag/last_commit

Yup, the workflow using the Mbed CLI 1 directly uploads a binary for the AD4130 while the workflow using no-OS uploads binaries for all projects.

Maintaining both build systems doesn't make sense in the longer term, but for now there is nothing blocking having both. Besides the integration with Github, one nice benefit of the Github YAML is it is very concise making it easy to maintain and serves as documentation of how to build the system. Even if documentation is out-of-date the Github YAML is a fairly concise description of how to build the project (see build-no-os.yaml). Another benefit is that people who fork the repository can run the CI/CD before opening a merge request.

Note you'll probably also want to develop on a separate branch internally rather than committing on main in this repo and main in your internal repo.

Also, if you haven't already you may want to configure the Github repository to not use merge commits (see Rebasing and merging your commits and Configuring commit rebasing for pull requests).

mphalke commented 1 year ago

@pamolloy Are you generating artifacts for the builds as well through CI scripts? e.g. this is how daily build is creating artifacts on the no-os repo. https://github.com/analogdevicesinc/no-OS/releases/tag/last_commit

Yup, the workflow using the Mbed CLI 1 directly uploads a binary for the AD4130 while the workflow using no-OS uploads binaries for all projects.

Maintaining both build systems doesn't make sense in the longer term, but for now there is nothing blocking having both. Besides the integration with Github, one nice benefit of the Github YAML is it is very concise making it easy to maintain and serves as documentation of how to build the system. Even if documentation is out-of-date the Github YAML is a fairly concise description of how to build the project (see build-no-os.yaml). Another benefit is that people who fork the repository can run the CI/CD before opening a merge request.

Note you'll probably also want to develop on a separate branch internally rather than committing on main in this repo and main in your internal repo.

Also, if you haven't already you may want to configure the Github repository to not use merge commits (see Rebasing and merging your commits and Configuring commit rebasing for pull requests).

Thanks for the reply. Make sense to have git based build system, specially for end users. Going forward we can decide if we want to have only one or both of them (i.e. Jenkins and github). I see currently it is just building the project and not doing any hardware test. Do you have testing in mind as well? We have added jenkins scripts to test the hardware as well, which relies on Mbed CLI and CMSIS-DAP hardware link to load the binary into MCU and then running pytest based python scripts to do hardware testing. Could something similar to done with github? E.g. https://github.com/analogdevicesinc/precision-converters-firmware/blob/main/projects/ad4130_iio/ci_build.groovy#L217 https://github.com/analogdevicesinc/precision-converters-firmware/tree/main/projects/ad4130_iio/tests

I also saw open issue about building multiple FW matrixes for single project, which is great. Thanks for that: https://github.com/pamolloy/precision-converters-firmware/blob/philip/build-config-matrix/.github/workflows/build-ad4130-iio.yaml

Let me closely review PR again and discuss with team. I see there are 16 commits added for this PR, looks like those needs to be squashed together for this initial work which can be done once this work is about to be finished and ready to merge.

Thanks for other points mentioned above as well.

CLAassistant commented 1 year ago

CLA assistant check
All committers have signed the CLA.

mphalke commented 1 year ago

@pamolloy I have deleted the commits from main branch which were added to add git action based support to our repo. The files were mainly taken from this PR that you have created. Intention was that until some work is progressed on this PR, we could have basic build support present on the repo and later once this PR is ready, it can be merged back to main branch. In my last review comment also I have mentioned that this PR looks good to merge in it's current form. Apologies not keeping you in loop while moving these changes directly into main branch. We can merge them one you are ready. We need to fix some conflicts and fix some of the review comments provided above.

pamolloy commented 1 year ago

In my implementation I had the following setup, with which I was able to run mbed deploy:

libraries/
  no-OS <-- submodule
libs/
  no-OS.lib

The current repo looks like:

libraries/
  no-OS <-- submodule
  no-OS.lib

Now when I run mbed deploy I run into the following:

[mbed] ERROR: Library reference "CMSIS-DSP.lib" points to a folder "/home/runner/work/precision-converters-firmware/precision-converters-firmware/libraries/CMSIS-DSP", which is not a valid repository.

I've tried building normally and also cloning the submodules first, but they result in the same error. I've reproduced the issue in a local container as well.

Note that mbed deploy appears to create directories under libs for no-OS and precision-converters-library.

pamolloy commented 1 year ago

By moving the lib files I was able to get the project to compile, but it currently is failing on a missing include in no-OS.

pamolloy commented 1 year ago

Giving up for tonight. I'm not sure what changed, but I just spent a lot of time adding parts of no-OS to .mbedignore, which I didn't need to do when I had this working initially.

mphalke commented 1 year ago

Giving up for tonight. I'm not sure what changed, but I just spent a lot of time adding parts of no-OS to .mbedignore, which I didn't need to do when I had this working initially.

Not sure what is the issue. We have been able to build all the mbed projects correctly using make. I have given some comments on PR so have a look if anything is missed out. Also we should not use any customized yaml scripts for each platform. We should leverage the existing 'make' scripts from no-os to build any platform such as Mbed, STM32, Maxim, etc. That way we don't add any extra dependency on the platform here.

*Note on STM32 build: @CMinajigi from firmware team was exploring the adaption of make files to build our STM32 projects. He has some initial observation listed down below: At the moment STM32 build works only on Linux system and as github actions uses linux, they should work well. However we have discovered some issues using existing no-os STM32 make script. That script is written to target only STM32F4 platform by default if you run make command to build the project based on .ioc file. There is alternate target called sdk_build which can be used to build any STM32 MCU/board as it uses .ioc file to create a new project and then do the headless build. Another catch is, .ioc file needs to be placed in root directory of project and not inside STM32 folder. But this might be avoided by making changed into STM32 make script from no-os. Just wanted to give you heads-up, so that you are aware of this.

Ultimately we don't want to create any new scripts for building the projects and just rely on make script support.

mphalke commented 1 year ago

In my implementation I had the following setup, with which I was able to run mbed deploy:

libraries/
  no-OS <-- submodule
libs/
  no-OS.lib

The current repo looks like:

libraries/
  no-OS <-- submodule
  no-OS.lib

Now when I run mbed deploy I run into the following:

[mbed] ERROR: Library reference "CMSIS-DSP.lib" points to a folder "/home/runner/work/precision-converters-firmware/precision-converters-firmware/libraries/CMSIS-DSP", which is not a valid repository.

I've tried building normally and also cloning the submodules first, but they result in the same error. I've reproduced the issue in a local container as well.

Note that mbed deploy appears to create directories under libs for no-OS and precision-converters-library.

@pamolloy let's try just using libraries folder which contains both submodules and .lib files. We have been able to build all the projects with this approach (both with make scripts from no-os repo and with online keil studio). We should not create unnecessary confusion by creating another folder for .lib files.

pamolloy commented 1 year ago

Closing this since the series has been broken up into separate pull requests.