Skip to content

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

Merged
bors[bot] merged 2 commits into
rust-embedded:masterfrom
Dirbaio:cs
Dec 8, 2022
Merged

Use critical-section for heap locking, rename to embedded-alloc.#56
bors[bot] merged 2 commits into
rust-embedded:masterfrom
Dirbaio:cs

Conversation

@Dirbaio

@Dirbaio Dirbaio commented Aug 12, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

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

@Dirbaio Dirbaio requested a review from a team as a code owner August 12, 2022 13:17
@rust-highfive

rust-highfive commented Aug 12, 2022

Copy link
Copy Markdown

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

jonathanpallant commented Aug 12, 2022

Copy link
Copy Markdown

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

@Dirbaio Dirbaio mentioned this pull request Aug 12, 2022
Closed
@reitermarkus

reitermarkus commented Sep 4, 2022

Copy link
Copy Markdown
Member

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

@chrysn

chrysn commented Dec 1, 2022

Copy link
Copy Markdown

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

Dirbaio commented Dec 6, 2022

Copy link
Copy Markdown
Member Author

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

adamgreig
adamgreig approved these changes Dec 6, 2022
View reviewed changes

@adamgreig adamgreig left a comment

Copy link
Copy Markdown
Member

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?

reitermarkus
reitermarkus reviewed Dec 7, 2022
View reviewed changes
Comment thread README.md

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

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

@reitermarkus reitermarkus Dec 7, 2022

Copy link
Copy Markdown
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?

@adamgreig adamgreig Dec 8, 2022

Copy link
Copy Markdown
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.

newAM
newAM approved these changes Dec 7, 2022
View reviewed changes

@newAM newAM left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@adamgreig

adamgreig commented Dec 8, 2022

Copy link
Copy Markdown
Member

bors r+

@bors

bors Bot commented Dec 8, 2022

Copy link
Copy Markdown
Contributor

Build succeeded:

@bors bors Bot merged commit 89cb8d5 into rust-embedded:master Dec 8, 2022
@adamgreig

adamgreig commented Dec 8, 2022

Copy link
Copy Markdown
Member

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

@eldruin

eldruin commented Dec 19, 2022

Copy link
Copy Markdown
Member

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

@adamgreig

adamgreig commented Dec 19, 2022

Copy link
Copy Markdown
Member

Thanks for the reminder, done.

@Jzow Jzow mentioned this pull request Dec 21, 2022
Merged
bors Bot added a commit to rust-embedded/wg that referenced this pull request Jan 17, 2023
@bors @Jzow
Merge #653
315a609 
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

Reviewers

@adamgreig adamgreig adamgreig approved these changes

@reitermarkus reitermarkus reitermarkus left review comments

@newAM newAM newAM approved these changes

Assignees

@thalesfragoso thalesfragoso

Labels

S-waiting-on-review T-cortex-m

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@Dirbaio @rust-highfive @jonathanpallant @reitermarkus @chrysn @adamgreig @eldruin @newAM @thalesfragoso