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

Should we have this crate as no_std? #61

Closed
Tracked by #275
andreeaflorescu opened this issue Mar 6, 2023 · 8 comments · Fixed by #68
Closed
Tracked by #275

Should we have this crate as no_std? #61

andreeaflorescu opened this issue Mar 6, 2023 · 8 comments · Fixed by #68

Comments

@andreeaflorescu
Copy link
Member

I was thinking about making this crate no_std either by default or behind a feature for now (at least until the error is stabilized in core). I gave it a try and it was pretty easy to convert it to no_std: https://github.com/andreeaflorescu/vm-fdt/tree/no_std. This should make the crate available to be used in embeded as well.

@rbradford
Copy link
Contributor

Sounds great, I would support this as I might have a use for this in Rust Hypervisor Firmware.

@andreeaflorescu
Copy link
Member Author

@danielverkamp what do you think? If this is needed sooner (before the error is stabilized), we can have it as an optional feature. I see that libc and serde also have a no_std option. The way that is implemented is by having std as the default feature. I think this could easily work for this crate as well.

Examples:

@danielverkamp
Copy link
Collaborator

I agree making it no_std would be nice. I suppose since it still requires alloc, it may not be a good fit for really limited embedded environments, but no_std alone is a good improvement. I guess this would also justify a version bump.

The proposed change looks reasonable; the only minor concern I have is that each property_phandle() call becomes an O(n) operation (where n is the number of existing phandles), since it uses a linear search in a Vec rather than a HashSet lookup, but this is probably fine for any reasonably-sized devicetree. (Perhaps we could remove the duplicate phandle checking to avoid that, although I don't have a strong opinion about it.)

@andreeaflorescu
Copy link
Member Author

I agree making it no_std would be nice. I suppose since it still requires alloc, it may not be a good fit for really limited embedded environments, but no_std alone is a good improvement. I guess this would also justify a version bump.

We can extend it once we have it as no_std to also have it as no_alloc I guess. I think we can have static allocation for the data blob, but this require some significant re-working of how it currently works, so I would leave it for the next iteration.

The proposed change looks reasonable; the only minor concern I have is that each property_phandle() call becomes an O(n) operation (where n is the number of existing phandles), since it uses a linear search in a Vec rather than a HashSet lookup, but this is probably fine for any reasonably-sized devicetree. (Perhaps we could remove the duplicate phandle checking to avoid that, although I don't have a strong opinion about it.)

Yes, in my branch I did a quick and dirty fix to get it to compile, I would improve that for the actual PR.

@rbradford
Copy link
Contributor

rbradford commented Mar 8, 2023

I think having alloc is very common for many no_std users so I wouldn't worry about that too much for a first version.

@mkroening
Copy link
Contributor

It would be great to have no_std support. We could make use of this in @hermit-os.

How can we help to move this forward? Can we open a PR, or would you rather do that yourself, @andreeaflorescu? :)

@andreeaflorescu
Copy link
Member Author

It would be great to have no_std support. We could make use of this in @hermit-os.

How can we help to move this forward? Can we open a PR, or would you rather do that yourself, @andreeaflorescu? :)

Oh, I didn't get much time to work on that, so feel free to open a PR and I'll make time to review it. Here is my branch in case it helps with anything: https://github.com/andreeaflorescu/vm-fdt/commits/no_std

@mkroening mkroening mentioned this issue Nov 9, 2023
4 tasks
@mkroening
Copy link
Contributor

I opened #68.

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 a pull request may close this issue.

4 participants