Skip to content

USBD CDC package maybe splited when using SerialUSB.write() #2372

Open
@yp05327

Description

@yp05327

As the title, I noticed that sometimes the output buffer will be splited into two bulk packages.
e.g. if I send [1, 2, 3, 4, 5, 6, 7] by using SerialUSB.write(), sometimes the output package will be [1, 2, 3, 4] and [5, 6, 7], not [1, 2, 3, 4, 5, 6, 7].

Then I found that, in cdc_queue.c, we are using TransmitQueue to handle the output buffer, maybe for avoiding overflow? Not sure about this.

And for TransmitQueue, actually, it is a ring buffer. So when write pointer < read pointer, which means something like this:
[x4 x5 x6 x7 0 0 ...... 0 0 x0 x1 x2 x3]
x(n) means the output buffer we provided, 0 means no-related bytes.

Then the current approach will run USBD_LL_Transmit twice to send the whole output buffer.
first time, we send [x0 x1 x2 x3]
then, we send [x4 x5 x6 x7]
(it seems that each of them are called block in the code, I will call them block bellow)

Function call:
USBSerial::write
|-> CDC_TransmitQueue_Enqueue
|-> CDC_continue_transmit
|-> |-> CDC_TransmitQueue_ReadBlock
|-> |-> USBD_CDC_SetTxBuffer
|-> |-> USBD_CDC_TransmitPacket
|-> |-> |-> USBD_LL_Transmit

Before we send the data, as the comment in L980, the following code will set the total length of the packet.

/* Update the packet total length */
pdev->ep_in[CDCInEpAdd & 0xFU].total_length = hcdc->TxLength;

But it seems that the total length is incorrect. It is same to the length of the block.
So two packets will be sent instead of one as expected.

For the solusion, I'm using USBD_LL_Transmit directly, instead of using the ring buffer TransmitQueue.
And it works as expected now, so I think there's a bug in the output buffer handling approach.

Desktop (please complete the following information):

  • OS: Windows
  • Arduino Cli: 0.34.2
  • STM32 core version: 2.7.1

Board (please complete the following information):

  • Name: Nucleo H753ZI

Activity

fpistm

fpistm commented on May 28, 2024

@fpistm
Member

Hi @yp05327
Thanks for this detailed report.
I will not have time soon to check this, do not hesitate to provide a fix if you have it. 😉

yp05327

yp05327 commented on May 28, 2024

@yp05327
ContributorAuthor

Actually, I don't have. I'm not a pro developer of embedded system, so I have no knowledge about what embedded system engineers will do to fix it.
Just a discussion, maybe we can provide a new buffer when write pointer < read pointer, but this may increase the memory usage. 😕

warmonkey

warmonkey commented on Nov 3, 2024

@warmonkey
Contributor

The behavior is CORRECT nothing need to be fixed. CDC is emulating serial port behavior which is a bytestream with correct ordering, no lost byte or duplication. packet boundary does not exist in serial port nor CDC.

yp05327

yp05327 commented on Nov 4, 2024

@yp05327
ContributorAuthor

I also have Arduino Due, but the behavior is different.
Maybe you are right, I'm not sure which is correct, as I said above I'm not a pro developer of embedded system.

But I can image this:
If it will be split into 2 packets (or maybe more?) randomly, then it will increase the usage of transport?
As there will be more header bytes for these additional packages.
So will this decrease the transport speed?

warmonkey

warmonkey commented on Nov 5, 2024

@warmonkey
Contributor

I also have Arduino Due, but the behavior is different. Maybe you are right, I'm not sure which is correct, as I said above I'm not a pro developer of embedded system.

But I can image this: If it will be split into 2 packets (or maybe more?) randomly, then it will increase the usage of transport? As there will be more header bytes for these additional packages. So will this decrease the transport speed?

If u write the cdc serial fast enough there's no splitting.

yp05327

yp05327 commented on Nov 5, 2024

@yp05327
ContributorAuthor

No, IIRC, the splitting is caused by TransmitQueue. It is a ring buffer, so:
It will always transform all bytes from read pointer to the end of the array defined in TransmitQueue
(of cause, your data should be long enough, or it will only simply send all of them)
Then, in the next turn (package), send the rest of your data from the start of the array to write pointer.
Why I said randomly because it is depends on the length of your data and the value of read pointer and the length of TransmitQueue.
Actually, it is not real randomly happen.
Unless you send bytes one by one, I think you can reproduce it, no matter how fast you send the data by using SerialUSB.write()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      No branches or pull requests

        Participants

        @warmonkey@yp05327@fpistm

        Issue actions

          USBD CDC package maybe splited when using SerialUSB.write() · Issue #2372 · stm32duino/Arduino_Core_STM32