hw/usb/tinyusb: usb cdc console and tinyusb library sysinit before console uses it #3090
Conversation
vrahane
force-pushed
the
vipul/tinyusb_sysinit_fixes_
branch
from
8bb257d to
124cbdc
Compare
vrahane
changed the title
hw/usb/tinyusb/syscfg.yml
Outdated
| @@ -33,7 +33,7 @@ syscfg.defs: | |||
| USBD_SYSINIT_STAGE: | |||
| description: > | |||
| Sysinit stage for USB device functionality. | |||
| value: 500 | |||
| value: 21 | |||
There was a problem hiding this comment.
add syscfg for this sysinit and change priority in your local target if needed
There was a problem hiding this comment.
CONSOLE_SYSINIT_STAGE Is set to 20 by default and that same level(syscfg) is used for CDC console. Tinyusb needs to be initialized before the CDC console if we(mynewt) want the CDC console to work fine by default, else it is going to have race conditions which is what I am seeing.
There was a problem hiding this comment.
I understand that but are you sure that changing default value for tinyusb from 500 to 21 will not introduce any side effects in other configurations (<- @kasjer)? You can add conditional restriction instead so newt will print error if CDC console is enabled and USBD_SYSINIT_STAGE >= CONSOLE_USB_CDC_SYSINIT_STAGE and then change USBD_SYSINIT_STAGE in your local config accordingly.
There was a problem hiding this comment.
I'm not sure what will happen if we move USB initialization to 21. So maybe it is safer to keep default priority as @andrzej-kaczmarek suggested.
There was a problem hiding this comment.
Well, I have tested it and seems to be working fine. @benmccrea can second me on that.
There was a problem hiding this comment.
I think adding CONSOLE_USB_CDC_SYSINIT_STAGE still makes sense as otherwise you won't be able to set proper priority order in scenario you mentioned (correct me if I'm wrong). I'm just reluctant to change default configuration for all other cases as well if we're not sure what could be the side-effects.
There was a problem hiding this comment.
Though, if you guys feel nervous about initializing it that early, cdc_console will have to be initialized later which will be in the 500s which will be too late. I am fine either ways. Adding a restriction seems fine but the default does need to change.
There was a problem hiding this comment.
One more thing to consider is all hardware stuff gets initialized quite early and only tinyusb which in turn initializes the USB controller for da1469x in our specific case gets initialized in the 500s.
There was a problem hiding this comment.
Having said that I understand the concerns, I will just add CONSOLE_USB_CDC_SYSINIT_STAGE and set it to 500+ sysinit level. I will change our targets locally to initialize both early on. We (mynewt) can probably make a well tested change later to do it early on.
vrahane
force-pushed
the
vipul/tinyusb_sysinit_fixes_
branch
2 times, most recently
from
eca5acf to
ed7c3da
Compare
|
lgtm, but please fix the commit message. also the same restriction added in two different places is redundant. |
before console uses it - Changing cdc_console sysinit to 502 adding a new syscfg CONSOLE_USB_CDC_SYSINIT_STAGE for CDC USB console and use that instead - Also add restrictions so that usbd is never inited after cdc_console
vrahane
force-pushed
the
vipul/tinyusb_sysinit_fixes_
branch
from
ed7c3da to
1050fae
Compare
|
@andrzej-kaczmarek Done |
hw/usb/tinyusb: Fix sysinit levels so that tinyusb gets initialized
before console uses it
CONSOLE_USB_CDC_SYSINIT_STAGEfor CDC USB consoleand use that instead
cdc_console