Redo the USB device stack on 32U4

From what I understand, the USBCore.cpp really needs a do-over on the EP0's ISR. It's currently written as an if-else if-else if to handle various standard IN and OUT control requests in a big mix.

The resulting code is very unclear which part does what part of the control transfer process. I recommend a do-over. First split between standard and class/vendor requests, then under standard, split between IN and OUT requests, because they have different status stages. Then within IN you can enumerate all the IN requests one at a time with switch cases. Also have a well-defined expected behavior for all of the request handlers say they should NOT wait for TXIN etc.

Right now it's just a mess. You don't know which level of code is handling for the waiting for an IN bank because it's done here and there. Total mess in my opinion.

Non EP0 endpoints are not interrupt driven but that's a bigger rewrite which could improve Leonardo and Micro a lot when acting as kybd/mouse.

It seems like one of Atmel's own demo code for a generic HID device was written this way as well. Here is Atmel's example code snippit:

//! @brief This function reads the SETUP request sent to the default control endpoint
//! and calls the appropriate function. When exiting of the usb_read_request
//! function, the device is ready to manage the next request.
//!
void usb_process_request(void)
{
   U8 bmRequestType;
   U8 bmRequest;

   Usb_ack_control_out();
   bmRequestType = Usb_read_byte();
   bmRequest     = Usb_read_byte();

   switch (bmRequest)
   {
      case SETUP_GET_DESCRIPTOR:
      if (USB_SETUP_GET_STAND_DEVICE == bmRequestType)
      {     
         if( usb_get_descriptor() )
            return;
      }
      break;

      case SETUP_GET_CONFIGURATION:
      if (USB_SETUP_GET_STAND_DEVICE == bmRequestType)
      {
         usb_get_configuration();
         return;
      }
      break;

      case SETUP_SET_ADDRESS:
      if (USB_SETUP_SET_STAND_DEVICE == bmRequestType)
      {
         usb_set_address();
         return;
      }
      break;

      case SETUP_SET_CONFIGURATION:
      if (USB_SETUP_SET_STAND_DEVICE == bmRequestType)
      { 
         if( usb_set_configuration() )
            return;
      }
      break;

      case SETUP_CLEAR_FEATURE:
      if (usb_clear_feature(bmRequestType))
         return;
      break;

      case SETUP_SET_FEATURE:
      if (usb_set_feature(bmRequestType))
         return;
      break;

      case SETUP_GET_STATUS:
      if (usb_get_status(bmRequestType))
         return;
      break;

      case SETUP_GET_INTERFACE:
      if (USB_SETUP_GET_STAND_INTERFACE == bmRequestType)
      {
         if( usb_get_interface() )
            return;
      }
      break;

      case SETUP_SET_INTERFACE:
      if (bmRequestType == USB_SETUP_SET_STAND_INTERFACE)
      {
         if( usb_set_interface() )
            return;
      }
      break;

      default:
      break;
   }

   // un-supported like standard request => call to user read request
   if( !usb_user_read_request(bmRequestType, bmRequest) )
   {
      // Request unknow in the specific request list from interface
      // keep that order (set StallRq/clear RxSetup) or a
      // OUT request following the SETUP may be acknowledged
      Usb_enable_stall_handshake();
      Usb_ack_receive_setup();
      endpoint_status[(EP_CONTROL & MSK_EP_DIR)] = 0x01;
   }
}

This is only slightly cleaner in that it says after this call the device is ready for another request and it doesn't have the "ok" stuff like here in arduino 1.8.13 USBCore.cpp:

