Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ZLP not sent in USB device #15384

Closed
daniel-starke opened this issue Feb 24, 2023 · 8 comments
Closed

ZLP not sent in USB device #15384

daniel-starke opened this issue Feb 24, 2023 · 8 comments

Comments

@daniel-starke
Copy link
Contributor

Description of defect

Sending out a 64 byte buffer on USB CDC does not show up on the receiving side until more data with a length != 64 bytes is sent.
The USB protocol is a physical 1:1 half-duplex bus protocol with multiple channels (a.k.a. endpoints). That means only one side can send at a time. The device defines a buffer size for each endpoint. Any data larger than this needs to be split into multiple packets. To form a transaction and block the bus until end of the data transmission pakets with the maximum size for the endpoint are sent. The end of the transaction is signalled by sending a packet with less than the maximum size. A zero-length packet (ZLP) can be sent in case no more data is available but the complete data size was a multiple of the maximum endpoint buffer size.
STM32 does not automatically sends out ZLPs at the end of a trancaction as it has no knowledge about it. This needs to be done in the USB device driver. The user can not do so as the driver prevents transmissions of zero length. See line 402

void USBCDC::_send_isr_start()
{
assert_locked();
if (!_tx_in_progress && _tx_size) {
if (USBDevice::write_start(_bulk_in, _tx_buffer, _tx_size)) {
_tx_in_progress = true;
}
}
}

As this appears to be a USB protocol issue instead of a STM32 HAL specific one I would suggest solving it in

void USBDevice::in(usb_ep_t endpoint)
{
assert_locked();
if (!EP_INDEXABLE(endpoint)) {
#if MBED_TRAP_ERRORS_ENABLED
MBED_ERROR(
MBED_MAKE_ERROR(
MBED_MODULE_DRIVER_USB,
MBED_ERROR_CODE_INVALID_INDEX
),
"The endpoint is not indexable."
);
#else
return;
#endif // MBED_TRAP_ERRORS_ENABLED
}

However, I am not aware of how other device platforms handle this. It may be necessary to send a ZLP if the last packet was a multiple of the maximum endpoint buffer size and track its transmission state in

void HAL_PCD_DataInStageCallback(PCD_HandleTypeDef *hpcd, uint8_t epnum)
{
USBPhyHw *priv = ((USBPhyHw *)(hpcd->pData));
uint8_t endpoint = LOG_IN_TO_EP(epnum);
priv->epComplete[EP_TO_IDX(endpoint)] = 1;
if (epnum) {
priv->events->in(endpoint);
} else {
priv->events->ep0_in();
}
}

Target(s) affected by this defect ?

STM32

Toolchain(s) (name and version) displaying this defect ?

PlatformIO latest version

What version of Mbed-os are you using (tag or sha) ?

mbed-os-6.15.0

What version(s) of tools are you using. List all that apply (E.g. mbed-cli)

PlatformIO latest version

How is this defect reproduced ?

Send out 64 bytes after a client connects to a USB CDC port. Read from it on the client side (e.g. by opening the serial device with PuTTY). The 64 bytes of data should show up but don't.

@0xc0170
Copy link
Contributor

0xc0170 commented Mar 2, 2023

cc @ARMmbed/team-st-mcd

@jeromecoutant
Copy link
Collaborator

Hi @daniel-starke
I think if you could propose a patch in USB drivers source and/or in STM port, it will help lot!
Thx

@daniel-starke
Copy link
Contributor Author

The easiest solution is to disallow any write with a size equal to the endpoint buffer size. However, this solution may not be desired.
I could create a correct patch for the STM port but only if it is clear that it needs to be fixed there and not in the generic driver part.

@cyliangtw
Copy link
Contributor

@daniel-starke , some USB device may have the capability to detcect packet size & auto send ZLP.
So, it's better to create the patch for STM port only instead of generic part.

@agausmann
Copy link
Contributor

agausmann commented Sep 6, 2023

I also encountered this, and put together a reproduction (#15451)

daniel-starke added a commit to daniel-starke/mbed-os that referenced this issue Nov 18, 2023
Fix issue ARMmbed#15384 by sending a zero-length packet if the packet sent had
the same size as the endpoint buffer size. This is needed to comply with
the USB protocol which assumes an ongoing multi-packet USB transaction
otherwise.

STM32 HAL PCD handle parameters are used to avoid data duplication in RAM.

Signed-off-by: Daniel Starke <daniel-email@gmx.net>
@jeromecoutant
Copy link
Collaborator

Hi

@cyliangtw @agausmann Could you have a look on #15465 ?

Thx

@cyliangtw
Copy link
Contributor

Hi

@cyliangtw @agausmann Could you have a look on #15465 ?

Thx

About usbd control packet, https://github.com/ARMmbed/mbed-os/blob/72f27cee92a4bdafa30513f732e007ba635dc93f/drivers/usb/source/USBDevice.cpp#L262C4-L262C4 could handle & send ZLP.

About usbd non-control packet, it's possible to wait ZLP.
However let H/W driver auto send ZLP, it's possible double ZLP in control packet case and may impact the next USBH IN token.
So, I revised my opinion, let USBCDC to determine whether or not to send ZLP should be the better choice.

@daniel-starke
Copy link
Contributor Author

Pull request #15473 has been merged. Therefore, this issue can be closed.

@0xc0170 0xc0170 closed this as completed Dec 28, 2023
Issue Workflow automation moved this from Needs Triage to Done Dec 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

No branches or pull requests

6 participants