Rp2040 uart flush - resolves #3537

[soypat]

Resolves #3537

Below are comparisons. First white signal is UART. Notice it shares the SPI busy time. When calling Flush before an SPI transaction it no longer shares busy time.

Before

After

[kenbell]

@soypat do we need a new method? Would it be better to change WriteByte?

nrf looks like this:

// WriteByte writes a byte of data to the UART.
func (uart *UART) WriteByte(c byte) error {
	nrf.UART0.EVENTS_TXDRDY.Set(0)
	nrf.UART0.TXD.Set(uint32(c))
	for nrf.UART0.EVENTS_TXDRDY.Get() == 0 {
	}
	return nil
}

stm32 looks like this:

// WriteByte writes a byte of data to the UART.
func (uart *UART) WriteByte(c byte) error {
	uart.txReg.Set(uint32(c))

	for !uart.statusReg.HasBits(uart.txEmptyFlag) {
	}
	return nil
}

ESP32, though, is more like RP2040:

func (uart *UART) WriteByte(b byte) error {
	for (uart.Bus.STATUS.Get()&esp.UART_STATUS_TXFIFO_CNT_Msk)>>esp.UART_STATUS_TXFIFO_CNT_Pos >= 128 {
		// Read UART_TXFIFO_CNT from the status register, which indicates how
		// many bytes there are in the transmit buffer. Wait until there are
		// less than 128 bytes in this buffer (the default buffer size).
	}
	uart.Bus.FIFO.Set(uint32(b))
	return nil
}

Before we add a Flush method, it seems like we should clarify the semantics of 'WriteByte' - it seems like there's currently no consistency on whether it blocks (or not).

Personally, I think not blocking on WriteByte to complete with an explicit Flush is better from a performance perspective - but it seems like changing the UART interface probably needs more discussion to make sure it's consistent across platforms.

[soypat]

So maybe we don't need Flush today. If we standardize WriteByte to be flushing I could get by. At the same time I agree with your point on the performance of this solution. I believe we'd eventually implement Flush given enough time since it's a simple change to get performance.

Personal opinion/doubt: whole WriteByte==blocking, Write==non-blocking is weird to me, what problem does each solve? I understand the existence of ReadByte, I've used that one a bunch but not so much for WriteByte.

[kenbell]

I don't think it's as simple as WriteByte==blocking, Write==non-blocking, Write is just defined as:

// Write data to the UART.
func (uart *UART) Write(data []byte) (n int, err error) {
	for _, v := range data {
		uart.WriteByte(v)
	}
	return len(data), nil
}

Write will be just as blocking (or not) as WriteByte... It's just that the implementations of WriteByte have inconsistent behaviour.

[soypat]

Aha, I understand. So if we do include this change the Flush mechanics would have to be extracted from the implementations of WriteByte which do that underneath? Are we sure the blocking is not needed in some of the implementations to prevent overflowing the transmit buffer?

[rminnich]

I initially approved but I think I take it back a bit -- hardware sucks, and, usually, I like to see a timeout on something like this. I've had systems hang when hardware messed up. Also, at least on some hardware, I've seen cases where banging it this hard will cause the hardware to spend its time telling you it's busy when it's supposed to be moving data. Not joking.
I wonder if some sort of 100 microsecond sleep in the loop, and "give up when it has to have flushed", would make sense?

[soypat]

The pico-sdk has a function for what you say called tightloopcontents() which last I checked was empty in their case but we could define it to be runtime.Gosched, which just yields the process to other goroutines which are waiting. We could also add a timeout like you say, for say, 40ms?

[aykevl]

I agree with @kenbell, we should fix the implementation of WriteByte to block until the byte is fully written. If Write blocks until it is done, it would be consistent to also let WriteByte block until it has fully written the data.

If this becomes a performance problem, I am of the opinion we should instead look into DMA for supported chips, like here: tinygo-org/tinygo-site#342

I initially approved but I think I take it back a bit -- hardware sucks, and, usually, I like to see a timeout on something like this.

Unless the datasheet or errata says there needs to be a timeout, I don't think we should be adding one. If it is needed, it likely indicates a bug somewhere else in the code.
A timeout might make sense when reading a byte because the sender may get stuck for some reason, but I don't see why it would be needed when writing a byte.

[soypat]

So we now have gosched to call in the runtime scheduler.

One reservation I have on having WriteByte block is that then you'd never fill up the tx buffer more than a single byte. In this case we should maybe expect an unexported UART.writeByte that does not block, have Write use this one underneath and have WriteByte, and have both Write and WriteByte call a blocking method before returning, i.e. flush.

[soypat]

Closing since #3832 is merged.