//	Endpoint 0 interrupt
ISR(USB_COM_vect)
{
    SetEP(0);					// My comment - 2021-03-26: this only services EP0 control requests, no EP0 BULK OUT or IN or any other EPs. Because no other EPs' interrupts are enabled, only EP0 interrupts are serviced.
	if (!ReceivedSetupInt())	// My comment- 2021-03-26: this only services EP0 control requests, no EP0 BULK OUT or IN
		return;

	USBSetup setup;
	Recv((u8*)&setup,8);
	ClearSetupInt();

	u8 requestType = setup.bmRequestType;
	if (requestType & REQUEST_DEVICETOHOST)
		WaitIN();
	else
		ClearIN();

    bool ok = true;
	if (REQUEST_STANDARD == (requestType & REQUEST_TYPE))
	{
		//	Standard Requests
		u8 r = setup.bRequest;
		u16 wValue = setup.wValueL | (setup.wValueH << 8);
		if (GET_STATUS == r)
		{
			if (requestType == (REQUEST_DEVICETOHOST | REQUEST_STANDARD | REQUEST_DEVICE))
			{
				Send8(_usbCurrentStatus);
				Send8(0);
			}
			else
			{
				// TODO: handle the HALT state of an endpoint here
				// see "Figure 9-6. Information Returned by a GetStatus() Request to an Endpoint" in usb_20.pdf for more information
				Send8(0);
				Send8(0);
			}
		}
		else if (CLEAR_FEATURE == r)
		{
			if((requestType == (REQUEST_HOSTTODEVICE | REQUEST_STANDARD | REQUEST_DEVICE))
				&& (wValue == DEVICE_REMOTE_WAKEUP))
			{
				_usbCurrentStatus &= ~FEATURE_REMOTE_WAKEUP_ENABLED;
			}
		}
		else if (SET_FEATURE == r)
		{
			if((requestType == (REQUEST_HOSTTODEVICE | REQUEST_STANDARD | REQUEST_DEVICE))
				&& (wValue == DEVICE_REMOTE_WAKEUP))
			{
				_usbCurrentStatus |= FEATURE_REMOTE_WAKEUP_ENABLED;
			}
		}
		else if (SET_ADDRESS == r)
		{
			WaitIN();
			UDADDR = setup.wValueL | (1<<ADDEN);
		}
		else if (GET_DESCRIPTOR == r)
		{
			ok = SendDescriptor(setup);
		}
		else if (SET_DESCRIPTOR == r)
		{
			ok = false;
		}
		else if (GET_CONFIGURATION == r)
		{
			Send8(1);
		}
		else if (SET_CONFIGURATION == r)
		{
			if (REQUEST_DEVICE == (requestType & REQUEST_RECIPIENT))
			{
				InitEndpoints();
				_usbConfiguration = setup.wValueL;
			} else
				ok = false;
		}
		else if (GET_INTERFACE == r)
		{
		}
		else if (SET_INTERFACE == r)
		{
		}
	}
	else
	{
		InitControl(setup.wLength);		//	Max length of transfer
		ok = ClassInterfaceRequest(setup);
	}

	if (ok)
		ClearIN();
	else
	{
		Stall();
	}
}

Why don't you do it?

I could give it a try but I don't have any Arduino team connections. I'll just mock up a version that I'd happy with and keep chugging away at it until it works.

Here is what I wish the structure looks like instead:

if (REQUEST_STANDARD == (requestType & REQUEST_TYPE))	// standard reqeuests
{
	if (requestType&REQUEST_DEVICETOHOST)				// ctrl xfer with IN data
	{
		switch (setup.bRequest)
		{
			case GET_STATUS:
			
			break;
			
			case GET_DESCRIPTOR:
			
			break;
			
			case GET_CONFIGURATION:
			
			break;
			
			case GET_INTERFACE:
			
			break;
			
			default:
			// Error, unsupported standard request. Maybe stall.
			break;
		}
	}
	else											// ctrl xfer with OUT data
	{
		switch (setup.bRequest)
		{
			case CLEAR_FEATURE:
			
			break;
			
			case SET_FEATURE:
			
			break;
			
			case SET_ADDRESS:
			
			break;
			
			case SET_DESCRIPTOR:
			
			break;
			
			case SET_CONFIGURATION:
			
			break;
			
			case SET_INTERFACE:
			
			break;
			
			default:
			// Error, unsupported standard request. Maybe stall.
			break;
		}
	}
}
else												// class and vendor requests
{
	process_class_vendor_requests(setup);
}

You don't need team connections. Just open a pull request.

I'll get my code into a working state first. Observe all arduino conventions and maybe I'll make a pull request. Thanks.

This topic was automatically closed 120 days after the last reply. New replies are no longer allowed.