Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

machine: add no-op io.Reader implementation to NullSerial #2593

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
+17 −3
Open

machine: add no-op io.Reader implementation to NullSerial #2593

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/machine/machine_rp2040_gpio.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import (
"unsafe"
)

type io struct {
type rpio struct {
status volatile.Register32
ctrl volatile.Register32
}
Expand All @@ -22,7 +22,7 @@ type irqCtrl struct {
}

type ioBank0Type struct {
io [30]io
io [30]rpio
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this change intentional?

Copy link
Member Author

@bgould bgould Feb 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Importing the io package as part of the type check causes a compile error in the smoke test for RP2040 because it defines an internal type called io as well. The alternative would have been to import io with an alias in serial.go, but I thought it would be better and more appropriate to rename the type for the rp2040 to prevent this problem from happening again in the future.

intR [4]volatile.Register32
proc0IRQctrl irqCtrl
proc1IRQctrl irqCtrl
Expand Down
16 changes: 15 additions & 1 deletion src/machine/serial.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
package machine

import "errors"
import (
"errors"
"io"
)

var errNoByte = errors.New("machine: no byte read")

Expand Down Expand Up @@ -44,3 +47,14 @@ func (ns NullSerial) Buffered() int {
func (ns NullSerial) Write(p []byte) (n int, err error) {
return len(p), nil
}

// Read is a no-op; always returns that zero bytes were read with no error
func (ns NullSerial) Read(p []byte) (n int, err error) {
return 0, nil
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wondering, shouldn't it return io.EOF to show that there is nothing to read?

As per doc

// 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.

The same as ReadByte() always returns an error

tinygo/src/machine/serial.go

Lines 32 to 34 in 3458726

func (ns NullSerial) ReadByte() (byte, error) {
return 0, errNoByte
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, returning an error seems more correct to me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think should return io.EOF or should it return errNoByte (the same as ReadByte)?

}

// type check to ensure NullSerial implements interfaces for drivers.UART
var (
_ io.Reader = NullSerial{}
_ io.Writer = NullSerial{}
Comment on lines +56 to +59
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

)