Repository navigation
Conversation
projectgus
left a comment
projectgus
left a comment
There was a problem hiding this comment.
Generally this looks good to me, just a couple of minor things! Sorry to ask you to leave most of your work behind for now, but this is a really good foundation.
There is also a file shared/runtime/tinyusb_helpers.c which contains the implementation for MICROPY_HW_USB_CDC_1200BPS_TOUCH. It might make sense to rename this to shared/tinyusb/usbd_cdc_common.c or something like that, I'm not sure. But feel free to leave that for now.
| #if MICROPY_HW_ENABLE_USBDEV | ||
| bi_decl(bi_program_feature("USB REPL")) | ||
| tusb_init(); | ||
| usbd_reset_all(); // run now just in case usb initialization occurs early |
There was a problem hiding this comment.
I originally thought this would somehow trigger a USB device reset and was worried it might interrupt continuous serial port REPL access if the device de- and re-enumerates (in particular when it happens inside the soft reset loop).
Now I look at the code, I see that in the bigger PR it's used to discard any runtime modifications to the USB descriptors. It looks like it doesn't actually trigger a device reset or re-enumeration, so the host will still have the old descriptors. This is something of a fiddly problem to work through!
Looking at the big picture, it'd be ideal to have a name that doesn't overlap with USB protocol terminology (although all the good names like "reset" and "setup" are taken, so I don't actually have a good suggestion for a better name - sorry!)
For this PR, while the implementation is a no-op then it's probably best to remove it entirely and add it back once it's needed.
| const int len = 8; | ||
|
|
||
| pico_get_unique_board_id(&id); | ||
| memcpy(buf, id.id, len); |
There was a problem hiding this comment.
suggest using sizeof(id.id) here and for the return value
(there is also a constant PICO_UNIQUE_BOARD_ID_SIZE_BYTES which would be fine, but sizeof(id.id) seems like the best choice to me as that's the actual field you're copying from.)
|
@Molorius I used your commit as the basis for a branch to implement USB device drivers in Python (see linked PR). I also have a separate refactoring commit in that PR - if it's OK with you then I'll submit a new USB refactoring PR with your commit plus that one. |
|
Hello @projectgus yes it is okay with me. Thanks for asking. |
|
@projectgus how shall we proceed here? Do you want to submit a separate PR (separate to this and #9497) to do the refactoring? Otherwise I see it's already part of #9497 and can be reviewed with that set of commits, although that's not my preference because it's easier to review and merge smaller chunks of work. |
|
@dpgeorge Yes, my plan has been to submit a separate refactor PR. I wanted to wait until hearing back from @Molorius (thanks!), and also until I thought the refactor changes were finalised and I wasn't going to do much more tweaking. I'm probably are at this point now, so I'll submit a refactoring PR today. |
3 participants
No description provided.