Uh oh!
There was an error while loading. Please reload this page.
Add convenience method for resolving chosen stdout node - #9
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a convenience API to resolve the /chosen node’s stdout-path property directly to a referenced node, including handling of option suffixes after :.
Changes:
- Add
Chosen::stdout()which resolvesstdout-pathto aNode, ignoring:<options>suffixes. - Add unit and integration tests for option-splitting and stdout-node resolution.
- Update README to document the new helper.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| fdt-raw/src/node/chosen.rs | Implements Chosen::stdout() and adds split_path_options() + unit tests. |
| fdt-raw/tests/classify.rs | Adds an integration assertion for Chosen::stdout() resolution. |
| README.md | Documents the new Chosen::stdout() convenience method. |
| .github/workflows/deploy.yml | Removes the GitHub Pages deploy workflow. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// ignored when resolving the referenced node. | ||
| pub fn stdout(&self) -> Option<Node<'a>> { | ||
| let path = split_path_options(self.stdout_path()?); | ||
| self.node._fdt.find_by_path(path) |
There was a problem hiding this comment.
stdout() reaches into self.node._fdt directly, which tightly couples Chosen to Node internals (and the underscore name suggests this field is meant to be internal). Consider routing this through a public/semipublic API (e.g., a NodeBase/Node method like find_by_path(...) / fdt() accessor) so Chosen doesn’t depend on Node’s internal layout.
| self.node._fdt.find_by_path(path) | |
| self.node.find_by_path(path) |
| pub fn stdout(&self) -> Option<Node<'a>> { | ||
| let path = split_path_options(self.stdout_path()?); | ||
| self.node._fdt.find_by_path(path) | ||
| } |
There was a problem hiding this comment.
The added tests cover split_path_options() and one integration case for an absolute path, but there’s no direct test that Chosen::stdout() correctly resolves when stdout-path contains :<options> (the behavior implemented here). Adding a test that exercises stdout() with a stdout-path value like /pl011@9000000:115200n8 would validate the end-to-end behavior of this helper (not just the string splitter).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - `memory_reservation_blocks()`: Get memory reservations as iterator | ||
| - `aliases()`: Get node aliases for path shortcuts | ||
| - `chosen()`: Get chosen boot parameters | ||
| - `Chosen::stdout()`: Resolve `/chosen/stdout-path` to the referenced node |
There was a problem hiding this comment.
Chosen::stdout() is a method on Chosen, but it’s currently listed under “Fdt Methods”, which can mislead API consumers. Consider moving it to a “Chosen Methods” list or listing the call pattern via fdt.chosen()?.stdout() instead.
| -`Chosen::stdout()`: Resolve `/chosen/stdout-path` to the referenced node | |
| -`fdt.chosen()?.stdout()`: Resolve `/chosen/stdout-path` to the referenced node |
| - uses: dtolnay/rust-toolchain@stable | ||
| - name: Install dtc | ||
| run: sudo apt-get install device-tree-compiler | ||
| - name: Check code format | ||
| run: cargo fmt --all -- --check | ||
| - name: Clippy | ||
| run: cargo clippy --all-features |
There was a problem hiding this comment.
The workflow runs cargo fmt and cargo clippy, but the dtolnay/rust-toolchain@stable step doesn’t request the rustfmt/clippy components. This can make CI flaky depending on the runner image; consider adding with: components: rustfmt, clippy (and any other required components) to the toolchain step.
| on: | ||
| workflow_call: | ||
| push: | ||
| branches: | ||
| - main | ||
| pull_request: | ||
| branches: | ||
| - main | ||
There was a problem hiding this comment.
This PR’s description focuses on adding Chosen::stdout(), but it also changes CI behavior (switches test.yml from workflow_call to push/pull_request, and removes other workflows in this PR). Please either update the PR description to reflect the CI changes or split the workflow updates into a separate PR to keep review scope clear.
Summary
Chosen::stdout()to resolve/chosen/stdout-pathdirectly to a node:when resolving the referenced pathTesting
cargo test -p fdt-rawCloses#7