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

Move to tinyusb 0.9.0 #321

Closed
wants to merge 2 commits into from
Closed

Move to tinyusb 0.9.0 #321

wants to merge 2 commits into from

Conversation

kilograham
Copy link
Contributor

tinyusb: add cmake functions to add example from tinyusb itself; fixup for sdtio_usb for tinyusb API changes

@kilograham kilograham added this to the 1.2.0 milestone Apr 8, 2021
@kilograham kilograham marked this pull request as draft April 8, 2021 17:28
@kilograham
Copy link
Contributor Author

Note right now, this requires a patched tinyusb 0.9.0 which is in the raspberry pi repo here

https://github.com/raspberrypi/tinyusb/tree/pico-0.9.0-wip

This fixes some bugs in the family implementation, and provides a somewhat sub-optimal workaround with some impedence mismatches with the tinyusb examples proper

  1. Use their own board abstraction distinct from that of the SDK
  2. Use cmake, but as standalone apps with the SDK as a depedency rather than as apps that use TinyUSB via the tinyusb wrapper libraries in the SDK.

Both of these probably require some rationalization

@kilograham
Copy link
Contributor Author

fixes #85
fixes #161

static bool resetd_control_request_cb(uint8_t __unused rhport, tusb_control_request_t const *request) {
static bool resetd_control_xfer_cb(uint8_t rhport, uint8_t stage, tusb_control_request_t const * request) {
// nothing to do with DATA & ACK stage
if (stage != CONTROL_STAGE_SETUP) return true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you are reset mcu, it should be better to reset when stage == ACK (ACK is complete), this will prevent host OS to complain about the failure of control transfer. For example, touch1200 is common way for Arduino to do reset to DFU.

https://github.com/hathach/tinyusb/blob/master/src/class/cdc/cdc_device.c#L354

    case CDC_REQUEST_SET_LINE_CODING:
      if (stage == CONTROL_STAGE_SETUP)
      {
        TU_LOG2("  Set Line Coding\r\n");
        tud_control_xfer(rhport, request, &p_cdc->line_coding, sizeof(cdc_line_coding_t));
      }
      else if ( stage == CONTROL_STAGE_ACK)
      {
        if ( tud_cdc_line_coding_cb ) tud_cdc_line_coding_cb(itf, &p_cdc->line_coding);
      }
    break;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah; good point thanks

@@ -101,6 +101,12 @@ static inline uint uart_get_index(uart_inst_t *uart) {
return uart == uart1 ? 1 : 0;
}

static inline uart_inst_t *uart_get_instance(uint instance) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this change needed by this PR, or was it just pulled in coincidentally?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

add_library(tinyusb_example_support INTERFACE)
target_compile_definitions(tinyusb_example_support INTERFACE
CFG_TUSB_OS=OPT_OS_PICO
BOARD=pants
Copy link
Contributor

Choose a reason for hiding this comment

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

👖

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops - i was checking something by deliberately using an invalid board ;-) that ain't gonna compile

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@kilograham
Copy link
Contributor Author

kilograham commented Apr 9, 2021

@hathach I wanted to have a discussion somewhere, and this is probably as good a place as any.

It might be nice to unify the tinyusb and sdk notions of a board, and possible share CMakeLists.txt between the two. I don't have a good mental picture of what the intentions are in TinyUSB to help me decide how to proceed. Here's some random points/questions.

  1. Part of the confusion comes from the fact that TinyUSB has TinyUSB->pico-sdk and we have pico-sdk->TinyUSB ;-)
  2. I assume/believe that part of the reason for the common FAMILY/BOARD setup within the examples is for CI and automated builds. Do people also expect to be able to build run the examples from within the TinyUSB tree (note there were some bugs in the new family.c which suggest they might not have). Is it a requirement of CI to be able to specify BOARD explicitly here rather than via PICO_BOARD when using the SDK
  3. I'm a bit confused by the CMakeLists.txt in the examples. They are IFed based on the FAMILY, but the esp32 (or whatever I'm not looking atm) code paths in the CMakeLists.txt don't actually add any targets or set any sources. The SDK based GLOBing for examples works for now (to create our own build targets within pico-examples later), but if the example CMakeLists.txt provided some information on source files in a common way, maybe we could utilize those in some way. The way this was(/would have been :-) ) handled across multiple families would have helped here.

@hathach
Copy link
Contributor

hathach commented Apr 9, 2021

@hathach I wanted to have a discussion somewhere, and this is probably as good a place as any.

It might be nice to unify the tinyusb and sdk notions of a board, and possible share CMakeLists.txt between the two. I don't have a good mental picture of what the intentions are in TinyUSB to help me decide how to proceed. Here's some random points/questions.

yeah, it would be nice to have tinyusb example to work out of the box with pico-sdk.

  1. Part of the confusion comes from the fact that TinyUSB has TinyUSB->pico-sdk and we have pico-sdk->TinyUSB ;-)

Yeah, I feel the pain as well, like all other mcus, the sdk/low level driver (clock, gpio ) are needed only for running tinyusb stock examples and mostly for me to develop and test out new feature (basically maintaining the repo). It is actually not needed when tinyusb is part of another project or in this case pico-sdk. I am not sure what is best to ease this out. Typically it shouldn't be an issue if user don't submodule init with --recursive. For this very reason, I have recently created an source-only mirror https://github.com/hathach/tinyusb_src that can be used to be included as part of comprehensive project. The src is exactly the same to the tinyusb/src, it is pushed automatically by ci whenever there is changes within the src folder of tinyusb's master branch https://github.com/hathach/tinyusb/blob/master/.github/workflows/trigger.yml#L45. Maybe it is a good alternative.

  1. I assume/believe that part of the reason for the common FAMILY/BOARD setup within the examples is for CI and automated builds. Do people also expect to be able to build run the examples from within the TinyUSB tree (note there were some bugs in the new family.c which suggest they might not have). Is it a requirement of CI to be able to specify BOARD explicitly here rather than via PICO_BOARD when using the SDK

The family/board is way to ease adding new board within a same mcu family. make BOARD= is actually what I used to develop and maintain the repo. And yes, it is what user expect to run examples to testing out the stack. TinyUSB only use a thin board api that only use 1 LED, 1 Button and Uart (and tick timer mostly for blinky). https://github.com/hathach/tinyusb/blob/master/docs/boards.md https://github.com/hathach/tinyusb/blob/master/docs/getting_started.md#examples . In general, I will also ask user to try out this way when opening an issue.

  1. I'm a bit confused by the CMakeLists.txt in the examples. They are IFed based on the FAMILY, but the esp32 (or whatever I'm not looking atm) code paths in the CMakeLists.txt don't actually add any targets or set any sources. The SDK based GLOBing for examples works for now (to create our own build targets within pico-examples later), but if the example CMakeLists.txt provided some information on source files in a common way, maybe we could utilize those in some way. The way this was(/would have been :-) ) handled across multiple families would have helped here.

esp32s2 requires to run with FreeRTOS config, you would see it in the _freertos example https://github.com/hathach/tinyusb/blob/master/examples/device/cdc_msc_freertos/CMakeLists.txt#L13 . Actually, I don't use CMakeLists.txt as top level, I typically type make BOARD=pico which include the make.mk
which will include and run the all target by family.mk. It is a bit complicated, but I did to keep it consistent with all other mcu ports.

Actually, I am not too familiar with CMake, and I only try enough to get it running with existing make BOARD=( have no time to play with build system :D ). If you have any suggestion or better an PR, I am more than happy to collaborate with.

@kilograham
Copy link
Contributor Author

yup, for the initial SDK release we copied some of the examples into the sdk because we weren't sure about changing some settings (like CFG_TUSB_OS being unconditionally set to OPT_OS_NONE in the the configs - which have now mostly been changed upstream to be guarded by #ifndef)

This raspberrypi/pico-examples#99 PR in in pico-examples changes us to use a curated list of your examples, but by globbing the the source files using the cmake helper function in this PR.

Given that you already have some support for saying which examples belong with which families, I am hoping that maybe long term we could just import a CMakeLists.txt from your example root into our pico-examples tree to add all your current examples so there is non manual work. This should be possible, but I wanted to understand the current usage of the CMakeLists.txt in tinyusb examples first because figuring out how best to refactor them.

Note also there is also this https://github.com/raspberrypi/tinyusb/tree/pico-0.9.0-wip branch, which has a few tweaks, and allows for specifying a BOARD on the TinyUSB side which just uses the PICO_BOARD settings from the pico-sdk

@lurch
Copy link
Contributor

lurch commented Apr 10, 2021

Part of the confusion comes from the fact that TinyUSB has TinyUSB->pico-sdk and we have pico-sdk->TinyUSB ;-)

And there's also MicroPython which submodules both lib/tinyusb and lib/pico-sdk ;-)

@hathach
Copy link
Contributor

hathach commented Apr 10, 2021

yup, for the initial SDK release we copied some of the examples into the sdk because we weren't sure about changing some settings (like CFG_TUSB_OS being unconditionally set to OPT_OS_NONE in the the configs - which have now mostly been changed upstream to be guarded by #ifndef)

This raspberrypi/pico-examples#99 PR in in pico-examples changes us to use a curated list of your examples, but by globbing the the source files using the cmake helper function in this PR.

Ah yeah, naturally it is default no OS configuration previously. I made some changes when adding rp2040 support. There maybe a few left, feel free to make PR to fix those.

Given that you already have some support for saying which examples belong with which families, I am hoping that maybe long term we could just import a CMakeLists.txt from your example root into our pico-examples tree to add all your current examples so there is non manual work. This should be possible, but I wanted to understand the current usage of the CMakeLists.txt in tinyusb examples first because figuring out how best to refactor them.

Yeah, ready to use CMakeLists.txt from tinyusb example for pico-sdk is what I really want as well. That will help people testing out examples easier. I will try to tackle the build system later on when having more time.

Note also there is also this https://github.com/raspberrypi/tinyusb/tree/pico-0.9.0-wip branch, which has a few tweaks, and allows for specifying a BOARD on the TinyUSB side which just uses the PICO_BOARD settings from the pico-sdk

Thanks for the link, I will check it out next time I rework the cmake build for rp2040.

@hathach
Copy link
Contributor

hathach commented Apr 10, 2021

Part of the confusion comes from the fact that TinyUSB has TinyUSB->pico-sdk and we have pico-sdk->TinyUSB ;-)

And there's also MicroPython which submodules both lib/tinyusb and lib/pico-sdk ;-)

Yeah, lots of project depends on each others. pico-sdk and tinyusb is kind of weird since they include each other, therefore submodule init with recursive flag shouldn't be run 😅 . Maybe I will drop the pico-sdk submodule in tinyusb, and have an instruction to tell people to install pico-sdk themself. After all, tinyusb is supposed to be included as submodules by others. Will do it later on.

@hathach
Copy link
Contributor

hathach commented Apr 11, 2021

update I have removed pico-sdk as part of tinyusb submodules, it would be much better for rp2040 user. They definitely need to pico-sdk somewhere already. hathach/tinyusb#782

@kilograham
Copy link
Contributor Author

obsoleted by new #462

@kilograham kilograham closed this May 31, 2021
@kilograham kilograham deleted the tinyusb-0.9.0 branch June 17, 2021 01:28
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.

When tinyusb upstream repo will be used in the sdk? bump TinyUSB to latest upstream release
4 participants