Allow an interface to be deconfigured, better state tracking,

[pigrew]

Fixes #166

This patch does a few things:

• Bit field struct without a name is not C99 compliant, so give it a name
• Bit field structs in C99 can only have "int", "unsigned int", or "signed int" members.
• Fix HID class driver to verify that control requests are interface requests.
• Keep track of if the interface is configured or not, based on set_config calls. Return proper value in get_config.
• enums in C99 all must be signed int. We had one that is above max value of signed int, so convert it to a const.
• Calls _mount and _umount upon set_config(0) and set_config(1)
• Additional bounds checking on the set_config value
• Because of use of data types of size "not integer" (like uint8/uint16), some additional type casts are necessary.
• Changes state variable to use the names that are used in the USB spec (powered,default,address,configured).
• Remove PACKED attribute from bit field (we should be able to let the compiler do whatever; no need to keep it consistent with some standard struct layout. Might use a byte more RAM, not sure).

It has been tested with the (USB 3.2 validation tool)[https://www.usb.org/usb32tools]. This version works for XHCI interfaces. There's also one of EHCI interfaces (called usb 2.0 validation tool). Be careful since it will take over your USB interface, so you may need to use remote desktop when your keyboard and mouse stop working. :) It's Windows OS only. I did the standard device test, but not the HID tests.

In order to pass on the STM32F0, it also requires PR #165 which implements suspend and such.

The state variables are marked as volatile, but I'm not sure this is sufficient. We may need to use a mutex to guard them. The current patch is no worse than the old code in this regard, but thought in the future will be required.

[pigrew]

(I'm not sure this is ready for merging yet... I'm running into occasional issues during enumeration). Perhaps we can break up the changes into smaller parts, or merge into devel or something.

The test code was on my really weird branch with all sorts of debug printf and usbtmc stuff, so I don't know if my issues were caused by that, or something in the patch itself.

[pigrew]

I think I should undo this one; if it fails, it doesn't necessarily imply any sort of error.

[pigrew]

Change to if(x) {return false;} since this doesn't imply an error.

[pigrew]

Is there a need for an "attached" state? I'm not sure how it would change the rest of the code, though, perhaps goes from attached to default after a reset?

[pigrew]

the 0xFFFFFFFFUL was being interpreted as signed, which was above the max value of a signed long.... only way around it is to declare it as a const int. I don't like it, though.

[pigrew]

@hathach

Could you comment on this PR?

I need to resolve the conflicts (merge master back into it), but am curious about feedback before I put more time into it.

[hathach]

@pigrew I will do, this later on, but I really need to switch task now since I am blocking other people now.

[majbthrd]

I have an interest in seeing SET_CONFIGURATION support multiple configuration because I have an upcoming class driver than can use it. (It is also a handy feature to have in the stack.)

I can do a new PR specifically for this and have local code on one approach, but I don't want to step on toes due to the existing PR #167 by @pigrew.

Suppose the current, existing usbd_reset() in ./src/device/usbd.c were to be split into two functions like so:

static void usbd_reset_config(uint8_t rhport)
{
  memset(_usbd_dev.itf2drv, DRVID_INVALID, sizeof(_usbd_dev.itf2drv)); // invalid mapping
  memset(_usbd_dev.ep2drv , DRVID_INVALID, sizeof(_usbd_dev.ep2drv )); // invalid mapping

  for (uint8_t i = 0; i < USBD_CLASS_DRIVER_COUNT; i++)
  {
    if ( _usbd_driver[i].reset ) _usbd_driver[i].reset( rhport );
  }
}

static void usbd_reset(uint8_t rhport)
{
  tu_varclr(&_usbd_dev);
  usbd_control_reset();

  usbd_reset_config(rhport);
}

By taking the changes from PR #167 that are associated with configuration and adding a call to usbd_reset_config() to the TUSB_REQ_SET_CONFIGURATION handler (adjacent to the "if (tud_umount_cb) tud_umount_cb()") , the class driver can use its reset() as a mechanism to de-init as needed. Otherwise, subsequent open() calls by process_set_config() for the alternate configuration will cause issues.

@hathach mentioned in #318 that multiple configuration support was only not initially written into the stack due a lack of a way to test it. Here's how anyone can do this in Linux:

Start with your favorite ./examples/device project. Open usb_descriptors.c and make the following changes:

1. Change ".bnumConfigurations = 1" to ".bnumConfigurations = 2"
2. Copy and paste a duplicate of desc_configuration[], and rename the duplicate to alt_configuration[]
3. In alt_configuration[], change the TUD_CONFIG_DESCRIPTOR config_num argument from "1" to "2"
4. In tud_descriptor_configuration_cb(), change "return desc_configuration;" to "return (1 == index) ? alt_configuration : desc_configuration;"

At this point, you now have code capable of multiple configurations (subject to also making ./src/device/usbd.c SET_CONFIGURATION modifications of choice).

To test in Linux, you need "usb_modeswitch", which comes with assorted Linux distributions but may well be horribly obsolete. The best approach is to download the latest source code (2019-11-28 at this moment) here:

https://www.draisberghof.de/usb_modeswitch/#download

Example usage is a simple:

sudo ./usb-modeswitch -v 0xcafe -p 0x4001 --configuration 1

sudo ./usb-modeswitch -v 0xcafe -p 0x4001 --configuration 2

The utility has feedback on the success or failure, and it provides a great way of debugging and/or monitoring with a USB sniffer.

@cr1901, per your question in #318 about OS support for multiple configurations, the above utility might also be of interest (if you are not already aware of it).

[pigrew]

@majbthrd

I don't think I'll have time to write this code in the next week, so feel free to create a PR with your changes.

I'm coming to the conclusion that the class driver interfaces needs to change, perhaps to:

class->init(); // Done once, at boot, for all interfaces of all configurations?
class->open(); // opens endpoints
class->close(); //closes endpoints and gets ready for another ->open().
class->reset(); // Event notice that a USB reset has happened. Reset's driver state to be ready for opening endpoints, under the assumption that the hardware driver has already closed endpoints

Some thoughts towards concurrency also need to be made.

Another discussion is if different class drivers are allowed with different configurations. I.e., config one has a CDC device whereas config two is USBTMC. I'm not yet sure how that will be handled. I think interface numbers must be sequential (starting from zero) in all configurations.

[hathach]

I believe this PR is superseded by #1058 . Configuration can be de-configure (0) and switch back and forth to other configure state. Thank you very much for the PR and everyone for the helpful discussion.