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.

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

Netdev2 #3452

Closed
wants to merge 184 commits into from
Closed

Netdev2 #3452

wants to merge 184 commits into from

Conversation

scottfeldman
Copy link
Contributor

This PR adds a network device driver model called netdev. There will be a companion PR for TinyGo drivers to update the netdev drivers and network examples. This PR covers the core "net" package.

An RFC for the work is here: #tinygo-org/drivers#487. Some things have changed from the RFC, but nothing major.

The "net" package is a partial port of Go's "net" package, version 1.19.3. The src/net/README file has details on what is modified from Go's "net" package.

Most "net" features are working as they would in normal Go. TCP/UDP/TLS protocol support is there. As well as HTTP client and server support. Standard Go network packages such as golang.org/x/net/websockets and Paho MQTT client work as-is. Other packages are likely to work as-is.

Testing results are here.

src/net/net.go Outdated
"time"

"tinygo.org/x/drivers/netdev"
Copy link
Member

Choose a reason for hiding this comment

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

I think maybe this shouldn't be referencing the drivers module; it might be preferable to have a copy of the netdev.Netdever interface in TinyGo instead of in drivers (or alternatively, have the primary definition be in TinyGo and referenced from drivers).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had it that way at one point, but changed it as to not add any new exports to "net" package.

Copy link
Member

Choose a reason for hiding this comment

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

I understand the motivation, but we for sure do not wish for the circular reference. Either use a subpackage, just add another file with the exported interface, or copy the interface definition into both the tinygo repo and also a duplicate the drivers package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, new commit moving drivers/netdev to tinygo/src/net/netdev. This should decouple "net" and "netdev" and remove the circular reference.

// UDPConn is the implementation of the Conn and PacketConn interfaces
// for UDP network connections.
type UDPConn struct {
fd netdev.Sockfd
Copy link
Member

Choose a reason for hiding this comment

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

Similar comment as for netdev.Netdever ... Sockfd should maybe be defined in the TinyGo repo instead of drivers. Also wondering if Sockfd should be uintptr instead of int (in os for instance file descriptors are uintptr)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

int was what socket(2) calls return, so just copied that. And -1 is a common error return value for socket(2).

@bgould
Copy link
Member

bgould commented Feb 16, 2023

Great work, this is very exciting :)

@deadprogram
Copy link
Member

@scottfeldman
Copy link
Contributor Author

@deadprogram I'm stuck. Both netdev PRs for tinygo and drivers builds are failing because they depend on each other. tinygo needs to import drivers/netdev and drivers needs to import tinygo "net" for net.useNetdev. These build fine in my go workspace where they're both present. I'm at a loss on what to do.

@scottfeldman
Copy link
Contributor Author

@deadprogram I started over in a fresh go workspace with a fresh clone of tinygo and drivers, checked out netdev branch for each and built everything again. Ran make test on tinygo and drivers and both PASS.

It seems both PRs need to be merged at the same time and folks would have to refresh both to avoid hitting the build issue we're hitting here.

deadprogram and others added 18 commits February 17, 2023 17:52
… extra rebuilds (#3453)

builds/macos, linux, windows: update to explicit restore/save for LLVM source and LLVM builds
This is a big commit that changes the way runtime type information is stored in
the binary. Instead of compressing it and storing it in a number of sidetables,
it is stored similar to how the Go compiler toolchain stores it (but still more
compactly).

This has a number of advantages:

  * It is much easier to add new features to reflect support. They can simply
    be added to these structs without requiring massive changes (especially in
    the reflect lowering pass).
  * It removes the reflect lowering pass, which was a large amount of hard to
    understand and debug code.
  * The reflect lowering pass also required merging all LLVM IR into one
    module, which is terrible for performance especially when compiling large
    amounts of code. See issue 2870 for details.
  * It is (probably!) easier to reason about for the compiler.

The downside is that it increases code size a bit, especially when reflect is
involved. I hope to fix some of that in later patches.
machine/usb/hid: add MediaKey support
gen: Better handling of sub-clusters in SVD
Running binaries in QEMU (when debugging on Linux for example) did not
work correctly as qemu-user expects the `-g` flag to be first on the
command line before the program name. Putting it after will make it a
command line parameter for the emulated program, which is not what we
want.

I don't think this ever worked correctly.
This is unsafe and should never be done.
Some vector registers must be preserved across calls, but this wasn't
happening on Linux and MacOS. When I added support for windows/arm64, I
saw that it required these vector registers to be preserved and assumed
this was Windows deviating from the standard calling convention. But
actually, Windows was just implementing the standard calling convention
and the bug was on Linux and MacOS.

This commit fixes the bug on Linux and MacOS and at the same time merges
the Go and assembly files as they no longer need to be separate.
It can be difficult to find what went wrong in a test. Omitting -v
should make it easier to see the failing tests and the output for them
(note that output is still printed for tests that fail).
@scottfeldman
Copy link
Contributor Author

Hi @soypat, pester you on the review...how's it going?

@soypat
Copy link
Contributor

soypat commented Mar 21, 2023

My apologies @scottfeldman, I totally missed the updated version. Thanks for letting me know. I'll get to it ASAP

@soypat
Copy link
Contributor

soypat commented Mar 22, 2023

Unfortunately, F_SETFL is defined in syscall_libc_darwin.go and syscall_libc_wasi.go, so not sure where the real F_SETFL should live.

Scott, the way the go tool works is that it interprets files that end with _windows.go, _darwin.go, _linux.go, etc. as having an applied build tag corresponding to the GOOS environment variable. Similarly _386.go and _amd64.go suffixed files will have the GOARCH build tag applied. See https://pkg.go.dev/go/build#hdr-Build_Constraints

@scottfeldman
Copy link
Contributor Author

@soypat

I have some updates in my local tree to this PR. Should I push those or wait until your review? Changes are:

  • rebase with dev
  • fix this F_SETFL issue
  • get the espat driver working

@soypat
Copy link
Contributor

soypat commented Mar 23, 2023

Push em!

@deadprogram
Copy link
Member

@scottfeldman scottfeldman mentioned this pull request Mar 28, 2023
@scottfeldman
Copy link
Contributor Author

I messed up my git repo for this PR, so I'm closing this PR and replacing it with a fresh one: #3614.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.