[embassy nrf] remove panic on uarte overrun, return error on read instead

[Kezii]

fixes #4750

probably not the optimal solution? let me know what to do instead

[i509VCB]

In an ideal world the reader would be told that an overrun happened, rather than an error message. read would return an error at the point where an overrun happened.

Edit: An example of this is what is done in the mspm0 HAL:

embassy/embassy-mspm0/src/uart/buffered.rs

Lines 950 to 965 in 35b0ba4

	if (data.0 >> 8) != 0 {
	// Cortex-M0 does not support atomic fetch_or, must do 2 operations.
	critical_section::with(|_cs| {
	let mut value = state.rx_error.load(Ordering::Relaxed);
	value |= (data.0 >> 8) as u8;
	state.rx_error.store(value, Ordering::Relaxed);
	});
	error = true;
	// only fill the buffer with valid characters. the current character is fine
	// if the error is an overrun, but if we add it to the buffer we'll report
	// the overrun one character too late. drop it instead and pretend we were
	// a bit slower at draining the rx fifo than we actually were.
	// this is consistent with blocking uart error reporting.
	break;
	}

[Kezii]

Hello,
Thank you for your comment, I tested this patch, based on your suggestion, to work correctly. this is ready for review

[lulf]

I think you can convert these load + store into a swap:

if s.rx_overrun.swap(false, ...) {
}

https://doc.rust-lang.org/std/sync/atomic/struct.AtomicU32.html#method.swap

[Kezii]

thank you, done

[lulf]

Should this be Ordering::Acquire since the write is in an interrupt and the read is not?

[Kezii]

apparently that breaks the build

error: atomic stores cannot have Acquire or AcqRel ordering
--> src/buffered_uarte.rs:93:46
|
93 | s.rx_overrun.store(true, Ordering::Acquire);
| ^^^^^^^^^^^^^^^^^
|
= help: consider using ordering modes Release, SeqCst or Relaxed
= note: #[deny(invalid_atomic_ordering)] on by default

[Kezii]

should I use Release for the store and AcqRel for the swap?

[lulf]

Ah, yes, my bad. This one should be Release.

[lulf]

Should this be Ordering::AcqRel?

[lulf]

Also Ordering::AcqRel?

[Kezii]

I used Release for the interrupt handler, and Acquire for the user code

In my understanding, there's no need for AcqRel since we are not polling on the false value ?

[lulf]

I used Release for the interrupt handler, and Acquire for the user code

In my understanding, there's no need for AcqRel since we are not polling on the false value ?

Hmm, yeah. You're right