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

STM32 FS Device Driver #123

Merged
merged 15 commits into from Sep 10, 2019
Merged

STM32 FS Device Driver #123

merged 15 commits into from Sep 10, 2019

Conversation

pigrew
Copy link
Collaborator

@pigrew pigrew commented Sep 9, 2019

The attached driver has been tested on the F070RB, but should apply to other models (though I've not yet tested on any model). It has a bunch of limitations (like not supporting isochronous transfers), but is a good starting point.

For development, I've been using STM32CubeIDE, in a project created by STM32CubeMX.

This is related to issue #104.

@pigrew pigrew mentioned this pull request Sep 9, 2019
4 tasks
@pigrew
Copy link
Collaborator Author

pigrew commented Sep 9, 2019

It looks like the warnings I added to the source are causing the build to fail. They are saying that the driver is untested.... Should I remove them?

Also, the linker is running out of RAM for the "msc_dual_lun" example on the F070. The MCU has 16k but says it needs an additional 4k.

Should we create a "#define CFG_EXAMPLE_MSC_DISK_DUAL_READONLY" which we add into the BSP makefile?

Copy link
Owner

@hathach hathach left a comment

Choose a reason for hiding this comment

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

Thank you very much for an excellent PR. I am happy with the major of your PR, only a few request mainly for the naming of macro OPT_MCU_* and other minor things. Please check out my review to see if you could modify it. I am open to any suggesttion/discussion

hw/bsp/stm32f070rbnucleo/board.mk Outdated Show resolved Hide resolved
@@ -105,7 +105,7 @@
#define TU_VERIFY_1ARGS(_cond) TU_VERIFY_DEFINE(_cond, , false)
#define TU_VERIFY_2ARGS(_cond, _ret) TU_VERIFY_DEFINE(_cond, , _ret)

#define TU_VERIFY(...) GET_3RD_ARG(__VA_ARGS__, TU_VERIFY_2ARGS, TU_VERIFY_1ARGS)(__VA_ARGS__)
#define TU_VERIFY(...) GET_3RD_ARG(__VA_ARGS__, TU_VERIFY_2ARGS, TU_VERIFY_1ARGS, UNUSED)(__VA_ARGS__)
Copy link
Owner

Choose a reason for hiding this comment

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

would you please tell me why would you add the _UNUSED arg to these macro ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

GCC gave warnings (in pedantic mode) about C99 compliance stating that a variadic define must have at least one argument for the "...". When TU_VERIFY() was called here with one argument, it gave exactly three to GET_3RD_ARG(), leading to the warning.

-mcpu=cortex-m0 \
-mfloat-abi=soft \
-nostdlib -nostartfiles \
-DCFG_TUSB_MCU=OPT_MCU_STM32_FSDEV
Copy link
Owner

Choose a reason for hiding this comment

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

it should really be OPT_MCU_STM32F0 for mcu family instead of the usb controller used by the mcu.

IMO, OPT_MCU should really is the exact mcu family rather than the controller using by the mcu. The controller can be inferred from specific mcu family. There is probably minor difference between mcu we need to know as well (chip header, peripheral address or naming etc...) It will also be more consistent with other vendor mcu such as NXP, Microchip.

Copy link
Collaborator Author

@pigrew pigrew Sep 10, 2019

Choose a reason for hiding this comment

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

The fact that the F102 uses FSDEV while the F105 uses synopsis adds complexity. I'll use the something like STM32F1x2 or STM32F3x3 for all of the ST parts. It seems like this is enough to decide which driver to use.

Copy link
Owner

@hathach hathach Sep 10, 2019

Choose a reason for hiding this comment

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

The fact that the F102 uses FSDEV while the F105 uses synopsis adds complexity. I'll use the something like STM32F1x2 or STM32F3x3 for all of the ST parts. It seems like this is enough to decide which driver to use.

OK, I didn't know this until you mention, now I understand why you separate them

hw/bsp/stm32f303disco/board.mk Outdated Show resolved Hide resolved
defined(STM32F070x6) | defined(STM32F070xB) | \
defined(STM32F072xB) | \
defined(STM32F078xx)
#include "stm32f0xx.h"
Copy link
Owner

Choose a reason for hiding this comment

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

it would be simpler to use OPT_MCU_STM32F0 to just include #include "stm32f0xx.h" here right :)

}

