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

feat(ThirdParty): Add TinyUSB library to MSDK #1101

Merged
merged 13 commits into from
Sep 5, 2024

Conversation

BrentK-ADI
Copy link
Contributor

@BrentK-ADI BrentK-ADI commented Aug 1, 2024

Add TinyUSB support into MSDK.

TinyUSB was ported to the MAX32 devices and the PR is currently under review at the TinyUSB Repo (PR 2708). This PR is to bring the implementation back into the MSDK for use within MSDK specific applications.

The current strategy is:
Library

  • Add TinyUSB as a library into MSDK similar to MAXUSB, FreeRTOS, etc
  • Rather than a Git submodule, bring in only the necessary files for device mode operation with the MAX32
  • Files are not modified (clang-format, cpplint, etc)
  • Do not incorporate the TinyUSB BSP system as it will conflict with the MSDK BSP
  • Examples will be included verbatim to be used as reference until all/most examples are port to MSDK projects

Examples

  • Create examples in the MSDK/Examples folder similar to other MSDK example projects
  • Use the TinyUSB examples verbatim, adding wrappers to bridge the MSDK BSP with the TinyUSB expected BSP calls
  • Examples a clang-formatted and cpp-linted to comply with standards
  • Add a simple README to the TinyUSB/Examples folder to instruct user's how to port other examples which have not yet been added.

@github-actions github-actions bot added the MAX32650 Related to the MAX32650 (ME10) label Aug 1, 2024
@BrentK-ADI
Copy link
Contributor Author

BrentK-ADI commented Aug 1, 2024

@Jake-Carter , @EricB-ADI . This is incomplete, still a lot of work to be done on the examples and documentation side of things. Before going too far, I wanted to kick off a discussion/review of the approach for both the library and example projects. Once you are happy with how things will get integrated, creating all the examples will be cookie-cutter. Just rather it be right with the first two, before going all in.

Also, I'm sure we'll need to engage someone to approve and confirm compatibility with Apache and MIT licensing.

@Jake-Carter
Copy link
Contributor

Thanks @BrentK-ADI will take a look asap

Copy link
Contributor

@Jake-Carter Jake-Carter left a comment

Choose a reason for hiding this comment

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

Looks good @BrentK-ADI

I opened a PR for some changes I'd like to make in BrentK-ADI#1

In general these are just to clean things up a bit and make this process easier for the future

@github-actions github-actions bot added the MAX32665 Related to the MAX32665 (ME14) label Aug 15, 2024
@github-actions github-actions bot added MAX32690 Related to the MAX32690 (ME18) MAX78002 Related to the MAX78002 (AI87) labels Aug 15, 2024
@BrentK-ADI
Copy link
Contributor Author

I think at this point, everything is pushed for review. Once the PR at the TinyUSB repo is merged, I'll re-grab the library code to make sure our inclusion is in sync with the official repo.

I added examples for all the supported parts and updated the documentation best I could see where. I assume the OSPO will auto generate an update to NOTICE when the time comes.

Feel free to start a review now, or wait until I update the TUSB code and take this out of draft.

@BrentK-ADI BrentK-ADI marked this pull request as ready for review August 19, 2024 11:57
Copy link
Contributor

@Jake-Carter Jake-Carter left a comment

Choose a reason for hiding this comment

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

Looks good @BrentK-ADI, let me know if you need any help resolving the merge conflicts in the example projects

BrentK-ADI and others added 9 commits August 23, 2024 14:14
This commit is intented to be enough to start the review process with maintainers to verify approach with
library and examples is the correct path, before adding additional examples and other parts.

 - Created TinyUSB library and Makefile for support library build
    - Added TinyUSB source files for device mode and MAX32 support
    - Added TinyUSB example projects verbatim as reference code. Not compiled in.
 - Added cdc_msc and cdc_msc_freertos examples for MAX32650
 - Moved existing MAX32650 MAXUSB example to subfolder in Examples.
* Update tinyusb build system integration

- Swap out Makefile for latest project "driver" Makefile.  The only
  difference here is that TARGET is floating.
- Simplify tinyusb.mk
- Add build-specific configuration to project.mk, and simplify
  IPATH/VPATH requirements
- Add clean.tinyusb
- Build library inside the tinyusb library by default
- Add default configuration directory

* Use override on PROJECT
 - Updated TinyUSB build system to incorporate FreeRTOS include folders
 - Updated existing MAX32650 examples for new build variables.
 - Added README to existing examples
 - Added HID and MSC example projects for MAX32650
 - Cleanedup .vscode folders to match existing MSDK examples.
 - Added example project (cdc_msc, cdc_msc_freertos, hid_composite, msc_dual_lun) for MAX32666
 - Relocated legacy MAXUSB examples to subfolder.
 - Added TinyUSB Examples (cdc_msc, cdc_msc_freertos, hid_composite, msc_dual_lun) for MAX32690
 - Moved MAXUSB examples to subfolder
 - Added TinyUSB Example (cdc_msc, cdc_msc_freertos, hid_composite, msc_dual_lun) for MAX78002
 - Moved MAXUSB examples to sub folder.
