Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add USB MSC + MSC/CDC Composite Class #1088

Open
wants to merge 37 commits into
base: main
Choose a base branch
from

Conversation

rudihorn
Copy link

@rudihorn rudihorn commented Jun 2, 2020

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

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

Code formatting

  • Needs to be checked

Closing issues

#586

@rudihorn
Copy link
Author

rudihorn commented Jun 2, 2020

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

@rudihorn
Copy link
Author

rudihorn commented Jun 2, 2020

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
Copy link
Author

rudihorn commented Jun 2, 2020

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
Copy link
Member

fpistm commented Jun 3, 2020

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

@fpistm fpistm added this to In progress in STM32 core based on ST HAL via automation Jun 3, 2020
@rudihorn
Copy link
Author

rudihorn commented Jun 3, 2020

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
Copy link
Member

fpistm commented Jun 3, 2020

@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
Copy link
Member

fpistm commented Jun 3, 2020

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

@rudihorn
Copy link
Author

rudihorn commented Jun 3, 2020

Is there a reason why

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

@fpistm
Copy link
Member

fpistm commented Jun 3, 2020

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
Copy link
Author

rudihorn commented Jun 3, 2020

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
Copy link
Author

rudihorn commented Jun 4, 2020

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
Copy link
Member

fpistm commented Jun 4, 2020

@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
Copy link
Member

fpistm commented Jun 4, 2020

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

@rudihorn
Copy link
Author

rudihorn commented Jun 7, 2020

@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
Copy link
Author

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

@fpistm
Copy link
Member

fpistm commented Jun 22, 2020

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
Copy link

Status?

@fpistm
Copy link
Member

fpistm commented Oct 8, 2020

Currently not sorry.

@Bob-the-Kuhn Bob-the-Kuhn mentioned this pull request Dec 7, 2020
6 tasks
@rhapsodyv
Copy link

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

@fpistm
Copy link
Member

fpistm commented Dec 10, 2020

@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
Copy link

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

@rhapsodyv
Copy link

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.


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all comments are equal

@Tjeerdie
Copy link
Contributor

Tjeerdie commented Mar 4, 2021

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
Copy link

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
Copy link
Contributor

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
Copy link

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
Copy link

What about rename the USB class to USBComposite?

@rhapsodyv
Copy link

Is this dead?

I can help here too.

@rhapsodyv
Copy link

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
Copy link
Member

fpistm commented Jun 3, 2021

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

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];

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo here... ep_dep => ep_def

@bobemoe
Copy link

bobemoe commented Sep 21, 2021

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
Copy link

zhscust commented Jan 2, 2022

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
Copy link

When is the merge release??
Thanks in advance.

@shreeshan97
Copy link

shreeshan97 commented Feb 4, 2022

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
Copy link

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
Copy link

thinkyhead commented Jan 17, 2023

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
Copy link

skforlee commented Oct 2, 2023

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 :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet