Add USB MSC + MSC/CDC Composite Class

[rudihorn]

trafficstars

This PR is an alternative to #586. It extends the USB profiles with an MSC class and an MSC+CDC composite class.

Summary

• This PR gets rid of the dependencies on pdev->pUserData and pdev-pClassData, allowing existing classes to be reused in composite class interfaces.
• The usbd_ep_conf files have been simplified and improved to prevent redundancy.
• The MSC class has been taken from the STM32 USB device library already being used by stm32duino, and adapted to remove the dependencies mentioned earlier.
• The MSC+CDC composite class has been added. The build flag USE_USB_CDC_MSC enables this composite class.

Validation

• [x] CI check
• [X] This has been tested on https://github.com/rudihorn/Marlin/tree/usb-core using the bigtree skr pro v1.1 controller. While the implementation isn't perfect, as the order of initialisation of sd card / usb / msc isn't ideal yet, https://github.com/rudihorn/Marlin/blob/usb-core/Marlin/src/HAL/STM32/msc_sd.cpp shows how a MSC handler can be implemented independent of the target device. Note that it is necessary to add both the PID and a VID to the build flags or this will not work.

The board used has a STM32F407ZG, but it should hypothetically work on any currently supported MCU. In particular the MCU's using HAL_PCDEx_PMAConfig should probably be tested.

Discussion points

• [ ] Ensure that the FIFO code is correct, the old one seemed wrong: https://github.com/rudihorn/Arduino_Core_STM32/blob/24bec9a4a6de8a60aed50172dbd71f15b809724f/cores/arduino/stm32/usb/usbd_conf.c#L513-L523 It is poorly documented.
• [ ] How to best handle USBSerial / USB classes. Currently USBSerial implicitly enables USB, which may not be desired before MSC has been correctly initialised. Alternatively adding code to reset the USB automatically.
• [x] Possibly better interface for exposing the MSC code.
• [x] Currently only FS is supported for composite class, need to test others. This seems redundant though, and it may be nicer to find a better way of handling this.

Code formatting

• [x] Needs to be checked

Closing issues

#586

[rudihorn]

https://github.com/stm32duino/Arduino_Core_STM32/pull/586#issuecomment-636298309 explains what is being done in this PR as well.

[rudihorn]

There is progress on the SD card initialization. I've changed it so that the default MSC handler pretends to be busy, and in my marlin implementation the sd card continues to not be ready until a valid capacity can be read. This means that despite the incorrect behaviour of marlin to initialise the SD card first, it is possible to exchange the MSC handler and still use the SD card. Unfortunately it is not possible to do the same for the logical unit numbers. I also found a bad bug in the STM32 MSC SCSI implementation, which seems fixed in later versions.

I have been using this feature to perform firmware upgrades on my board by uploading to the SD card and I have to say that it seems to work nicely :)

[rudihorn]

Is there a way to disable / change the AStyle check for usbd_ep_conf.h? It currently wants to remove indentation and I find the behavior horrible. Sure it might be readable in an editor supporting highlighting, but without I find it just isn't nice to read.

[fpistm]

Is there a way to disable / change the AStyle check for usbd_ep_conf.h? It currently wants to remove indentation and I find the behavior horrible. Sure it might be readable in an editor supporting highlighting, but without I find it just isn't nice to read.

No, the rule is the same for all the repo.

I also found a bad bug in the STM32 MSC SCSI implementation, which seems fixed in later versions.

I will update soon the USB middleware, it is in the pipe, see #1027

[rudihorn]

I have now added a new USBMscHandler abstract class, which can be used for implementing the interface. Unlike the other interface, it contains default implementations for IsReady / WriteProtection / Init and to simplify the interface it is one handler per LUN.

The following is a simple example of what a handler can look like:

  class MyUSBMscHandler : public USBMscHandler {
    public:
      bool GetCapacity(uint32_t *pBlockNum, uint16_t *pBlockSize) {
        *pBlockNum = // ...
        *pBlockSize = // ...
        return true;
      }

      bool Write(uint8_t *pBuf, uint32_t blkAddr, uint16_t blkLen) {
        // ...
      }

      bool Read(uint8_t *pBuf, uint32_t blkAddr, uint16_t blkLen) {
        // ...
      }
  };

The user can then use either of these methods on USBDevice to initialize one or many handlers:

    void registerMscHandler(USBMscHandler &pHandler);
    void registerMscHandlers(uint8_t count, USBMscHandler **pHandlers, uint8_t *pInquiryData);

@fpistm As the MSC handler has been duplicated because alterations were necessary, an update to the USB middleware is not urgently required yet.

[fpistm]

@fpistm As the MSC handler has been duplicated because alterations were necessary, an update to the USB middleware is not urgently required yet.

In fact I will merge it soon as I'm currently validate it.

[fpistm]

@rudihorn for Astyle issue you can use the python script in CI/astyle/, it requires you have astyle installed.

[rudihorn]

Is there a reason why https://github.com/stm32duino/Arduino_Core_STM32/blob/ce7e3e17d8d0241fc5f4695e65a2e4c8e66b27bf/libraries/SrcWrapper/src/stm32/hw_config.c#L66 contains code to initialise USB serial? I have removed it now, as this same initialisation code is called from https://github.com/stm32duino/Arduino_Core_STM32/blob/ce7e3e17d8d0241fc5f4695e65a2e4c8e66b27bf/cores/arduino/USBSerial.cpp#L34 (and now from USB.cpp).

[fpistm]

yes, IIWR the goal is to init the USB as soon as possible. The fact it is also called in begin() is to allow to init it again after a end() call.

[rudihorn]

I've added a SerialUSB.begin() call to the hw_init function again so that the old behaviour is restored and it compiles. I've also added back the other descriptor speeds. The CI check also passes now. As far as I can tell the most important things should be done.

For the two remaining unchecked points: I can't think of a better way to handle the USB/USBSerial/... so I would suggest we just leave it like this. I've looked over the endpoint buffer configuration code and I think it should produce the same behavior as before, but I'm not entirely sure why there is this number of eps * 8 base offset and I don't have devices to test it.

As such I think I am happy for this code to be reviewed / merged, and if there is anything else I can do for this PR to be merged in let me know.

[rudihorn]

Although some behaviour does still seem a bit odd, so I will try syncing the MSC and CDC class code to the latest version and will wait until #1027 is merged in.

[fpistm]

@rudihorn I see no change in the boards.txt. I guess you use PIO and define the correct definition is your config? Anyway, we should be able to select the config via the Arduino menu.

Could you also share some use case example ?

[fpistm]

@rudihorn I've merged the new USB device middleware thanks #1027 Could you rebase and update accordingly? Thanks in advance

[rudihorn]

@rudihorn I've merged the new USB device middleware thanks #1027 Could you rebase and update accordingly? Thanks in advance

Thanks for this! I've just rebased and noticed where a few more errors are. Also haven't checked to see if it all compiles etc., but hoping this will help.

I don't have an arduino compatible board to test an example on but can still try rustling something up. And yes I've been using PIO until now.

[rudihorn]

With the new framework and some other changes this now seems stable to me. I did have some issues with the very high priority (default #define USBD_IRQ_PRIO 1) in combination with other interrupts and Marlin, but changing this to a lower priority (tested #define USBD_IRQ_PRIO 9) this seemed to work. I am wondering if the default USB priority is a bit high.

[rudihorn]

@fpistm I have updated the boards.txt config and have come up with the following example. Not sure where the example should go though. I have only compiled and not tested it though.

/*
  SD card read/write

  This example shows how to attach an SD card as a mass storage device.
  
   SD card attached to SPI bus as follows:
 ** MOSI - pin 11
 ** MISO - pin 12
 ** CLK - pin 13
 ** CS - pin 4 (for MKRZero SD: SDCARD_SS_PIN)

*/

#include <SPI.h>
#include <SD.h>
#include <USB.h>
#include <USBMscHandler.h>

#define BLOCK_SIZE 512

Sd2Card card;

class Sd2CardUSBMscHandler : public USBMscHandler {
  public:
    bool GetCapacity(uint32_t *pBlockNum, uint16_t *pBlockSize) {
      *pBlockNum = card.cardSize();
      *pBlockSize = BLOCK_SIZE;
      return true;
    }

    bool Write(uint8_t *pBuf, uint32_t blkAddr, uint16_t blkLen) {
      for (uint32_t i = 0; i < blkLen; i++) {
        if (!card.writeBlock(blkAddr + i, pBuf))
          return false;
        pBuf += BLOCK_SIZE;
      }
      return true;
    }

    bool Read(uint8_t *pBuf, uint32_t blkAddr, uint16_t blkLen) {
      for (uint32_t i = 0; i < blkLen; i++) {
        if (!card.readBlock(blkAddr + i, pBuf))
          return false;
        pBuf += BLOCK_SIZE;
      }
      return true;
    }

    bool IsReady() {
      return true;
    }

  private:
};

Sd2CardUSBMscHandler usbMscHandler;

void setup() {
  // Open serial communications and wait for port to open:
  Serial.begin(9600);
  while (!Serial) {
    ; // wait for serial port to connect. Needed for native USB port only
  }

  Serial.print("Initializing SD card...");

  uint8_t csPin = SD_CHIP_SELECT_PIN;

  // initialise sd card
  if (!card.init(SPI_HALF_SPEED, csPin)) {
    Serial.println("initialization failed!");
    while (1);
  }
  Serial.println("initialization done.");

  USBDevice.registerMscHandler(usbMscHandler);
}

void loop() {
  // nothing happens after setup
}

[rudihorn]

@fpistm Are there any updates on this and what more is required to get this merged?

[fpistm]

Hi @rudihorn currently not. what is missing is time. I've several stuff in the pipe so I do my best but it is hard to handle all the flow at the same time. This PR is not small and requires some times to properly review it and test.

[Bob-the-Kuhn]

Status?

[fpistm]

Currently not sorry.

[rhapsodyv]

@fpistm It would be nice do join this work with mine, in #1196.

[fpistm]

@fpistm It would be nice do join this work with mine, in #1196.

Yes. I will probably focus on USB stuff after the 2.0.0 release.

[rhapsodyv]

@rudihorn is there any standard for the USB class? If not, maybe we can follow lib maple interface, as USBCompositeSerial and USBMassStorage.

[rhapsodyv]

I'm getting framework-arduinoststm32/system/Drivers/STM32F4xx_HAL_Driver/Inc/stm32f4xx_hal_rcc_ex.h:2023:41: error: 'UNUSED' was not declared in this scope if a include "usbh_core.h" or just USB.h. I need to manually include a header that defines UNUSED before including usbh_core.h, but it didn't happen before testing this PR.

[Tjeerdie]

Any update on if this is going to be added? Or is the PR dead? I did not test it yet but it seems to be a great addition to the core to be able to use Serial and mass storage over USB.

[rhapsodyv]

Any update on if this is going to be added? Or is the PR dead? I did not test it yet but it seems to be a great addition to the core to be able to use Serial and mass storage over USB.

We are actively using it on Marlin for some boards, together with #1196. It seems pretty stable. Just waiting for the merge.

[Tjeerdie]

I tried it today and it indeed worked! I adapted the example to use the SDIO interface and the SD library write/read function in STM32SD.h. It is slow in writing (probably because of the 512 byte blocks) but it works. This example worked in PIO using the black development boards of STM32F407

Ow and for who ever want to try with PIO don forget to put the "-DUSBD_USE_CDC_MSC" in your build_flags

Only thing i noticed is the wait until it connects to Serial USB is not working the way it did before. Its not waiting for any connection on my system anymore? Not sure why. It just continues.

Another question, is it possible to de-initialize the usb mass storage from software? I want to be able to enable/disable the USB mass storage function from software on some triggers inside the use code. Then the user code can take control of the SD card again to log data to it.

#include <STM32SD.h>
#include <USB.h>
#include <USBMscHandler.h>

#define BLOCK_SIZE 512

#define LED_PIN PA1

class Sd2CardUSBMscHandler : public USBMscHandler {
  public:
    bool GetCapacity(uint32_t *pBlockNum, uint16_t *pBlockSize) {
      BSP_SD_CardInfo CardInfo;
      BSP_SD_GetCardInfo(&CardInfo);
      *pBlockNum = CardInfo.LogBlockNbr;
      *pBlockSize = CardInfo.LogBlockSize;
      return true;
    }

    bool Write(uint8_t *pBuf, uint32_t blkAddr, uint16_t blkLen) {
      digitalWrite(LED_PIN, LOW);
      bool res = false;
        if(BSP_SD_WriteBlocks((uint32_t*)pBuf,(uint32_t)(blkAddr),blkLen, 5000) == MSD_OK)
        {
          /* wait until the Write operation is finished */
          while(BSP_SD_GetCardState() != MSD_OK){}
          res = true; 
        }
      digitalWrite(LED_PIN, HIGH);
      return res;
    }

    bool Read(uint8_t *pBuf, uint32_t blkAddr, uint16_t blkLen) {
      digitalWrite(LED_PIN, LOW);
      bool res = false;
      if(BSP_SD_ReadBlocks((uint32_t*)pBuf,(uint32_t)(blkAddr),blkLen, 5000) == MSD_OK)
      {
        /* wait until the read operation is finished */
        while(BSP_SD_GetCardState()!= MSD_OK){}
        res = true;
      }
      digitalWrite(LED_PIN, HIGH);
      return res;
    }

    bool IsReady() {
      
      return true;
    }

  private:
};

Sd2CardUSBMscHandler usbMscHandler;

void setup() {
  //Set led pin to output for visual feedback on write/read actions
  pinMode(LED_PIN, OUTPUT);
  // Open serial communications and wait for port to open:
  Serial.begin(9600);
  while (!Serial) {
    ; // wait for serial port to connect. Needed for native USB port only
  }

  Serial.print("Initializing SD card...");
  // initialise sd card
  if (BSP_SD_Init()!=MSD_OK) {
    Serial.println("initialization failed!");
    while (1);
  }
  Serial.println("initialization done.");
  USBDevice.registerMscHandler(usbMscHandler);
}

void loop() {
  // nothing happens after setup
}

[rhapsodyv]

Today I found an issue with this PR, when using with a F1 board.

This PR add a class called USB... but, we have this on F1:

#define USB                 ((USB_TypeDef *)USB_BASE)

File: system/Drivers/CMSIS/Device/ST/STM32F1xx/Include/stm32f103xe.h

So we need a new class name...

[rhapsodyv]

What about rename the USB class to USBComposite?

[rhapsodyv]

Is this dead?

I can help here too.