Make the control transfer buffer size actually configurable

[mvirkkunen]

Currently there is only a hack to double its size. Ideally the user should be able to supply their own &'static [u8] to use for control transfers - however this is easier said than done.

Design goals:

1. Enable the user to supply their own buffer...
2. ...but still somehow have a default buffer that is used automatically if the user doesn't supply one.
3. Don't waste memory by having the default buffer allocated even when not used.

I tried to deal with "3" by using a generic argument for the control buffer type. It threaded through a lot of types and was too ugly to justify (having a generic argument on UsbDevice just for this is excessively verbose)

[mvirkkunen]

I suppose one idea would be to make the "custom buffer" a configuration option as it is now, but instead make it get rid of the default buffer and force the user to provide one. It would be more ideal to have a way without any configuration options though.

[Windfisch]

Hi @mvirkkunen,

Another option might be to make the buffer size a generic argument using the typenum crate. This would still waste memory, though, when not used.

I made a quick fix in https://github.com/Windfisch/usb-device/commit/ba098f847d8cb5e1228426d2153ab02ac49cc34a where i just added control-buffer-512 and control-buffer-1024 flags.

I'm currently building a MIDI interface, and I am hitting even the 256 bytes limit with as few as 12 midi ports. :(

I really think that we should offer some way to increase the buffer size further right now, even if there might be a better option implemented later.

Would you agree to merge such a pull request (possibly with more buffer sizes; 2k, 4k and 8k, maybe with intermediate values)? In that case, I'd prepare one with the granularity we agree on.

(After all, even the cheap STM32F103 blue pill boards have 20kb of RAM, which is mainly unused in a lot of projects)

[mvirkkunen]

Now that we have at least some const generics support in stable Rust, maybe the generic argument could be worth looking into now. I seem to remember it being an extremely noisy addition when I looked into it last time but... I'll have to try and see.

Actually even without const generics in the crate code, asking the user to pass something like a BorrowMut<[u8]> might be ergonomic, since large arrays have had trait implementations since 1.47.0.

Another idea I've been toying with before const generics is having a method in the builder to supply your own &'static mut [u8] to use as the buffer. It unfortunately requires unsafe code due to the static mut but such is life. If the slice is given, then it is used and the internal buffer is ignored (and wastes some space). There could be a configuration flag to disable the internal buffer if you want a larger buffer and you don't want to waste the 128 bytes on the unused buffer.

[Windfisch]

I don't think that passing a &'static mut [u8] is a good idea if we can just have "const generics" (which aren't really a thing yet, but can be emulated using types using typenum and generic-array).

If you like, I could try adapting the code to use typenum, and file a pull request if it works. I made an example.

[vitalyvb]

Hi,

DFU protocol (firmware upgrade via USB) uses control endpoint to transfer firmware blocks to/from the host. It works with a 128-byte transfers, looks like the only drawback is that it's slightly slower, so not a big deal. From the code point of view, it also basically just works, but a little bit awkward.

Anyway, while it's not about configurable buffer size at all, as a possible alternative, here's a proof of concept for optional "chunked" Control IN transfers. Something similar should be possible for Control OUT transfers, and maybe this path can be used to override the built-in descriptor generator if class needs something more powerful or large.

At this point the code is very experimental, I haven't tried flashing it to the actual controller to see if it really works, it just passes software DFU tests.

usb-device patch - https://github.com/vitalyvb/usbd-dfu/commit/7416c72603aa95457fee52fc6e8af0be6c0c0f56. I guess the main change is in write_in_chunk() which handles two ControlStates and three possible buffer sources. From class' perspective, API succeeds with one of the two types of successes - 1) success, we're done, and 2) success, we're transferring chunks. Distinction is useful because most likely class will need to track the transfer state itself, and class' code looks a bit cleaner compared to a few other things I tried.

Class' patch - https://github.com/vitalyvb/usbd-dfu/commit/19ebd4022bb73b2fbee0b1389a0abe48e4d873a1. Most convoluted part is in control_in_chunk() which tries to figure out when transfer should stop, and I didn't spend too much time cleaning this up...

Tests I used are focused on DFU. It's using the Control Endpoint, but not well suited to test various USB protocol errors, error handling is certainly not complete. In particular, when a chunked transfer aborts for some reason, class' implementation should be aware of that and change its internal state accordingly.

Get descriptor, string test - https://github.com/vitalyvb/usbd-dfu/blob/7c43e7b689939f3512ce2e6c9b32b801619d4d8c/tests/dfu_1k_tests.rs#L217-L251 Chunked transfer test - https://github.com/vitalyvb/usbd-dfu/blob/7c43e7b689939f3512ce2e6c9b32b801619d4d8c/tests/dfu_1k_tests.rs#L266-L296

Code branch - https://github.com/vitalyvb/usbd-dfu/tree/usbdevtest2

Is this approach worth pursuing any further?

Thanks!

[Windfisch]

Hi @vitalyvb, do you think that your proposed change would make it possible to support "long" descriptors for other drivers without having to modify them? Or would we still need to increase the buffer size in these cases?

[vitalyvb]

Hi @Windfisch, as far as I understand, control_in() in a class should be able to override usb-device's handling of descriptors, so it should be possible to craft a fully custom reply on a GET_DESCRIPTOR request. I haven't tried this yet. Perhaps in a few days I can try.

[vitalyvb]

@Windfisch, unfortunately, it didn't work easily. Primary problem is that GET_DESCRIPTOR needs descriptors from all classes, but every single control_in() handler works with one class only, so this needs a new API. If there's only one class, this is how it would look with the existing API - https://github.com/vitalyvb/usbd-dfu/commit/ab601b871ad3bca563d343926eb6486d51f4849f

[ryan-summers]

I think the right approach here is to follow what smoltcp does. Specifically. the user is required to supply a control pipe buffer. This eliminates the need for feature flags and const-generics to the repo. I don't see a need to provide a default buffer if the user doesn't supply one, as this just obfuscates what's going on.