Replace vhci with usbip

[ttyridal]

usbip won out and is now bundled with the kernel

[buserror]

Thanks for that -- some files are missing licences..

Also, I don't like having to drag in libpthread mutexes around, so far I decided that thread safety was not supposed to be done by simavr, but by any 'board' that wants to deal with that. I resent having to drag that bloat around for the 2 people who are going to want to use it...
Would there be a way to make isolate the thread safety outside of the module?

[ttyridal]

I'll check up on the licenses..

Regarding mutexes I'll have to think some more on that. With writers on both sides it's kind'a hard to avoid I think - And I'm definitively not sure it's better to reinvent something with eg atomics.

Potentially I could change ioctl with part irq's (which seems to be the preferred way to communicate? not sure why I opted for ioctls). What's the story on threading and custom irqs?

[buserror]

Well IRQs are single threaded as well, same as the rest. If your notifications are asynchronous, you could perhaps queue/FIFO them and use a socketpair() for semaphore -- that's usually what I do, it's clean, doesn't involved mutexes and is very scaleable.

For example, IRQs notifications could queue queued by one thread, dequeued and 'forwarded' by another. In fact we could make a generic proxy layer to do that potentially. Depends on the use case I suppose...

[ttyridal]

Still not completely sold on this. If using socketpair, the straight forward approach would be to move all (register) state to the part, which seems kind of wrong. Also, it wouldn't solve the notifications back to simavr (can't block-recv indefinitely in simavr thread). One could write to the socket and signal an interrupt or something - But that's really undefined behavior I think. It would mean (usb) thread updating the state of simavr thread without locking.

The only(?) clean solution would be to keep the lock state in usb thread, and then in simavr thread:

    send(LOCK_REQ)
    recv() // lock confirmation
    ... // critical section
    send(LOCK_RELEASE)
    recv() // lock release confirmation, to catch errors and deadlocks

Possible, but feels a bit like over engineering for the sake of avoiding a standard library ... Also it would probably work on x86, but uncertain about cache coherency etc on other platforms (socket send is not a fence/memory-barrier afaik)

How did you envision (safe) communication from part to simavr with sockets?

[buserror]

The idea here is to remove contention, so you don't need locks. So you get one writer per 'shared' object, and there is a communication using a small FIFO+Semaphore for any 'other' access.
I haven#t looked at your code in details tho, i'm going to pull this PR and have a look...

[ttyridal]

Cool, I did have another look at it to possibly get rid of the usb thread.. It may be an alternative to

while(1) {
    avr_run()
    usb_run()
}

Would avoid the issues. Unfortunately I haven't found the time to implement it yet

[buserror]

@ttyridal perhaps you could even replace the 'run' and 'sleep' callbacks in a avr_t instance, or even implement a small #iomodule 'peripheral', or both... shouldnt be hard to find a way...

[buserror]

Any more work on that? it'd be sad to let it drop, as the USB bit is borken at the minute, and I'm tempted to yank it...

[ttyridal]

It's been terrible busy at the $job lately, but usb is part of 99% of my avr projects 😆 so...

[ttyridal]

I'm sad to say this PR will not make it into my schedule for any foreseeable future.. Probably best to if some one else volunteers to get it merged, or close it.

[vintagepc]

Hah, figures I'll find this after getting usb-vhci finally working only to discover it's unreliable. I'll have a look at this offering; if I end up using it appreciably I will look at making an upstream contribution.

[vintagepc]

Can I ask where usbip_types.h is generated/sourced from? It is slightly outdated now as the main protocol is now v 111 instead of 106 and I'm hoping to determine if it's a compatible change or not.

[ttyridal]

Can I ask where usbip_types.h is generated/sourced from?

That would be the usb standard.

[vintagepc]

Thanks, I found what I needed in the usbip sources/headers. I am not seeing any differences in the relevant structs as a result of the protocol version change.

[vintagepc]

I'm sad to say that this seems to have reliability issues outside of the provided example board; once I try to use this with a simulated 32u2 (parts/example code works fine built for my added 32u2 definition) I regularly get stalls, incomplete replies from the device (as in device not writing message to endpoint buffer), or complete inability for usbip to see the avr.

This is with a known-working-on-real-hardware firmware running LUFA for CDC serial; it looks like there is some expected behaviour that is missing.

I am currently attempting to debug them and will follow up if I make any finds.

[vintagepc]

BTW, this won't work on OSX, it can't even compile:

/Users/runner/work/MK404/MK404/parts/components/usbip.c:318:35: error: fields must have a constant size: 'variable length array in structure' extension will never be supported
                struct usb_interface_descriptor interf[p->udev.bNumInterfaces];
                                                ^

[ttyridal]

That's to be expected i guess.. if there is no support for usbip (or virtual usb hosts in general) from the kernel

…

On Mon, 4 Oct 2021, 01:25 vintagepc, ***@***.***> wrote: BTW, this won't work on OSX, it can't even compile: — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#247 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB4LWPZTQFDCRZZ2LQBRKI3UFDQ7RANCNFSM4DZ7HF2Q> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.