Skip to content

Comments

Workflow for avr build#405

Draft
brentfpage wants to merge 11 commits intoespotek-org:masterfrom
brentfpage:workflow_for_avr_build_b
Draft

Workflow for avr build#405
brentfpage wants to merge 11 commits intoespotek-org:masterfrom
brentfpage:workflow_for_avr_build_b

Conversation

@brentfpage
Copy link
Contributor

@brentfpage brentfpage commented Jan 30, 2026

(Following up on some of the discussion in #381.)
This PR:

  • adds a workflow (avr.yml) for the firmware
  • makes edits to continuous.yml:
    • adds a firmware compilation job
    • adds support for the mac build scheme described in the bullet below
  • revises mac.yml so that:
    • when mac.yml is called by continuous.yml, the firmware .hex asset produced by continuous.yml is packaged into the mac desktop interface
    • when mac.yml is called in some other context, the firmware .hex asset produced by the most recent call to continuous.yml is packaged into the desktop interface

The versioning is centralized, but not automatic. Namely, the current version of the firmware has to be set in a repository-wide variable, AVR_VER, under (repo home)->settings->secrets+variables->actions->variables->(repository variables) avr_ver_def.yml. In the fork, AVR_VER=0x0007. At compile time, this variable gets injected:

  • into AVR_Code as the macro FIRMWARE_VERSION_ID, which previously was defined in AVR_Code/globals.h
  • into the desktop interface as the macro EXPECTED_FIRMWARE_VERSION, which previously was defined in Desktop_Interface/genericusbdriver.h

It would be nicer to have AVR_VER defined in some location like continuous.yml instead, but it needs to be available to mac.yml when that workflow is called individually. I don't know of any other place to define AVR_VER where it would be available to both workflows.

Also, the inclusion of the version number in the .hex file names has been retained, so they are, e.g., labrafirm_0007_0{1 or 2}.hex. To me, this is worthwhile because then the firmware that's packaged into a given release can be very easily determined.

If we decide to go forward with this approach, extending it to encompass the android, linux, and windows builds I think would be straightforward. Also, a prerequisite for merging would be #404 .


Strikethrough and replacement edits made 2/15/2026

…; also removed hard-coded firmware version macros in the Desktop Interface and the firmware
@brentfpage brentfpage marked this pull request as draft January 30, 2026 08:13
@brentfpage
Copy link
Contributor Author

Copying from a previous draft of this PR:


Posted by @mi-hol

@brentfpage I like the approach documented above and would approve it. With the number of changes a real review seems impossible though.

Lets see how other maintainers react.

The is only a minor nit pick

Namely, the current version of the firmware has to be set in a repository-wide variable, AVR_VER

I'd suggest to change name of variable from AVR_VER to AVR_FIRMWARE_VERSION so its purpose is completely self explanatory

@brentfpage brentfpage changed the title Workflow for avr build (ver. b) Workflow for avr build Jan 31, 2026
@mi-hol
Copy link
Contributor

mi-hol commented Feb 7, 2026

@brentfpage I triggered re-run of all workflows after merging #404 but they still fail

@brentfpage
Copy link
Contributor Author

brentfpage commented Feb 7, 2026

@brentfpage I triggered re-run of all workflows after merging #404 but they still fail

I just added AVR_VER as a repo-wide variable here (harmless and reversible) , which should make the mac build work. The other builds with automated firmware packaging have not been implemented. AVR_VER is now defined in avr_ver_def.yml

@brentfpage
Copy link
Contributor Author

As an aside, the firmware that's currently configured to be packaged is that from the most recent "continuous release" in my fork. This practice will be necessary unless/until this PR is fully merged into the genuine repo and a continuous release is run that generates firmware files.

@mi-hol
Copy link
Contributor

mi-hol commented Feb 8, 2026

added AVR_VER as a repo-wide variable here (harmless and reversible) , which should make the mac build work.

Well I re-ran and it still failed :(
Is something preventing you from running these actions in your fork?
Running them would give you the confidence that the changes actually work as expected and is the key to merging them quickly.
I trust in contributors to provide 100% working changes after initial discussions have completed within the team about a proposed approach.


scoperangeenterdialog.cpp:4:131: warning: unused parameter 'delay' [-Wunused-parameter]
    4 | scopeRangeEnterDialog::scopeRangeEnterDialog(QWidget *parent, bool buttonVisible, double yTop, double yBot, double window, double delay) :
      |                                                                                                                                   ^
genericusbdriver.cpp:479:75: error: expected expression
  479 |     qDebug("EXPECTING FIRMWARE VERSION 0x%04hx", EXPECTED_FIRMWARE_VERSION);
      |                                                                           ^
genericusbdriver.cpp:484:45: error: expected expression
  484 |     if((firmver != EXPECTED_FIRMWARE_VERSION) || (variant != DEFINED_EXPECTED_VARIANT)){
      |                                             ^
1 warning generated.
2 errors generated.

The other builds with automated firmware packaging have not been implemented.

Please add these changes, test, change status to "ready for review" and we'll review again after that has happened.

@brentfpage
Copy link
Contributor Author

brentfpage commented Feb 8, 2026

Unfortunately I had missed this key point:
"[variables] are not passed to workflows that are triggered by a pull request from a fork"
from the heading of https://github.com/espotek-org/Labrador/settings/variables/actions . So, the workflow won't work in full unless/until the PR is merged. As a stop-gap solution, I simply hard-coded the value of AVR_VER (0x0007) in mac.yml. The mac build now works.

The PR is still a draft (only one workflow has been implemented), so I'll hold off on indicating that it's "Ready for review".

@brentfpage brentfpage marked this pull request as ready for review February 8, 2026 23:25
@brentfpage brentfpage marked this pull request as draft February 8, 2026 23:25
@brentfpage
Copy link
Contributor Author

Now, AVR_VER is defined using avr_ver_def.yml and has been removed as a repo-wide variable. This approach seems preferable as avr_ver_def.yml can be more easily found than the Github variables tab and can also be more easily edited without an internet browser.

@mi-hol
Copy link
Contributor

mi-hol commented Feb 16, 2026

This approach seems preferable as avr_ver_def.yml can be more easily found than the Github variables tab and can also be more easily edited without an internet browser.

@brentfpage I fully share your conclusion.
It seems we are getting very close to merging.
Keep on the great work!

@EspoTek
Copy link
Collaborator

EspoTek commented Feb 19, 2026

Just to add some hairiness, the Windows installer currently needs to be manually updated whenever we bump the firmware version.

This is one of the reasons we were sticking with pre-built binaries for everything.

I think it makes sense to have a job for automatically building the firmware, but unless we find a clean solution to the Windows Installer problem I don't think we want to be auto-pulling new builds in.

@brentfpage
Copy link
Contributor Author

brentfpage commented Feb 23, 2026

@EspoTek Is it possibly the case that the manually updated installer is downloaded in windows.yml with this line?

Invoke-WebRequest -Uri "http://espotek.com/ai167.msi" -OutFile "ai167.msi"

If so, could you please elaborate on what steps are necessary for generating that .msi file?

@EspoTek
Copy link
Collaborator

EspoTek commented Feb 23, 2026

@EspoTek Is it possibly the case that the manually updated installer is downloaded in windows.yml with this line?

Invoke-WebRequest -Uri "http://espotek.com/ai167.msi" -OutFile "ai167.msi"

If so, could you please elaborate on what steps are necessary for generating that .msi file?

That like there is installed a (proprietary) program called "Advanced Installer", that's converts the .aip (Advanced Installer Project) file to the .msi package.

If we ever increment the firmware version, we'll need to either manually add them to the Advanced Installer project or possibly take advantage of some Advanced Installer feature that automatically adds them for us?

@mi-hol
Copy link
Contributor

mi-hol commented Feb 23, 2026

we'll need to either manually add them to the Advanced Installer project or possibly take advantage of some Advanced Installer feature that automatically adds them for us?

Updated
@brentfpage @EspoTek I started already some time ago a branch to simplify & standardize the Advanced Installer project.

  • Therefore continuing to improve the currently used approaches seems to be a short-term fix only
  • mid-term I'd prefer to simplify the whole handling of firmware variants and updates.

Reasoning is:

  • major differences in code dealing with USB for unclear (aka undocumented) reasons (at least to me)
  • many "loose ends" due to unmanaged dependencies to old USB library releases

The open questions in this regard are for me:

Does my reasoning make sense?

@mi-hol
Copy link
Contributor

mi-hol commented Feb 23, 2026

@EspoTek Is it possibly the case that the manually updated installer is downloaded in windows.yml with this line?

Invoke-WebRequest -Uri "http://espotek.com/ai167.msi" -OutFile "ai167.msi"

If so, could you please elaborate on what steps are necessary for generating that .msi file?

That link there is installed a (proprietary) program called "Advanced Installer", that's converts the .aip (Advanced Installer Project) file to the .msi package.

If we ever increment the firmware version, we'll need to either manually add them to the Advanced Installer project or possibly take advantage of some Advanced Installer feature that automatically adds them for us?

To reduce dependency on the specific server http://espotek.com via insecure http protocol I'm in favour of converting this piece of code to a GH action from marketplace. I'd be happy to own that task.

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.

3 participants