DMA example? (tu_fifo_t with DMA) #839

mfp20 · 2021-05-18T20:50:09Z

mfp20
May 18, 2021

I'm searching for a way to use tu_fifo_t with DMA on rp2040, in order to move data from/to USB to/from other interfaces without cpu intervention.

If I get it right, I can set f->wr_idx as dma destination address (or f->rd_idx as source address), let it go for n bytes, and then on DMA block completion call tu_fifo_advance_write_pointer (or tu_fifo_advance_read_pointer) to update the indexes of n bytes. And do not be worried of wrap around and max_pointer_idx as the advance_pointer() already takes care of it. And in the case I need for this to be done from multiple tasks/cores, I can use the existing mutex (with OS_PICO) to avoid clashes.
But how do I get the tu_fifo_t of the existing cdc? Those are called rx_ff and tx_ff in each cdcd_interface_t of

CFG_TUSB_MEM_SECTION static cdcd_interface_t _cdcd_itf[CFG_TUD_CDC];

not accessible outside of src/class/cdc/cdc_device.c unit. And there are no functions to get a pointer.

Answered by PanRe

May 20, 2021

I don't know if this was clear so far, in case you want to use the DMA, use the functions

void tu_fifo_get_read_info (tu_fifo_t *f, tu_fifo_buffer_info_t *info);
void tu_fifo_get_write_info(tu_fifo_t *f, tu_fifo_buffer_info_t *info);

to get the corresponding pointers and possible lengths to read/write. You must reduce the possible lenghts to your desired one (if you have any of course). As for audio class (i am not familiar with the details of CDC), you get a callback function called every time data is to be sent/received via USB interface or another callback function called when data was sent/received informing you that the corresponding FIFOs are empty now. You may add something like t…

View full answer

hathach · 2021-05-19T03:04:33Z

hathach
May 19, 2021
Maintainer

Yes, it is how you should use fifo + dma. However since the cdc data is not time sensitive and the dma usage is new. It is currently only applied to audio driver. Why do you want to do fifo+dma in the first place ?

9 replies

hathach May 19, 2021
Maintainer

thanks for your suggestion. Though it is still not clear to me what you want to do, and what prevent you to that. Can you elaborate it with pseudo code ? Providing buffer from tusb_init() or statically allocated by compiler is not much of difference, I think what matter is how the stack manage/use those buffer. Though I am open to all suggestion.

BufferBloat may be an issue with one MCU (e.g rp2040) but is required for another e.g LPC55xx where the MCU DMA requires all buffer transferring must be aligned by 64 bytes, that make fifo only approach impossible with stream/buffered class such as cdc ( random number of bytes read/write) without changing the APIs. Though It is doable with audio since it send data frame by frame.

I am fully aware of this and plan to improve this later on with dcd_edpt_xfer_fifo() which is added recently to facilitate supported MCU. However, I am overwhelmed by other feature/bug e.g host, audio driver etc .. Since time is what we all lacking,I rather spent time working on thing 90% people want than neat thing only 10% people may need.

mfp20 May 19, 2021
Author

thanks for your suggestion. Though it is still not clear to me what you want to do

I apologize for my english proficiency, it isn't my natural language. Looks like I completely missed the point.

and what prevent you to that

Well, it is a major change in tinyusb. If I'd do that, I'd have to maintain my local custom copy of it. Doable, but pretty dangerous in time.

Can you elaborate it with pseudo code ?

// --- tusb_fifo.h and tusb_fifo.c, become a custom fifo wrapper, with default fallback to current tusb_fifo_t

// first rename current methods from tu_fifo_XXX to default_fifo_XXX

typedef struct tusb_custom_fifo_define_s {
    bool (*custom_fifo_write)                          (void* f, void const * p_data);
    uint16_t (*custom_fifo_write_n)                (void* f, void const * p_data, uint16_t count);
    [...all the fifo function pointers needed by tinyusb... currently declared in tusb_fifo.h]
} tusb_custom_fifo_define_t;

bool (*tu_fifo_write)                          (void* f, void const * p_data) = default_fifo_write;
uint16_t (*tu_fifo_write_n)                (void* f, void const * p_data, uint16_t count) = default_fifo_write_n;
[...all the fifo function pointers needed by tinyusb... currently declared in tusb_fifo.h]


// --- tusb.h and tusb.c

bool tusb_init(tusb_custom_fifo_t *fifodef, tusb_fifo_t *rx_fifo, tusb_fifo_t *tx_fifo) {
    [...]
    if (fifodef) {
        // set all the custom methods in place of the default ones
        tu_fifo_write = fifodef->custom_fifo_write;
        tu_fifo_write_n = fifodef->custom_fifo_write_n;
        [...all the fifo function pointers needed by tinyusb...]
    }
    if (rx_fifo) {
        // for each device spawned assign one tx buffer from the given array
        for(int itf=0;itf<CFG_TUD_X;itf++) {
            tud_set_rx_buffer(itf, &rx_fifo[itf]);
        }
    } else {
        // default rx fifo init, same as now using 
        // bool tu_fifo_config(tu_fifo_t *f, void* buffer, uint16_t depth, uint16_t item_size, bool overwritable);
    }
    if (tx_fifo) {
        // for each device spawned, one tx buffer from the given array
        for(int itf=0;itf<CFG_TUD_X;itf++) {
            tud_set_tx_buffer(itf, &tx_fifo[itf]);
        }
    } else {
        // default tx fifo init, same as now using 
        // bool tu_fifo_config(tu_fifo_t *f, void* buffer, uint16_t depth, uint16_t item_size, bool overwritable);
    }
    [...]
}

Allows to use own ringbuffer implementation as long as compatible APIs are supplied in tusb_init().
Allows to use default tu_fifo_t and pass external buffers, rx only, tx only, or both directions.
Allows to use the default setup, the same we are using now; just call tusb_init(NULL, NULL, NULL).

Though It is doable with audio since it send data frame by frame.

It is doable with audio because you have control on all the required code.

Since time is what we all lacking,I rather spent time working on thing 90% people want than neat thing only 10% people may need.

I agree. That's why I offered myself to write the working draft you can then evolve to get it to sufficient code quality for your standards.

Let me discuss your 'neat thing' qualifier only. I think this feature is way bigger than 'neat'; not because I need it (I don't, I'm good with my small API addition and actually I'm happy to use tu_fifo_t as my default for ring buffers!!!); but because there's an entire world of apps to be ported to new MCUs having an sdk using TinyUSB by default, and probably most of them have their own ring buffer implementation because it is the best data structure for IO peripherals. I think actually that the ring buffer is out of TinyUSB scope; it's a dependency. Forcing all developers in this exclusive relationship between TinyUSB and its ringbuffer of choice, is an hard constrain limiting tinyusb adoption and as a consequence the sdk adoption (ie: buying hw using tinyusb).

hathach May 20, 2021
Maintainer

I apologize for my english proficiency, it isn't my natural language. Looks like I completely missed the point.

Your English is perfect, I am not native English speaker and terrible with it. That is why I prefer to ask for code in discussion.

Allows to use own ringbuffer implementation as long as compatible APIs are supplied in tusb_init().

Thanks to pseudo code, I now kind of understanding your suggestion a bit better. What you want to do is having an custom fifo read() write() function right ? But I still don't see why application would need to do that ? For DMA operation, the current API already support it with tu_fifo_advance_write_pointer() and tu_fifo_get_write_info()

Allows to use default tu_fifo_t and pass external buffers, rx only, tx only, or both directions.

        // for each device spawned assign one tx buffer from the given array
        for(int itf=0;itf<CFG_TUD_X;itf++) {
            tud_set_rx_buffer(itf, &rx_fifo[itf]);
        }

The changes where device and host driver (usbd, usbh) allocated fifo for class driver does not make sense to me. The class driver sit on top of usbd therefore usbd cannot dictate how the class driver would be implemented, how it manages or uses buffer, usbd also has no (need to) access to any of internal data of class driver. There is only a Class-USBD interface API https://github.com/hathach/tinyusb/blob/master/src/device/usbd_pvt.h#L52 . This allow class driver can be written in cross-platform way

Allows to use the default setup, the same we are using now; just call tusb_init(NULL, NULL, NULL).

The API does not make sense to me as well.I don't see why initializing usb stack API would need an fifo def as argument. For buffer rx tx as mentioned above I don't see why it would be benefit to be allocated from application than the linker. It is another dependency that make tinyusb less portable.

It is doable with audio because you have control on all the required code.

If the current cdc driver does not fit you, you could write your own cdc driver without actual changing the stack. The custom driver take priority than default one like. Here is how rpi team add their raspberrypi/pico-sdk#197 which is implemented by #385

I agree. That's why I offered myself to write the working draft you can then evolve to get it to sufficient code quality for your standards.

That is very nice, I appreciate your willingness.

Let me discuss your 'neat thing' qualifier only. I think this feature is way bigger than 'neat'; not because I need it (I don't, I'm good with my small API addition and actually I'm happy to use tu_fifo_t as my default for ring buffers!!!); but because there's an entire world of apps to be ported to new MCUs having an sdk using TinyUSB by default, and probably most of them have their own ring buffer implementation because it is the best data structure for IO peripherals. I think actually that the ring buffer is out of TinyUSB scope; it's a dependency. Forcing all developers in this exclusive relationship between TinyUSB and its ringbuffer of choice, is an hard constrain limiting tinyusb adoption and as a consequence the sdk adoption (ie: buying hw using tinyusb).

To be honest, I would be much more happy if application does not make any use of tu_fifo_t. By now you should notice that all the public device API start with tud_, host starts with tuh_, and common is tusb_. All of the tu_ (function) and TU_ (macro) are meant to be used internally. I have even tried to not expose the tusb_fifo.h header to application as well previously, but relax it recently with latest audio driver PR #593 . There is lots of fifo discussion there if you are interested.

mfp20 May 20, 2021
Author

Allows to use own ringbuffer implementation as long as compatible APIs are supplied in tusb_init().

Thanks to pseudo code, I now kind of understanding your suggestion a bit better. What you want to do is having an custom fifo read() write() function right ? But I still don't see why application would need to do that ? For DMA operation, the current API already support it with tu_fifo_advance_write_pointer() and tu_fifo_get_write_info()

As I wrote, the rationale is to allow developers to use their own buffer implementation. Ring buffers are (arguably) the best data structure, but sometimes people use linked lists or other data structures for fifos. In any case they might have their own applications to be ported, or libraries to be ported, using their own ring buffer implementation.
By having pluggable funcions you (1) allow them to port the application without replacing the ring buffers around their pre-existing code, and (2) reduce the binary size because 1 single ring buffer implementation must be flashed in.

The other 2 problems are:

no access to tu_fifo_t in use by CDC;
the whole point of DMA engines is to not run code on cpu. If you need to call functions to advance pointers, some of the benefits are gone. In some cases (ex: Raspberry Pi's bcm2835 dma engine) there are no IRQs between each control block; there's a linked list to be pushed with more items before it is too late (ie: before the dma engine pops the last element in list): there's no chance to call advance_pointer().

Allows to use default tu_fifo_t and pass external buffers, rx only, tx only, or both directions.
        // for each device spawned assign one tx buffer from the given array
        for(int itf=0;itf<CFG_TUD_X;itf++) {
            tud_set_rx_buffer(itf, &rx_fifo[itf]);
        }
The changes where device and host driver (usbd, usbh) allocated fifo for class driver does not make sense to me. The class driver sit on top of usbd therefore usbd cannot dictate how the class driver would be implemented

My pseudo-code was just ... pseudo-code, an example. I placed the for loop in there just to show you a procedure importing external buffers. It can be done in the class driver instead; maybe using some defines in tusb_config.h or alike.
The important thing is to give the option to use pre-existing buffers instead of allocating tinyusb internal buffers.

Allows to use the default setup, the same we are using now; just call tusb_init(NULL, NULL, NULL).

The API does not make sense to me as well.I don't see why initializing usb stack API would need an fifo def as argument. For buffer rx tx as mentioned above I don't see why it would be benefit to be allocated from application than the linker. It is another dependency that make tinyusb less portable.

Now TinyUSB depends on tu_fifo_t; after this mod, it wouldn't depend on it any more. It wouldn't depend on any ring buffer implementation at all.

Given the architecture you mentioned (ie: class driver sits on top of usbd), it is out of place to pass buffers in tusb_init(). But the whole mod would make tinyusb MORE portable simply because it doesn't rely on 1 single proprietary ring buffer implementation. It can be used with ANY developer-supplied data structure as long as compatible read() and write() methods are supplied. More options, ANY, not 1 only.

It is doable with audio because you have control on all the required code.

If the current cdc driver does not fit you, you could write your own cdc driver without actual changing the stack. The custom driver take priority than default one like. Here is how rpi team add their raspberrypi/pico-sdk#197 which is implemented by #385

AFAIK, cdc driver is good as is: it works. The only thing missing for my use case is a way to access the tu_fifo_t, and I already posted a simple patch to have it. But my case is simple because I'm starting from a zero-length file, I don't have my own ring buffer implementation in use in the same application, and I don't know any better data structure for an I/O peripheral's buffer. Other use cases can be much more complicated than this one.

I agree. That's why I offered myself to write the working draft you can then evolve to get it to sufficient code quality for your standards.

That is very nice, I appreciate your willingness.

I think actually that the ring buffer is out of TinyUSB scope; it's a dependency. Forcing all developers in this exclusive relationship between TinyUSB and its ringbuffer of choice, is an hard constrain limiting tinyusb adoption and as a consequence the sdk adoption (ie: buying hw using tinyusb).

To be honest, I would be much more happy if application does not make any use of tu_fifo_t. By now you should notice that all the public device API start with tud_, host starts with tuh_, and common is tusb_. All of the tu_ (function) and TU_ (macro) are meant to be used internally. I have even tried to not expose the tusb_fifo.h header to application as well previously, but relax it recently with latest audio driver PR #593 . There is lots of fifo discussion there if you are interested.

Awesome. Why don't you get rid of tu_fifo_t then? I'm suggesting to externalize the ring buffer to app developers rather than you being in need of maintaining your own ring buffer implementation. At this point in time you already have it, and looks good to me, so... it's a pity to trash it. And a fallback it is comfortable anyway; so you can keep current tu_fifo_t as default_fifo_t and define some read(), write(), flush() ... function pointers, initialized by default with default_read() default_write() ... and let the developers set their read() write() flush(). It is the natural solution you were sarching in the PR #593 you linked above!

Advantage of doing it in fifo is it can be generalized for other classes such as CDC.

I think we should make it available as general as possible!

I think it is best to add buffer passthrough (zero-copy) API to the transfer.

And no need to say that it is 100% portable simply because you don't need to worry about the developer's use case (DMA, audio, different architectures and mcus). Whatever the developer needs it is under his control. TinyUSB become transparent.

Things like making a loopback between 2 CDCs become as simple as passing the same rx_fifo and tx_fifo in both DCDs, inverting rx and tx in one of them. No extra code needed.

hathach May 20, 2021
Maintainer

I am confused. I don't see any reason to get rid of tu fifo and replies on any external code for fifo, or even using any other external ringbuffer from an sdk. Not every fifo implementation is the same. The only dependency between usbstack and application should be the class driver and how those driver operates. I don't think your approach is not portable or anything, it just doesn't fit to my point of view.

Words is tricky, so I would prefer to review actual code. Please continue to what you are working on. Once it clearly portrait your approach, please make an PR so that we could further discuss with actual code. It doesn't need to be fully functional or performance optimized, it just make sure we don't misunderstand/mis-interpreter each other.

mfp20 · 2021-05-19T12:16:10Z

mfp20
May 19, 2021
Author

This might be enough

diff --git a/src/class/cdc/cdc_device.c b/src/class/cdc/cdc_device.c
index e54b7d26..9dc562fc 100644
--- a/src/class/cdc/cdc_device.c
+++ b/src/class/cdc/cdc_device.c
@@ -128,6 +128,15 @@ void tud_cdc_n_set_wanted_char (uint8_t itf, char wanted)
   _cdcd_itf[itf].wanted_char = wanted;
 }
 
+tu_fifo_t *tud_cdc_n_get_rx_fifo (uint8_t itf)
+{
+       return &_cdcd_itf[itf].rx_ff;
+}
+
+tu_fifo_t *tud_cdc_n_get_tx_fifo (uint8_t itf)
+{
+       return &_cdcd_itf[itf].tx_ff;
+}
 
 //--------------------------------------------------------------------+
 // READ API
diff --git a/src/class/cdc/cdc_device.h b/src/class/cdc/cdc_device.h
index 3c679c48..e9900356 100644
--- a/src/class/cdc/cdc_device.h
+++ b/src/class/cdc/cdc_device.h
@@ -28,6 +28,8 @@
 #define _TUSB_CDC_DEVICE_H_
 
 #include "common/tusb_common.h"
+#include "osal/osal.h"
+#include "common/tusb_fifo.h"
 #include "device/usbd.h"
 #include "cdc.h"
 
@@ -69,6 +71,12 @@ void     tud_cdc_n_get_line_coding (uint8_t itf, cdc_line_coding_t* coding);
 // Set special character that will trigger tud_cdc_rx_wanted_cb() callback on receiving
 void     tud_cdc_n_set_wanted_char (uint8_t itf, char wanted);
 
+// Get rx fifo ptr for external use (ex: DMA)
+tu_fifo_t *tud_cdc_n_get_rx_fifo (uint8_t itf);
+
+// Get tx fifo ptr for external use (ex: DMA)
+tu_fifo_t *tud_cdc_n_get_tx_fifo (uint8_t itf);
+
 // Get the number of bytes available for reading
 uint32_t tud_cdc_n_available       (uint8_t itf);

It builds but I need more time to use this in my code and see if it works.

5 replies

hathach May 19, 2021
Maintainer

I choose not to expose the API for now, could you keep the local modification and then provide feedback later on in regards of performance before vs after. If it is good enough, we could do further testing and decide if those APIs can be included.

mfp20 May 19, 2021
Author

provide feedback later on in regards of performance before vs after

ofc, I will report my results; but it is unlikely for me to conduct proper tests about performance.

I understand this is a quick and dirty solution; I mean: I have no idea what are your coding rules, design patterns and so on. But I can't make 2 versions of the same app and deploy the proper test bench.
Anyway, sometimes the "quick and dirty" can emerge as the best solution because of its simplicity so... please keep this mod into consideration for later use in the case you can't find a better hook for DMA or other external manipulations of USB buffers.

In the while time I added similar methods to export the mutex pointers too.

hathach May 19, 2021
Maintainer

for mutex, fifo read and write pointer is independent to each other. Therefore it will needs up to 2 mutex for read/write for each fifo. There are 2 within cdc (RX and TX). Depending on your application your will need 1 or 2.

mfp20 May 19, 2021
Author

for mutex, fifo read and write pointer is independent to each other. Therefore it will needs up to 2 mutex for read/write for each fifo. There are 2 within cdc (RX and TX). Depending on your application your will need 1 or 2.

Ah ok, I got it now: data mutex (and the exporting API) is in fifo's consumer code, not in fifo itself. The mutex in tu_fifo_t is for config only and there's already the config API.

Thanks

mfp20 May 21, 2021
Author

There are 2 within cdc (RX and TX).

I see the 2 mutex, but I don't see them in use. Looks all class drivers configure the mutex but don't acquire/relase later in code. Correct?

mfp20 · 2021-05-19T13:51:59Z

mfp20
May 19, 2021
Author

2 replies

hathach May 19, 2021
Maintainer

osal queue therefore osal.h need fifo.h, while fifo need mutex. In short they include each other, it has been an issue recently. I will resolve this with forward declaration along with revising the include hierarchy later on.

mfp20 May 19, 2021
Author

ah ok, sorry to bother. I wrongly guessed it could be useful to raise the issue.

PanRe · 2021-05-20T06:43:29Z

PanRe
May 20, 2021
Collaborator

I don't know if this was clear so far, in case you want to use the DMA, use the functions

void tu_fifo_get_read_info (tu_fifo_t *f, tu_fifo_buffer_info_t *info);
void tu_fifo_get_write_info(tu_fifo_t *f, tu_fifo_buffer_info_t *info);

to get the corresponding pointers and possible lengths to read/write. You must reduce the possible lenghts to your desired one (if you have any of course). As for audio class (i am not familiar with the details of CDC), you get a callback function called every time data is to be sent/received via USB interface or another callback function called when data was sent/received informing you that the corresponding FIFOs are empty now. You may add something like that into the CDC class driver to get informed when to update the DMA and when to advance the read/write pointers.
Currently, there is no example for such an approach available because me and @HiFiPhile who came up with this do not posses one of the demo boards used in the examples. We all work on our custom boards ^^.

2 replies

mfp20 May 21, 2021
Author

I don't have such functions in my local copy of TinyUSB; I'm using the default Raspberry Pi SDK, it has TinyUSB as a submodule. Probably can fetch newer tinyusb code in the pico sdk. Thanks for the tip.

EDIT: yes, pico's sdk uses a few commits after 0.7.0 and before 0.8.0. Those functions have been added after 0.8.0 and breaks APIs because it changes peek() too.

I hope the fifo, as well as eventual excessive development of usb class drivers, will be taken out of TinyUSB. In order to grant more freedom to developers. TinyUSB is a library, and its target are developers. Overdeveloping the library forces all adopters to practices that might not fit the role.
Don't get me wrong: it is good to have extended features in ready-to-use code. But might be better in an external project, so that TinyUSB can be adopted&adapted easier, then add the extended features on top if needed (by importing the other lib with extended features).
Just compare avr-libc, arduino, esp32 sdk, and RPi Pico sdk: avr and pico allow maximum freedom and efficency, esp32 and Aarduino don't because arduino and espressif opted for full featured libs in the sdk.

By forcing the tu_fifo_t as the only ring buffer, makes the things less flexible (and in fact, any new adopter needs to extend the existing APIs).

MrJake222 Oct 31, 2024

I don't know if this was clear so far, in case you want to use the DMA, use the functions
void tu_fifo_get_read_info (tu_fifo_t *f, tu_fifo_buffer_info_t *info);
void tu_fifo_get_write_info(tu_fifo_t *f, tu_fifo_buffer_info_t *info);

So it's still not possible because there is no way to extract either rx_ff or tx_ff from cdc_device.c?

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DMA example? (tu_fifo_t with DMA) #839

{{title}}

Replies: 4 comments 18 replies

{{title}}

{{title}}

{{editor}}'s edit

{{editor}}'s edit

{{title}}

{{editor}}'s edit

{{editor}}'s edit

{{title}}

{{title}}

{{title}}

{{editor}}'s edit

{{editor}}'s edit

{{title}}

{{editor}}'s edit

{{editor}}'s edit

{{title}}

{{title}}

{{title}}

{{title}}

{{title}}

{{title}}

{{title}}

{{title}}

{{title}}

{{title}}

{{editor}}'s edit

{{editor}}'s edit

{{title}}

Select a reply

DMA example? (tu_fifo_t with DMA) #839

mfp20 May 18, 2021

Replies: 4 comments · 18 replies

hathach May 19, 2021 Maintainer

hathach May 19, 2021 Maintainer

mfp20 May 19, 2021 Author

hathach May 20, 2021 Maintainer

mfp20 May 20, 2021 Author

hathach May 20, 2021 Maintainer

mfp20 May 19, 2021 Author

hathach May 19, 2021 Maintainer

mfp20 May 19, 2021 Author

hathach May 19, 2021 Maintainer

mfp20 May 19, 2021 Author

mfp20 May 21, 2021 Author

mfp20 May 19, 2021 Author

hathach May 19, 2021 Maintainer

mfp20 May 19, 2021 Author

PanRe May 20, 2021 Collaborator

mfp20 May 21, 2021 Author

MrJake222 Oct 31, 2024

mfp20
May 18, 2021

Replies: 4 comments 18 replies

hathach
May 19, 2021
Maintainer

hathach May 19, 2021
Maintainer

mfp20 May 19, 2021
Author

hathach May 20, 2021
Maintainer

mfp20 May 20, 2021
Author

hathach May 20, 2021
Maintainer

mfp20
May 19, 2021
Author

hathach May 19, 2021
Maintainer

mfp20 May 19, 2021
Author

hathach May 19, 2021
Maintainer

mfp20 May 19, 2021
Author

mfp20 May 21, 2021
Author

mfp20
May 19, 2021
Author

hathach May 19, 2021
Maintainer

mfp20 May 19, 2021
Author

PanRe
May 20, 2021
Collaborator

mfp20 May 21, 2021
Author