Replace option-based RAW_IO support with new API functions.

[martinling]

This PR implements an improved API for controlling the WinUSB RAW_IO feature.

The LIBUSB_OPTION_WINUSB_RAW_IO option is removed. This is not an API break, as this option was not yet included in a release. The aim is to replace this API before the release of 1.0.27.

The option-based approach worked, but the requirement to attach several variadic arguments made it ugly and error-prone. It also had the undesirable effect of coupling two operations: fetching the maximum supported transfer size, and enabling or disabling raw I/O.

Instead of the option-based mechanism, three new functions are added to the API as follows:

/* Check whether the backend in use supports raw I/O. Returns 1 if so, otherwise 0.
 *
 * When raw I/O is available, an application must take some extra steps if it needs to
 * achieve the highest possible inbound throughput on bulk and interrupt endpoints.
 * See libusb_set_raw_io.
 */
int libusb_backend_supports_raw_io(void);

/* Retrieve the maximum transfer size supported for raw I/O for an inbound bulk or
*  interrupt endpoint on an open device.
 *
 * Returns a maximum transfer size, or a negative error code.
 * If the backend does not support raw I/O, returns LIBUSB_ERROR_NOT_SUPPORTED.
 */
int libusb_get_max_raw_io_transfer_size(libusb_device_handle *dev_handle, unsigned int endpoint_address);

/* Enable or disable raw I/O for an inbound bulk or interrupt endpoint on an open device.
 *
 * When raw I/O is in use on an endpoint, all transfers on the endpoint must have a size that is
 * a multiple of the maximum packet size returned by `libusb_get_max_packet_size`, and less
 * than or equal to the maximum transfer size returned by `libusb_get_max_raw_io_transfer_size`.
 *
 * Returns LIBUSB_SUCCESS or a negative error code.
 * If the backend does not support raw I/O, returns LIBUSB_ERROR_NOT_SUPPORTED.
 */
int libusb_set_raw_io(libusb_device_handle *dev_handle, unsigned int endpoint_address, int enable);

For further background, see #1208 (comment) and the discussion preceding it.

[mcuee]

@tormodvolden and @hjelmn

Please help to look at the proposed API. Thanks.

[tormodvolden]

Maybe it is easier to review if one commit reverses commit 9e4bb9c and a second commit adds the new functions "from scratch"? I also note that LIBUSB_OPTION_LOG_CB and LIBUSB_OPTION_MAX should be decremented.

@hjelmn What do you think about the suggested API functions here? Could this be the lesser of evils?

[martinling]

Changed to two commits, one reverting the old API and one adding the new one.

[martinling]

One thought I've had about the proposed API is that at the moment, whether an endpoint supports RAW_IO depends only on the backend in use, and the proposed libusb_backend_supports_raw_io function reflects that. However, the current libusb backend system is an internal implementation detail, and I can imagine scenarios in which the situation becomes more complicated, with different devices/interfaces being handled by different drivers & backend code.

What we actually need to know is just whether we can enable RAW_IO on a specific endpoint on an open device, so perhaps I should replace:

int libusb_backend_supports_raw_io(void);

with:

int libusb_endpoint_supports_raw_io(libusb_device_handle *dev_handle, unsigned int endpoint_address);

[martinling]

On that last point, see also the discussion in #1260.

[mcuee]

@hjelmn
Please help to comment whether you like the proposal or not. Thanks.

[mcuee]

@jwrdegoede

Sorry to disturb you as well. I understand that you are mostly out of libusb project in the past few years, still just wondering if you have some comments here for this new API proposal, as you are very much familiar with libusb and Linux USB internals. Thanks.

[mcuee]

@LudovicRousseau
Please help here as well. Thanks.

[mcuee]

Reference:

• Misc: To release 1.0.27 #1094 (comment)

From Martin:

The only detail I am unsure about is what API the libusb project will accept for this feature.

I've made multiple proposals, including the latest one in #1297 for which I've added a couple of possible variations, and I'm still awaiting an answer or even any feedback.

Either "yes, this version is good, please move forward with it" or "no, please change X or satisfy condition Y" would allow me to make progress. With neither I cannot do anything further.

[LudovicRousseau]

I do not use or know Windows.
The change looks OK for me.

[patstew]

One thought I've had about the proposed API is that at the moment, whether an endpoint supports RAW_IO depends only on the backend in use, and the proposed libusb_backend_supports_raw_io function reflects that. However, the current libusb backend system is an internal implementation detail, and I can imagine scenarios in which the situation becomes more complicated, with different devices/interfaces being handled by different drivers & backend code.

What we actually need to know is just whether we can enable RAW_IO on a specific endpoint on an open device, so perhaps I should replace:

int libusb_backend_supports_raw_io(void);

with:

int libusb_endpoint_supports_raw_io(libusb_device_handle *dev_handle, unsigned int endpoint_address);

Isn't libusb_backend_supports_raw_io redundant if you change the signature like that? You get that information from libusb_get_max_raw_io_transfer_size, which either gives you a size or NOT_SUPPORTED. You could just remove libusb_backend_supports_raw_io entirely instead.

Or you could have libusb_get_max_transfer_size(dev, endpoint) which returns INT_MAX on a normal endpoint, and the raw IO maximum once you've enabled libusb_set_raw_io, so that libusb_set_raw_io is the only raw_io specific function.

