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

add controller register interface and support TinyUSB #22

Conversation

hathach
Copy link
Contributor

@hathach hathach commented May 3, 2022

Whoop, after 2-3 weeks of works, I finally wrap up this PR. This is more difficult than my initial thought. Your repo is going uber low (bit banging) into USB, I learn quite a lot more from your code despite working with USB for quite some time. I could only imagine amount of time and effort you spend on oscilloscope to get this working. Really brilliant work !!

This PR implement #14 and add an register interface for pio-usb port (host/device) similar to native usb controller on rp2040 e.g event interrupt status, ep complete/error/stalled and irq handler. This allow pio-usb "low-level controller part" can be used with tinyusb.

Following is the notable changes:

  • Move host and device specific code to pio_usb_host/device.c . This is mostly for maintaining purpose, I think it would be easier to separate code from separated mode. The cons is: this requires common function/variable declaration need to be placed in the an header. Let me know if you prefer to keep it all in one file, I will merge it back.
  • Add new internal header pio_usb_ll.h for controller register interface (along with other internal common function of host & device)
  • add some extra field for endpoint_t so that I could work as standalone without usb_devices variable. Also allow it to transfer more than max packet size for transfer like control, msc bulk in the future.
  • Support for endpoint flow control with NAK if there is no "active" transfer to prevent overrun error. Also support endpoint stalled sent/received
  • add PIO_USB_USE_TINYUSB to off-out internal usb host/device stack.
  • All the existing API of pio_usb still and should work as it is and require no changes from running examples.

This is no way perfect and not optimized, but I did quite a bit of testing with both host and device. Let me know what you think and would change, I hope we could merge this soon.

Note: For testing with TinyUSB

  • check out https://github.com/hathach/tinyusb/tree/pio-host and its submodule lib/Pico-PIO-USB
  • compile example host/hid_to_cdc, which is configured to use native usb as cdc device, and pio usb as host to get hid input.
  • any input event e.g mouse move, button will be printed to cdc on the native usb.

@tannewt @ladyada

* add root port suspended bit, more with hw root port
* move host and device specific code to pio_usb_host/device.c
* support endpoint stalled, nak
* merge send in/out/setup into send_token()
* add PIO_USB_USE_TINYUSB to select tinyusb
@sekigon-gonnoc
Copy link
Owner

thank you for your PR, I will review changes within a few days.

@hathach
Copy link
Contributor Author

hathach commented May 4, 2022

thank you for your PR, I will review changes within a few days.

no problems, take your time :)

Copy link
Owner

@sekigon-gonnoc sekigon-gonnoc left a comment

Choose a reason for hiding this comment

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

I think this PR can be merged after minor revisions.

I confirmed that we can use PIO USB port with TinyUSB stack, and maintainability is improved by separating files. Great work!!

Please refer following review comments.

  • I'm palnning move all sources/headers to src/ folder. Because this may affect tinyusb makefiles, could you do that in this PR?
  • Could you add description about TinyUSB's hid_to_cdc example to README.md?
  • There are also some codes that need to be confirmed.

pio_usb_configuration.h Outdated Show resolved Hide resolved
pio_usb_host.c Outdated Show resolved Hide resolved
pio_usb_host.c Outdated Show resolved Hide resolved
pio_usb_host.c Outdated Show resolved Hide resolved
@sekigon-gonnoc
Copy link
Owner

Following comments are not necessary to accept this PR, but I want to hear your opinion:

How do you think about PIO_USB_DEFAULT_CONFIG?
Currently, we have to edit pio_usb_configuration.h to edit configuration. To use Pico-PIO-USB as a libraly, it is better to these are configurable without modifying header files (e.g. use #ifndef-#define in header definitions and overwrite them by compile time definitions).
Also, we may have to check PIO sm/instruction memory availability at compile time or initializing.

This is also not related to this PR:

TinyUSB's hid_to_cdc example are not work on Windows because ITF_NUM_TOTAL is wrong. This works by comment out line 84 of usb_descriptors.c
https://github.com/hathach/tinyusb/blob/pio-host/examples/host/hid_to_cdc/src/usb_descriptors.c#L84

@hathach
Copy link
Contributor Author

hathach commented May 6, 2022

I think this PR can be merged after minor revisions.

Great thank you very much 👍 💯

I confirmed that we can use PIO USB port with TinyUSB stack, and maintainability is improved by separating files. Great work!!

Please refer following review comments.

  • I'm palnning move all sources/headers to src/ folder. Because this may affect tinyusb makefiles, could you do that in this PR?

Will do this in the next push

  • Could you add description about TinyUSB's hid_to_cdc example to README.md?
  • There are also some codes that need to be confirmed.

Sure will do, maybe I will add it to examples of this repo as well. Since the tinyusb example is a bit more generic and include too many cross-platform #ifdef that complicate the rp2040 user.

Following comments are not necessary to accept this PR, but I want to hear your opinion:

Ah you are right on the multiple host ports scenario. I will try to fix those and do a full revise as well.

How do you think about PIO_USB_DEFAULT_CONFIG? Currently, we have to edit pio_usb_configuration.h to edit configuration. To use Pico-PIO-USB as a libraly, it is better to these are configurable without modifying header files (e.g. use #ifndef-#define in header definitions and overwrite them by compile time definitions). Also, we may have to check PIO sm/instruction memory availability at compile time or initializing.

Yeah, I am glad you ask, I think about this previously, and intend to make follow up PRs for this very purpose. IMHO, there should be an application config file such as pio_usb_config.h much like tusb_config.h which is located within application sources to set/change value such as DP, DM pin. The pio_usb_configuration.h as you mentioned define default value for those that aren't defined by application. This allow pico-pio-usb to be included as submodule by other project without the need to modify its file (pio_usb_configuration.h) to have desired behavior. To stay compatible with existing app without the configure header, we can use __has_include to check for the header availability.

This is also not related to this PR:

TinyUSB's hid_to_cdc example are not work on Windows because ITF_NUM_TOTAL is wrong. This works by comment out line 84 of usb_descriptors.c https://github.com/hathach/tinyusb/blob/pio-host/examples/host/hid_to_cdc/src/usb_descriptors.c#L84

Ah right, just fixed it, it is copy-paste mistake, Linux is apparently more forgiving on this.

@mnesarco
Copy link

mnesarco commented May 6, 2022

Hey Dudes, The work that you are doing here is simply amazing. Thank you so much.

@hathach
Copy link
Contributor Author

hathach commented May 6, 2022

@sekigon-gonnoc 2 new commits doesn't change the code yet, since we are moving codes around (to /src). I think it is best to add an ci to verify build first. I also take the chance to upload the example_binaries (hex + uf2: for usb_device & capture_hid_report), this would help others that want to test out the PR and latest changes without having to do the proper set-up + compile.

https://github.com/hathach/Pico-PIO-USB/actions/runs/2283033358

Will push more update in upcoming days.

@hathach
Copy link
Contributor Author

hathach commented May 10, 2022

  • I have followed pr reviews to fix 2 host_send_setup() and remove the un-comment in the configuration.h
  • Also add new example/host_hid_to_device_cdc based on the example in tinyusb. It has a simpler tusb_config.h and can be compile right within this repo for reference. Note: pico-sdk/lib/tinyusb need to be switch to tinyusb lastest (but that only need time for pico-sdk to propagate)
  • Add ci to build all examples and upload hex/uf2 as downloaded binaries.

TODO: try out multiple host ports, will push/post when there is more update.

PS: Let me know if you have any plan to change or wrap macro in pio_usb_configuratio.h . I could make the changes as well, or we can wait and make the change afterward (follow up PR) if you want to spend time to think more

@hathach
Copy link
Contributor Author

hathach commented May 11, 2022

I still haven't tried out multiple ports, since TinyUSB currently only support max 2 roothub ports (1 for native device, 1 for pio-usb host). Adding more rhport is possible, but required a bit of changing in macro naming etc ... I will try that later on. But the changes of c12e4c5 should be sufficient enough.

For PIO_USB_DEFAULT_CONFIG, I wrap PIO_USB_DP_PIN_DEFAULT with ifdef, since that macro is something user will for sure modify especially the default 0/1 is mostly used for UART TX/RX. The other macros is safely left as it is (until you decide which is best way to handle lib configuration).

Let me know if update look good and/or any request changes you would like to be done.

PS: the examples/host/hid_to_cdc is renamed to examples/dual/host_hid_to_device_cdc in tinyusb

@sekigon-gonnoc
Copy link
Owner

@hathach I tried multiple ports with example/capture_hid_report, and it works fine. So c12e4c5 is proper.

Do you have any other TODOs in this PR? If not, I will merge this.

@hathach
Copy link
Contributor Author

hathach commented May 13, 2022

Great, thank you for testing out the multiple ports changes. I have no other TODO, please merge it when you have time.

PS: Since the current ci use an tinyusb branch pio-host, I will probably make an pr update when tinyusb's own PR is update as follow up one later on.

@sekigon-gonnoc sekigon-gonnoc merged commit 2a9fefd into sekigon-gonnoc:main May 14, 2022
@sekigon-gonnoc
Copy link
Owner

Thank you for your corporation.

@hathach
Copy link
Contributor Author

hathach commented May 16, 2022

it is my pleasure

@hathach
Copy link
Contributor Author

hathach commented May 18, 2022

FYI, I made an twitter video about this PR with mentioned to your github:

@hathach hathach deleted the add-register-interface-and-tinyusb-support branch November 23, 2022 05:15
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.

3 participants