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

Add particle flash --tachyon #780

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

Conversation

keeramis
Copy link
Contributor

@keeramis keeramis commented Dec 6, 2024

Description

Add particle flash --tachyon to flash tachyon

Story: https://app.shortcut.com/particle/story/132827/create-particle-flash-tachyon-command

Usage

  1. Checkout this branch. Use nvm use to update the node version and then run npm ci

One of the following ways to test the command:

  1. Enter qcm6490_qfil_download_ufs folder - Runparticle flash --tachyon
  2. From any other folder - Runnpm start -- flash --tachyon /path/to/qcm6490_qfil_download_ufs
  3. Enter qcm6490_qfil_download_ufs folder - Runnpm start -- flash --tachyon prog_firehose_ddr.elf rawprogram_unsparse0.xml patch0.xml rawprogram1.xml patch1.xml rawprogram2.xml patch2.xml rawprogram3.xml patch3.xml rawprogram4.xml patch4.xml rawprogram5.xml patch5.xml

Use --verbose to see logs during the update process

Expected output

See this slack thread -

Related Issues / Discussions

Story

Completeness

  • User is totes amazing for contributing!
  • Contributor has signed CLA
  • Problem and solution clearly stated
  • Tests have been provided
  • Docs have been updated
  • CI is passing

@keeramis keeramis force-pushed the feature/linux-flashing branch from b4660c2 to 8d62e71 Compare December 6, 2024 23:19
@keeramis keeramis requested review from monkbroc and eberseth December 9, 2024 07:02
src/cli/flash.js Outdated
boolean: true,
description: 'Flash Tachyon'
},
'verbose' : {
Copy link
Member

Choose a reason for hiding this comment

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

You don't need to add verbose here. It's a global flag available to all commands already https://github.com/particle-iot/particle-cli/blob/master/src/app/cli.js#L42

src/cmd/flash.js Outdated
const rawProgramFiles = linxuFiles.filter(f => f.startsWith('rawprogram') && f.endsWith('.xml'));
const patchFiles = linxuFiles.filter(f => f.startsWith('patch') && f.endsWith('.xml'));

if (!elfFiles.length || !rawProgramFiles.length || !patchFiles.length) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think patch files are mandatory.

throw new Error('The directory should contain at least one .elf file, one rawprogram file, and one patch file');
}

const sortByNumber = (a, b) => {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's necessary to sort the files, as long as the program files are before the patch files.

Copy link

Choose a reason for hiding this comment

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

I think you want to process in the order given in the command. The original tools assume that the user correctly orders the raw program and patch files accordingly

Copy link
Contributor Author

@keeramis keeramis Dec 10, 2024

Choose a reason for hiding this comment

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

I brought these back (for time being) and kept the below changes as well (assuming they are important too)

rawProgramFiles.sort(sortByNumber);
patchFiles.sort(sortByNumber);

if (rawProgramFiles.length !== patchFiles.length) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is necessary either

Copy link
Member

Choose a reason for hiding this comment

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

I really think this is extra. What if a program doesn't have a patch file to go with it?

throw new Error('The number of rawprogram files should match the number of patch files');
}

let filesToProgram = [];
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let filesToProgram = [];
const filesToProgram = [...rawProgramFiles, ...patchFiles];

src/cmd/flash.js Outdated
unpackToolFolder = path.dirname(files[0]);
}

const parsedFiles = await this._analyzeFiles(files);
Copy link
Member

Choose a reason for hiding this comment

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

Looking at the implementation of _analyzeFiles, I'm not sure this call is useful

Copy link
Member

@monkbroc monkbroc left a comment

Choose a reason for hiding this comment

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

Looks good 👍 Next step will be to add a version of qdl compiled for each platform.

@keeramis keeramis force-pushed the feature/linux-flashing branch from 812ed57 to 93ba5ba Compare December 10, 2024 01:20
Copy link
Member

Choose a reason for hiding this comment

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

The directory must be <os>_<architecture> like darwin_x64. Use the same words as in https://binaries.particle.io/particle-cli/manifest.json

let linxuFiles = await this._findLinuxExecutableFiles(files, { directory: unpackToolFolder });
linxuFiles = linxuFiles.map(f => path.basename(f));

const elfFiles = linxuFiles.filter(f => f.startsWith('prog') && f.endsWith('.elf'));
Copy link
Member

Choose a reason for hiding this comment

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

Change the criteria to

Suggested change
const elfFiles = linxuFiles.filter(f => f.startsWith('prog') && f.endsWith('.elf'));
const elfFiles = linxuFiles.filter(f => f.includes('firehose') && f.endsWith('.elf'));

This matches https://www.notion.so/particle/PRD-Particle-CLI-Support-for-Flashing-Tachyon-157d40caa5dc805ab72fdabddf713381?pvs=4#158d40caa5dc80db8131ecd541a48d17

Comment on lines +138 to +140
const archType = utilities.getArchType();
const qdl = path.join(__dirname, `../../assets/qdl/${archType}/qdl`);
await fs.ensureFile(qdl);
Copy link
Member

Choose a reason for hiding this comment

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

Another piece of feedback: extract the execution of qdl to a separate file lib/qdl.js. See the example of the old dfu.js https://github.com/particle-iot/particle-cli/blob/v1.43.2/src/lib/dfu.js

Keep using the new execa, but put stuff like getting the correct executable for the current OS and platform in a helper function in lib/qdl.js so that flash.js just needs to call a function to perform the qdl flash.

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