search

rust-embedded-community / usb-device

Experimental device-side USB framework for microcontrollers in Rust.
MIT License
437 stars 75 forks source link

Support architectures without atomics with polyfill #115

Closed ia0 closed 1 year ago

ia0 commented 1 year ago

Actually, atomic-polyfill should be replaced with portable-atomic but it is not there yet so I'm fine with going ahead with this merge and replacing it later on.

Oh I wasn't aware of that, good to know. Should I create an issue and/or add a # TODO(#118): Use portable-atomic. in the Cargo.toml to track this?

eldruin commented 1 year ago

Yes, please create a follow-up issue.

ia0 commented 1 year ago

Yes, please create a follow-up issue.

I created #118. Should I also add a TODO in the Cargo.toml?

Also, I might be misunderstanding something, but it looks like https://github.com/taiki-e/portable-atomic/pull/51 is merged. And I would assume we can already use portable-atomic. In which case, it's probably better to do it directly in this PR no?

Dirbaio commented 1 year ago

I intend to deprecate the atomic-polyfill crate, please don't add it as a dependency.

Which architecture are you trying to use? Is it riscv32imc? It is capable of atomic loads/stores, It is a Rust bug that it is not currently exposed. See https://github.com/embassy-rs/embassy/issues/745#issuecomment-1445069621, https://github.com/rust-lang/rust/issues/99668 . This crate only does atomic loads/stores, it doesn't use CAS operations, so it should be able to use the native core::sync::atomic once that Rust issue is fixed. Also, atomic-polyfill incurs the overhead of acquiring/releasing a critical section for loads/stores, because it's designed to also support CAS. The native atomics won't have this overhead.

(btw embassy-usb already works on architectures with this rust bug, as an alternative to usb-device, since it doesn't use atomics at all).

The trouble with portable-atomic is it requires nonstandard RUSTFLAGS=--cfg=blah to build for single core chips (or you get unnecessary overhead of critical sections for loads/stores). I've proposed changing it to Cargo features here, but haven't heard back. https://github.com/taiki-e/portable-atomic/pull/94

taiki-e commented 1 year ago

FWIW, if the required atomic operations are only load/store, portable-atomic always provides them on RISC-V, even without critical-section features or single-core cfg.

ia0 commented 1 year ago

Thanks for the explanation and pointers!

Indeed, I confirm using portable-atomic works out of the box and doesn't need the --cfg for CAS operations. I also disabled the default feature as suggested by the documentation since usb-device doesn't use atomic types larger than the pointer width. Note that I've also reverted the documentation change since there's nothing to be done by the end-user anymore.

If this PR looks good, I'll just close #118 as obsolete.