libusb: remove the timeouts in hid_write()

[jsquyres]

The libusb/hid.c:hid_write() has timeouts in its calls to libusb_interrupt_transfer and libusb_control_transfer(). This timeout can sometimes be tripped (e.g., if the underlying hardware is slow), and no sensible error code is returned -- you just get a -1. Hence, the caller can't know that it could just try again because the only error that occurred was a timeout (or that the message may have actually gotten through).

Additionally, the other hid.c:hid_write() implementations -- linux, mac, and windows -- are all blocking and do not timeout. This makes the behavior of the libusb hid_write() different than the others, which seems weird.

This commit simply sets the libusb timeouts to 0 (i.e., infinite) to make the libusb hid_write() behave like the others.

[jsquyres]

Making this change fixes GEMakers/gea-sdk#2.

[signal11]

I suppose this makes sense. Will merge.

[jsquyres]

Great!

Do you have an estimate on when you'll be able to merge it, perchance?

[jsquyres]

Sorry to be a pest, but do you have an estimate on when you'll merge this? I ask because this fixes an real bug for us.

Thank you!

[signal11]

Actually, I'm thinking now that this may not be a good idea. There needs to be some kind of timeout so that applications don't hang forever when hardware misbehaves. Inside the Linux kernel, 5 seconds seems to be used frequently for this kind of thing.

Would this work for your application?

[jsquyres]

I don't understand -- setting a timeout of 5 seconds would keep the libusb implementation of hid_write() different than the other three:

1. linux/hid.c: uses blocking write(2)
2. mac/hid.c: uses blocking IOHIDDeviceSetReport()
3. windows/hid.c: uses blocking GetOverlappedResult()

Why should libusb/hid.c be different?

[signal11]

I think you'll find that all of the implementations have a timeout.

[signal11]

Hmm... looks like I'm wrong. It's been a while.

linux/hid.c does timeout (write() will timeout in the kernel). The others block forever.

Timeouts could easily be implemented on all the platforms and should be so that poorly-functioning (broken) devices don't hang the application, which I've observed.

Alan.

[jsquyres]

Agreed; timeouts could be implemented.

But I think that the Linux hid_write() should be blocking so that it's like the others -- at least in this application, it is assumed that the hid_write() in libusb/hid.c behaves like the other 3.

Perhaps a new function, hid_write_timeout() could expose timeout functionality?

[jsquyres]

I missed your comment about linux/hid.c and the kernel timeout.

How long does the kernel take to timeout? Is it more than 1 second?

[signal11]

All the timeouts I've seen related to USB in the kernel are 5 seconds, iirc.

[jsquyres]

Gotcha.

How's this for a suggestion:

• make all hid_write()'s block forever. I can amend this PR to include a loop around in linux/hid.c to keep trying if it gets ETIMEDOUT.
• make a new hid_write_timeout() that will take a timeout value. If it times out, it returns an obvious "I timed out" condition to differentiate it from other errors (which is the current problem with libusb/hid.c -- hid_write() just returns an error and there's no way to know that it's a timeout and you should just try again because the hardware may be slow). This would probably be a new PR; I don't have enough knowledge of the OS X / Windows APIs to write this kind of functionality.

[jsquyres]

New commit pushed to implement the suggestion.

Note that I am not a Mac or Windows programmer -- the hid_write_timeout()'s for those two are currently hardwired to return -1.

[signal11]

• windows: Currently returns -1. Needs to be written (by someone other than me).
• mac: Currently returns -1. Needs to be written (by someone other than me).

That's unfortunate then.

[jsquyres]

Ok, I removed the new hid_write_timeout() functions, and just make both the Linux and libusb versions of hid_write() be truly blocking. With these 2 commits, now all 4 versions of hid_write() are truly blocking.

Someone else can add the hid_write_timeout() functions (to match the hid_read_timeout()) functions in a different PR.

[signal11]

Blocking forever on writes is bad, like I said above.

For example, see #198

What we really need is a consistent timeout across platforms.

[jsquyres]

There are 2 related-but-different issues here:

1. Make hid_write() be blocking across all 4 implementations. Not only will that make hid_write() symmetric with the blocking hid_read() semantics, it makes the semantics of all 4 hid_write() implementations consistent with each other.
2. Add hid_write_timeout(), which will then make the read and write interfaces be symmetric (i.e., hid_read()/hid_write() are blocking, and hid_read_timeout()/hid_write_timeout() which will eventually timeout).

A previous patch on this PR tried to (at least partially) address both issues by adding all 4 hid_write_timeout() implementations, but the Mac/Windows ones were just skeletons because I have no knowledge/experience on those systems (and no way to test them). You rejected that. So I removed the hid_write_timeout() interfaces and now this PR addresses just the first issue: making hid_write() be consistently blocking.

In short: there are valid use cases for both hid_write_timeout() and hid_write() -- blocking is not always bad.

I agree that adding hid_write_timeout() will fix #198. But fixing this bug should not be contingent upon also adding hid_write_timeout(), because that's a different issue.

[amullins83]

I feel dumb now. I didn't bother to read the existing PR's before adding my own. I'm sure yours is more well thought-out. I was mainly focused on the Windows implementation.

[jsquyres]

@amullins83 Ah, you're referring to #199.

Looks like you added hid_write_timeout() versions for everything except the Mac. Great!

I still think these are related-but-different issues. There's some optional overlap because hid_write() may be able to be implemented in terms of hid_write_timeout(), but it doesn't have to be.

[jsquyres]

@signal11 Given that the hid_write_timeout() work is progressing in #199, how about merging this one? :-)

[jsquyres]

@signal11 How about a nice Christmas present of merging this PR? :-)

[Hixie]

This is also causing issues for me due to the aforementioned GEMakers/gea-sdk#2.