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

STM32F3 IRQ remap option and some minor improvements #520

Merged
merged 6 commits into from
Oct 11, 2020

Conversation

salkinium
Copy link
Contributor

@salkinium salkinium commented Sep 26, 2020

Hi, thanks for this awesome library! I'm currently integrating TinyUSB into modm (modm-io/modm#478) and I found some minor things to improve:

  1. I added a simple STM32F3_IRQ_REMAP, which can be defined in the config to remap the IRQs. The user must remap the IRQs in their hardware setup (it's just SYSCFG->CFGR1 |= SYSCFG_CFGR1_USB_IT_RMP;).
  2. We're using TinyUSB in C++, which doesn't define the __STDC_VERSION__ macro, thus throwing a warning. Just a simple fix.
  3. The _MESS_ERR() and _MESS_FAILED() macros use printf instead of tu_printf, which is unfortunate for debugging and also links the newlib version of printf which requires heap, thus pulling a whole lot of unrelated code.
  4. To improve debugability I'm overwriting the TU_ASSERT macros with our own fault reporting implementation, and to allow this I just wrapped them in #ifndef. Unfortunately OpenOCD does not by default reset the debugger connected flag on disconnect, thus TU_BREAKPOINT() causes a HardFault, which isn't great to debug.
  5. We're using SCons to compile, which has issues resolving #include CFG_TUSB_CONFIG_FILE, thus not recompiling TinyUSB sources when the config changes. So I changed the mechanism to "fallback" to a plain #include "tusb_config.h", which solves this issue.

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 for the PR, sorry for late response, I have been too busy lately. The PR looks great, your modification and explanation all make sense. I only want to know a bit more about the STM32 remap to see if we could implement it without having an extra DCD specific macro.

src/common/tusb_verify.h Outdated Show resolved Hide resolved
src/common/tusb_compiler.h Show resolved Hide resolved
src/common/tusb_verify.h Outdated Show resolved Hide resolved
src/common/tusb_verify.h Show resolved Hide resolved
src/common/tusb_verify.h Show resolved Hide resolved
@@ -150,6 +150,12 @@
# define USE_SOF 0
#endif

// Some STM32F3 devices allow to remap the USB interrupt vectors from
// shared USB/CAN IRQs to separate CAN and USB IRQs
#ifndef STM32F3_IRQ_REMAP
Copy link
Owner

Choose a reason for hiding this comment

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

This is interesting, I didn't know about this, could you tell me more about the specific MCU that support this, and which section in the reference manual for this. I would like to read this up a bit, to see if we could have a better systematic solution. I kind of not wanting to have custom macro for specific DCD if there is a better way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This only exists for the STM32F302 and the larger STM32F303 devices.

See RM0365 Table 40, page 208, footnote 1 (STM32F302) and RM0318 Table 82, page 288, footnote 1 (STM32F303xb/c/d/e):

  1. It is possible to remap the USB interrupts (USB_HP, USB_LP and USB_WKUP) on interrupt lines 74, 75 and 76 respectively by setting the USB_IT_RMP bit in the Section 11.1.1: SYSCFG configuration register 1 (SYSCFG_CFGR1) on page 172.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess you could read the bit at runtime and select the right interrupts. Wouldn't be too much overhead.

Copy link
Owner

Choose a reason for hiding this comment

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

@salkinium sorry for late response, this is great idea to solve the shared CAN/USB. I like your last suggestion by checking the actual remap bit. The overhead is too small ( a couple of cycle for bit test) and this aren't call at any fast rate either. Would you mind changing it to dynamic check. Also since it only available to selected chips, it should be wrapped with SYSCFG_CFGR1_USB_IT_RMP e.g

#ifdef SYSCFG_CFGR1_USB_IT_RMP
if ( remapped )
{
 use RMP IRQ
}else
#endif
{
  use normal IRQ 
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I implemented it like this and remapped the IRQs on the STM32F303 DISCO BSP.

@salkinium salkinium force-pushed the feature/misc_enhancements branch from 00bef8a to d6de148 Compare October 3, 2020 18:18
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.

Thanks for the great idea with remap, would you mind taking a bit more time to make it dynamically check of the remapped bit in the SYSCFG_CFGR1, that would be perfect.

@@ -150,6 +150,12 @@
# define USE_SOF 0
#endif

// Some STM32F3 devices allow to remap the USB interrupt vectors from
// shared USB/CAN IRQs to separate CAN and USB IRQs
#ifndef STM32F3_IRQ_REMAP
Copy link
Owner

Choose a reason for hiding this comment

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

@salkinium sorry for late response, this is great idea to solve the shared CAN/USB. I like your last suggestion by checking the actual remap bit. The overhead is too small ( a couple of cycle for bit test) and this aren't call at any fast rate either. Would you mind changing it to dynamic check. Also since it only available to selected chips, it should be wrapped with SYSCFG_CFGR1_USB_IT_RMP e.g

#ifdef SYSCFG_CFGR1_USB_IT_RMP
if ( remapped )
{
 use RMP IRQ
}else
#endif
{
  use normal IRQ 
}

@salkinium salkinium force-pushed the feature/misc_enhancements branch from d6de148 to 64e5f57 Compare October 10, 2020 03:03
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.

almost perfect, though hand-on testing with f3 disco prove that syscfg clk must be enabled before we could do the remap.

hw/bsp/stm32f303disco/stm32f303disco.c Show resolved Hide resolved
@salkinium salkinium force-pushed the feature/misc_enhancements branch from 64e5f57 to 7fda95f Compare October 11, 2020 07:27
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 now, thank you very much for your PR. I will merge when ci passed.

@hathach hathach merged commit 80c509a into hathach:master Oct 11, 2020
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.

2 participants