ron minnich has posted comments on this change. ( https://review.coreboot.org/c/SerialTest/+/39083 )
Change subject: Add rough approximation of SerialTest scaffold ......................................................................
Patch Set 5:
(9 comments)
https://review.coreboot.org/c/SerialTest/+/39083/5/drivers/drivers.go File drivers/drivers.go:
https://review.coreboot.org/c/SerialTest/+/39083/5/drivers/drivers.go@24 PS5, Line 24: // ConsoleDriverInit is the signature of a function creating a console driver generally, it is not done this way (sorry I missed it first time through). You would not define these types. Rather, you might declare the following: func NewConsole(name string, output <- chan string) Driver { return &Console{Name: name, Output: output,} }
and func NewFlash(whatever) Driver{ }
and so on. All your types are supposed (I think) to satisfy the Driver interface.
Also, since this is the drivers package (really, driver would do), then you can just call them Console, Flash, and Button, else the names get a bit awkward.
So then your code: c := driver.NewConsole(whatever)
Now this leaves the driver interface, above, a but confusing to me. Do all these devices have IO?
A common way to do this is actually to have them implement some other interface, typically io.Reader or io.Writer Then it does not matter what the internals are.
So: n, err := c.Read(bytes)
For the flash driver it could even implement io.WriterAt / io.ReaderAt, then you can use the offset param as the offset in flash.
Sorry if this review is a little long and a little late, but if we can take the time we can make it a pretty good example for Go in coreboot, which would be nice to do as we crush C out of existence, ha ha :-)
You can, btw, consider having them implement some other interface (pipe comes to mind) but in most cases, io.Reader etc. do the job.
https://review.coreboot.org/c/SerialTest/+/39083/5/drivers/servo/servo.go File drivers/servo/servo.go:
https://review.coreboot.org/c/SerialTest/+/39083/5/drivers/servo/servo.go@16 PS5, Line 16: // This package is a singleton, so global variables are no problem. could we ever want to have the ability to run more than one server from one program? it seems you are giving something up here.
https://review.coreboot.org/c/SerialTest/+/39083/5/drivers/servo/servo.go@29 PS5, Line 29: /* I'd consider making a servo driver which embeds the other Drivers. Or even a thing that returns a Servo, no big deal.
Then you'd have something like type Servo struct { cpu, ec, cr50 Driver // maybe? or just os.File button Driver }
func NewServo() (*Servo, error) {
/// most of the code you have below, but no globals }
https://review.coreboot.org/c/SerialTest/+/39083/5/drivers/servo/servo.go@64 PS5, Line 64: func ConsoleDriver() *drivers.ConsoleDriver { I would consider separating out the creation from the IO. Your driver interface might include a Start() method, or something, but this is putting allocation and file opening and IO in this one thing, which seems wrong somehow.
https://review.coreboot.org/c/SerialTest/+/39083/5/eve.toml File eve.toml:
https://review.coreboot.org/c/SerialTest/+/39083/5/eve.toml@2 PS5, Line 2: ConsoleDriverName = "servo" same thing here: you're in a package called driver(s), and the repetition is frowned upon. ConsoleName etc. would be nicer.
https://review.coreboot.org/c/SerialTest/+/39083/5/events/events.go File events/events.go:
https://review.coreboot.org/c/SerialTest/+/39083/5/events/events.go@10 PS5, Line 10: // An Event defines what an EventStream is looking for on its input. this is neat. I'll have to reread it again to fully get it.
https://review.coreboot.org/c/SerialTest/+/39083/5/events/events.go@34 PS5, Line 34: console *drivers.ConsoleDriver if we think of a Driver as a special type of fd or os.File, then we ought to be able to make this console Driver and then anything that implements Driver could be part of an event stream. I would say anywhere you have these concrete types is a good place to look again and see if you can have a Driver instead.
https://review.coreboot.org/c/SerialTest/+/39083/5/serialtest.go File serialtest.go:
https://review.coreboot.org/c/SerialTest/+/39083/5/serialtest.go@34 PS5, Line 34: ConsoleDriverName string I still feel like the Driver word should not be here: drivers.Configuration.ConsoleDriverName is a bit much. I'd still feel better if this were done in a way that concrete types became Drivers.
https://review.coreboot.org/c/SerialTest/+/39083/5/serialtest.go@114 PS5, Line 114: quit := make(chan bool) who uses quit? Also, there are standard Go ways of dealing with "reader closes channel" via select that would let you avoid using the quit channel at all, which would in turn let you change or remove the Driver interface and have your types satisfy io.Reader instead.
-
ron minnich (Code Review)