Updated TinyUSB to baseline against commit ca3925a (PR #2708) which merged MAX32 support into master.
@BrentK-ADI
Copy link
Contributor Author

@Jake-Carter I rebased against main to resolve the merge conflicts. Looked like it was just a minor Makefile update causing the issue. Probably worth checking once over to make sure I didn't goof something up...I have by Git noob moments sometimes.

@Jake-Carter
Copy link
Contributor

@BrentK-ADI looks like there are some linker errors in hid_composite now - can you check?

@BrentK-ADI
Copy link
Contributor Author

@Jake-Carter I think the issue here is building a TinyUSB library for re-use versus compiling the source directly into the project. The contents of TUSB library is dependent on the tusb_config.h file. The Makefile trys to compensate for this with the ${TINYUSB_CONFIG_DIR}/tusb_config.h dependency, but the path pointed to by TINYUSB_CONFIG_DIR will change from example to example, so I don't think the dependency will ever catch.

Do we do anything with FreeRTOS that compensates for this? I would imagine it has the same issue with FreeRTOSConfig.h sitting in the project folder, but a library is being built.

@Jake-Carter
Copy link
Contributor

@Jake-Carter I think the issue here is building a TinyUSB library for re-use versus compiling the source directly into the project. The contents of TUSB library is dependent on the tusb_config.h file. The Makefile trys to compensate for this with the ${TINYUSB_CONFIG_DIR}/tusb_config.h dependency, but the path pointed to by TINYUSB_CONFIG_DIR will change from example to example, so I don't think the dependency will ever catch.

Do we do anything with FreeRTOS that compensates for this? I would imagine it has the same issue with FreeRTOSConfig.h sitting in the project folder, but a library is being built.

Agreed. This is now a common issue across Cordio, FreeRTOS, and now TinyUSB.

Looking into a solution for tracking the path and contents of the config file as a dependency. One interesting option is here.

I implemented it and it works for tracking changes to options inside a project. It will need some more work to track changes across projects and for the library files.

Once tinyusb.a exists, there's no other way to trigger a rebuild unless we did something like $HASH/tinyusb.a or tinyusb-$HASH.a + another recipe to copy or symlink back to tinyusb.a. Not a huge fan of the external tool dependencies or renaming the files but so far this is the most promising solution to this problem I've come across.

@BrentK-ADI
Copy link
Contributor Author

Does it still make sense to retain these libraries as libraries in the linking sense? Thinking about FreeRTOS and TinyUSB especially, since what gets compiled into these projects is so dependent on the project settings itself, does it make more sense to just have the respective .mk files just append to SRCS, I_PATH, PROJ_CFLAGS, etc when including those libraries and have it built with the project and the dependencies tracked for changes as normal?

@Jake-Carter
Copy link
Contributor

Yes, that's definitely a reasonable approach. We do it for MiscDrivers and a couple other libraries.

Just updated to go that route (3d408d4).

I like the modularity of building each library as a static file, but this is simpler to get this working across config changes.

@github-actions github-actions bot added the Workflow Related to Workflow development label Sep 4, 2024
@BrentK-ADI
Copy link
Contributor Author

Looks like the relocation of the MAXUSB examples cause a linting failure with header guards. I'll go ahead and fix those today.

Is there a good way to run all the automated checks locally before committing? I've been running cpplint and clang-format somewhat manually to check the files. My mistake on these examples not realizing the path contribution to the #ifdefs, but generally speaking didn't see a one-shot command in the contribution guide.

BrentK-ADI and others added 2 commits September 4, 2024 14:29
Updated header guards for MAXUSB examples due to relocation of projects.
@Jake-Carter
Copy link
Contributor

Thanks!

You can run the linter/formatter locally on files/folders (https://analogdevicesinc.github.io/msdk//CONTRIBUTING/#running-the-linter-formatter) but there's not a great way to run the entire check locally, since most of it is written in the Github workflow file.

It would be nice in the future to consolidate it to a local script like I did with the build test Python script.

@Jake-Carter Jake-Carter merged commit 6b6f2c3 into analogdevicesinc:main Sep 5, 2024
8 checks passed
@Jake-Carter
Copy link
Contributor

@BrentK-ADI merged, thanks again. This will be a great addition

sihyung-maxim pushed a commit to analogdevicesinc/hal_adi that referenced this pull request Sep 5, 2024
ozersa pushed a commit to analogdevicesinc/hal_adi that referenced this pull request Sep 11, 2024
ozersa pushed a commit to analogdevicesinc/hal_adi that referenced this pull request Sep 11, 2024
@BrentK-ADI BrentK-ADI deleted the tinyusb_support branch October 23, 2024 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
MAX32650 Related to the MAX32650 (ME10) MAX32665 Related to the MAX32665 (ME14) MAX32690 Related to the MAX32690 (ME18) MAX78002 Related to the MAX78002 (AI87) Workflow Related to Workflow development
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants