Improvements for dcd_transdimension by HiFiPhile · Pull Request #1062 · hathach/tinyusb

HiFiPhile · 2021-08-31T10:06:05Z

Describe the PR
1st part of #926.

Add xfer_fifo support, fifo must aligned to 4k boundary and size must be multiple of 4k.
Add dcd_edpt_close and iso xfer.

hathach

thank you for your great PR as usual. Though can you explain a bit for the usage of ptr wrap in the qtd_init_fifo()

hathach · 2021-09-08T10:22:33Z

+    // Set linear part
+    uint8_t i = 1;
+    for(; i<5; i++)
+    {
+      if (len_lin <= 0) break;
+      p_qtd->buffer[i] |= tu_align4k( p_qtd->buffer[i-1] ) + 4096;
+      len_lin -= 4096;
+    }
+    // Set wrapped part
+    for(uint8_t page = 0; i<5; i++, page++)
+    {
+      p_qtd->buffer[i] |= (uint32_t) info->ptr_wrap + 4096 * page;
+    }


I don't quite understand these lines, are you trying to queue both linear and wrap buffer into one TD ? Which I don't think is possible, we probably need 2 TDs for this.

Yes I'm trying to queue both parts in 5*4kB TDs. Fifo need to be 4k aligned and has a size of multiple of 4K, for a max of 20K.
TD[0] can start at a arbitrary address and TD[1]-TD[4] must start at 4k boundary.

Take an example of a 8k fifo filled between 2000-100:

TD[0] is set to offset of fifo->ptr_lin, then each TD is aligned to 4k until len_lin is finished.

Then it wrap to top and start at offset 0.

I think you mis-read the TD specs, within this function, it only preps 1 TD. The buffer[] page array is only used when the total bytes is large enough and memory cross the 4k boundary e.g when total bytes is 20 KB.

within this function, it only preps 1 TD

You're right.

In fact I mean 5 pointers within the same TD, sorry for the confusion.

we probably need 2 TDs for this.

I've tried 2 TDs, one for linear and one for wrapped part. In this case it cut the data into 2 separate transfers, for example cdc will receive half data.

we can transfer 1 TD after another instead of using 2 TDs at once. Can you update the above code to initialize the buffer page correctly. It should only affected by ptr_lin, and is mostly identical to existing qtd_init(). In fact, I think we should just use the qtd_init(ptr_lin, len) instead of having additional qtd_init_fifo()

the requirement with 4K aligned is too specific to make its way into the buffer page array set up. And this does not work with other MCU dcd.

Yes it is specific, so I've introduced a on/off switch in #1063 to not enable fifo_xfer by default, except explicitly.

For ISO, you have to understand this limit and need to declare the fifo depth to be multiple of ISO frame.

That's won't work since sample length could be +-1, wrapped part has to be always considered.

Yes it is specific, so I've introduced a on/off switch in #1063 to not enable fifo_xfer by default, except explicitly.

I don't like the TUD_AUDIO_PREFER_RING_BUFFER, it is only temporarily solution, we will eventual remove that.

That's won't work since sample length could be +-1, wrapped part has to be always considered.

Apparently, I am not aware of this issue. In this case, can you modify to make it more generic. xfer_fifo() will make use of qtd_init() with the linear part first. Then update the buffer pagep[] with wrapped part if following conditions is met

Linear len < total _bytes

wrap pointer is 4k alignment.

Then update the buffer pagep[] with wrapped

you mean modify buffer pagep[] inside xfer_fifo() and remove qtd_init_fifo() ?

it is mostly to reduce code duplication, for now, just keep it as it is if it is too confused. Only set up the buffer page only above 2 condition are met. currently wrapped part is always set up.

@HiFiPhile to clarify: not all edpt_xfer_fifo() need to combine linear + wrapped part, e.g CDC should have no issue breaking these into 2 sub transfers (one after another when USB complete ISR). Therefore not all fifo buffer need to be 4K. It is mostly for ISO applcation/driver, so having a 4K check would fit both.

HiFiPhile · 2021-09-09T14:54:56Z

+  //------------- Prepare qtd -------------//
+
+  // In case of : wrapped part is present & buffer is aligned to 4k & buffer size is multiple of 4k
+  if (total_bytes > fifo_info.len_lin && (uint32_t)fifo_info.ptr_wrap == tu_align4k((uint32_t)fifo_info.ptr_wrap) && tu_fifo_depth(ff) == tu_align4k(tu_fifo_depth(ff)))


I've add the switch, should be good but haven't tested. (better to have a macro bool is_aligned_4k(x))

yeah, the condition look good, you can use the !tu_offset4k() as is_aligned_4k(), which make it easier to read.

…e/tinyusb

hathach · 2021-09-12T10:55:54Z

@HiFiPhile I am doing resolving conflict and refactor this pr just now. Please hold on a bit if you plan to work more on this

hathach

Thank you for your effort adding the xfer_fifo. I am happy with the refactored code now, please verify if it works with your ISO setup. Once it is confirmed, we could merge it and resolve other case (total > len and buffer is not 4k aligned) later on when needed.

HiFiPhile · 2021-09-14T09:57:55Z

Now UAC2 is broken with no transfer, maybe forget to reset EP busy flag in usbd_edpt_close ?

But I don't remember busy was set to false before...

USBD Setup Received 01 0B 03 00 01 00 00 00
  Set Interface
  AUDIO control request
  Set itf: 1 - alt: 3
  Open EP 01 with Size = 776
  Queue EP 01 with 776 bytes ...
  Open EP 81 with Size = 4
  Queue EP 81 with 4 bytes ...
  Queue EP 80 with 0 bytes ...
USBD Xfer Complete on EP 80 with 0 bytes

USBD Setup Received 01 0B 00 00 01 00 00 00
  Set Interface
  AUDIO control request
  Set itf: 1 - alt: 0
  CLOSING Endpoint: 0x01
  CLOSING Endpoint: 0x81
  Queue EP 80 with 0 bytes ...
USBD Xfer Complete on EP 80 with 0 bytes

USBD Setup Received 21 01 00 01 00 04 04 00
  AUDIO control request
  Queue EP 00 with 4 bytes ...
USBD Xfer Complete on EP 00 with 4 bytes
  0000:  00 77 01 00                                      |.w..|
  AUDIO control complete
Clock set current freq: 96000
  Queue EP 80 with 0 bytes ...
USBD Xfer Complete on EP 80 with 0 bytes

USBD Setup Received 01 0B 03 00 01 00 00 00
  Set Interface
  AUDIO control request
  Set itf: 1 - alt: 3
  Open EP 01 with Size = 776
  Queue EP 01 with 776 bytes ...
usbd_edpt_xfer 1267: ASSERT FAILED
  Queue EP 80 with 0 bytes ...
USBD Xfer Complete on EP 80 with 0 bytes

hathach · 2021-09-14T14:37:35Z

@HiFiPhile which audio example you are running

HiFiPhile · 2021-09-14T15:08:14Z

which audio example you are running

I've tested both uac2_headset and my own config.

Somehow EP flags are not reset when closed.

With xfer_fifo enabled it fails for the same reason:

USBD Setup Received A1 01 01 01 00 02 01 00
  AUDIO control request
Get channel 1 mute 0
  Queue EP 80 with 1 bytes ...
USBD Xfer Complete on EP 80 with 1 bytes
  AUDIO control complete
  Queue EP
USBD Setup Received 01 0B 01 00 01 00 00 00
  Set Interface
  AUDIO control request
  Set itf: 1 - alt: 1
  Open EP 01 with Size = 52
Set interface 1 alt 1
  Queue ISO EP 01 with 52 bytes ... OK
  Queue EP 80 with 0 bytes ...
USBD Xfer Complete on EP 80 with 0 bytes

USBD Setup Received 01 0B 00 00 01 00 00 00
  Set Interface
  AUDIO control request
  Set itf: 1 - alt: 0
  CLOSING Endpoint: 0x01
  Queue EP 80 with 0 bytes ...
USBD Xfer Complete on EP 80 with 0 bytes

USBD Setup Received 01 0B 01 00 01 00 00 00
  Set Interface
  AUDIO control request
  Set itf: 1 - alt: 1
  Open EP 01 with Size = 52
Set interface 1 alt 1
  Queue ISO EP 01 with 52 bytes ... usbd_edpt
USBD Setup Received 01 0B 00 00 01 00 00 00
  Set Interface
  AUDIO control request
  Set itf: 1 - alt: 0
  CLOSING Endpoint: 0x01
  Queue EP 80 with 0 bytes ...
USBD Xfer Complete on EP 80 with 0 bytes

