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.

[deadprogram]

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

[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 fe186e971e58101b92b003de0c4e64d24e4bc787, 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?