Use atomic_polyfill on targets without atomics

[TheButlah]

Most targets, even those without atomic instructions, support atomic load and stores. RISC-V is one of the few platforms that lacks this ability, and rustc is planning on solving that in the future (rust-lang/rust#99668).

However, in the meantime it is impossible to use defmt-rtt on the riscv non-atomic platform, such as the ESP32-C3.

atomic-polyfill is a useful crate that emulates the atomics via critical_section, but only on platforms without atomic support. On platforms with atomic support, it just re-exports the types from core::sync::atomic.

Would this PR be considered for inclusion in defmt-rtt? A prior PR was rejected here: #702

[TheButlah]

To clarify: things have changed since then, chiefly these work now:
esp-rs/esp-hal#191
esp-rs/esp-hal#175

It works on -imac, but this PR will allow it to work on -imc also (preferred because it's not using a trap handler anymore)

[Urhengulas]

I will try to come back to you about this next year week

Edit: lol, I thought "week" and wrote "year". Let's hope this wasn't an unconscious prophesy 😓 😆

[Dirbaio]

defmt-rtt only uses atomic load/store, no CAS, so using atomic-poyfill shouldn't be necessary. It'll acquire/release the critical section unnecessarily for each load/store.

Since all accesses are done within a critical section already, I think they could be replaced with UnsafeCell + write/read_volatile + fences.

[Urhengulas]

defmt-rtt only uses atomic load/store, no CAS, so using atomic-poyfill shouldn't be necessary. It'll acquire/release the critical section unnecessarily for each load/store.

Since all accesses are done within a critical section already, I think they could be replaced with UnsafeCell + write/read_volatile + fences.

Thank you @Dirbaio. Could you elaborate how "UnsafeCell + write/read_volatile + fences" would roughly look like?

[Dirbaio]

Using UnsafeCell<u32> instead of AtomicU32, and do volatile loads/stores to it. It's sound because these are always done within the critical section.

It's an ugly workaround though. Ideally Rust should expose load/store atomics on riscv32-imc (what the esp32c3 is) like it does in thumbv6m. See relevant Rust issue: rust-lang/rust#99668

[Urhengulas]

@TheButlah do you have bandwidth to change your PR to UnsafeCell as described by @adamgreig?

[TheButlah]

Is it really a good idea to add unsafe code when non-CAS atomics work fine?

I think the issue was more of a lack of knowledge about the relevant -C flags on my end. Ideally users could be educated that the following flags should be used on RISCV:

.cargo/config.toml

[target.riscv32imc-unknown-none-elf]
rustflags = [
  # enable the atomic codegen option for RISCV
  "-C", "target-feature=+a",

  # Let the core library use atomics even though it's not specified in the target definition.
  # NOTE: This is fine for load/store, but RISCV doesn't support compare-and-swap, so we don't get
  # full atomic features. For that, atomic_polyfill or the atomic trap handler should be used
  # instead.
  "--cfg", "target_has_atomic_load_store",
  "--cfg", 'target_has_atomic_load_store="8"',
  "--cfg", 'target_has_atomic_load_store="16"',
  "--cfg", 'target_has_atomic_load_store="32"',
  "--cfg", 'target_has_atomic_load_store="ptr"',
]

This is probably more the responsibility of the example code for HALs for chips using RISCV than it is a problem with defmt. But please correct me if I'm mistaken.

[Urhengulas]

Sooo ... there aren't changes to defmt necessary? I am slightly unsure about the status.

[Dirbaio]

they're not, I think this can be closed.

It'll Just Work when rust-lang/rust#99668 gets fixed for riscv. Until then users can use these flags.

[Urhengulas]

Thank you for the summary @Dirbaio