-
Notifications
You must be signed in to change notification settings - Fork 12
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
Update board catalog generator #412
Conversation
The build will pass when commands listed above are run. That will remove old generated files and regenerate them. I didn't want to do that before the review because it will bloat the PR with 1.2k files changed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
scripts/device-catalog/index.js
Outdated
} | ||
|
||
function getAllBoardPaths(dirPath, arrayOfFiles) { | ||
files = fs.readdirSync(dirPath) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you change this to
files = fs.readdirSync(dirPath) | |
files = fs.readdirSync(dirPath, {withFileTypes: true, recursive: true}) |
You'll get objects that already tell you which ones are files and directories, and you get recursion out of the box.
Can reduce this entire function to
function getAllBoardPaths(dirPath) {
return fs.readdirSync(dirPath, { withFileTypes: true, recursive: true})
.filter(e => e.isFile() && e.name === 'board.yml')
.map(f => f.path) // the directory path relative to dirPath
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worked like a charm, thanks!
scripts/device-catalog/index.js
Outdated
|
||
files.forEach(function(file) { | ||
if (fs.statSync(dirPath + "/" + file).isDirectory()) { | ||
arrayOfFiles = getAllBoardPaths(dirPath + "/" + file, arrayOfFiles) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really a bug, but worth mentioning that this arrayOfFiles =
assignment is redundant, as arrays are mutable references to the same data. All pushes that happen to arrayOfFiles
within this call are reflected in the original array.
All these three variations of this line have the same outcome:
arrayOfFiles = getAllBoardPaths(dirPath + "/" + file, arrayOfFiles)
getAllBoardPaths(dirPath + "/" + file, arrayOfFiles)
arrayOfFiles.push(...getAllBoardPaths(dirPath + "/" + file))
It's usually safest to go for the third variant in this case, as passing arrays for manipulation is a common source of bugs, because it's easy to forget that mutations have an effect outside of the current scope. For instance, the following variant of this line looks innocent, but actually adds the entries twice:
arrayOfFiles.push(...getAllBoardPaths(dirPath + "/" + file, arrayOfFiles))
scripts/device-catalog/index.js
Outdated
let yamlPath = path.join(boardsRoot, board.path, yamlName); | ||
if (fs.existsSync(yamlPath)) { return yamlPath; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JS convention for locally repeated behavior like this is to make a function within the current scope:
const getFileIfExists = (yamlName) => {
const yamlPath = path.join(boardsRoot, board.path, yamlName);
if (fs.existsSync(yamlPath) return yamlPath
}
let yamlPath = getFileIfExists(`${board.boardId}.yaml`);
if (yamlPath) return yamlPath;
if (board.rev) {
yamlPath = getFileIfExists(`${board.boardId}_${board.rev.replace(/\./g, '_')}.yaml`);
if (yamlPath) return yamlPath;
}
// ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is much cleaner now, thank you!
4d2f5ec
to
743dfd5
Compare
This PR is waiting on the release of Zephyr v3.7.0 and the adoption of it by the Golioth Firmware SDK. Once this happens, one more commit should be made: Before merging:
|
Zephyr 3.7 implements a new hardware model that changes the path to each board definition, and the naming of the yaml files in those definitions. This commit updates the automatic generation of the Golioth device catalog to work with the new model. - Search for board.yml files instead of scraping arch directories (which no longer exist). - Use the first image in each board's .rst file as the image in the device catalog. - Make several attempts to automatically recognize the device-specific yaml files. This is a tad tricky as different vendors approach this differently, especially when it comes to including or excluding the SOC name from the device ID and for multi-core processors like the ESP32 family. - Use the `scripts/device-catalog/custom_images` directory to override the image used for any board. The filename (excluding the extension) must exactly match the board ID. This may be used even when no board image is present in the Zephyr tree. The `scripts/device-catalog/manually_added` directory may still be used to add boards to the library, as long as the added board file structure matches the new hardware model. Signed-off-by: Mike Szczys <mike@golioth.io>
The new hardware model no longer uses arch for directory structure. This commit removes arch keys from the support.json, placing the board definitions at the root of the data. - Device names that have changed with the new model are updated. - Custom images moved to `scripts/device-catalog/custom_images` folder. - The mimxrt1060_evkb now has it's own entry and doesn't reqire manual addition. - Remove qemu_x86 from continuoulsy verified -- only the corex_m3 qemu variant should be on that list. Signed-off-by: Mike Szczys <mike@golioth.io> remove x86
At v3.7.0, Zephyr change to a new hardware model for defining boards. This commit removes files generated using the old hardware model and regenerates them using the new one. Signed-off-by: Mike Szczys <mike@golioth.io>
743dfd5
to
c306a69
Compare
Visit the preview URL for this PR (updated for commit c306a69): https://golioth-docs-dev--pr412-szczys-update-board-0x23f2jf.web.app (expires Sat, 05 Oct 2024 18:03:54 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: f53b02bdc98ce6f5593931ec4c339aa96bac84df |
Zephyr 3.7 implements a new hardware model that changes the path to each board
definition, and the naming of the yaml files in those definitions. This commit
updates the automatic generation of the Golioth device catalog to work with the
new model.
longer exist).
catalog.
files. This is a tad tricky as different vendors approach this differently,
especially when it comes to including or excluding the SOC name from the
device ID and for multi-core processors like the ESP32 family.
scripts/device-catalog/custom_images
directory to override the imageused for any board. The filename (excluding the extension) must exactly match
the board ID. This may be used even when no board image is present in the
Zephyr tree. The
scripts/device-catalog/manually_added
directory may stillbe used to add boards to the library, as long as the added board file
structure matches the new hardware model.
Running this script
resolves https://github.com/golioth/devrel-issue-tracker/issues/418