Skip to content

Add composite MSC CDC USB device #586

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

Closed
wants to merge 20 commits into from
Closed

Add composite MSC CDC USB device #586

wants to merge 20 commits into from

Conversation

Bob-the-Kuhn
Copy link

This PR should be considered experimental until more testing is done.

I'm definitely not an expert in USB or the STM32 devices. I'm sure there's lots of corrections/improvements that will be made before this PR is merged.


Summary

This PR implements a composite MSC CDC USB device.

The SD card on the STM32 device will appear as a USB drive on the host PC. This allows file transfers between the PC and the STM32 device. Hopefully this minimizes the need to physically move the SD card between the PC and the STM32 device when transferring files.

Implementation

The flag USE_USB_COMPOSITE is used to enable the new feature.

SDIO is used to access the SD card.

Almost all the changes are in the files in the new directory msc_cdc_composite. There are some shared files that were also changed.

The biggest change is to modify the USBD_HandleTypeDef structure so that the MSC and CDC classes have dedicated/unique data structures when USE_USB_COMPOSITE is defined.

The USBD_HandleTypeDef change is copied from the GPSlogger repo. See the article CDC + MSC USB Composite Device for STM32 HAL for the theory behind the MSC CDC composite device as implemented in the GPSLogger repository. Here is an English version of the article.

The most of the customization & new code are in the files:

  • usbd_desc_msc_cdc_composite.c
  • usbd_msc_cdc_composite_def.h
  • usbd_msc_cdc_composite_if.c
  • SdMscDriver
  • usbd_def.h
  • usbd_conf.c

CPU support

So far all the testing has been done on a BLACK_STM32F407ve board using Marlin software and PlatformIO

There are some hooks in it for use with the STM32F103Z cpu. I have not been able to compile Marlin with this repo (Arduino_Core_STM32) for a F103 device as the Marlin F103 software uses STM32GENERIC.

There are also hooks in it for use with High Speed USB. No testing has been done on that. All testing so far has been done with the F407VE in Full Speed mode.


Marlin sketch

I used Marlin 2.0 for testing. The changes needed are:
file platformio.ini - under the [env:black_stm32f407ve] section add -DUSBD_USE_CDC_COMPOSITE to the build_flags list.
file Configuration.h - replace #define MOTHERBOARD BOARD_RAMPS_14_EFB with #define MOTHERBOARD BOARD_BLACK_STM32F103ZE

@Bob-the-Kuhn
Copy link
Author

Bob-the-Kuhn commented Jul 30, 2019

I'll add details on the changed files in this comment as time permits.


usbd_def.h

This is a copy of the core file with the USBD_HandleTypeDef structure changes.

I just noticed that the file usbd_msc_cdc_composite_def.h is almost identical. One of them needs to disappear. TBD

CHANGES:

  • Added header explaining where the USBD_HandleTypeDef changes come from.
  • Added forward declarations to solve a circular definition problem.
  • Added some fields to USBD_EndpointTypeDef because they were present in the PlatformIO library version.
  • USBD_HandleTypeDef - added field dev_connection_status it was present in the PlatformIO library version.
  • USBD_HandleTypeDef - added separate data structures for the CDC and MSC classes. The device requires a single USBD_HandleTypeDef but each class has to have copies of variables unique to that class.

usbd_conf.c

I just noticed that I didn't fully update this file before submitting the PR. I need to bring this file up to date with the latest Arduino_Core_STM32 version.

CHANGES:

  • Added header explaining where the USBD_HandleTypeDef changes come from.
  • The pdev->pData construct needs to change depending on if the composite USB is used or not. pData was changed to p_Data and conditional code added to define it appropriately.
  • Added HAL_PCDE section for composite MSC CDC. This may need to change depending on the size of the packets in High Speed mode.

1) usbd_conf.c - remove unintended changes (restore to current version)
2) usbd_conf.c - restore lines inadvertently removed
3) convert to formatting style used by this repo (partial)
@fpistm fpistm added enhancement New feature or request New feature labels Jul 31, 2019
@fpistm
Copy link
Member

fpistm commented Jul 31, 2019

Hi @Bob-the-Kuhn
thanks for this PR.
I will not be able to review it carefully until September due to vacation period.
Note that USBD middleware will be updated soon, I think there will be no much change.

@Bob-the-Kuhn
Copy link
Author

No problem. Hope your vacation works out as planned.

@Bob-the-Kuhn
Copy link
Author

This just got messy.

I copied the PR's files back into the PlatformIO library on my PC and ran into problems. Looks like when I merged usbd_conf.c with the one from this repo I also brought in some HAL_PCD routines that were NO-OPs in PlatformIO.

Is there a separate branch/repository for the PlatformIO library that this PR should be directed toward?

