Add getters for some standard properties #16
Conversation
src/fdt/node.rs
Outdated
| /// Returns the value of the standard `compatible` property. | ||
| /// | ||
| /// # Errors | ||
| /// | ||
| /// Returns an error if a property's name or value cannot be read. | ||
| pub fn compatible( | ||
| &self, | ||
| ) -> Result<Option<impl Iterator<Item = &'a str> + use<'a>>, FdtParseError> { | ||
| Ok(self | ||
| .property("compatible")? | ||
| .map(|property| property.as_str_list())) | ||
| } | ||
|
|
||
| /// Returns the value of the standard `model` property. | ||
| /// | ||
| /// # Errors | ||
| /// | ||
| /// Returns an error if a property's name or value cannot be read, or the | ||
| /// value isn't a valid UTF-8 string. | ||
| pub fn model(&self) -> Result<Option<&'a str>, FdtError> { | ||
| Ok(if let Some(model) = self.property("model")? { | ||
| Some(model.as_str()?) | ||
| } else { | ||
| None | ||
| }) | ||
| } | ||
|
|
||
| /// Returns the value of the standard `phandle` property. | ||
| /// | ||
| /// # Errors | ||
| /// | ||
| /// Returns an error if a property's name or value cannot be read, or the | ||
| /// value isn't a valid u32. | ||
| pub fn phandle(&self) -> Result<Option<u32>, FdtError> { | ||
| Ok(if let Some(property) = self.property("phandle")? { | ||
| Some(property.as_u32()?) | ||
| } else { | ||
| None | ||
| }) | ||
| } | ||
|
|
||
| /// Returns the value of the standard `status` property. | ||
| /// | ||
| /// If there is no `status` property then `okay` is assumed. | ||
| /// | ||
| /// # Errors | ||
| /// | ||
| /// Returns an error if a property's name or value cannot be read, or the | ||
| /// value isn't a valid status. | ||
| pub fn status(&self) -> Result<Status, FdtError> { | ||
| Ok(if let Some(status) = self.property("status")? { | ||
| status.as_str()?.parse()? | ||
| } else { | ||
| Status::Okay | ||
| }) | ||
| } | ||
|
|
||
| /// Returns the value of the standard `#address-cells` property. | ||
| /// | ||
| /// # Errors | ||
| /// | ||
| /// Returns an error if a property's name or value cannot be read, or the | ||
| /// value isn't a valid u32. | ||
| pub fn address_cells(&self) -> Result<u32, FdtError> { | ||
| Ok(if let Some(property) = self.property("#address-cells")? { | ||
| property.as_u32()? | ||
| } else { | ||
| DEFAULT_ADDRESS_CELLS | ||
| }) | ||
| } | ||
|
|
||
| /// Returns the value of the standard `#size-cells` property. | ||
| /// | ||
| /// # Errors | ||
| /// | ||
| /// Returns an error if a property's name or value cannot be read, or the | ||
| /// value isn't a valid u32. | ||
| pub fn size_cells(&self) -> Result<u32, FdtError> { | ||
| Ok(if let Some(model) = self.property("#size-cells")? { | ||
| model.as_u32()? | ||
| } else { | ||
| DEFAULT_SIZE_CELLS | ||
| }) | ||
| } | ||
|
|
||
| /// Returns the value of the standard `virtual-reg` property. | ||
| /// | ||
| /// # Errors | ||
| /// | ||
| /// Returns an error if a property's name or value cannot be read, or the | ||
| /// value isn't a valid u32. | ||
| pub fn virtual_reg(&self) -> Result<Option<u32>, FdtError> { | ||
| Ok(if let Some(property) = self.property("virtual-reg")? { | ||
| Some(property.as_u32()?) | ||
| } else { | ||
| None | ||
| }) | ||
| } | ||
|
|
||
| /// Returns whether the standard `dma-coherent` property is present. | ||
| /// | ||
| /// # Errors | ||
| /// | ||
| /// Returns an error if a property can't be read. | ||
| pub fn dma_coherent(&self) -> Result<bool, FdtError> { | ||
| Ok(self.property("dma-coherent")?.is_some()) | ||
| } | ||
|
|
There was a problem hiding this comment.
I know this is similar to the APIs currently in the flat_device_tree crate, although I'm not sure if I like this. Two biggest problems I see are:
- There is no separation between high-level APIs (standard property accessors) and lower-level APIs (
.name(),.child(),.property()). - We have two APIs that users might want to use with this - in-memory
Fdtand the intermediate representationDeviceTree. This only supports the former.
It feels like both could be solved by defining traits to define common behavior:
Property,Nodetraits (implemented forFdtandDeviceTree) for reading data in nodes and propertiesStandardNode(implemented for anyTwhereT: Node) trait that can be used to access standard properties inside given node.
Each of the Node/Property implementations would also define its own error types (with DeviceTree possibly setting it to Infallible), and StandardNode would probably have its own error type that would wrap this "inner" type - so FdtError would probably become StandardNodeError<FdtParseError>/StandardNodeError<Infallible>.
This doesn't necessarily has to be done in this PR, but I think should be done as soon as possible to limit further API discrepancies.
What do you think?
There was a problem hiding this comment.
I've had a bit of a look into doing this, by adding a trait that both FdtNode and DeviceTreeNode with the various getter methods. The problem I've run into is with the lifetimes of return values.
FdtNode doesn't own its contents, but rather borrows them for some parameterised lifetime parameter 'a. This means for example FdtNode::name returns a &'a str, whose lifetime is bound to the underlying FDT, not to the node. On the other hand, DeviceTreeNode owns its contents, so DeviceTreeNode::name returns an &str tied to the lifetime of &self. I can't see a way to make these both work with the same trait. For FdtNode we'd need something like
trait DtNode<'a> {
fn name(&self) -> Result<&'a str, Self::Error>;
}While for DeviceTreeNode we'd need something like (writing the implicit lifetimes out to be clear):
trait DtNode {
fn name<'b>(&'b self) -> Result<&'b str, Self::Error>;
}The methods to get properties and children have similar issues.
There was a problem hiding this comment.
Ahh, I see! Well, if we really want to do this, we can achieve this with GATs, I think: https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=ffb2c466f428a1033fb1d95399cdf693
Perhaps though it's just fine to return a self-bound reference, even for Fdt? It's a bit limiting, but I believe it's somewhat expected and idiomatic for the references returned by methods to be bound to self, and not a "parent" object: https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=19ba10c5f31f19b2bb82a37df71f4e2a
Maybe we can even define these traits and use them for the StandardNode trait implementation, while still keeping the methods defined directly in the Fdt and DeviceTree structs if the users want more flexibility?
WDYT?
There was a problem hiding this comment.
I think it's useful for the results of these getters to have lifetimes tied only to the underlying data, as that is the only thing they actually refer to. This means for example that if you construct an Fdt wrapping a static slice of memory (as is a common case), then the various string slices you get back will also be &'static str.
I've attempted to implement a trait using the approach you suggested in #20, but I don't think it's powerful enough.
I'd like to merge this anyway to make progress, and we can come back to it later if we find a better way to generalise this.
There was a problem hiding this comment.
Sounds good. I've left the comment here with yet another way to unify the APIs, and we can merge this for now.
qwandor
force-pushed
the
properties
branch
2 times, most recently
from
554f054 to
cc2e597
Compare
2 participants
As part of this I've renamed the existing
FdtErrortoFdtParseError, and introduced a newFdtErrorenum that includes both parse errors, and errors without an associated offset. The latter is currently just used for an error parsing thestatusproperty, but other variants will be added in subsequent PRs.