USBD Setup Received 01 0B 01 00 01 00 00 00
  Set Interface
  AUDIO control request
  Set itf: 1 - alt: 1
  Open EP 01 with Size = 52
Set interface 1 alt 1
  Queue ISO EP 01 with 52 bytes ... usbd_edpt_xfer_fifo 1299: ASSERT FAILED
  Queue EP 80 with 0 bytes ...
USBD Xfer Complete on EP 80 with 0 bytes

Could be fixed by adding:

void usbd_edpt_close(uint8_t rhport, uint8_t ep_addr)
{
  TU_ASSERT(dcd_edpt_close, /**/);
  TU_LOG2("  CLOSING Endpoint: 0x%02X\r\n", ep_addr);

  uint8_t const epnum = tu_edpt_number(ep_addr);
  uint8_t const dir   = tu_edpt_dir(ep_addr);

  dcd_edpt_close(rhport, ep_addr);
  + _usbd_dev.ep_status[epnum][dir].stalled = false;
  + _usbd_dev.ep_status[epnum][dir].busy = false;
  + _usbd_dev.ep_status[epnum][dir].claimed = false;

  return;
}

hathach · 2021-09-14T17:58:31Z

@HiFiPhile yeah, I think this is the right fix, closing endpoint should also reset its state. Please make the commit if possible

void usbd_edpt_close(uint8_t rhport, uint8_t ep_addr)
{
  TU_ASSERT(dcd_edpt_close, /**/);
  TU_LOG2("  CLOSING Endpoint: 0x%02X\r\n", ep_addr);

  uint8_t const epnum = tu_edpt_number(ep_addr);
  uint8_t const dir   = tu_edpt_dir(ep_addr);

  dcd_edpt_close(rhport, ep_addr);
  + _usbd_dev.ep_status[epnum][dir].stalled = false;
  + _usbd_dev.ep_status[epnum][dir].busy = false;
  + _usbd_dev.ep_status[epnum][dir].claimed = false;

  return;
}

hathach · 2021-09-15T06:02:52Z

@HiFiPhile let me know if the rest of your testing is going well enough

HiFiPhile · 2021-09-15T09:04:30Z

It goes well in my side, you can merge it.

hathach · 2021-09-15T09:07:02Z

It goes well in my side, you can merge it.

great, thank you very much for the PR and testing

HiFiPhile added 2 commits August 31, 2021 11:59

dcd_transmission xfer_fifo support.

864d838

add dcd_edpt_close and iso xfer.

16b802d

hathach reviewed Sep 8, 2021

View reviewed changes

dcd_edpt_xfer_fifo: use qtd_init if restriction not met.

283783c

HiFiPhile commented Sep 9, 2021

View reviewed changes

HiFiPhile and others added 2 commits September 10, 2021 11:17

Shorter expr.

580893b

Merge branch 'fifo_dcd_transdimension' of https://github.com/HiFiPhil…

038ec34

…e/tinyusb

refactor nxp TDI dcd_edpt_xfer_fifo

9bed4e2

hathach force-pushed the fifo_dcd_transdimension branch from cc454bb to 9bed4e2 Compare September 12, 2021 17:46

hathach reviewed Sep 12, 2021

View reviewed changes

Comment thread src/portable/nxp/transdimension/dcd_transdimension.c Outdated

clean up

43e6555

hathach approved these changes Sep 13, 2021

View reviewed changes

Fix warning.

3639168

Reset EP flags on close.

0ded1c5

hathach merged commit 03866dd into hathach:master Sep 15, 2021

HiFiPhile mentioned this pull request Oct 1, 2021

CDC class dcd_transdimension improvements #926

Closed

HiFiPhile deleted the fifo_dcd_transdimension branch April 1, 2024 10:15

Uh oh!

Conversation

HiFiPhile commented Aug 31, 2021

Uh oh!

hathach left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hathach Sep 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hathach commented Sep 12, 2021

Uh oh!

Uh oh!

hathach left a comment

Choose a reason for hiding this comment

Uh oh!

HiFiPhile commented Sep 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hathach commented Sep 14, 2021

Uh oh!

HiFiPhile commented Sep 14, 2021

Uh oh!

hathach commented Sep 14, 2021

Uh oh!

hathach commented Sep 15, 2021

Uh oh!

HiFiPhile commented Sep 15, 2021

Uh oh!

hathach commented Sep 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

hathach Sep 8, 2021 •

edited

Loading

HiFiPhile commented Sep 14, 2021 •

edited

Loading