I think the next step is to get some idea of a long term plan for this feature. I don't know enough about the STM32 world to do this so I think I'll put it on the back burner until you get back from vacation and have enough time to look at it.

This started out as a wish item for Marlin. If I get ambitious I may try to add this to Marlin. Definitely a smaller scope which I might be able to handle.

@fpistm
Copy link
Member

fpistm commented Jul 31, 2019

I do not use PIO so could not advise you on this.
Anyway, I could review your PR and see how to well integrate it.

@darknode
Copy link

Hi @fpistm, did you have time to have a look at this PR? It would be great to have such a feature integrated and used in Marlin.

@fpistm
Copy link
Member

fpistm commented Sep 11, 2019

Hi @fpistm, did you have time to have a look at this PR? It would be great to have such a feature integrated and used in Marlin.

Currently not, I am preparing the next delivery 1.7.0.
But don't worry I will review it.

@Bob-the-Kuhn
Copy link
Author

I'm waiting for the middleware to be updated before working on this again.

I did see that the maple flavor of STM32 support has an open PR on Marlin for composite USB. I know that Marlin uses maple for the STM32F1 based boards. I'm not sure if the Marling PR also applies to the F4 and F7 boards.

@fpistm
Copy link
Member

fpistm commented Sep 15, 2019

I will update USB middleware next week. I've identified issue and it comes from F1 FW update so I can now update it.

@rflulling
Copy link

rflulling commented Nov 2, 2019

So it looks like this has been potentially abandoned for a month and half. Is marlin perhaps marching off in a direction recreating the code here from scratch? Everything in the repository looks so perfect and clean, I want to cut and past it into my own folder and use it some how... It looks like it would build a perfect version of my Controller board, only without a marlin interface... Although I do not see the STM32F407ZGT6 specifically named in the files.

@Bob-the-Kuhn
Copy link
Author

Updated code using the updated USB middleware.

This is the code from PR #1004. That PR has been closed.


This code was tested with two boards: Black STM32F407 and STEVAL

The Marlin sketch this was tested with is at PR 17222.


OPEN ITEMS:

  • Should I update the USB middleware files to match this code?
  • Resolve conflicts
  • General cleanup - I'm sure some of my comments to myself and test code weren't all purged.
  • Initialization sequence needs to be reviewed in detail. I may still have some redundant items in it. The switching to 4 bit wide mode routine may be redundant.
  • Look at a way to stay in 1 bit wide mode. I have a vague recollection of one board not connecting all the SDIO data lines.
  • The FIFO configuration needs more investigation. Currently I can only set the PCD FIFOs to the minimum needed length. Previously I was able to set FIFOs to use almost all the available space.
  • My 4G SD card doesn't work with this software. I need to root cause that.

1 - revert CDC endpoints to match current released code

2 - copy Txlastlength changes from released code into this branch

3 - switch to released code's method of setting FIFOs
Looks like my copy of usbd_conf.c and the PR's copy are different.  Doing a white spce change to try to force the correct code to appear in the PR.
@Bob-the-Kuhn
Copy link
Author

I think we have a Travis problem.

The PlatformIO test returns the following error. I don't think there is anything I can do to fix this.
python3: can't open file 'platformio-builder.py': [Errno 2] No such file or directory

@fpistm
Copy link
Member

fpistm commented Mar 22, 2020

I think we have a Travis problem.

The PlatformIO test returns the following error. I don't think there is anything I can do to fix this.
python3: can't open file 'platformio-builder.py': [Errno 2] No such file or directory

In fact, don't know how you rebase but you remove lot of thing. See the diff stats: +336,653 −1,674,570
That's why you get this error, the platformio-builder.py has been removed...

Basically just copied everything over from MASTER except the USB directory
@Bob-the-Kuhn
Copy link
Author

Thanks

My copy of master was out of synch with the repository. Hopefully that has been fixed.

@fpistm fpistm marked this pull request as draft April 21, 2020 08:41
@rudihorn
Copy link

rudihorn commented May 30, 2020

So after nearly reimplementing this myself, I just realised there already is a pull request. Having pretty much done the same thing I do have some highlevel comments:

There is an example of the STM32 USB library which has some composite devices here: https://www.st.com/en/embedded-software/stsw-stm32046.html. What they do is they get rid of the pdev->pClassData and pdev->pUserData dependencies, only keeping track of the USBD handle. This allows them to reuse the class code in different files. It also has the small advantage of being able to remove some heap allocations, which I'm not the biggest fan of in embedded settings when they can be avoided.

I also think ensuring the bulk of the class files can be reused is good. A class file then only becomes a descriptor and a forwarder for the used USB interface functions like this: https://github.com/rudihorn/Arduino_Core_STM32/blob/usb_msc_cdc/cores/arduino/stm32/usb/cdc_msc/usbd_cdc_msc.cpp, making it easy and short to implement new custom composite devices.

To do this though, it is necessary to ensure that the endpoint numbers don't collide. Currently the endpoint numbers are defined in usbd_ep_conf.h, so I just added an extra configuration for the USB CDC + MSC composite like this: https://github.com/rudihorn/Arduino_Core_STM32/blob/usb_msc_cdc/cores/arduino/stm32/usb/usbd_ep_conf.h.

I would also be more explicit about the composite name, as there are many possible composite devices and I think it would be nice to be more open for more in the future. The USB library shows examples for HID + CDC and HID + MSC composites as further examples.

Unfortunately my code hasn't been actually tested, so I cannot for sure say that this is the right approach, but it should work. Do happily compare and merge in any changes as you see fit: https://github.com/rudihorn/Arduino_Core_STM32/tree/usb_msc_cdc/cores/arduino/stm32/usb. It would be nice if this could be fast tracked a little, as this seems to be a feature Marlin has been waiting for a long time. I'm happy to help make any necessary changes.

There are also some file permission changes which you should double check.

@fpistm
Copy link
Member

fpistm commented May 30, 2020

@rudihorn do not hssitate to provide a PR too. Some of your points seems interesting

@rudihorn
Copy link

@fpistm This was my plan, but there is no point in having two unmergable PR's so I would like it if things could be coordinated a little. But yes, I am happy to help with this issue.

@fpistm
Copy link
Member

fpistm commented May 30, 2020

This is a draft PR. Anyway some alignement can be done.

@Bob-the-Kuhn
Copy link
Author

@rudihorn - I was hoping that someone with better knowledge of USB & of the code base would take this up.

I'm not qualified to say what is a reasonable approach. I just copied an example I found.

I never was able to make a reliable system.

I have put this aside until I can figure out how to test with the latest code base. Marlin has recently switched to framework-arduinoststm32 4.10700.200103 with PlatformIO. Is that platform close enough to the latest code base that testing/debugging would be useful?

It looks like it'll be about a week before I can devote some meaningful time to this again.


There a a lot of STM32 based 3D printer controller cards out there. Some use a FTDI type chip to implement the USB interface so they'll never have composite USB. Some have a SDIO interface to the SD card. That's what I've been concentrating on. Some have a SPI interface to the SD card. Eventually I hope to have composite USB on them also.


The Maple based STM32 controllers have a class based USB & MSC interface between the application and the Maple code base. I like that approach because I think it allows making the decision as to SDIO vs. SPI based MSC in the application rather than defining global flags in the PlatformIO environment.

@rudihorn
Copy link

Okay well I'll try hack together a PR in the next few days.

I will probably leave out the SD card stuff and instead expose the interface that the STM32 library offers. Essentially you can register an interface, which is a bunch of pointers to a few classes. Possibly this could be done using a classes approach. By default this can just offer an empty storage device.

Going up a version should be fine, though I admittedly am still trying to figure out how best to use platformio projects with local framework copies.

@fpistm
Copy link
Member

fpistm commented May 30, 2020

One approach several people suggest is the Arduino Pluggable USB but I never had time to investigate.
About PIO, they are some work around this. @stas2z has opened an issue about a script to properly use the git repo.

@Bob-the-Kuhn
Copy link
Author

Here's how I've been trying to test:

  • UUT - Black STM32F407V board - BT0 and BT1 are jumped to ground (they are near the SD card connector)
  • Application - Marlin bugfix_2.0.x branch. Here is a copy of the branch that has been compiled and uploaded to a board. ...\Marlin-Bob-2.pio\build\STM32F407VE_black\firmware.bin and ...\Marlin-Bob-2.pio\build\STM32F407VE_black\firmware.elf are in there.
  • Using GitHub Desktop to manage the code.
  • Added the Marlin branch (linked above) as a repository in GitHub Desktop. Branch name is Black-STM32F407
  • Added ....platformio\packages\framework-arduinoststm32 as a repository in GitHub Desktop.
  • I'm using Segger's Ozone as the debugger. The probe is the St-Link portion of a Nucleo board reprogrammed as a J-Link via Segger's STLinkReflash utility.
    That's mostly because it was laying around in my junk box..

@Bob-the-Kuhn Bob-the-Kuhn changed the title Add composite MSC CDC USB device ~Add composite MSC CDC USB device~ (see See PR [#1088](https://github.com/stm32duino/Arduino_Core_STM32/pull/1088) ) Jun 7, 2020
@Bob-the-Kuhn Bob-the-Kuhn changed the title ~Add composite MSC CDC USB device~ (see See PR [#1088](https://github.com/stm32duino/Arduino_Core_STM32/pull/1088) ) Add composite MSC CDC USB device Jun 7, 2020
@Bob-the-Kuhn
Copy link
Author

See PR #1088 for a better solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants