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

USB - Interfaces Other Than Default #19

Closed
MelbourneDeveloper opened this issue Jan 4, 2019 · 59 comments

Comments

@MelbourneDeveloper
Copy link
Owner

commented Jan 4, 2019

Any given USB device can have multiple different interfaces, and pipes within those interfaces. There should be a way to select which interface is used before InitializeAsync is called.

@schotime

This comment has been minimized.

Copy link

commented Jan 16, 2019

I'm assuming that will fix being able to connect like: usbDevice.OpenEndpointReader(ReadEndpointID.Ep02);
?

@MelbourneDeveloper

This comment has been minimized.

Copy link
Owner Author

commented Jan 16, 2019

@schotime , I believe the code you have posted is from LibUsbDotNet. I've had problems with that library, but yes, the plan is to allow for a similar pattern to be able to select a different interface.

There is some work involved because the way to do this is different on each platform. If this is causing you grief, I could perhaps fast track this issue. Are you currently trying to use Usb.Net and hitting this problem?

@schotime

This comment has been minimized.

Copy link

commented Jan 16, 2019

Yeh, I was trying to use your library, but read through the code and found it selects the first one which isn't going to work for my application. libusbdotnet is working for now for me, but from your comments it seems like i want to migrate.

@schotime

This comment has been minimized.

Copy link

commented Jan 16, 2019

oh and i'm currently using winusb ;)

@MelbourneDeveloper

This comment has been minimized.

Copy link
Owner Author

commented Jan 16, 2019

@schotime I'm happy to do whatever it takes to help you out. What platform are you using?

@MelbourneDeveloper

This comment has been minimized.

Copy link
Owner Author

commented Jan 16, 2019

OK. Great. So, basically your trouble is that you want to use an endpoint other than the default one on Windows. Is that right?

@schotime

This comment has been minimized.

Copy link

commented Jan 16, 2019

Its for connecting to a golf simulator. ;) Where abouts in Melb are you? I work in Hawthorn.

@schotime

This comment has been minimized.

Copy link

commented Jan 16, 2019

yep, thats exactly right.

@MelbourneDeveloper

This comment has been minimized.

Copy link
Owner Author

commented Jan 16, 2019

Awesome. I live close enough! What I will do is try to fix the bug for your situation. Then you can try it out. If that doesn't solve the problem I can remote on to your computer with Teamviewer and work through the issue.

@MelbourneDeveloper

This comment has been minimized.

Copy link
Owner Author

commented Jan 16, 2019

I will have a look at this after work tonight.

@schotime

This comment has been minimized.

Copy link

commented Jan 16, 2019

No probs. Cheers

@MelbourneDeveloper

This comment has been minimized.

Copy link
Owner Author

commented Jan 16, 2019

@schotime , are you part of an indoor golf simulator business? Do you have a website?

@MelbourneDeveloper MelbourneDeveloper added this to Active Work in Release 2.4.0 Jan 16, 2019

@schotime

This comment has been minimized.

Copy link

commented Jan 16, 2019

na. integrating with Foresight GC2 launch monitor
image

@MelbourneDeveloper

This comment has been minimized.

Copy link
Owner Author

commented Jan 16, 2019

Cool!

@MelbourneDeveloper

This comment has been minimized.

Copy link
Owner Author

commented Jan 17, 2019

@schotime so this is how the code is looking. I will put out an alpha release soon methinks...

            var devices = await DeviceManager.Current.GetDevices(_DeviceDefinitions);
            TrezorDevice = devices.FirstOrDefault();
            await TrezorDevice.InitializeAsync();

            var windowsDevice = (WindowsUsbDevice)TrezorDevice;
            var firstInterface = windowsDevice.UsbInterfaces.First();
            var writePipe = firstInterface.UsbInterfaceEndpoints.FirstOrDefault(p => p.IsWrite);
            var readPipe = firstInterface.UsbInterfaceEndpoints.FirstOrDefault(p => p.IsRead);
            firstInterface.ReadEndpoint = readPipe;
            firstInterface.WriteEndpoint = writePipe;
            windowsDevice.WriteUsbInterface = firstInterface;
            windowsDevice.ReadUsbInterface = firstInterface;
@MelbourneDeveloper

This comment has been minimized.

Copy link
Owner Author

commented Jan 17, 2019

Look right?

@MelbourneDeveloper

This comment has been minimized.

Copy link
Owner Author

commented Jan 17, 2019

@schotime, the NuGet is here and should be listed soon. It's alpha so you have to include prerelease:

https://www.nuget.org/packages/Usb.Net/2.3.1-alpha

The code is as above. You have to first get the device, and then call InitializeAsync. This should get all the available interfaces. The will be in the UsbInterfaces collection. However, it won't give you a textual name of the interfaces. It will just set up the handle to the interface for writing. You can set the read/write endpoints as well, but that probably won't be necessary.

If you have any trouble, I strongly recommend cloning the repo, turning on break on all errors and send me any information about what goes wrong.

@MelbourneDeveloper

This comment has been minimized.

Copy link
Owner Author

commented Jan 17, 2019

This is the code branch where the fix has been made:

https://github.com/MelbourneDeveloper/Device.Net/tree/%2319

@MelbourneDeveloper MelbourneDeveloper moved this from Active Work to Maybe In Release in Release 2.4.0 Jan 28, 2019

@MelbourneDeveloper MelbourneDeveloper moved this from Maybe In Release to Active Work in Release 2.4.0 Jan 28, 2019

@MelbourneDeveloper MelbourneDeveloper moved this from Active Work to Maybe In Release in Release 2.4.0 Jan 28, 2019

@MelbourneDeveloper

This comment has been minimized.

Copy link
Owner Author

commented Feb 19, 2019

@schotime , this issue is going to go back on the low priority list. Let me know if it's still important.

@schotime

This comment has been minimized.

Copy link

commented Feb 19, 2019

Oh mate. Apologies. I've been swamped and must have missed your update. Appreciate the effort to get something for me.

Low priority is fine. It's probably what mine is to swap libraries at the moment as it is currently working with libusb.

I'll keep you updated. Sorry again.

@MelbourneDeveloper

This comment has been minimized.

Copy link
Owner Author

commented Feb 19, 2019

@schotime no problem. There is now available wrapper for libusb. If you use it the code will be compatible with other platforms and you will get device listening. It's been tested on windows and Mac.

Sample
https://github.com/MelbourneDeveloper/Device.Net/tree/master/src/Device.Net.LibUsbSample

Wrapper
https://github.com/MelbourneDeveloper/Device.Net/blob/master/src/Device.Net.LibUsb/LibUsbDevice.cs

@MelbourneDeveloper

This comment has been minimized.

Copy link
Owner Author

commented Jul 3, 2019

@mxjones thanks! I'm going to sit on this for a while. There's a reason I didn't finish off this branch last time.

I want to make sure I get the design right. The design issue is the inheritance hierarchy. WindowsUsbDevice inherits from WindowsDeviceBase. But, it doesn't inherit from a hypothetical UsbDeviceBase so it can't share any code between the AndroidUsbDevice and WindowsUsbDevice. To make that happen, C# would need multiple inheritance which would be a bad design anyway.

So, I'm considering a redesign where all of the platform specific stuff is dependency injected in. I will let you know how the progress goes.

@MelbourneDeveloper

This comment has been minimized.

Copy link
Owner Author

commented Jul 3, 2019

@mxjones have a look at the #19-Android branch now. I've completely separated the platform specific implementation from the usb interface handling. This means that the handling can be reused on Android.

Thoughts are welcome...

@mxjones

This comment has been minimized.

Copy link

commented Jul 4, 2019

@MelbourneDeveloper didn't get as much time to look at this as I planned - but I think it looks really good. Design does feel nicer (I personally like composition over inheritance most of the time anyway)

I did make some progress with getting more advanced stuff in, and had the interrupt endpoint working (+ timeout) - shall I create a new issue for adding the interrupt endpoint? or is that something you were not thinking of adding? (Interrupt stuff I did in my fork is here, https://github.com/mxjones/Device.Net/tree/%2319-Android - feel free to comment on that if you see anything wrong/want done differently)

I plan on spending some time getting an android test up and running on the weekend, so will have a chance then to test out the new android code you committed.

@MelbourneDeveloper

This comment has been minimized.

Copy link
Owner Author

commented Jul 4, 2019

@mxjones yes please create a new issue with as much background info as possible. I don't know what an interupt endpoint is so you'll have to explain it.

I won't be able to look at this until the new design is done. I can only focus on one thing at a time.

I think it looks really good. Design does feel nicer (I personally like composition over inheritance most of the time anyway)

Awesome. I think this style will eventually get rolled out to all the devices . It will solve a lot of the code sharing issues that exist.

@mxjones

This comment has been minimized.

Copy link

commented Jul 4, 2019

Sure will do, and no problem

@MelbourneDeveloper

This comment has been minimized.

Copy link
Owner Author

commented Jul 6, 2019

@mxjones I've gone further. I think the general design is pretty close now. I've tested Windows but not Android. If you feel like testing Android, that would be great. I will hopefully finish up tomorrow.

@mxjones

This comment has been minimized.

Copy link

commented Jul 6, 2019

Hi @MelbourneDeveloper - I created a super simple Android project for testing your branch and with a couple of minor changes all of the basics are working great (Connecting, Writing some bytes, Reading response, Disconnecting) - awesome!

Couple of minor things noted when testing Android - they are minor things that just may need slight refactoring after the design changes.

  • I needed to move opening the Android device higher up in AndroidUsbDeviceHandler, because it is used in the block above it (passing to endpoint ctor & claiming interface)

_UsbDeviceConnection = UsbManager.OpenDevice(_UsbDevice);
if (_UsbDeviceConnection == null)
{
throw new Exception("could not open connection");
}

to

  • The default way of finding the endpoints didn't work for me, I suspect it is because it is looking for interrupt endpoints:

var isRead = usbEndpoint.Type == UsbAddressing.XferInterrupt && (int)usbEndpoint.Address == 129;
var isWrite = usbEndpoint.Type == UsbAddressing.XferInterrupt && ((int)usbEndpoint.Address == 1 || (int)usbEndpoint.Address == 2);

On my local, I just changed this to look for the bulk endpoint type and then read/write depending on direction In/Out

https://github.com/mxjones/Device.Net/blob/a896060ebb360ec57a101ae899dd12be594c5297/src/Usb.Net.Android/AndroidUsbDeviceHandler.cs#L167-L168

Another related minor tweak is that it is checking if the endpoint is null before assigning:

if (androidUsbInterface.ReadEndpoint == null && isRead)
{
androidUsbInterface.ReadEndpoint = androidUsbEndpoint;
ReadUsbInterface = androidUsbInterface;
}
if (androidUsbInterface.WriteEndpoint == null && isWrite)
{
androidUsbInterface.WriteEndpoint = androidUsbEndpoint;
WriteUsbInterface = androidUsbInterface;
}

this is always false because the getter will set it to the first compatible endpoint if it is null - I just changed this to check the associated interface instead. (The end result of this was that the Interfaces never get assigned, by default - but manually assigning these works OK)

https://github.com/mxjones/Device.Net/blob/a896060ebb360ec57a101ae899dd12be594c5297/src/Usb.Net.Android/AndroidUsbDeviceHandler.cs#L173-L183

  • The DeviceListener wouldn't find my Device correctly, I'm not sure why - didn't look into it too much as I haven't tried it in develop branch yet & probably unrelated to these changes - I just handled the finding/permission/receiver directly

No notes when testing Windows, require no changes & minimum test app is working (Connecting, Writing some bytes, Reading response, Disconnecting)

Sorry for the long post - just a few notes whilst I was testing that may prove useful. This is great work though, good job so far!

@MelbourneDeveloper

This comment has been minimized.

Copy link
Owner Author

commented Jul 6, 2019

@mxjones all good points here. I will digest these slowly.

Question: IntializeAsync is needed to get the interfaces. But, then, we may need to "claim" the interface. Currently, this is done in one step on Android, but that doesn't make sense. We should enumerate the interfaces and "claim" only one when we have the list. So, I can't see how initialization can be done without two steps... This creates problems for the rest of the design of the system...

Any thoughts?

Maybe the intended interface number can be specified in the constructor, and if it is specified, it will try to claim that one?

@MelbourneDeveloper

This comment has been minimized.

Copy link
Owner Author

commented Jul 7, 2019

@mxjones I did a PR on your behalf and I've merged your code in to this branch: https://github.com/MelbourneDeveloper/Device.Net/tree/%2377-AndroidAndInterrupt . I hope you don't mind. This branch is up to date with the latest from develop. Lots of refactoring.

I think what you've done with endpoint defaulting there is much better. I only copied that from some library so there's a good chance the original code only worked with the Trezor.

I've added a stub here for interrupts, so feel free to flesh this out so I understand what it is about.
#77

@MelbourneDeveloper

This comment has been minimized.

Copy link
Owner Author

commented Jul 7, 2019

@mxjones I will leave the interrupt and timeout stuff for 3.1. It's definitely on the right track, but I've gotta fix the UWP and LibUsb versions which is going to take more time.

@mxjones

This comment has been minimized.

Copy link

commented Jul 8, 2019

@MelbourneDeveloper Thanks for this! Theres a bit of cleaning up to do on that no doubt, so will see what is needed (eg. I made changes to the Sample project for my local testing, I will revert those etc)

@mxjones all good points here. I will digest these slowly.

Question: IntializeAsync is needed to get the interfaces. But, then, we may need to "claim" the interface. Currently, this is done in one step on Android, but that doesn't make sense. We should enumerate the interfaces and "claim" only one when we have the list. So, I can't see how initialization can be done without two steps... This creates problems for the rest of the design of the system...

Any thoughts?

Maybe the intended interface number can be specified in the constructor, and if it is specified, it will try to claim that one?

I see, for default initialisation perhaps it only grabs the first interface and claims that. I think passing the interface number in the ctor is a good option, to override the default behaviour (eg. It will default to 0, but can be set in ctor if a different interface is needed).
Could also move claiming to the UsbInterface instead, adding a 'Claim' method so if someone is setting it up/choosing the interface manually themselves then they can issue the call as needed.

So by default, it would claim Interface[0] - the one that is sets as the default interface in for the Device - but with your added work exposing multiple interfaces, someone could find the one they want and then just call UsbInterface.Claim() (not 100% sure on this - it could just be a call on the device - Claim(int interfaceId) ) if needed. Is this only required for Android? Couldn't see anything similiar to this in the Wiindows code, and not checked UWP yet.

@mxjones I will leave the interrupt and timeout stuff for 3.1. It's definitely on the right track, but I've gotta fix the UWP and LibUsb versions which is going to take more time.

No problem, I think it makes sense to leave that for 3.1 as these changes are already quite large, Thanks for the Merging that, as mentioned I will look at cleaning my bits up a little & implementing the Android Interrupt read method (yet to start testing the Android interrupt, next on my list! :) )