[martinling]

Isn't libusb_backend_supports_raw_io redundant if you change the signature like that? You get that information from libusb_get_max_raw_io_transfer_size, which either gives you a size or NOT_SUPPORTED. You could just remove libusb_backend_supports_raw_io entirely instead.

That was basically the approach I had in this earlier proposal. I rejected it in favour of this version because in practice, the extra function made for cleaner and clearer code. It is a lot simpler to check first whether the whole RAW_IO business is necessary, and then if it is, to treat any subsequent error as a failure, than to need to special-case one particular error code that should be treated differently from others.

If we're already making the decision to add new API functions for this feature, I don't think there is any particular reason to aggressively minimise the number of functions involved. That was the approach of the previously merged libusb_set_option based version, and it just resulted in a very overloaded function call.

Or you could have libusb_get_max_transfer_size(dev, endpoint) which returns INT_MAX on a normal endpoint, and the raw IO maximum once you've enabled libusb_set_raw_io, so that libusb_set_raw_io is the only raw_io specific function.

With that design, you would not be able to check what the maximum transfer size would be with RAW_IO, until you have already enabled RAW_IO. In practice that value can be obtained in advance, and it makes sense to do so so that buffer sizes etc can be allocated accordingly.

I also think that a generically named libusb_get_max_transfer_size would be confusing. Its presence in the API would give the impression that there may be a maximum transfer size for any endpoint. In practice libusb has no maximum transfer size, and is only going to have one in a very specific case: for an inbound bulk/interrupt endpoint when RAW_IO is in use. The existing function name helps makes it clear that this isn't something you have to worry about if you're not doing RAW_IO.

[martinling]

I realised recently that there is another approach we could take with this. It requires a little more work behind the scenes on the libusb side, but would make for a much simpler API, with only one new function needed - one that simply enforces an existing best-practice recommendation.

As we've already discussed, it's not possible for libusb to abstract away all of this RAW_IO business, because there is no safe way to handle the case where the application submits a transfer with a sub-max-packet size.

However, we can handle the case where the application submits a transfer larger than the WinUSB maximum: we just split that transfer into multiple OS transfers behind the scenes. And we can do that perfectly safely for a RAW_IO endpoint, so long as the application's transfer size is a multiple of the endpoint's maximum packet size. If that condition is met, we can split the transfer into smaller OS transfers that each meet the same requirement.

Therefore, the only API we actually need for RAW_IO is a way for the application to guarantee that its transfers on a given endpoint will be a multiple of the maximum packet size. Once we have that commitment from the application, we can safely go ahead and enable RAW_IO for the endpoint, and deal with the other requirements behind the scenes.

Making that commitment is good practice for the application anyway, because of the issue of overflow. In practice, a device can send any packet length up to the maximum in response to an IN token. If the application isn't ready for all the resulting bytes, libusb is stuck with a the problem of what to do with the excess. As discussed in the documentation, there's no good way to deal with this, so it's advised that applications should just stick to multiples of the endpoint maximum packet size to avoid this being a problem.

In fact, the issue of overflow is so fundamentally messy, that it's quite arguable that libusb should never have supported transfer sizes that weren't a multiple of the maximum packet size. It's too late to go back on that decision now, but we can allow the application to opt-in to this requirement, and it makes sense to do so.

So how about a single call, something like:

/* Allow libusb to require that subsequent transfers, on the specified endpoint, will have a size
 * that is a multiple of the maximum packet size returned by \ref libusb_get_max_packet_size
 * for that endpoint.
 * 
 * While this requirement is in force, transfers with a size that is not a multiple of the endpoint's
 * maximum packet size will be rejected.
 * 
 * Enabling this requirement avoids the possibility of an overflow where a device sends a packet
 * larger than expected (see \ref libusb_packetoverflow). It also allows libusb to enable features
 * to improve performance, such as the use of RAW_IO on WinUSB.
 */
int libusb_enforce_max_packet_transfers(
    libusb_device_handle *dev_handle,
    unsigned int endpoint_address,
    int enable);

Behind the scenes, this call would have the side effect of enabling RAW_IO.

[patstew]

I think realistically nobody is going to write that code and get the error handling right etc, and this PR does what we want, so I'm in favour of sticking with this approach. There's a bunch of packet sizes you can choose that always work, e.g. 64k, and higher on bulk, so all this packet sizes stuff is not crucial anyway for the usual RAW usecase of recieving a long stream of bytes.

[whitequark]

I would really like to have this available for the Glasgow Interface Explorer--any way to move this forward?

[HannesFranke-smartoptics]

Shouldn't this function return max_transfer_size?

[HannesFranke-smartoptics]

I am currently designing a composite usb device with multiple interfaces.
This leads to the device getting the USB_API_COMPOSITE backend and the raw_io functions failing.
But the interface (which is the one which winusb_handle is actually used) does have the correct API.

Looking at how all the other calls are routed (e.g. submit_bulk_transfer) I think to be consistent you should add functions composite_get_max_raw_io_transfer_size() and _set_raw_io(), which are added to the windows_usb_api_backend struct. Then in the composite_() functions dispatch based on the interface.

[HannesFranke-smartoptics]

A quick fix for testing I am using is to set

    sub_api = priv->usb_interface[interface].sub_api;