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

Use critical-section for heap locking, rename to embedded-alloc. #56

Merged
merged 2 commits into from
Dec 8, 2022

Conversation

Dirbaio
Copy link
Member

@Dirbaio Dirbaio commented Aug 12, 2022

This uses critical_section::with instead of cortex_m::interrupt::free to acquire a critical section. This allows customizing the critical section implementation, to make it sound for multicore chips for example.

This is a breaking change, so it'll require a 0.5 release.

Interestingly this makes the crate not cortex-m specific anymore. Perhaps it could be renamed to something more general?

TODO

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @thalesfragoso (or someone else) soon.

Please see the contribution instructions for more information.

@jonathanpallant
Copy link

Maybe it should be embedded-alloc in line with other crates in this org?

@reitermarkus
Copy link
Member

Maybe add a critical-section feature upstream to linked_list_allocator instead?

@chrysn
Copy link

chrysn commented Dec 1, 2022

Having customizable critical sections also benefits the use case of the nRF softdevice (which requires users never to turn off interrupts completely).

If this requires an API change, it might be worth considering whether that change could also be used for non-critical-section exclusitivness. A concrete setup I have in mind is always failing attempts to allocate from interrupts (which in setups without context switching would mean that little to no is needed; of course, if critical_section also allows such a null-implementation, all the better).

@Dirbaio Dirbaio changed the title Use critical-section for heap locking. Use critical-section for heap locking, rename to embedded-alloc. Dec 6, 2022
@Dirbaio
Copy link
Member Author

Dirbaio commented Dec 6, 2022

Discussed on today's WG meeting. chatlogs, it was decided to rename to embedded-alloc.

Copy link
Member

@adamgreig adamgreig left a comment

Choose a reason for hiding this comment

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

LGTM, @rust-embedded/cortex-m?

> A heap allocator for Cortex-M processors
> A heap allocator for embedded systems.

Note that using this as your global allocator requires nightly Rust.

This project is developed and maintained by the [Cortex-M team][team].
Copy link
Member

Choose a reason for hiding this comment

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

Should this be moved to the HAL team?

Copy link
Member

Choose a reason for hiding this comment

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

That might make sense if they agree, but I think we can do it as a separate PR.

Copy link
Member

@newAM newAM left a comment

Choose a reason for hiding this comment

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

LGTM

@adamgreig
Copy link
Member

bors r+

@bors
Copy link
Contributor

bors bot commented Dec 8, 2022

@bors bors bot merged commit 89cb8d5 into rust-embedded:master Dec 8, 2022
@adamgreig
Copy link
Member

I've renamed this repo and published to https://crates.io/crates/embedded-alloc 🎉

@eldruin
Copy link
Member

eldruin commented Dec 19, 2022

@adamgreig could you add the cortex-m team as an owner on crates.io as well?

@adamgreig
Copy link
Member

Thanks for the reminder, done.

bors bot added a commit to rust-embedded/wg that referenced this pull request Jan 17, 2023
653: Rename to embedded-alloc r=eldruin a=Jzow

CC [#56](rust-embedded/embedded-alloc#56).

Co-authored-by: James Zow <jameszow@163.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants