USBCDC support ZLP #15473

cyliangtw · 2023-12-01T12:29:41Z

This PR is to support ZLP handling in USBCDC.
It could resolve #15451.

Pull request type

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[x] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers

daniel-starke · 2023-12-01T20:47:41Z

This does not work if there is no ongoing data stream but just some request/response interaction.
I suggest something like this:

--- drivers/usb/source/USBCDC.cpp	2023-02-23 15:00:40 +0000
+++ drivers/usb/source/USBCDC.cpp	2023-12-01 20:39:54 +0000
@@ -186,6 +186,7 @@
     _rx_in_progress = false;
     _rx_buf = _rx_buffer;
     _rx_size = 0;
+    _trans_zlp = false;
 }
 
 void USBCDC::callback_reset()
@@ -402,6 +403,9 @@
     if (!_tx_in_progress && _tx_size) {
         if (USBDevice::write_start(_bulk_in, _tx_buffer, _tx_size)) {
             _tx_in_progress = true;
+            if (_tx_size == CDC_MAX_PACKET_SIZE) {
+                _trans_zlp = true;
+            }
         }
     }
 }
@@ -414,6 +418,11 @@
 {
     assert_locked();
 
+    if (_trans_zlp && USBDevice::write_start(_bulk_in, _tx_buffer, 0)) {
+        _trans_zlp = false;
+        return;
+    }
+
     write_finish(_bulk_in);
     _tx_buf = _tx_buffer;
     _tx_size = 0;

Note that something similar needs to be done for USBCDC_ECM and maybe other devices.
P.S.: Tested with NUCLEO-H723ZG.

cyliangtw · 2023-12-05T11:35:44Z

@daniel-starke ,
thanks of your advice, I update it to send "ZLP write start" after last alignment packet sent.
More, USBCDC_ECM already handle ZLP at https://github.com/cyliangtw/mbed-os/blob/ca616c865acb59c6f6311952787f242831ced02f/drivers/usb/source/USBCDC_ECM.cpp#L226

daniel-starke · 2023-12-05T12:39:07Z

I believe that the condition for _trans_zlp = true; is still wrong as it does not appear to handle transfer continuation (e.g. send 63 byte without flush and 1 byte afterwards to a total of CDC_MAX_PACKET_SIZE). Important is the actual transfer size not any intermediate buffer handling size. See my diff above.

cyliangtw · 2023-12-06T11:47:10Z

@daniel-starke,
For accurency, to move the judgement of ZLP into USBCDC::AsyncWrite.
It will be the better choice. Then send_nb() with zero size to trigger trans-zlp.

daniel-starke · 2023-12-06T19:47:23Z

@cyliangtw
I will have a deeper look at it around the end of the week.
Your change basically moves the responsibility for multi-packet transactions from the driver to the user space. Which is ok in my opinion. However, this probably also requires an update for the USBCDC_ECM driver for homogeneity and definitely of the documentation. Most users do not have such in-depth knowledge about the USB protocol.
Also, what tests did you run with these changes? When I wrote my diff above I had troubles putting the ZPL transmission in _send_isr_start(). Which is why I moved it to _send_isr().

cyliangtw · 2023-12-07T06:32:03Z

Hi @daniel-starke,
In USBCDC_ECM, there is no async-write and already handle ZLP at USBCDC_ECM::send() /USBCDC_ECM.cpp#L226
In USBCDC, USBCDC::send() will invoke async-write. This PR was to follow original async-write mechansim and verified on 2 platforms M467 & M487, also verified the scenario as my expextation on transfer 64 bytes, 128 bytes, 127 bytes and so on.

ccli8

Verified on NUMAKER_IOT_M467/NUMAKER_IOT_M487 targets

0xc0170 · 2023-12-08T09:36:16Z

drivers/usb/source/USBCDC.cpp

@@ -641,4 +663,4 @@ const uint8_t *USBCDC::configuration_desc(uint8_t index)
    } else {
        return NULL;
    }
-}
+}


please add an empty line as it was

Thanks of your reminder.
I rollback the empty line.

daniel-starke · 2023-12-10T15:07:24Z

@cyliangtw
I can confirm that it works. Tested on NUCLEO-H723ZG.
Note that I still believe that send_nb() needs an update of its documentation as mentioned before.
send_nb() also appears to be broken for the continuation case. Someone may want to fix that:

--- drivers/usb/source/USBCDC.cpp  2023-12-10 15:37:58.259849800 +0100
+++ drivers/usb/source/USBCDC.cpp  2023-12-10 15:58:26.240938600 +0100
@@ -391,7 +391,7 @@
         uint32_t free = sizeof(_tx_buffer) - _tx_size;
         uint32_t write_size = free > size ? size : free;
         if (size > 0) {
-            memcpy(_tx_buf, buffer, write_size);
+            memcpy(_tx_buf + _tx_size, buffer, write_size);
         }
         _tx_size += write_size;
         *actual = write_size;

cyliangtw · 2023-12-11T02:25:34Z

@daniel-starke,
Thanks of your confirmation pass on NUCLEO-H723ZG.

About send_nb() memcpy possible break the continuation use case, I agree with your point and it's no matter with ZLP, you could add another PR to contribute your good viewpoint.
If you prefer to append this change into this ZLP PR, it's fine to me to add one more commit.

daniel-starke · 2023-12-11T13:29:37Z

@cyliangtw
Yes, feel free to add it here.

0xc0170 · 2023-12-13T13:56:15Z

CI started

mbed-ci · 2023-12-13T17:02:24Z

Jenkins CI Test : ❌ FAILED

Build Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs	Status
jenkins-ci/mbed-os-ci_build-cloud-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_unittests	✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM	✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_greentea-test	❌

cyliangtw · 2023-12-14T10:06:12Z

@0xc0170 ,
CI Test failed on NUCLEO_WB55RG at: storage-blockdevice-tests-tests-blockdevice-general_block_device | TIMEOUT | 1800.63 | default |
mbedgt: test suite results: 1 TIMEOUT

After review the update of USBCDC, seems unrelated with this PR.
Could send "CI start" again ?

mbed-ci · 2023-12-18T18:11:13Z

Jenkins CI Test : ❌ FAILED

Build Number: 4 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs	Status
jenkins-ci/mbed-os-ci_build-cloud-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_unittests	✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM	✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_greentea-test	❌

cyliangtw · 2023-12-19T02:02:26Z

I can't spot anything merged recently causing this, I'll check nightly and find out what is going on. Meanwhile, happy to merge this as it is.

@0xc0170 ,
The system seems abnormal, mergify added "need CI" and removed "need CI" repeatedly over 700 times.
Should I close this PR and new another PR to replace it ?

0xc0170 · 2023-12-19T15:31:43Z

Mergify sometimes find 2 conditions that are met and goes back and forth.. we might revisit the rules.

cyliangtw added 2 commits December 1, 2023 20:17

Update USBCDC.h to support ZLP

794ee97

Update USBCDC.cpp to support ZLP

1cccc7e

cyliangtw mentioned this pull request Dec 1, 2023

TARGET_STM: fix unterminated multi-packet USB transaction #15465

Closed

Update USBCDC.cpp to fulfill A style

e2aed18

Move "send ZLP write start" after last alignment packet sent

ca616c8

cyliangtw force-pushed the master branch from 92a0466 to 2bd5aab Compare December 6, 2023 11:40

ccli8 approved these changes Dec 7, 2023

View reviewed changes

0xc0170 added needs: review release-type: patch Indentifies a PR as containing just a patch labels Dec 8, 2023

0xc0170 reviewed Dec 8, 2023

View reviewed changes

To judge send ZLP or not in USBCDC::AsyncWrite

c30af6a

cyliangtw force-pushed the master branch from 2bd5aab to c30af6a Compare December 8, 2023 11:40

Avoid USBCDC send_nb break the continuation use case

126d767

0xc0170 approved these changes Dec 13, 2023

View reviewed changes

mergify bot added needs: CI and removed needs: review labels Dec 13, 2023

mergify bot added needs: work and removed needs: CI labels Dec 13, 2023

mergify bot added needs: work needs: CI and removed needs: work needs: CI labels Dec 18, 2023

mergify bot added needs: work and removed needs: CI labels Dec 18, 2023

0xc0170 merged commit 6403ec7 into ARMmbed:master Dec 19, 2023

0xc0170 removed the needs: work label Dec 19, 2023

daniel-starke mentioned this pull request Dec 25, 2023

ZLP not sent in USB device #15384

Closed

multiplemonomials mentioned this pull request Jul 20, 2024

Synchronize changes with Mbed upstream: July 2024 edition mbed-ce/mbed-os#309

Merged

USBCDC support ZLP #15473

USBCDC support ZLP #15473

Uh oh!

Conversation

cyliangtw commented Dec 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull request type

Test results

Reviewers

Uh oh!

daniel-starke commented Dec 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cyliangtw commented Dec 5, 2023

Uh oh!

daniel-starke commented Dec 5, 2023

Uh oh!

cyliangtw commented Dec 6, 2023

Uh oh!

daniel-starke commented Dec 6, 2023

Uh oh!

cyliangtw commented Dec 7, 2023

Uh oh!

ccli8 left a comment

Choose a reason for hiding this comment

Uh oh!

0xc0170 Dec 8, 2023

Choose a reason for hiding this comment

Uh oh!

cyliangtw Dec 8, 2023

Choose a reason for hiding this comment

Uh oh!

daniel-starke commented Dec 10, 2023

Uh oh!

cyliangtw commented Dec 11, 2023

Uh oh!

daniel-starke commented Dec 11, 2023

Uh oh!

0xc0170 commented Dec 13, 2023

Uh oh!

mbed-ci commented Dec 13, 2023

Jenkins CI Test : ❌ FAILED

Build Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

Uh oh!

cyliangtw commented Dec 14, 2023

Uh oh!

mbed-ci commented Dec 18, 2023

Jenkins CI Test : ❌ FAILED

Build Number: 4 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

Uh oh!

cyliangtw commented Dec 19, 2023

Uh oh!

0xc0170 commented Dec 19, 2023

Uh oh!

Uh oh!

cyliangtw commented Dec 1, 2023 •

edited

Loading

daniel-starke commented Dec 1, 2023 •

edited

Loading