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

Do not require nightly for no_std #22

Merged
merged 1 commit into from
Jul 7, 2024
Merged

Conversation

ivmarkov
Copy link
Contributor

@ivmarkov ivmarkov commented Jul 7, 2024

This PR puts the InPlaceInit trait behind the a #[cfg(feature = "alloc")] flag, which - in turn - allows us to only require the unstable core::alloc::AllocError type when feature alloc (or std) is enabled.

Thus, the no_std portion of this crate compiles on stable Rust.
I need this because rs-matter - where we might start using pinned-init - requires Rust stable. Since rs-matter is no_std (and no-alloc) we don't really need that much if at all the smart-pointer in-place initialization logic of InPlaceInit. Not to mention that its Arc/Rc/Box implementations currently depend on other nightly features anyway so we can't use those.

The justification for putting the InPlaceInit trait behind the alloc feature is that I'm struggling a bit to find use cases for it when the alloc module is not enabled. While it is indeed generic and can be used for custom smart pointers, still:

  • It is designed to decorate existing smart pointers. Namely and (primarily?) the ones from the alloc crate
  • If I have a custom smart-pointer, I would not decorate it with InPlaceInit, but would rather just implement the init method & friends directly on the custom smart pointer?

An alternative to the change here is to introduce another feature: in-place-init and then hide the InPlaceInit trait behind this feature, which would be enabled by default when alloc is enabled.

I'm fine with either of these approaches.
What would be your suggestion?

@y86-dev
Copy link
Member

y86-dev commented Jul 7, 2024

This seems very reasonable. How urgent is this for you? I am asking, because I plan to do some major rewriting of how this library is in the kernel (make it into its own crate) and the less the userspace version is diverged, the easier it becomes. So if you would not be using it in a couple months, then I will probably have finished the rewrite. But if say you would like to use it in the next couple of weeks, then it wouldn't be an issue to just merge your contribution (since it's so small). What do you think?

Copy link
Member

@y86-dev y86-dev left a comment

Choose a reason for hiding this comment

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

I gave it some more thought and since the change is so small, I think I will go ahead and just merge it and publish a new version.

@y86-dev y86-dev merged commit 366a43b into Rust-for-Linux:main Jul 7, 2024
14 checks passed
@ivmarkov
Copy link
Contributor Author

ivmarkov commented Jul 7, 2024

Thank you for reacting so quickly (and pushing a new version to crates.io)!

As for when we plan to start using it in rs-matter - it could be as soon as a couple of weeks or even earlier, as the change turned out to be easy to implement, incremental, additive, and easily reversible too, in case (god forbid!) we hit a roadblock with the in-place initialization or if another obstacle/reason for going against it pops up that I don't see yet.

Most important is the other maintainers on the project to understand what I'm trying to address in there and why pinned-init is a good solution for the problem (in the meantime I'm a little bit the maintainer who understands the no_std intricacies, including stack blowups).

Needless to say, I'm - in the meantime - a big fan of the pinned-init crate. I advertised it a bit yesterday in the "Rust Embedded" Matrix channel. We'll see how it goes.

Ideally - and over time of course - I would like to see support for it in heapless (an alternative of alloc used very frequently in the embedded world) as well as in embassy-sync which is a collection of async synchronization primitives, like mutexes, queues, signals and a few others (embassy is a collection of micro-crates; in the meantime it is kind of the "de-facto standard" for doing Rust async stuff on embedded).

@y86-dev
Copy link
Member

y86-dev commented Jul 7, 2024

Thank you for reacting so quickly (and pushing a new version to crates.io)!

My pleasure!

As for when we plan to start using it in rs-matter - it could be as soon as a couple of weeks or even earlier, as the change turned out to be easy to implement, incremental, additive, and easily reversible too, in case (god forbid!) we hit a roadblock with the in-place initialization or if another obstacle/reason for going against it pops up that I don't see yet.

That sounds great! If you hit anything feel free to open an issue (also if you have any questions regarding usage or think that documentation is unclear etc.).

Most important is the other maintainers on the project to understand what I'm trying to address in there and why pinned-init is a good solution for the problem (in the meantime I'm a little bit the maintainer who understands the no_std intricacies, including stack blowups).

Needless to say, I'm - in the meantime - a big fan of the pinned-init crate. I advertised it a bit yesterday in the "Rust Embedded" Matrix channel. We'll see how it goes.

I plan to eventually create an RFC to get this into the Rust language, since the syntax is not ideal and certain errors are rather difficult to parse... (also it would be nice to have rustfmt understand the marco)

Ideally - and over time of course - I would like to see support for it in heapless (an alternative of alloc used very frequently in the embedded world) as well as in embassy-sync which is a collection of async synchronization primitives, like mutexes, queues, signals and a few others (embassy is a collection of micro-crates; in the meantime it is kind of the "de-facto standard" for doing Rust async stuff on embedded).

I see, I haven't really thought about use-cases outside of the kernel, since that is what I created this for and I personally don't do embedded. But I'd be happy to see that come to fruition.

@ivmarkov
Copy link
Contributor Author

ivmarkov commented Jul 7, 2024

I see, I haven't really thought about use-cases outside of the kernel, since that is what I created this for and I personally don't do embedded. But I'd be happy to see that come to fruition.

Stack blow-ups in Rust due to memory being moved around are a common source of annoyance in the embedded world. MCUs typically have something like 400KB RAM (and this is the beefy Espressif MCU line). Others have much less. (Of course program size does not count towards these 400KB, MCUs map the flash where the firmware is written via MMU directly in the e.g. the first 64KB of the RAM or so; which is used as a form of program/execution "cache" which is refreshed on "page faults" of sorts; but even the firmware is rarely bigger than 2MB).

Even if you are running a single-threaded program (say, with async cooperative multitasking) so you need to take care of a single stack only, once you allocate statically the "big guys" (Wifi, BT and other memory eaters), you are often left with 60KB or less for the stack.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants