-
Notifications
You must be signed in to change notification settings - Fork 12
Use usb_gadget to configure gadget devices #32
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
base: master
Are you sure you want to change the base?
Conversation
| File.write!("/sys/class/net/bond0/bonding/slaves", "+usb0") | ||
| File.write!("/sys/class/net/bond0/bonding/slaves", "+usb1") | ||
| File.write!("/sys/class/net/bond0/bonding/primary", "usb1") | ||
| :os.cmd('ip link set bond0 up') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:os.cmd
I think we could use nerves_network_interface to do this correct? Might not be worth the trouble given the current state of the network stack.
File.write
Having all these File.write!/2's here makes me feel a bit weird. I don't know of a better solution however.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'm not opposed to having this bonding functionality built into nerves_network or nerves_network_interface, but it from scanning nerves_network_interface, it wasn't obvious to me how to do it/where to add it. I'm not familiar with the C APIs and these /sys/class/net interfaces made it really easy to do this one thing I need to do here.
Maybe we leave it like this for now and migrate it into the other librar(ies) as we have time to get them more fleshed-out and complete?
|
Ok, I re-worked things a bit so that it'll still work like it does today if you're using a system that doesn't support configfs. I also removed the terrible link-local DHCP server because I realized that it actually works fine without it, as long as you have mDNS set up properly on your host machine. The only weirdness is that on Ubuntu, I have to tell it to explicitly use link-local - it doesn't fail-back from DHCP to link-local. It's annoying, but I figure people who are using Linux on their desktop will be fine with configuring their network interface to use link-local mode. It works automatically on OSX and Windows without the DHCP server or any special configuration required. |
|
Just wanted to let you know that I've been watching this PR. It seems like it's in varying states of readiness. If there are little things to get in that are ready, could you split those out? |
|
@fhunleth @mobileoverlord @ConnorRigby have you had a chance to look at the https://github.com/nerves-project/usb_gadget code yet? It doesn't make much sense to merge this PR until we're happy with how that works. I think if we're happy with that, I just need to push it to Hex and we need to get it all integrated into our Slack alerting, CircleCI, etc. Other than that and the comments I made above, I think this is ready to go because it should gracefully work with existing systems. |
|
@GregMefford I've had the quickest read through the usb-gadget code a few days ago. I think it is great for now. One thing i think we should address some day is the way the built in works. I think this should be either a protocol or behaviour of some sort. NBD for now tho. I'm still looking through the changes here. FWIW i have to manually select link-local on Ubuntu/Linux with the current implementation anyway. |
|
Ok, this PR should be ready for final review now. It should be safe to merge independent of the system release. The only missing piece is that I need to push an initial release of I included some code to detect when you're running on a system that doesn't support the configfs gadgets we're trying to use (whether than be an I need to double-check tonight that this works as advertised on I think we should hold off on updating the README here until there is actually a system release available that uses the new configfs interface, so that we can say something like:
|
|
✅ Confirmed working on |
|
Could you break this into multiple PRs? There's a mix of changes in here. Some are easy to approve and some require more conversation. |
b2e1143 to
630ae3d
Compare
|
I cleaned up this branch so it's just one commit on top of #35, since it directly depends on that change. |
9811339 to
99ca5da
Compare
99ca5da to
ec8d21e
Compare
|
I've got this PR rebased (and added the It looks like the build is broken, probably because |
This still needs the docs updated and other polishing, but I wanted to get it out there so people can give feedback and try it on their machines.
One thing I noticed is that Windows 10 is kind of annoying about how it does link-local addressing with IPv6 vs. IPv4, which I think is making the mDNS lookup fail even though I can ping the device by checking on the serial console to figure out its IP address.
Related to (but no longer depends on): nerves-project/nerves_system_rpi0#62
Resolves #28