tinygo-org / drivers

TinyGo drivers for sensors, displays, wireless adaptors, and other devices that use I2C, SPI, GPIO, ADC, and UART interfaces.
https://tinygo.org
BSD 3-Clause "New" or "Revised" License
585 stars 180 forks source link

netif: new package with netdev redesign ideas #629

Open soypat opened 6 months ago

soypat commented 6 months ago

@deadprogram @scottfeldman

Created ~nets~ netif package with new design ideas learned from building a network stack in Go over the last month.

Observations, suggestions, constructive criticism most welcome! I took some inspiration from gvisor's registration.go.

Note: I tried to keep true to Scott's original layout best I could, reusing netlink and netdev names- but you'll notice some shifting around, notably:

soypat commented 6 months ago

I was looking at net.Interface type. It's something that is not implemented with the current TinyGo's "net" package, but I'd like to port it over. It provides a lot of things I see duplicated here, like MAC addr, IP addr, MTU, etc.

That's actually a great idea! With net/interface.go added I think nets.Link could be rewritten as:

// Interface is the minimum interface that need be implemented by any network
// device driver.
type Interface interface {
    // HardwareAddr6 returns the device's 6-byte [MAC address].
    //
    // [MAC address]: https://en.wikipedia.org/wiki/MAC_address
    HardwareAddr6() ([6]byte, error)
    // Flags returns the net.Flag values for the interface. It includes state of connection.
    Flags() net.Flags
    // MTU returns the maximum transmission unit size.
    MTU() int
}

I'd avoid relying on the net.Interface type, as it seems as it is a heavily OS dependendent type.

Also, I was looking at net.Resolver. It's also not implemented with TinyGo's "net" package, but I think we could and it would take the place of GetHostByName().

It would seem that rather than a Stack be composed of a net.Resolver- the net.Resolver uses a stack to resolve addresses, see the Dial field. That said I'm not knowledgable on the subject and would for the time being avoid adding resolvers to the interface until I understand them better. For now I've implemented ARP address resolution via seqs.

type Stack interface { Socketer Interfaces() net.Interfaces Resolver() net.Resolver }

Hmmm- so I do agree that this is the abstraction Go has in the net package, maybe we should replace each individually though I feel. I feel each Interface would have it's own Socketer (I've called it SocketStack)- though honestly I'm still not sure how that works behind the scenes. Man, I should really get around to reading https://man7.org/tlpi/...

I've applied some of the changes to this PR mentioned above. Might still require more research?

scottfeldman commented 5 months ago

+1 netif

deadprogram commented 5 months ago

@soypat note CI failure here https://github.com/tinygo-org/drivers/actions/runs/7440065478/job/20240657521?pr=629#step:7:128

scottfeldman commented 5 months ago

looking it over...

soypat commented 4 months ago

I've removed several sentinel errors which had not fully convinced me of their usefulness. In any case we can always add them back in the future, but once added they are there forever.

deadprogram commented 4 months ago

Overall this seems like a good set of patterns. I would love to see a simple mermaid diagram or something added to help better understand how it all fits together.

I presume that the idea is once this PR was merged, the next step would be to replace the implementations for the current wifi devices?

soypat commented 4 months ago

@deadprogram yes, the idea is to start using these abstractions once merged to get a better idea of issues with them if any. I'd go easy and replace them one at a time, or better yet, start by adding the Pico W (since its not in drivers yet) and explore by not breaking anything new.

scottfeldman commented 4 months ago

I have to update the tinygo net package periodically to back-port upstream changes, which I'm happy to do but would rather not...it's error prone and not so much fun. With this new PR, we'd still need to do that.

I think we should revisit fully porting the net package into tinygo with the goal of getting tinygo-specific code pushed upstream. The net package is structured to target multiple OS targets; to me it looks like we could add tinygo as a new target without too much work.

And the tinygo port would inform us of the interface(s) a tinygo stack must implement. For example, tcpsock_tinygo.go would provide sysDialer.dialTCP(), returning a netFD. The netFD is specific to tinygo but would implement the net.Conn interface.

A suggested approach would be to 1) do the full port using existing netdev so we can test with known good devices/drivers, and 2) replace netdev with what interface(s) naturally fall out of the full porting process.

I'm excluding net/http. We can tackle a full port of net/http separate from this effort.

soypat commented 4 months ago

@scottfeldman Interesting observations- Maybe we're going about this the wrong way and the API we wish to expose is simpler?

seqs already exposes a way of generating a net.Conn, but the HAL we are choosing in seqs is Berkeley Socket which means to link net to our seqs we must:

  1. Make the seqs stacks.TCPConn implement Berkeley sockets by way of some intermediary framework
  2. So that we can link that intermediary framework with the net.useNetdev which receives a berkeley socket abstraction
  3. So that we can use the Berkeley socket abstraction to make a net.TCPConn

So we just did extra work to get to the same place and probably also took a performance hit...

scottfeldman commented 4 months ago

@soypat Ya, I saw that seqs exposes a net.Conn, so it's already aligned with where I think we're going if we do a full net package port. My hope is the full port exercise would tell us exactly the interfaces needed from a stack. It's likely the existing netdev interfaces disappear and for the embedded stack devices (wifinina, rtl8720n) have a stack shim. Or maybe they just implement net.Conn and be done with it. There are other details like resolver and interfaces to work out, but again, I think a full port of net would tell us what's needed.

Should we try it (the full port)? I have some time to work on it, but you have first dibs.

The risks: