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

Allow vendor class to be used without FIFO. #2450

Merged
merged 16 commits into from
Sep 11, 2024

Conversation

HiFiPhile
Copy link
Collaborator

@HiFiPhile HiFiPhile commented Feb 1, 2024

Describe the PR
Currently vendor class manage data the same way as CDC class.

Many protocols using buck transfer are based on packets but not data stream like CDC.
In these cases a FIFO is not needed which adds overhead, also putting all packets in FIFO adds extra difficulty to separate them since length of each packet is lost.

Additional context
I'm trying to make a CMSIS-DAP dongle.

Copy link
Owner

@hathach hathach left a comment

Choose a reason for hiding this comment

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

we can use existing CFG_TUD_VENDOR_RX_BUFSIZE when not defined or defined to 0 mean no buffered. https://github.com/hathach/tinyusb/blob/master/examples/device/webusb_serial/src/tusb_config.h#L104-L105

I am off for TET holidays for a couple of weeks. Will try to review afterwards, thank you for your patient.

@HiFiPhile
Copy link
Collaborator Author

we can use existing CFG_TUD_VENDOR_RX_BUFSIZE when not defined or defined to 0 mean no buffered.

That's true.

Me too I was back to China for holidays:)

@hathach
Copy link
Owner

hathach commented Feb 6, 2024

we can use existing CFG_TUD_VENDOR_RX_BUFSIZE when not defined or defined to 0 mean no buffered.

That's true.

Me too I was back to China for holidays:)

Have a great year of 🐉

@HiFiPhile
Copy link
Collaborator Author

we can use existing CFG_TUD_VENDOR_RX_BUFSIZE when not defined or defined to 0 mean no buffered.

@hathach Done

@HiFiPhile
Copy link
Collaborator Author

Additional context
I'm trying to make a CMSIS-DAP dongle.

@kkitayam Didn't see that you've already done it :)

@HiFiPhile
Copy link
Collaborator Author

@hathach Could you take a look this one and #2593 ?

@hathach
Copy link
Owner

hathach commented Aug 27, 2024

@HiFiPhile sure, sorry, I kind of forgot. Will review these soon enough.

Copy link
Owner

@hathach hathach left a comment

Choose a reason for hiding this comment

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

@HiFiPhile thank you for the PR and sorry for huge delay. I initially want to migrate vendor driver to use new edpt stream API ( only used by host cdc but running rather well so far), therefore didn't review this as fast as I could. I think it is a good chance to do so, also update the edpt stream to also support fifo depth = 0 (no fifo/bufferred mode). This make the code clearer without too much of #ifelse, the down side is adding 8 bytes per endpoint for overhead

In the future, we can migrate most of the buffered driver e.g cdc/midi to use this stream API to reduce duplication (once it runs stable enough).

src/tusb.c Dismissed Show dismissed Hide dismissed
src/tusb.c Dismissed Show dismissed Hide dismissed
src/tusb.c Dismissed Show dismissed Hide dismissed
@HiFiPhile
Copy link
Collaborator Author

Thanks for looking into this, indeed having a stream api is simpler.

It seems like missing some #ifdef around functions when they are unavailable, like tud_vendor_n_peek ?

@hathach
Copy link
Owner

hathach commented Sep 10, 2024

Thanks for looking into this, indeed having a stream api is simpler.

It seems like missing some #ifdef around functions when they are unavailable, like tud_vendor_n_peek ?

that is ok, peek() and other API will return false since there is nothing in the fifo, except for write_available() which is essentially check for edpt busy status.

@HiFiPhile
Copy link
Collaborator Author

I think tu_edpt_stream_init() better to have OSAL_MUTEX_REQUIRED like tu_edpt_stream_deinit(), also mutex can be skipped when fifo is not used.

In the header tud_vendor_write_available is guarded by #if but not tud_vendor_n_write_available .

@hathach
Copy link
Owner

hathach commented Sep 10, 2024

I think tu_edpt_stream_init() better to have OSAL_MUTEX_REQUIRED like tu_edpt_stream_deinit(), also mutex can be skipped when fifo is not used.

In the header tud_vendor_write_available is guarded by #if but not tud_vendor_n_write_available .

thanks, you are right, we should only create fifo mutex if needed, also wrapped in the OSAL_MUTEX_REQUIRED similar to deinit().

PS: Let me know if you think it is good for the merge.

@hathach
Copy link
Owner

hathach commented Sep 10, 2024

@HiFiPhile just notice you want to make an cmsis dap, maybe just using pico debugdap, I have been using it with other mcus, and it works rather well with openocd https://github.com/raspberrypi/debugprobe

@HiFiPhile
Copy link
Collaborator Author

@HiFiPhile just notice you want to make an cmsis dap, maybe just using pico debugdap, I have been using it with other mcus, and it works rather well with openocd https://github.com/raspberrypi/debugprobe

Didn't know that :)

@kkitayam wrote a cmsis-dap class: https://github.com/kkitayam/akiprobe/blob/main/src/cmsis_dap_device.c, based on that
I did an implementation on LPC55. Speed is pretty good, ram download can achieve 1MB/s.

But it fell short compared to J-Link for debug, when the CPU is locked up or in hardfault the probe can't always recover, besides in IAR it can't have RTT output at the same time.

@hathach
Copy link
Owner

hathach commented Sep 10, 2024

@HiFiPhile just notice you want to make an cmsis dap, maybe just using pico debugdap, I have been using it with other mcus, and it works rather well with openocd https://github.com/raspberrypi/debugprobe

Didn't know that :)

@kkitayam wrote a cmsis-dap class: https://github.com/kkitayam/akiprobe/blob/main/src/cmsis_dap_device.c, based on that I did an implementation on LPC55. Speed is pretty good, ram download can achieve 1MB/s.

But it fell short compared to J-Link for debug, when the CPU is locked up or in hardfault the probe can't always recover, besides in IAR it can't have RTT output at the same time.

they also sell it for 12$ https://www.raspberrypi.com/products/debug-probe/, standard cmsis dap v2, work with mainline openocd therefore support most arm mcu, speed is stable at 5000 khz. You can test it out by loading the debugprobe_on_pico with normal pico from https://github.com/raspberrypi/debugprobe/releases/tag/debugprobe-v2.0.1 . Though I found their product has better buffered circuit for swdio and more reliable than using it on normal pico.

Note: their probe don't use rst signal at all, but you can enable it with normal pico if need to https://github.com/raspberrypi/debugprobe/blob/master/include/board_pico_config.h#L38

Note2: this debug probe work much better than jlink when debuggin mutliple core CPU on rp2040. I found jlink does not work well with 2 cpus on rp2040.

PS: I think openocd has added support for segger RTT as well but haven't tried it yet.

PPS: I currently used it with normal pico to flash both a rp2040 and analog max32666. It is 2nd choice when I have to use dev board without jlink.

Copy link
Owner

@hathach hathach left a comment

Choose a reason for hiding this comment

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

merge this for now, if there is any issue, we can do follow up

@hathach hathach merged commit 4b107a2 into hathach:master Sep 11, 2024
95 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants