Add USB MSC + MSC/CDC Composite Class #1088

rudihorn · 2020-06-02T09:59:29Z

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

CI check
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.
Possibly better interface for exposing the MSC code.
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

Needs to be checked

Closing issues

#586

rudihorn · 2020-06-02T10:14:29Z

#586 (comment) explains what is being done in this PR as well.

rudihorn · 2020-06-02T22:22:55Z

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 · 2020-06-02T22:27:52Z

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 · 2020-06-03T05:44:19Z

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 · 2020-06-03T12:35:51Z

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 · 2020-06-03T12:42:00Z

@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 · 2020-06-03T12:58:04Z

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

rudihorn · 2020-06-03T14:25:59Z

Is there a reason why

Arduino_Core_STM32/libraries/SrcWrapper/src/stm32/hw_config.c

Line 66 in ce7e3e1

USBD_CDC_init();

contains code to initialise USB serial? I have removed it now, as this same initialisation code is called from

Arduino_Core_STM32/cores/arduino/USBSerial.cpp

Line 34 in ce7e3e1

CDC_init();

(and now from USB.cpp).

fpistm · 2020-06-03T15:17:49Z

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 · 2020-06-03T18:47:46Z

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 · 2020-06-04T09:52:42Z

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 · 2020-06-04T14:32:12Z

@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 · 2020-06-04T16:49:18Z

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

…guration

rudihorn · 2020-06-21T17:39:51Z

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

fpistm · 2020-06-22T07:40:54Z

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 · 2020-10-08T00:56:51Z

Status?

fpistm · 2020-10-08T11:30:05Z

Currently not sorry.

rhapsodyv · 2020-12-10T02:10:26Z

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

fpistm · 2020-12-10T09:33:06Z

@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 · 2020-12-10T19:49:00Z

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

rhapsodyv · 2020-12-10T22:20:12Z

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.

rhapsodyv · 2020-12-14T23:45:23Z

cores/arduino/stm32/usb/msc/usbd_msc_storage_if.cpp

+
+/*******************************************************************************
+* Function Name  : Write_Memory
+* Description    : Handle the Write operation to the STORAGE card.


all comments are equal

Tjeerdie · 2021-03-04T09:33:11Z

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 · 2021-03-04T14:48:50Z

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 · 2021-03-19T12:30:29Z

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 · 2021-04-02T18:31:37Z

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 · 2021-04-14T00:36:49Z

What about rename the USB class to USBComposite?

rhapsodyv · 2021-05-19T02:17:32Z

Is this dead?

I can help here too.

rhapsodyv · 2021-06-03T17:52:30Z

This branch have this PR + #1196 working together: https://github.com/rhapsodyv/Arduino_Core_STM32/tree/usb-host-msc-cdc-msc-2, with the USB class name fixed. Yet for 1.9.x.

fpistm · 2021-06-03T18:41:35Z

It is not dead. I need some free time to review it carefully. I do my best... 😕

rhapsodyv · 2021-06-03T21:29:21Z

cores/arduino/stm32/usb/usbd_conf.c

  for (uint32_t i = 0; i < (DEV_NUM_EP + 1); i++) {
-    HAL_PCDEx_PMAConfig(&g_hpcd, ep_def[i].ep_adress, ep_def[i].ep_kind, ep_def[i].ep_size);
+    ep_desc_t *pDesc = &ep_dep[i];


typo here... ep_dep => ep_def

bobemoe · 2021-09-21T08:00:20Z

Should it be possible with this PR to connect a G3 Dongle to the USB port of a STM32F4 BlickPill running stm32dunio and open a serial connection to send AT commends etc? Or am I barking up the wrong tree?

The dongle presents itself on Linux as a GSM modem /dev/ttyUSB0 and I confirm it responds OK to AT command. It also presents 3 other GSM devices and a mass storage device that is not actually needed.

I guess the STM32 would need to enumerate them to get the the correct CDC?

Is there an example of this somewhere please? I see examples above of using the MSC but not so much for the CDC.

zhscust · 2022-01-02T15:56:04Z

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
}

Hi
I am new to this field , can you help me how to make it work ? i am working on small project ,where i am logging data in SD card (working ) . I want to access SD card from computer on usb port . i am using stm32duino official core verion 2.1.0 and Arduino IDE 1.8.16. when i try to compile it gives error of USB.h etc file missing , i think it is still not updated on main core , please guide how did you make it work . Thanks

shreeshan97 · 2022-02-03T12:11:13Z

When is the merge release??
Thanks in advance.

shreeshan97 · 2022-02-04T06:24:05Z

This branch have this PR + #1196 working together: https://github.com/rhapsodyv/Arduino_Core_STM32/tree/usb-host-msc-cdc-msc-2, with the USB class name fixed. Yet for 1.9.x.

@rhapsodyv Sir what files need to be copied and updated to the current version to get CDC/ MSC device working for STM32F1.

LukeHaberkern · 2022-07-26T22:42:51Z

If anyone could share an example project implementing the CDC+MSC composite I would appreciate seeing it all put together. I'm using a STMF4. I can get both working one at a time but struggling with the composite. I like this approach of leaving the classes separate though as i'm currently trying to vector them and it's ugly.

thinkyhead · 2023-01-17T07:42:57Z

Apparently this is not compatible with the latest ArduinoSTM32 (4.20200.221104)

https://github.com/rhapsodyv/Arduino_Core_STM32/tree/usb-host-msc-cdc-msc-3

…so it cannot yet be used with STM32H743 and others. How hard is that to port over to the latest?

skforlee · 2023-10-02T17:49:10Z

It is not dead. I need some free time to review it carefully. I do my best... 😕

@fpistm Seems like discussion of this trailed off a few years back. Wondering if the functionality provided by this PR (CDC+MSC composite) was rolled into the core somewhere along the way? As the previous comment mentioned, https://github.com/rhapsodyv/Arduino_Core_STM32/tree/usb-host-msc-cdc-msc-3 doesn't seem t be working any longer, and I haven't had any success, too :(

rudihorn mentioned this pull request Jun 2, 2020

[FR] STM32 USB CDC + MSC Composite MarlinFirmware/Marlin#18157

Open

fpistm added the New feature label Jun 3, 2020

Rudi Horn added 14 commits June 4, 2020 18:03

initial commit of new cdc_msc and updated msc

26f5806

added endpoint configuration

db11e1b

added endpoint define changes

1c247ed

added default msc interface

0b9e38e

fixed cdc handle nad renamed cdc msc flag

162ade3

added usb class

8d7e9d8

add new include dirs

4b489ef

formatting changes, improved usbd_ep_conf and now compiles in marlin

a144bca

fixed bug in descriptor, small changes to USB class

6717196

fixed duplicate endpoint definition and somewhat fixed endpoint confi…

6f3ebfc

…guration

fixed styling and changed dummy sd usb so it is always not ready

ccef556

fixed small type and more formatting

ff6cecf

added ifdef for USB.c

da5c702

fixed bug in ST MSC SCSI library

ffd635d

Bob-the-Kuhn mentioned this pull request Dec 7, 2020

USB Host + MSC #1196

Open

6 tasks

rhapsodyv reviewed Dec 14, 2020

View reviewed changes

rhapsodyv mentioned this pull request Apr 2, 2021

BTT SKR MINI E3 in STM32 MarlinFirmware/Marlin#21488

Merged

9 tasks

rhapsodyv reviewed Jun 3, 2021

View reviewed changes

fpistm force-pushed the main branch from dbc7063 to 74255d5 Compare July 2, 2024 09:35

fpistm added enhancement New feature or request and removed New feature labels Jul 16, 2024

Add USB MSC + MSC/CDC Composite Class #1088

Are you sure you want to change the base?

Add USB MSC + MSC/CDC Composite Class #1088

Conversation

rudihorn commented Jun 2, 2020 • edited Loading

rudihorn commented Jun 2, 2020

rudihorn commented Jun 2, 2020

rudihorn commented Jun 2, 2020

fpistm commented Jun 3, 2020

rudihorn commented Jun 3, 2020 • edited Loading

fpistm commented Jun 3, 2020

fpistm commented Jun 3, 2020

rudihorn commented Jun 3, 2020

fpistm commented Jun 3, 2020

rudihorn commented Jun 3, 2020 • edited Loading

rudihorn commented Jun 4, 2020 • edited Loading

fpistm commented Jun 4, 2020

fpistm commented Jun 4, 2020

rudihorn commented Jun 21, 2020

fpistm commented Jun 22, 2020

Bob-the-Kuhn commented Oct 8, 2020

fpistm commented Oct 8, 2020

rhapsodyv commented Dec 10, 2020

fpistm commented Dec 10, 2020

rhapsodyv commented Dec 10, 2020

rhapsodyv commented Dec 10, 2020

rhapsodyv Dec 14, 2020

Choose a reason for hiding this comment

Tjeerdie commented Mar 4, 2021

rhapsodyv commented Mar 4, 2021

Tjeerdie commented Mar 19, 2021

rhapsodyv commented Apr 2, 2021

rhapsodyv commented Apr 14, 2021

rhapsodyv commented May 19, 2021

rhapsodyv commented Jun 3, 2021

fpistm commented Jun 3, 2021

rhapsodyv Jun 3, 2021

Choose a reason for hiding this comment

bobemoe commented Sep 21, 2021

zhscust commented Jan 2, 2022

shreeshan97 commented Feb 3, 2022

shreeshan97 commented Feb 4, 2022 • edited Loading

LukeHaberkern commented Jul 26, 2022

thinkyhead commented Jan 17, 2023 • edited Loading

skforlee commented Oct 2, 2023

rudihorn commented Jun 2, 2020 •

edited

Loading

rudihorn commented Jun 3, 2020 •

edited

Loading

rudihorn commented Jun 3, 2020 •

edited

Loading

rudihorn commented Jun 4, 2020 •

edited

Loading

shreeshan97 commented Feb 4, 2022 •

edited

Loading

thinkyhead commented Jan 17, 2023 •

edited

Loading