I will reply to that specific issue with some more information.

@MelbourneDeveloper

This comment has been minimized.

Copy link
Owner Author

commented Jul 8, 2019

I think passing the interface number in the ctor is a good option, to override the default behaviour (eg. It will default to 0, but can be set in ctor if a different interface is needed).

Yep. This is looking light the right option I think. I might take a look at some other libraries and see what they are doing.

@MelbourneDeveloper

This comment has been minimized.

Copy link
Owner Author

commented Jul 14, 2019

@mxjones quick update.

The refactoring of UWP didn't according to plan, but it did elucidate some things. It turns out that my reference device for USB Trezor doesn't use bulk or control transfer. It uses Interrupt. This is what has caused so much confusion and why the code was all wrong on Android, and UWP.

You probably already saw this, but this is what I had to do to get the Trezor working on UWP. It is listening to interrupt events. One good thing about this is that it tells me that I am on the right track with the event idea... On UWP, they've implemented events for when interrupt data is received. So, that makes me more confident that events are the right choice.

Anyway, all the code I've written is hack code to make the Trezor work. So, I now need a new device to test with that is more normal. Do you know of any cheap devices I can order that require sending/receiving?

The latest code is in 78-UWPRefactor if you want to have a look.

@MelbourneDeveloper

This comment has been minimized.

Copy link
Owner Author

commented Jul 14, 2019

@mxjones , anyway, I'm getting there. All this should have never been necessary. It's just ended up this way because of the design of the Trezor.

@MelbourneDeveloper MelbourneDeveloper moved this from Todo to Done in 3.0 Jul 17, 2019

@mxjones

This comment has been minimized.

Copy link

commented Jul 17, 2019

@MelbourneDeveloper Sorry been busy but have been checking in on this

Aaah, I see where the confusion came from! That is odd for that device to be using the interrupt for standard communication, some devices don't support bulk transfer - but I would think it should use control over interrupt for that if that was the case - not sure!

I hadn't seen that, but makes sense - It sounds like you are in a good place now though and the design sounds promising - I especially like the event idea for interrupt data, so that the user doesn't need to explicitly poll themselves.

I will take a look at that branch / latest code tomorrow and pull it down / point my test applications at it and have a play.

As far as cheap devices, I'm not actually too sure. I am currently using DSLR Cameras (not cheap!) and don't have much experience with any other devices to be honest - I would say a mouse/keyboard would most likely use interrupt endpoint for their transfer (key press -> event model sounds reasonable to me) but apart from that not actually too sure - so sorry I can't be of more help on that one, but I am happy to test with the devices I have and feed that back.

@MelbourneDeveloper

This comment has been minimized.

Copy link
Owner Author

commented Jul 21, 2019

@mxjones @schotime

Version 3.0 of Usb.Net is up: https://www.nuget.org/packages/Usb.Net/3.0.0-beta

I would be really stoked if you could test and report issues. Thanks.

@mxjones

This comment has been minimized.

Copy link

commented Aug 1, 2019

I've been super busy and not had as much time to test as I wanted but can confirm it is working OK for me in my Windows test code, I will look porting over my Android test code and testing that also - will let you know, or raise any issues if anything comes up.

Thanks for your great work on this!

@MelbourneDeveloper

This comment has been minimized.

Copy link
Owner Author

commented Aug 3, 2019

@mxjones thanks! That's awesome. Beta 2 is up now. If you could give that a bash, it would be great.

@MelbourneDeveloper

This comment has been minimized.

Copy link
Owner Author

commented Aug 3, 2019

@schotime Version 3 Beta 2 is up. The main feature change from 2->3 has been the ability to select Usb Interfaces and Endpoints on all platforms. The UsbDevice class now has a property called UsbInterfaceManager. This allows you to set the interface after initialization, and then you can select endpoint on the IUsbInterface. Documentation is coming on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
3 participants
You can’t perform that action at this time.