Dark Mode

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

netif: new package with netdev redesign ideas#629

Open
soypat wants to merge 14 commits intodevfrom
netdev-redesign
Open

netif: new package with netdev redesign ideas#629
soypat wants to merge 14 commits intodevfrom
netdev-redesign

Conversation

Copy link
Contributor

soypat commented Dec 18, 2023 *
edited
Loading

@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:

  • netdev.Netdever interface is now netif.Stack
    • Reason: The method set represents what's often referred to as a Network Stack- and the method set has no reference to the actual Device.
  • probe.Probe() shall now return a netif.Netdev (new Probe not yet implemented)
    • Reason: More wieldy to have a single interface that contains the union of old Netlink and Netdev.

nrwiersma reacted with thumbs up emoji
soypat requested a review from deadprogram December 18, 2023 23:47
Copy link
Contributor

scottfeldman left a comment *
edited
Loading

Choose a reason for hiding this comment

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

I appreciate your effort to re-use names/interfaces/structs with existing code, but I can see it's constraining you and things are still a little messy. Would you consider a fresh rewrite? Maybe just keep the interfaces and get rid of the noisy #defines.

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.

Interface might be a better name than Link.

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().

Stack could be the top-level interface used by the "net" package:

type Stack interface {
Socketer
Interfaces() net.Interfaces
Resolver() net.Resolver
}
type Socketer interface {
// syscall replacements for socket(2) calls
Socket(...)
Bind(...)
...
// These move:
// GetHostByName() moves to net.Resolver.LookupIp()
// Addr() moves to net.Interface.Addrs()
}

Copy link
Contributor Author

soypat commented Dec 20, 2023

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?

nets/nets.go Outdated
// Disconnect device from network
NetDisconnect()
// Notify to register callback for network events
NetNotify(cb func(Event))
Copy link
Contributor

scottfeldman Dec 20, 2023

Choose a reason for hiding this comment

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

NetNotify isn't wifi specific. It's for wired/wireless interfaces to indicate interface UP/DOWN.

soypat reacted with thumbs up emoji
type InterfaceWifi interface {
Interface
// Connect device to network
NetConnect(params WifiParams) error
Copy link
Contributor

scottfeldman Dec 20, 2023

Choose a reason for hiding this comment

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

These weren't intended to be Wifi only. I need them for this ch9120 eth driver also. SSID/Passphrase are specific to Wifi, but connect time-out, retries, watchdog timeout, dhcp mode (static or dynamic), and maybe more.

Copy link
Contributor Author

soypat Dec 20, 2023

Choose a reason for hiding this comment

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

From what I recall programming the ENC28J60, the connection for RJ45 connections is passive, is it not?


// Addr returns IP address assigned to the interface, either by
// DHCP or statically
Addr() (netip.Addr, error)
Copy link
Contributor

scottfeldman Dec 20, 2023

Choose a reason for hiding this comment

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

this should move to Interface, correct?

Copy link
Contributor Author

soypat Dec 20, 2023 *
edited
Loading

Choose a reason for hiding this comment

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

IP Addresses are not actually Interface specific- it's network-stack level.

Copy link
Contributor Author

soypat Dec 20, 2023

Choose a reason for hiding this comment

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

Also, I think I'd leave it with no error returned since there is really no information that can be returned inside the error other than the IP address has not been set- and that piece of information can already be returned in-band inside netip.Addr's zero value.

nets/nets.go Outdated
Bind(sockfd int, ip netip.AddrPort) error
Connect(sockfd int, host string, ip netip.AddrPort) error
Listen(sockfd int, backlog int) error
Accept(sockfd int, ip netip.AddrPort) (int, error)
Copy link
Contributor

scottfeldman Dec 20, 2023

Choose a reason for hiding this comment

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

BTW, just so we don't regress, this is now:

Accept(sockfd int) (int, netip.AddrPort, error)

nets/nets.go Outdated
)

//go:linkname UseSocketStack net.useNetdev
func UseSocketStack(stack SocketStack)
Copy link
Contributor

scottfeldman Dec 20, 2023

Choose a reason for hiding this comment

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

I think I liked UseStack and Stack better.

nets/nets.go Outdated

// WifiStack is returned by `Probe` function for devices that communicate
// on the OSI level 4 (transport) layer.
type WifiStack interface {
Copy link
Contributor

scottfeldman Dec 20, 2023

Choose a reason for hiding this comment

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

I'm not following how this works. Can you add probe.go file(s) to show how this works for wired/wireless, and hw/sw stack devices? e.g. cyw43 wireless sw stack, wifinina wireless hw stack, ch9120 wired hw stack, lan8720 wired sw stack.

AuthTypeWPA2Mixed // WPA2/WPA mixed authorization
)

type WifiAutoconnectParams struct {
Copy link
Contributor

scottfeldman Dec 20, 2023

Choose a reason for hiding this comment

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

These aren't wifi-specific. Need same for wired.

return ErrConnectModeNoGood
}
go func() {
// Wifi autoconnect algorithm in one place,
Copy link
Contributor

scottfeldman Dec 20, 2023

Choose a reason for hiding this comment

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

Ya, that seems good. But for wired also.

}

type EthPoller interface {
Interface
Copy link
Contributor

scottfeldman Dec 20, 2023

Choose a reason for hiding this comment

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

Did you mean to have Interface here?

Copy link
Contributor Author

soypat Dec 20, 2023

Choose a reason for hiding this comment

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

An EthPoller should implement Interface as well I think. At least it should have the Flags method to check if the link is still up.

// HardwareAddr6 returns the device's 6-byte [MAC address].
//
// [MAC address]: https://en.wikipedia.org/wiki/MAC_address
HardwareAddr6() ([6]byte, error)
Copy link
Contributor

scottfeldman Dec 20, 2023

Choose a reason for hiding this comment

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

net.HardwareAddr?

Copy link
Contributor Author

soypat Dec 20, 2023

Choose a reason for hiding this comment

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

I'd avoid net.HardwareAddr for the same reasons we switched to netip.Addr, it'd be a pain to have to start checking probe implementations for the implementation details of the HardwareAddr method and if the memory was copied or a reference was passed. Also there's the issue of heap memory usage. All in all, if we can avoid net.HardwareAddr I'd do so.

soypat changed the title nets: new package with netdev redesign ideas netif: new package with netdev redesign ideas Jan 7, 2024
Copy link
Contributor

scottfeldman commented Jan 10, 2024

+1 netif

Copy link
Member

deadprogram commented Jan 10, 2024

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

soypat reacted with thumbs up emoji

soypat added 2 commits January 10, 2024 09:11
nets/nets.go was replaced by netif/netif.go
Copy link
Contributor

scottfeldman commented Feb 1, 2024

looking it over...

Copy link
Contributor Author

soypat commented Feb 25, 2024

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 reviewed Feb 25, 2024
type Stack interface {
// GetHostByName returns the IP address of either a hostname or IPv4
// address in standard dot notation
// GetHostByName(name string) (netip.Addr, error)
Copy link
Member

deadprogram Feb 25, 2024

Choose a reason for hiding this comment

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

Is this supposed to be commented?

Copy link
Contributor Author

soypat Feb 25, 2024

Choose a reason for hiding this comment

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

Yes, We can delete that- I've created the Resolver interface for that purpose.

Copy link
Member

deadprogram commented Feb 25, 2024

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?

Copy link
Contributor Author

soypat commented Feb 25, 2024

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

Copy link
Contributor

scottfeldman commented Feb 26, 2024

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.

Copy link
Contributor Author

soypat commented Feb 27, 2024

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

Copy link
Contributor

scottfeldman commented Feb 27, 2024 *
edited
Loading

@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:

  • Full port doesn't fit (flash/sram), or doesn't leave much for app
  • We hit something that's not ported or can't be ported to TinyGo (unlikely)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

deadprogram deadprogram left review comments

+1 more reviewer

scottfeldman scottfeldman left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants