• State Resolved
  • Locked Locked
  • Replies 16 replies
  • Answers 2 answers
  • Subscribers 30 subscribers
  • Views 610 views
  • Users 0 members are here
Support feedback
Options
Options
Similar topics

This thread has been locked.

If you have a related question, please click the "Ask a related question" button in the top right corner. The newly created question will be automatically linked to this question.

RTOS/TM4C129CNCZAD: USBBuffer in usblib doesn't account for overflow at all

Kaveh Vaghefi
Expert 1704 points
Part Number: TM4C129CNCZAD
Other Parts Discussed in Thread: EK-TM4C1294XL

Tool/software: TI-RTOS

I've been scratching my head as to why a buffered USB write behaves oddly when I put USBBuffer in front of my USBCDC implementation until I started looking at the code in usbbuffer.c in usblib.  As it turns out, while the buffer implementation there accounts for rollover of the index, it doesn't account for overflow....like at all.  Maybe I just missed it, but I can't find any references to overflow in the file.  Maybe I'm missing something, but this seems like a huge oversight for a buffer implementation.

I would have expected to see at least a callback with an event indicating a buffer overflow so the application can log it and flush the buffer because here's the thing, once you overflow the buffer implementation in usblib, it doesn't seem to ever recover without explicitly calling the flushing function.  The application would never no since no event/callback is triggered when this happens. 

  • 0
    TI__Guru*** 128125 points
    Hello Kaveh,

    The USB buffer is a Ring Buffer so Overflow is not something that happens with it, that is why there is no such implementation.
  • 0 in reply to Ralph Jacobi
    Expert 1704 points
    Huh?

    Any statically sized buffer can overflow if you write more data than it's capable of receiving to it. If I create a ring buffer of 1024 bytes and through the course of several calls to the write function write more than 1024 bytes, it will be incapable of accepting that new data.
  • 0
    TI__Guru 72500 points

    Perhaps I am not looking at the function you are using. The function USBBufferWrite() returns the number of bytes actually written. If there is not enough space in the transmit buffer, the value returned will be less than the value passed in the parameter ui32Length. Here are the comments in lines 793 through 797 of the file usbbuffer.c

    //! Attempts to send more data than there is space for in the transmit buffer
//! will result in fewer bytes than expected being written.  The value returned
//! by the function indicates the actual number of bytes copied to the buffer.
//!
//! \return Returns the number of bytes actually written.

  • 0 in reply to Kaveh Vaghefi
    TI__Guru*** 128125 points
    Hello Kaveh,

    The ring buffer implementation is that when it reaches 1024 bytes, it starts back at byte 0 again. That's the whole concept of it. There is not really an easy to define overflow concept with a ring buffer because it looks around. If your pointer for receiving new data is at byte 900, and you've already read out the data from 0-500, and you receive 200 more bytes, then you'd loop back around and be perfectly fine. But if you had the same situation and didn't read out the data from 0-500, you'd 'overflow' - but how would you know Case A from Case B?

    In the end, you have to set the ring buffers to be large enough to store data until you can read it out so you don't run into an 'overflow' condition, but there isn't going to be an easy way to just define that the buffer has overflowed due to the end-to-end unless you have data that is sorted in a way you could tell if a byte is wrong.

    If you are having trouble visualizing this, maybe the graphics in this Wiki article will help: en.wikipedia.org/.../Circular_buffer
  • 0 in reply to Bob Crosby
    TI__Guru*** 128125 points
    Hi Bob,

    Thanks for raising this, that is a great point to make - TivaWare API's are designed to give feedback about available buffer space. Another very useful API is USBBufferSpaceAvailable. Or when receiving, USBBufferDataAvailable.
  • 0 in reply to Bob Crosby
    Expert 1704 points
    USBBufferWrite is the API I use to send stuff to the buffer. Digging down deeper, the problem I seem to run into is when it calls ScheduleNextTransmission. The transmission happens, but nothing subsequently seems to update the index of the ring buffer (again when the buffer is full), so the buffer never recovers and the same set of data gets transmitted over and over again until there's an explicit call to flush the buffer.
  • 0 in reply to Ralph Jacobi
    Expert 1704 points

    Ralph Jacobi said:
    Hello Kaveh,

    The ring buffer implementation is that when it reaches 1024 bytes, it starts back at byte 0 again. That's the whole concept of it. There is not really an easy to define overflow concept with a ring buffer because it looks around. If your pointer for receiving new data is at byte 900, and you've already read out the data from 0-500, and you receive 200 more bytes, then you'd loop back around and be perfectly fine. But if you had the same situation and didn't read out the data from 0-500, you'd 'overflow' - but how would you know Case A from Case B?

    In the end, you have to set the ring buffers to be large enough to store data until you can read it out so you don't run into an 'overflow' condition, but there isn't going to be an easy way to just define that the buffer has overflowed due to the end-to-end unless you have data that is sorted in a way you could tell if a byte is wrong.

    If you are having trouble visualizing this, maybe the graphics in this Wiki article will help: en.wikipedia.org/.../Circular_buffer

    Hi Ralph,

    Thanks, but I'm quite familiar with how circular/ring buffers work.  What you're saying doesn't really seem correct though.  If you know the readIndex, writeIndex, and bufferSize, then with any call to USBBufferWrite, you could easily determine if the number of bytes being written will:

    1) surpass the size of the buffer (though I don't expect this to be the case and the buffer should be appropriately sized)

    2) cause the writeIndex to surpass the readIndex (accounting for rolling around the length of the entire buffer if needed)

    From what I've read so far, it sounds like I need to check the buffer before calling a write to it and account for fullness in the application rather than expecting the driver to do it.  

    Furthermore, what I mentioned in the previous post about how USBBufferWrite calls ScheduleNextTransmission still stands.  In fact the last comment in the ScheduleNextTransmission functions states

            //
        // Don't update the ring buffer read index yet.  We do this once we are
        // sure the packet was correctly transmitted.
        //

    The thing is, once that returns back to USBBufferWrite, the next operation is returning from USBBufferWrite.  The index never gets updated when the buffer is full and has zero space.  I must be missing something because this seems like it would never work.

  • 0 in reply to Kaveh Vaghefi
    TI__Guru*** 128125 points
    Hello Kaveh,

    Correct, I was more trying to explain why that not having an explicit overflow flag is not a flaw because of how ring buffers are supposed to work when handled properly. Your initial post was focused on buffer overflow, which is why I was trying to explain the reasoning behind why from a conceptual level we did not have such an implementation.

    Anyways, the back and forth isn't going to help solve your problem, hopefully this following information gets you headed in the right direction for that though.

    Regarding the topic at hand with the Index updating, the API is calling to the function that will actually handle the transfer. This function in your case of being a CDC device should be USBDCDCPacketWrite. The API ScheduleNextTransmission doesn't actually send the data, so it cannot update the index as it cannot tell if the data was sent correctly. You will see that USBDCDCPacketWrite has the handling for this.

    Looking back over to your first post now, I am wondering if you don't have the right setup. You mentioned putting USB Buffer in front of USBCDC, can you elaborate more on what you were trying to do there?
  • 0 in reply to Ralph Jacobi
    Expert 1704 points

    Invoking USBDCDCPacketWrite directly does work. According to the usblib user guide though, it should be possible to make this buffered and specify that write function as the `pfnTransferFxn` for the `tUSBBuffer` instance created for the buffer.

    USBDCDCPacketWrite doesn't depend on anything in usbbuffer.c so I don't know how it could know about the writeIndex/readIndex of a ring buffer it's not using. Searching around usbbuffer.c, it seems more likely that there's a code path I'm missing to call `USBRingBufAdvanceRead` where the readIndex actually gets updated.

    Is there a document that describes roughly how this library is supposed to work? I'm following spmu297d, and while it's generally pretty helpful, it seems like it's slightly out of date with the current implementation so I'm wondering if I'm just missing something.

  • 0 in reply to Kaveh Vaghefi
    TI__Guru*** 128125 points

    Hello Kaveh,

    The link you may be missing comes with the USB structs application file.

    A properly configured transmit buffer would look like this:

    //*****************************************************************************
//
// Transmit buffer (from the USB perspective).
//
//*****************************************************************************
uint8_t g_pui8USBTxBuffer[UART_BUFFER_SIZE];
tUSBBuffer g_sTxBuffer =
{
    true,                           // This is a transmit buffer.
	TxHandler,                      // pfnCallback
    (void *)&g_sCDCDevice,          // Callback data is our device pointer.
    USBDCDCPacketWrite,             // pfnTransfer
    USBDCDCTxPacketAvailable,       // pfnAvailable
    (void *)&g_sCDCDevice,          // pvHandle
    g_pui8USBTxBuffer,              // pui8Buffer
    UART_BUFFER_SIZE,               // ui32BufferSize
};

    And then the USBBufferInit(&g_sTxBuffer); API would then pass on this information into the buffer portion of the USB library and allow the use of the USB Buffer calls which would then make calls like USBDCDCPacketWrite wherever you see a psBuffer->pfnTransfer call in the usbbuffer file.

    This is how our TivaWare examples are setup when using the USBBuffer API's.

    If you want to play around with a TivaWare example for a single CDC port on the EK-TM4C1294XL, this is a project I put together a while ago - the comments aren't all up to the date but the functionality is there: 0640.usb_dev_cdcserial.zip

  • 0 in reply to Ralph Jacobi
    Expert 1704 points
    Yup, that's how mine is set up. It doesn't seem to be the root cause here.

    Tangentially, your example shows that the USB user guide is out of date as it declares the `tUSBBuffer` as `const` then proceeds to use `USBBufferInit()` on it, which results in a crash since attempts are made in the library to modify that data in the Init function.
  • 0 in reply to Kaveh Vaghefi
    TI__Guru*** 128125 points
    Hello Kaveh,

    Thanks for that mention, I'll see if we can get that updated in the User Guide.

    I've been pondering your situation but I haven't come up with an explanation yet. For debugging purposes, can you try checking the buffer status with the API's I had mentioned before like USBBufferSpaceAvailable? Also when you observe the issue, can you check what the return on USBBufferWrite is and compare it to the information you got from USBBufferSpaceAvailable? Maybe these experiments will help lead us towards the root cause.
  • 0 in reply to Ralph Jacobi
    Expert 1704 points

    I think I found the issue.  The tUSBDCDCDevice type has callbacks (pfnRxCallback and pfnTxCallback) that would typically be set to the Rx and Tx handlers supplied by the application.  I need to supply a pointer to USBBufferEventCallback from usblib for these callbacks to actually connect everything up properly.  Somehow I missed that earlier.

    static tUSBDCDCDevice logDevice = {
    .ui16VID = USB_VID_TI_1CBE,
    .ui16PID = USB_PID_COMP_SERIAL,
    .ui16MaxPowermA = 0,
    .ui8PwrAttributes = USB_CONF_ATTR_SELF_PWR,

    .pfnControlCallback = &LogControlCallback,
    .pvControlCBData = &logDevice,
    .pfnRxCallback = &USBBufferEventCallback,
    .pvRxCBData = (void*)&logRxBuffer,
    .pfnTxCallback = &USBBufferEventCallback,
    .pvTxCBData = (void*)&logTxBuffer,

    .ppui8StringDescriptors = stringDescriptors,
    .ui32NumStringDescriptors = STRINGDESCRIPTORSCOUNT,
};

    Thanks for being so responsive.

  • 0 in reply to Kaveh Vaghefi
    TI__Guru*** 128125 points
    Ahh, yes that would do it! I didn't realize you were using a different structure format so didn't think to ask about that piece. The TI-RTOS structures were basically taken straight out of TivaWare IIRC. Anyways, glad you found the issue and thanks for updating with the solution!
  • 0 in reply to Ralph Jacobi
    Expert 1704 points

    One more question. It seems like the USBBuffer.c functions in usblib aren't disabling interrupts when manipulating the buffer (with the exception of just updating the index). What's the expected use case TI was thinking about here when implementing these buffers? It seems like if you have multiple threads that attempt to write to it, you cannot guarantee the order of items in the buffer or that certain transactions to/from the buffer won't be interrupted.

    I was thinking that I wouldn't need to use a mailbox mechanism if I size the USB buffer sufficient, but if I'm not missing anything in my interpretation of the usblib code, I don't think I can.  Was the buffer provided simply to provide an easy way around the 64-byte payload size on USB?

  • 0 in reply to Kaveh Vaghefi
    TI__Guru*** 128125 points
    Hello Kaveh,

    I wasn't a developer for the USB library and so I don't have first hand knowledge of the thought processes behind the coding, but given the library was designed for the TM4C MCU's which are single core devices, and designed prior to the rise in popularity of RTOS systems, I don't believe there was any design intention to have it support a multi-thread situation like you described.

    As far as supporting a more complex interface than a simple USB device, the USB library was more designed to be able to support multiple pipes to allow the TM4C to function as a composite device than anything.