void dcd_fs_irqHandler(void);
void USB_IRQHandler(void)
Copy link
Owner

Choose a reason for hiding this comment

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

I like this idea, to give a bit more flexibility instead of binding the IRQ handler by default name. However, to be consistent with other port implementation. Would you mind just move this into the dcd. We can #if def to have the exactly IRQ name depending on the OPT_MCU_ if needed. We could revisit this dcd_irq_handler() later on if needed. It would need the whole codebase changes since we adding an API for dcd layer.

PS: this also seems to cause travis build failed on board_test example

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@hathach
Copy link
Owner

hathach commented Sep 10, 2019

The attached driver has been tested on the F070RB, but should apply to other models (though I've not yet tested on any model). It has a bunch of limitations (like not supporting isochronous transfers), but is a good starting point.

For development, I've been using STM32CubeIDE, in a project created by STM32CubeMX.

This is related to issue #104.

No problem, as long as it works, I will verify with other board/port at my side with gcc. Don't worry about Isochronous, the stack actually doesn't handle isochronous for now. We can always add features later :)

Should we create a "#define CFG_EXAMPLE_MSC_DISK_DUAL_READONLY" which we add into the BSP makefile?

Yeah, I think we could do this, let's called it CFG_EXAMPLE_MSC_READONLY, even with single disk, there is mcu that doesn't have enough 8KB of SRAM. If you don't have time, I will do it myself when the PR is merged.

@pigrew
Copy link
Collaborator Author

pigrew commented Sep 10, 2019

@hathach I believe I've resolved the issues you mentioned.

#define OPT_MCU_STM32H7 302 ///< ST STM32H7
// ST FSDEV Devices
#define OPT_MCU_STM32F0x0 330 ///< ST STM32F0x0
Copy link
Owner

Choose a reason for hiding this comment

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

why would you need this many of OPT_MCU. I would only prefer a family line such as OPT_MCU_STM32F0 / F1/F3 . They are 90% like each other within the line, these OPT_MCU_* is meant for USBD and higher layer to know which family high layer working with. For specific/detail mcu, the dcd_*.c could check the mcu specific macro for usage.

Copy link
Owner

@hathach hathach left a comment

Choose a reason for hiding this comment

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

wow, thanks for the update, that is quick. But now you added too many OPT_MCU_ , please reduce it to family line F0/F1/F3 only

@pigrew
Copy link
Collaborator Author

pigrew commented Sep 10, 2019

Will remove in a few minutes. I had added them since they all use the same "ST_FSDEV" IP (so the driver should work on them all).

@pigrew
Copy link
Collaborator Author

pigrew commented Sep 10, 2019

Defines are now only for the F[013] series.

As I've only tested on the F070, I expect other bugs will appear.... My plan is to handle issues on other MCU with more of a "bug fixing" attitude. Once this PR merged, I'll do some testing on a STM32F042.

Note that the driver should work on many STM32 L/G/MP series MCUs. Once this PR is merged, I can add some comments on those feature requests mentioning that it's more of a testing issue than writing an entire driver from scratch.

@hathach
Copy link
Owner

hathach commented Sep 10, 2019

@pigrew OK, I only read your comment #123 (comment) in this issue. Now I understand why you separate them. For B,C,D usb core, they are all sysnopsis, I think we can merge them together. Only the A stm32_fsdev is different. F1 is probably the only line has 102,103 for A and 105/107 for B. Maybe we can have 2 OPT_MCU for that OPT_MCU_STM32F1x2_1x3 and OPT_MCU_STM32F1x5_1x7. Though since application should pick the correct dcd_.c, maybe it we can get away if #ifdef in the dcd_.c file

Each device with USB support embeds at least one of the following interfaces:
A: USB 2.0 FS device interface
B: USB 2.0 OTG FS, that is, USB 2.0 FS device/host/OTG controller with on-chip FS PHY
C: USB 2.0 OTG HS, that is, USB 2.0 FS/HS device/host/OTG controller, integrating the transceivers for full-
speed operation, and featuring an ULPI for high-speed operation: an external PHY device connected to the
ULPI is required.
D: USB 2.0 OTG HS controller with embedded on-chip HS PHYs

@pigrew
Copy link
Collaborator Author

pigrew commented Sep 10, 2019

@pigrew OK, I only read your comment #123 (comment) in this issue. Now I understand why you separate them. For B,C,D usb core, they are all sysnopsis, I think we can merge them together. Only the A stm32_fsdev is different. F1 is probably the only line has 102,103 for A and 105/107 for B. Maybe we can have 2 OPT_MCU for that OPT_MCU_STM32F1x2_1x3 and OPT_MCU_STM32F1x5_1x7. Though since application should pick the correct dcd__.c, maybe it we can get away if #ifdef in the dcd__.c file

My preference would be to leave all STM32 as a 3-digit code for three reasons:

  • Consistancy. If they are all [0-9]x[0-9], then I don't need to think about which define to use on STM32 since they all are using the same format.
  • Future-proof: It's possible that ST will extend a line (like the newer L or G) to have both the Synopsys and ST IP. I expect defines based on the first and third digits to work for the future.
  • I can include all of the _dcd drivers in my project at the moment. For my IDE, I include the entire "tusb/src" directory without needing to exclude certain drivers. If we need the user to only include the particular driver, then makefiles become more complicated.

@pigrew
Copy link
Collaborator Author

pigrew commented Sep 10, 2019

Or do you propose to have the drivers decide if they should be enabled based on the CMSIS defines, instead of only the OPT_MCU define?

@hathach
Copy link
Owner

hathach commented Sep 10, 2019

@pigrew OK, I only read your comment #123 (comment) in this issue. Now I understand why you separate them. For B,C,D usb core, they are all sysnopsis, I think we can merge them together. Only the A stm32_fsdev is different. F1 is probably the only line has 102,103 for A and 105/107 for B. Maybe we can have 2 OPT_MCU for that OPT_MCU_STM32F1x2_1x3 and OPT_MCU_STM32F1x5_1x7. Though since application should pick the correct dcd__.c, maybe it we can get away if #ifdef in the dcd__.c file

My preference would be to leave all STM32 as a 3-digit code for three reasons:

  • Consistancy. If they are all [0-9]x[0-9], then I don't need to think about which define to use on STM32 since they all are using the same format.
  • Future-proof: It's possible that ST will extend a line (like the newer L or G) to have both the Synopsys and ST IP. I expect defines based on the first and third digits to work for the future.
  • I can include all of the _dcd drivers in my project at the moment. For my IDE, I include the entire "tusb/src" directory without needing to exclude certain drivers. If we need the user to only include the particular driver, then makefiles become more complicated.

Yeah, that make sense, though I still think it is too many, Though I think we should use the ST defined macros in both dcd_*.c to include the correct driver F102,103 vs F105,107.

#if !defined (STM32F301x8) && !defined (STM32F302x8) && !defined (STM32F318xx) && \
    !defined (STM32F302xC) && !defined (STM32F303xC) && !defined (STM32F358xx) && \
    !defined (STM32F303x8) && !defined (STM32F334x8) && !defined (STM32F328xx) && \
    !defined (STM32F302xE) && !defined (STM32F303xE) && !defined (STM32F398xx) && \
    !defined (STM32F373xC) && !defined (STM32F378xx)

@hathach
Copy link
Owner

hathach commented Sep 10, 2019

Or do you propose to have the drivers decide if they should be enabled based on the CMSIS defines, instead of only the OPT_MCU define?

Yeah, in the dcd_*.c we are free to use any mcu specific macro, sorry, i think I cause you an misunderstanding in the previous comment

@pigrew
Copy link
Collaborator Author

pigrew commented Sep 10, 2019

Ok. Looks like the L4 series is another edge case. L4x2 uses ST-USB while L4x5 uses Synopsys.

I'll change to the MCU being OPT_MCU_STM32[FLG][0-9].

The driver will be included based on a proper (OPT_MCU_STM32F0 ) || (OPT_MCU_STM32F1 && any (supported CMSIS defines)) || (OPT_MCU_STM32F3).

I'll only specify F series.

@hathach
Copy link
Owner

hathach commented Sep 10, 2019

Ok. Looks like the L4 series is another edge case. L4x2 uses ST-USB while L4x5 uses Synopsys.

I'll change to the MCU being OPT_MCU_STM32[FLG][0-9].

The driver will be included based on a proper (OPT_MCU_STM32F0 ) || (OPT_MCU_STM32F1 && any (supported CMSIS defines)) || (OPT_MCU_STM32F3).

I'll only specify F series.

yeah, thanks, I think it is the best to handle specific mcu with #ifdef in the dcd.c file, ST make it too confusing for us :)

@pigrew
Copy link
Collaborator Author

pigrew commented Sep 10, 2019

Try again? Is the update what you had in mind? the #ifdef is pretty short, actually (for now).
Now that I've committed the change, perhaps I should just check if "USB_PMAADDR" is defined, in the headers, as that's a good way to tell (at least on the F1).

Should I change to #if defined(USB_PMAADDR) instead of #if defined(stm32f103x6)?

@hathach
Copy link
Owner

hathach commented Sep 10, 2019

Try again? Is the update what you had in mind? the #ifdef is pretty short, actually (for now).
Now that I've committed the change, perhaps I should just check if "USB_PMAADDR" is defined, in the headers, as that's a good way to tell (at least on the F1).

Should I change to #if defined(USB_PMAADDR) instead of #if defined(stm32f103x6)?

Maybe using USB_PMAADDR is good as well, since it will save us a lot of #ifdef for specific MCU ? Though you know this better than me, I am totally OK either way.

@pigrew
Copy link
Collaborator Author

pigrew commented Sep 10, 2019

Try again? Is the update what you had in mind? the #ifdef is pretty short, actually (for now).
Now that I've committed the change, perhaps I should just check if "USB_PMAADDR" is defined, in the headers, as that's a good way to tell (at least on the F1).
Should I change to #if defined(USB_PMAADDR) instead of #if defined(stm32f103x6)?

Maybe using USB_PMAADDR is good as well, since it will save us a lot of #ifdef for specific MCU ? Though you know this better than me, I am totally OK either way.

Nevermind, that's hard. I include the CMSIS header inside of the #if statement, so I don't have that define yet to use.... Lets go with what we have now.

Copy link
Owner

@hathach hathach left a comment

Choose a reason for hiding this comment

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

perfect as it is, thank you very much for effort and patient. I will do more test with other F3, F0, Lx . I have several of them on my desk.

@hathach hathach merged commit f5e58a0 into hathach:master Sep 10, 2019
@pigrew
Copy link
Collaborator Author

pigrew commented Sep 10, 2019

perfect as it is, thank you very much for effort and patient. I will do more test with other F3, F0, Lx . I have several of them on my desk.

Great. To use the boards without a USB port, I crimped a 0.100" pitch header onto a cut-up USB cable. Note that some of them need external series resistors, and others need a pull-up on D+. If the pull-up is external, the MCU has to have started USB not too long after the D+ pull-up is engaged.

@pigrew pigrew mentioned this pull request Sep 10, 2019
4 tasks
@hathach
Copy link
Owner

hathach commented Sep 11, 2019

perfect as it is, thank you very much for effort and patient. I will do more test with other F3, F0, Lx . I have several of them on my desk.

Great. To use the boards without a USB port, I crimped a 0.100" pitch header onto a cut-up USB cable. Note that some of them need external series resistors, and others need a pull-up on D+. If the pull-up is external, the MCU has to have started USB not too long after the D+ pull-up is engaged.

That is too much of hw work for me, I have an stm32f072disco which has usb available, I will add its bsp to test with. Other people probably added their fav board as well :) thanks for the head up.

@pigrew pigrew deleted the stm32_fsdev branch September 11, 2019 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants