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

Add no_std implementation based on critical-section. #195

Merged
merged 14 commits into from Oct 22, 2022

Conversation

reitermarkus
Copy link
Contributor

@reitermarkus reitermarkus commented Sep 6, 2022

Add implementation based on critical-section for embedded targets.

This started out using critical_section::Mutex, but tests were failing since critical_section::with blocks all threads during initialization, so I changed it to be basically the same as imp_pl without the parking_lot parts, since atomic-polyfill is based on critical-section.

Depends on rust-embedded/critical-section#26.

Copy link
Owner

@matklad matklad left a comment

Choose a reason for hiding this comment

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

🤔 so, historically I've been pretty opposed to "just works" implementation on no_std, as how exactly one does locking is pretty important, and spinlocks is decidedly not a way to go, there's some discussion in #61.

The approach roughly like this could work though:

  • if we use a critical section rather than spinning, that counts as proper blocking in at least some contexts
  • the experience is that the user has to explicitly enable the critical_section feature, so they are opting into this behavior

So, yeah, I think I am generally on board with it. However, it is important that we don't actually spin here (the current implementation does spin).

src/imp_cs.rs Outdated Show resolved Hide resolved
@reitermarkus
Copy link
Contributor Author

Okay, so I have a different implementation which doesn't spin and simply completely blocks all threads during initialize, but this causes cargo test to deadlock unless using --test-threads 1.

@reitermarkus reitermarkus force-pushed the critical-section branch 2 times, most recently from 89a5da8 to bdb7107 Compare September 7, 2022 03:39
@reitermarkus reitermarkus changed the title Add no_std implementation based on critical-section/atomic-polyfill. Add no_std implementation based on critical-section. Sep 7, 2022
@reitermarkus reitermarkus force-pushed the critical-section branch 3 times, most recently from ff0a4ba to 42e9c79 Compare September 7, 2022 05:42
@reitermarkus
Copy link
Contributor Author

@matklad, can you have another look?

src/imp_cs.rs Outdated Show resolved Hide resolved
src/imp_cs.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@reitermarkus reitermarkus force-pushed the critical-section branch 3 times, most recently from ced0703 to 124c01d Compare September 17, 2022 03:29
@reitermarkus
Copy link
Contributor Author

@matklad, can you have another look at this? Thanks.

@matklad
Copy link
Owner

matklad commented Oct 22, 2022

lgtm!

bors r+

@bors
Copy link
Contributor

bors bot commented Oct 22, 2022

Build succeeded:

@bors bors bot merged commit b56e329 into matklad:master Oct 22, 2022
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Nov 1, 2022
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [once_cell](https://github.com/matklad/once_cell) | dependencies | minor | `1.15.0` -> `1.16.0` |

---

### Release Notes

<details>
<summary>matklad/once_cell</summary>

### [`v1.16.0`](https://github.com/matklad/once_cell/blob/HEAD/CHANGELOG.md#&#8203;1160)

[Compare Source](matklad/once_cell@v1.15.0...v1.16.0)

-   Add `no_std` implementation based on `critical-section`,
    [#&#8203;195](matklad/once_cell#195).
-   Deprecate `atomic-polyfill` feature (use the new `critical-section` instead)

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC44LjAiLCJ1cGRhdGVkSW5WZXIiOiIzNC45LjEifQ==-->

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1615
Reviewed-by: crapStone <crapstone@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
@reitermarkus reitermarkus deleted the critical-section branch October 4, 2023 05:04
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 this pull request may close these issues.

None yet

2 participants