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

no_std: spinning_top, portable-atomic #222

Merged
merged 1 commit into from
Feb 15, 2024

Conversation

mammothbane
Copy link
Contributor

@mammothbane mammothbane commented Feb 8, 2024

Fix #![no_std] support.

  • Stop using parking_lot in no_std -- it never worked -- and switch to spinning_top instead. Ideally this would make use of lock_api directly, but I don't have time to chase the type signature changes through the tests at the moment.
  • Switch atomic usage to portable_atomic.

I'd also recommend adding a cargo check --target thumbv7em-none-eabi (or similar no_std target) to the CI pipeline in the future, as the tests don't cover no_std for dependencies (see below).

notes

#29 (which introduced this change) looks like it was generated by shotgunning dependency analysis tools without reading or understanding the implications (notably, it was during GitHub's Hacktoberfest event, which is notorious for this).

parking_lot does not work, and to my knowledge never has worked, in no_std environments:

Consider the following alternatives (all of which support no_std):
...

...

ci / testing

I notice that someone reported this issue in #96 and it was closed because CI is passing. However, rust's libtest transitively includes libstd, i.e. cargo test can't be run without including std. governor may be built for test without std, but its dependencies are not.

governor/ $ rustup target add thumbv7em-none-eabi
governor/ $ cargo check --target thumbv7em-none-eabi --no-default-features --features no_std
error[E0463]: can't find crate for `std`
  |
  = note: the `thumbv7em-none-eabi` target may not support the standard library
  = note: `std` is required by `parking_lot_core` because it does not declare `#![no_std]`
  = help: consider building the standard library from source with `cargo build -Zbuild-std`

Fix #![no_std] support. Stop using `parking_lot` -- it was never
supported -- and switch to `spinning_top` instead. Switch atomic usage
to `portable_atomic`.
Copy link

codecov bot commented Feb 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f0cb09c) 98.25% compared to head (c2cfc0b) 98.22%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #222      +/-   ##
==========================================
- Coverage   98.25%   98.22%   -0.04%     
==========================================
  Files          31       31              
  Lines        2182     2140      -42     
==========================================
- Hits         2144     2102      -42     
  Misses         38       38              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@antifuchs
Copy link
Collaborator

Aaah, thanks for reasoning through this. Now I finally get what the concern is, and how I missed all that. The change looks great, and I'll get it merged when CI passes.

There's one open question for me (and consider this optional for this PR, please!) - is there a way to verify that a repo can be considered no_std compatible? If so, we should probably add a CI job for that.

@antifuchs
Copy link
Collaborator

Another exciting development, it sounds like portable-atomic has a critical-section feature which might allow enabling AtomicU64 on platforms that don't support it, using a suitable critical-section implementation. This might be useful for #89!

@antifuchs antifuchs added this pull request to the merge queue Feb 15, 2024
Merged via the queue into boinkor-net:master with commit b8bc796 Feb 15, 2024
17 of 18 checks passed
@mammothbane
Copy link
Contributor Author

Another exciting development, it sounds like portable-atomic has a critical-section feature which might allow enabling AtomicU64 on platforms that don't support it, using a suitable critical-section implementation. This might be useful for #89!

Yes -- it works! Using it in my projects

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.

2 participants