Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support Separate Targets Repo #566

Merged
merged 7 commits into from
Oct 23, 2023
Merged

Conversation

justinlampley
Copy link
Collaborator

Initial attempt at supporting separate targets repo.

Issues to be resolved:

  • When selecting a version of the firmware that can use the binary configurator, but before the hardware folder was split out from the firmware and moved to the targets repo, if a target is selected that wasn't included in that firmware it will error out. Example: select version 3.3.0 and the device "DIY ESP32-S3 DevKit Gemini 2.4Ghz TX", the build will fail since that target does not exist in the hardware folder included with that firmware version.

@pkendall64
Copy link
Contributor

My thoughts are

  1. If the user is flashing a release then we should select the target from targets.json, which should be downloaded from artifact repository as hardware.zip.
  2. If the user is building/flashing a git branch, commit or PR then we should update the git repo, pull the targets repo into hardware folder and build/flash.

So ultimately if flashing a release there should not be any access to github at all!

src/ui/config/index.ts Outdated Show resolved Hide resolved
@justinlampley
Copy link
Collaborator Author

@pkendall64 The artifact repository could be used instead of pulling down from the targets repo, but the list of release versions would still be pulled from GitHub, so it would not entirely eliminate the need to access GitHub.

Regarding the issue of trying to use a target that didn't exist at the time of a particular version being released, if I check the latest cloud build for master (https://artifactory.expresslrs.org/ExpressLRS/0bc0fa3a6fa9733f56c7795d26ca5eb1f9ee8e61/firmware.zip), it has a hardware folder embedded as well. It seems like the current master cloud build would have the same issue of only supporting targets that were released at the time the bundle was created. Should the hardware folder that is included in those firmware bundles be being replaced by a fresh download of the hardware list before flashing?

@pkendall64
Copy link
Contributor

pkendall64 commented Oct 4, 2023

@pkendall64 The artifact repository could be used instead of pulling down from the targets repo, but the list of release versions would still be pulled from GitHub, so it would not entirely eliminate the need to access GitHub.

This file has all the release versions and their hashes which are the download coordinates
https://artifactory.expresslrs.org/ExpressLRS/index.json

For example, the web-flasher operates purely from that file and the firmware files. It does access any git content.

Regarding the issue of trying to use a target that didn't exist at the time of a particular version being released, if I check the latest cloud build for master (https://artifactory.expresslrs.org/ExpressLRS/0bc0fa3a6fa9733f56c7795d26ca5eb1f9ee8e61/firmware.zip), it has a hardware folder embedded as well. It seems like the current master cloud build would have the same issue of only supporting targets that were released at the time the bundle was created. Should the hardware folder that is included in those firmware bundles be being replaced by a fresh download of the hardware list before flashing?

You are correct that each firmware folder contains the targets repo as of the time the firmware was built.
Any targets for a version should use the ones in firmware.zip in preference. If a target is not in firmware.zip but is in the top-level hardware.zip then it should be safe to copy the hardware.zip into the hardware folder of the firmware release as they should only be new aliases that are added as supporting older versions.

@jurgelenas
Copy link
Member

This file has all the release versions and their hashes which are the download coordinates
https://artifactory.expresslrs.org/ExpressLRS/index.json

We must consider every possible use case:

  • Pull requests
  • Specific commit build
  • Branch build
  • Git tags (official releases)

Aforementioned index.json covers only git tags.

@justinlampley I think we could implement support for multiple sources while loading the hardware definitions:

  • Cloudflare R2 files repository
  • Github repo

@pkendall64 how we should specify the location of targets directory to the firmware build script?

@pkendall64
Copy link
Contributor

@pkendall64 how we should specify the location of targets directory to the firmware build script?

I've open a PR on the main repo (for 3.x.x) which allows you to pass 2 directories to the flasher.
--dir which is the directory which contains the "hardware" directory.
--fdir which is the directory containing the firmware files. i.e. FCC, LBT dirs etc.

If --fdir is not specified then it uses --dir

@pkendall64
Copy link
Contributor

@justinlampley I've updated the artifacts on R2 to have the updated flasher in them so you should be able to test with the new --fdir argument. Contact me if you need help with testing or have any questions etc.

…the git repository and add support for the flasher fdir argument
@justinlampley
Copy link
Collaborator Author

I have switched to using the hardware artifact instead of the git repo and also added support for the flasher fdir argument.

@jurgelenas
Copy link
Member

I have switched to using the hardware artifact instead of the git repo and also added support for the flasher fdir argument.

Should we fallback to git repo in case the targets are not available on cloudflare r2?

@pkendall64
Copy link
Contributor

I have switched to using the hardware artifact instead of the git repo and also added support for the flasher fdir argument.

Should we fallback to git repo in case the targets are not available on cloudflare r2?

I don't that would be necessary as each time targets repo master is built it uploads to R2.

@pkendall64
Copy link
Contributor

Something else that needs to be considered, we need a button to refresh/reload the targets from the R2 repo or a time-based reload i.e. once a week. Perhaps in the button to force a reload of the targets should be in the Support tab. I think we also need the "Clear firmware files" button to delete the binary artifact files so they can be re-downloaded in the case that we have to update the flasher in them, like we do with this PR. Either that or we put the flasher binary into the Configurator distribution?

@justinlampley
Copy link
Collaborator Author

Good points PK. The targets are actually refreshed too often, every time the version selection changes, I need to limit it to once each time the configurator is started. I'll also look into making sure that clearing the firmware files actually clears all the firmware files.

@jurgelenas
Copy link
Member

Good points PK. The targets are actually refreshed too often, every time the version selection changes, I need to limit it to once each time the configurator is started. I'll also look into making sure that clearing the firmware files actually clears all the firmware files.

I believe our users should not be required to manage the ExpressLRS Configurator cache state manually. Manual cache clearing should be used as a last resort action.

We could automate this by using S3 HeadObject API to check for file updates on the Cloudflare R2.

Relevant links:

We have 10M read requests free tier with Cloudflare R2.

@jurgelenas
Copy link
Member

@justinlampley for automatic cache management we should modify this class as well:

@pkendall64
Copy link
Contributor

Good points PK. The targets are actually refreshed too often, every time the version selection changes, I need to limit it to once each time the configurator is started. I'll also look into making sure that clearing the firmware files actually clears all the firmware files.

I believe our users should not be required to manage the ExpressLRS Configurator cache state manually. Manual cache clearing should be used as a last resort action.

We could automate this by using S3 HeadObject API to check for file updates on the Cloudflare R2.

Relevant links:

We have 10M read requests free tier with Cloudflare R2.

Perfect, I wasn't sure if there was a way to do this.

@justinlampley
Copy link
Collaborator Author

I have updated clear firmware files to clear all firmware related files including the cache so that users can clear it themselves if they need to do so. I will work on adding the automatic check for file updates.

@justinlampley
Copy link
Collaborator Author

Files are now only downloaded if they have been modified by passing the If-Modified-Since header with the GET request.

@pkendall64
Copy link
Contributor

pkendall64 commented Oct 15, 2023

I've done a local build and overwritten the flasher.pyz file from the PR in the main repo and I can use the 1.6.0 release to flash and also this PR.

With regards to the issue listed in the description:
There is a "min_version" in the targets.json in the hardware.zip which is the minimum supported version for a target so you should probably filter out versions in the target file that are newer than the selected version for flashing.

@justinlampley
Copy link
Collaborator Author

Filtering based on min_version has been added.

@justinlampley justinlampley marked this pull request as ready for review October 17, 2023 21:17
@pkendall64
Copy link
Contributor

I'm happy with this. I've updated the 3.1.1 artifact on R2 with the changes in ExpressLRS/ExpressLRS#2445 so you can test. I've tested with a 1.6.0 and a local build of this PR.
3.1.1 works with both versions. other releases fail with this PR (because the artifacts are not updated). If you guys are happy then it works can you review and approve the 2445 PR and I'll update all the artifacts.

@mha1
Copy link

mha1 commented Oct 20, 2023

is there a way to obtain a test version?

Copy link
Member

@jurgelenas jurgelenas left a comment

Choose a reason for hiding this comment

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

Let's fix files removal limit and we are good to merge!

@jurgelenas jurgelenas merged commit b48803f into ExpressLRS:master Oct 23, 2023
3 checks passed
@mha1
Copy link

mha1 commented Nov 24, 2023

@jurgelenas 1.6.1: can you please try to build a commit for a BETAFPV 2.4 GHz, BETAFPV SuperP 14Ch 2.4GHz RX target, e.g. commit deacc9d23b059dcd0ab2e72cbdd997f990dc93ed with Flashing Options "Flash" (device options shouldn't matter). It builds ok but fails at the end with error *** [.pio\build\Unified_ESP32_2400_RX_via_UART\firmware.bin] hardware/targets.json: No such file or directory, see log file: ExpressLRSBuildLog_20231124135316559.txt

@jurgelenas
Copy link
Member

@jurgelenas 1.6.1: can you please try to build a commit for a BETAFPV 2.4 GHz, BETAFPV SuperP 14Ch 2.4GHz RX target, e.g. commit deacc9d23b059dcd0ab2e72cbdd997f990dc93ed with Flashing Options "Flash" (device options shouldn't matter). It builds ok but fails at the end with error *** [.pio\build\Unified_ESP32_2400_RX_via_UART\firmware.bin] hardware/targets.json: No such file or directory, see log file: ExpressLRSBuildLog_20231124135316559.txtu

Pull Request builds do not work with commits from external repositories. The commit that you have mentioned deacc9d23b059dcd0ab2e72cbdd997f990dc93ed is from external repository (pull request).

SuperP builds are working with Configurator 1.6.1 and ExpressLRS 3.3.1.

@mha1
Copy link

mha1 commented Nov 24, 2023

I'am not building a pull request:

image

It worked fine before the external repo PR. I mean it's just the missing hardware folder again.

@jurgelenas
Copy link
Member

I'am not building a pull request:

image

It worked fine before the external repo PR. I mean it's just the missing hardware folder again.

That commit is never going to work. No way of fixing it. Cloudbuilds do not work with pull requests from external repositories for security reasons.

I can see it comes from your repository and it was merged ExpressLRS/ExpressLRS#2478

You can use master branch or commit or merge commit hash f1ea0830e65df9fc92cebb1c7b75021998514772. It will let you use your fix :)

@mha1
Copy link

mha1 commented Nov 24, 2023

It worked before and it will work again if you pull the hardware folder just like you do for other builds like the "OFFICIAL RELEASES"

What's the purpose of "GIT COMMIT" if you can't build a git commit?

@mha1
Copy link

mha1 commented Nov 24, 2023

That commit is never going to work.

Actually it does work. Configurator correctly pulls the hardware repo but for some reason targets.json is missing.

I just copied targets.json from my local repo to Configurator's working directory. Then hit retry and it generates a perfectly fine and working firmware.bin for the SuperD.

image

image

@jurgelenas
Copy link
Member

That commit is never going to work.

Actually it does work. Configurator correctly pulls the hardware repo but for some reason targets.json is missing.

I just copied targets.json from my local repo to Configurator's working directory. Then hit retry and it generates a perfectly fine and working firmware.bin for the SuperD.

image

image

External forks / repositories do not have full access to our GitHub actions based build environment. Your build fails because configurator is not able to download cached binaries and hardware defines. This can't be fixed due to security reasons and possible build environment credentials leak.

@mha1
Copy link

mha1 commented Nov 24, 2023

I deleted the ExpressLRS Configurator in C:\Users\10137\AppData\Roaming\ExpressLRS Configurator and tried again without any manual intervention. Building my external repo commit worked fine. This time the log shows it cloned the hardware repo which also installed targets.json correctly.

image

I suppose what was going on is somehow targets.json went missing in the existing hardware folder. As there is only a check for folder hardware existing yes/no there was no trigger to clone the hardware repo even if the existing hardware folder wasn't complete. Maybe clone the hardware repo in any case?

Thanks for looking into this and you scared me a little telling me building my local repo commit with "GIT COMMIT" will never work.

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.

4 participants