OTGv1: RX transaction size is invalid during callback.

[Kosyak]

Hello!

During improving and porting my isochronous USB example to new USB API, I've found problem with usbGetReceiveTransactionSizeX() function.

For STM32 this function looks like

Code: Select all

#define usb_lld_get_transaction_size(usbp, ep)                              \
  ((usbp)->epc[ep]->out_state->rxcnt)

So, as you can see, it returns rxcnt from endpoint state.

Now, here's simplified version of how STM32 driver works:

• Interrupt comes from USB core
• IRQ handler clears interrupt registers, resumes driver's pump thread, calls OUT callback
• ...Some time...
• Driver's pump thread resumed
• Data pumped from RX FIFO to buffer, rxcnt updated

So, as you can see, rxcnt updated only after data is being pumped out to RX buffer. That sounds quite reasonable, but results problems in some cases. We can't use usbGetReceiveTransactionSizeX() in OUT callback.

First, in case of isoc transfers, I have to start next transaction right after current transaction finished/failed (i.e. in IRQ handler / callback). If I'm using some kind of buffer, I need information about how much data was received (and will be pumped into buffer, isoc transfers can vary in size) in current transaction to get next buffer pointer. I can't do that now.

Second, new synchronous (usbReceive()) USB API isn't working properly (if I'm not missing something important). Look at code called during IRQ handler:

Code: Select all

#define _usb_isr_invoke_out_cb(usbp, ep) {                                  \
  (usbp)->receiving &= ~(1 << (ep));                                        \
  if ((usbp)->epc[ep]->out_cb != NULL) {                                    \
    (usbp)->epc[ep]->out_cb(usbp, ep);                                      \
  }                                                                         \
  osalSysLockFromISR();                                                     \
  osalThreadResumeI(&(usbp)->epc[ep]->out_state->thread,                    \
                    usbGetReceiveTransactionSizeX(usbp, ep));               \
  osalSysUnlockFromISR();                                                   \
}

That code has issues:

• usbReceive() will return invalid size, because usbGetReceiveTransactionSizeX() is called in IRQ handler.
• If driver's pump thread will have lower priority than user thread (in which usbReceive() was called) usbReceive() will return before data actually being pumped into buffer.

One way to (half-)fix that - somehow get transacrion size in IRQ handler, but I can't find any way to get RX size before reading data from RX FIFO at F4. My intuition says that XFRSIZ field in OTG_FS_DOEPTSIZx register should store real transfer size after it has been received, but I can find any proof for that. But this will not help in case of sync. API.

Another way - make additional callback, that's called only after data has actually been pumped out to buffer, and use it for synchronous API. But this will not help in case of isoc transactions.

What do think about that?

[Giovanni]

You just nuked my weekend

I need some time to analyze.

Giovanni

[Giovanni]

Hi,

The receive procedure works as follow:

- RX FIFO interrupt comes, the pump thread is activated, this interrupt does not call the OUT callback.
- The thread copies data and updates rxcnt accordingly.
- At transfer end an OUT endpoint interrupt is triggered, this one calls the OUT callback, rxcnt should be consistent at this point.

There could be an issue but it should not happen how you described it.

I see a potential problem here:

Code: Select all

    /* A short packet always terminates a transaction.*/
    if (((osp->rxcnt % usbp->epc[ep]->out_maxsize) == 0) &&
        (osp->rxsize < osp->totsize)) {
      /* In case the transaction covered only part of the total transfer
         then another transaction is immediately started in order to
         cover the remaining.*/
      osp->rxsize = osp->totsize - osp->rxsize;
      osp->rxcnt  = 0;   <-- I SUSPECT THIS SHOULD BE REMOVED
      osalSysLockFromISR();
      usb_lld_start_out(usbp, ep);
      osalSysUnlockFromISR();
    }
    else {
      /* End on OUT transfer.*/
      _usb_isr_invoke_out_cb(usbp, ep);
    }

If the transaction is restarted then rxcnt is cleared, that would make usb_lld_get_transaction_size() return a wrong value, this can only happen for EP0 however. Are you sure usbGetReceiveTransactionSizeX() is returning a wrong value? it is used also in the CDC driver and apparently there are no issues that.

Try looking at your problem considering the procedure I described.

Other opinions from the USB driver contributors are welcomed, this is a complex matter.

Giovanni

[Kosyak]

Hi,

The receive procedure works as follow:

- RX FIFO interrupt comes, the pump thread is activated, this interrupt does not call the OUT callback.
- The thread copies data and updates rxcnt accordingly.
- At transfer end an OUT endpoint interrupt is triggered, this one calls the OUT callback, rxcnt should be consistent at this point.

There could be an issue but it should not happen how you described it.

Oh, my bad. The problem was in my code and testcase (periodical isoc. transfers and wrong data handling) - I was taking OUT endpoint interrupt from previous transaction as current. So, I was thinking that OUT endpoint interrupt coming before data moved from FIFO. I had to read reference manual more carefully.

I see a potential problem here:

Code: Select all

    /* A short packet always terminates a transaction.*/
    if (((osp->rxcnt % usbp->epc[ep]->out_maxsize) == 0) &&
        (osp->rxsize < osp->totsize)) {
      /* In case the transaction covered only part of the total transfer
         then another transaction is immediately started in order to
         cover the remaining.*/
      osp->rxsize = osp->totsize - osp->rxsize;
      osp->rxcnt  = 0;   <-- I SUSPECT THIS SHOULD BE REMOVED
      osalSysLockFromISR();
      usb_lld_start_out(usbp, ep);
      osalSysUnlockFromISR();
    }
    else {
      /* End on OUT transfer.*/
      _usb_isr_invoke_out_cb(usbp, ep);
    }

If the transaction is restarted then rxcnt is cleared, that would make usb_lld_get_transaction_size() return a wrong value, this can only happen for EP0 however.

Yeah, that part also worried me a bit. Although, I can't clear understand what it is doing.