machine: modify UART and USB CDC ACM Read() to be blocking

[xudongzheng]

Using a callback for receiving serial data eliminates the need for polling.

So far I've only modified the files for RP2040 as that's what I'm testing on. If this PR is in the right direction, I will modify type UART struct for the other boards as well.

[sago35]

LGTM

[deadprogram]

The only change I would like to propose is to name this rxHandler instead. Great work @xudongzheng

[aykevl]

Using a callback for receiving serial data eliminates the need for polling.

Is that the only reason? Because handling things inside an interrupt is a pretty big hammer. If this is the only reason, a much better approach would be to make methods like ReadByte blocking (to avoid polling).

Also, this is a new API so should be added here: https://tinygo.org/docs/reference/machine/#uart

[aykevl]

This needs documentation that includes the following:

• it is an interrupt (not just a regular callback)
• it should describe what the return value is

See for example:

tinygo/src/machine/machine_nrf.go

Lines 73 to 80 in 731532c

	// SetInterrupt sets an interrupt to be executed when a particular pin changes
	// state. The pin should already be configured as an input, including a pull up
	// or down if no external pull is provided.
	//
	// This call will replace a previously set callback on this pin. You can pass a
	// nil func to unset the pin change interrupt. If you do so, the change
	// parameter is ignored and can be set to any value (such as 0).
	func (p Pin) SetInterrupt(change PinChange, callback func(Pin)) error {

[xudongzheng]

Is that the only reason? Because handling things inside an interrupt is a pretty big hammer. If this is the only reason, a much better approach would be to make methods like ReadByte blocking (to avoid polling).

Thanks for the suggestion. Please see the latest commit fe186e9, which implements blocking Read() on both RP2040 UART and USB CDC ACM. If this implementation is preferred, I will go ahead and make the change for other boards as well, and also get rid of the RX callback code.

I opted to change the Read() behavior without changing the ReadByte() behavior since the error return value becomes meaningless if ReadByte() becomes blocking. However I can change ReadByte() along with the function return value if that's preferred.

[sago35]

@xudongzheng
I believe the related PR and Issue are as follows.

#2739
#2597
#2625
#2625

The essence of this PR is to make the Read() functions for UART and USBCDC more aligned with the expected Go io.Reader interface. In other words, we want to ensure that it does not return a value where n == 0 and err == nil. This will enable standard Go functions like fmt.Scanf to work smoothly.

In that case, I believe it should be implemented without using a channel. Additionally, this change will break compatibility with TinyGo. Documentation is also necessary for this, and it needs to be reviewed carefully.

Of course, it might also be an option to define a struct like UARTxxxx instead of UART to maintain compatibility.

https://pkg.go.dev/io#Reader
Implementations of Read are discouraged from returning a zero byte count with a nil error, except when len(p) == 0.
Callers should treat a return of 0 and nil as indicating that nothing happened; in particular it does not indicate EOF.

[xudongzheng]

In that case, I believe it should be implemented without using a channel.

I don't quite see how this would be implemented without channels. For that reason, I initially opted for adding a callback rather than changing the Read() behavior - anyone who wants blocking reads can easily add a channel and write a wrapper to make reads block.

I think the overhead of channel should be pretty low, and it should be one or two at most since there won't be that many serial ports. Performance-wise, it works well enough as I was able to receive about 400KB/s on RP2040 over USB CDC ACM.

Additionally, this change will break compatibility with TinyGo. Documentation is also necessary for this, and it needs to be reviewed carefully.

I agree, though I would like a bit more clarity on the implementation direction before I start working on the documentation.

[lePereT]

Hi! Is there any progress on merging this PR?