Flood of CHN_DISCONNECTED events from USB Serial

Report here problems in any of ChibiOS components. This forum is NOT for support.
User avatar
FXCoder
Posts: 116
Joined: Sun Jun 12, 2016 4:10 am
Location: Sydney, Australia
Has thanked: 30 times
Been thanked: 30 times

Flood of CHN_DISCONNECTED events from USB Serial

Postby FXCoder » Thu Sep 06, 2018 8:37 am

When plugging and unplugging a USB cable the current usbcfg.c can create many sduSuspendHookI(...) calls to hal_serial_usb.c
These can come from the fall through of the switch statement cases for USB_EVENT_RESET, USB_EVENT_UNCONFIGURED and USB_EVENT_SUSPEND as the USB plug contacts make/unmake during plugging.

At the moment hal_serial_usb broadcasts a CHN_DISCONNECTED for each call to sduSuspendHookI(...) from usbfg.c and so there may be many CHN_DISCONNECTED events not balanced against CHN_CONNECTED events.

To reduce the CHN_DISCONNECTED events so that they are paired (mostly) with CHN_CONNECTED the serial USB driver can check if it is already suspended and not broadcast CHN_DISCONNECTED.
The below patch does that by checking the state of the queues.
This works although it is a bit "obtuse" so a more obvious solution might be warranted.

Code: Select all

Index: hal_serial_usb.c
===================================================================
--- hal_serial_usb.c   (revision 12237)
+++ hal_serial_usb.c   (working copy)
@@ -310,6 +310,8 @@
  */
 void sduSuspendHookI(SerialUSBDriver *sdup) {
 
+  if(bqIsSuspendedX(&sdup->ibqueue) && bqIsSuspendedX(&sdup->obqueue))
+    return;
   chnAddFlagsI(sdup, CHN_DISCONNECTED);
   bqSuspendI(&sdup->ibqueue);
   bqSuspendI(&sdup->obqueue);

User avatar
FXCoder
Posts: 116
Joined: Sun Jun 12, 2016 4:10 am
Location: Sydney, Australia
Has thanked: 30 times
Been thanked: 30 times

Re: Flood of CHN_DISCONNECTED events from USB Serial

Postby FXCoder » Thu Sep 13, 2018 4:09 am

Further updates to serial_usb...
1. In the case that there are insufficient buffers available to commence receive or continue to next receive packet the driver silently continues.
Proposed action is to add an assert so that the developer is aware of the issue and can increase SERIAL_USB_BUFFERS_NUMBER in halconf.h

2. A fault condition arises where a transmit is started but not passed a buffer.
This should raise an assert in development phase.

Code: Select all

Index: hal_serial_usb.c
===================================================================
--- hal_serial_usb.c   (revision 12255)
+++ hal_serial_usb.c   (working copy)
@@ -190,6 +190,7 @@
   if (!usbGetTransmitStatusI(sdup->config->usbp, sdup->config->bulk_in)) {
     /* Trying to get a full buffer.*/
     uint8_t *buf = obqGetFullBufferI(&sdup->obqueue, &n);
+    osalDbgAssert(buf != NULL, "full output buffer not found in queue");
     if (buf != NULL) {
       /* Buffer found, starting a new transaction.*/
       usbStartTransmitI(sdup->config->usbp, sdup->config->bulk_in, buf, n);
@@ -310,6 +311,8 @@
  */
 void sduSuspendHookI(SerialUSBDriver *sdup) {
 
+  if(bqIsSuspendedX(&sdup->ibqueue) && bqIsSuspendedX(&sdup->obqueue))
+    return;
   chnAddFlagsI(sdup, CHN_DISCONNECTED);
   bqSuspendI(&sdup->ibqueue);
   bqSuspendI(&sdup->obqueue);
@@ -348,7 +351,8 @@
   obqResetI(&sdup->obqueue);
   bqResumeX(&sdup->obqueue);
   chnAddFlagsI(sdup, CHN_CONNECTED);
-  (void) sdu_start_receive(sdup);
+  bool r = sdu_start_receive(sdup);
+  osalDbgAssert(!r, "failed start receive");
 }
 
 /**
@@ -507,7 +511,8 @@
   /* The endpoint cannot be busy, we are in the context of the callback,
      so a packet is in the buffer for sure. Trying to get a free buffer
      for the next transaction.*/
-  (void) sdu_start_receive(sdup);
+  bool r = sdu_start_receive(sdup);
+  osalDbgAssert(!r, "failed setup for next receive");
 
   osalSysUnlockFromISR();
 }

User avatar
Giovanni
Site Admin
Posts: 11152
Joined: Wed May 27, 2009 8:48 am
Location: Salerno, Italy
Has thanked: 391 times
Been thanked: 322 times
Contact:

Re: Flood of CHN_DISCONNECTED events from USB Serial

Postby Giovanni » Thu Sep 13, 2018 8:02 am

I am not sure if an assertion is the right choice, the conditions should be handled at runtime.

Giovanni

User avatar
FXCoder
Posts: 116
Joined: Sun Jun 12, 2016 4:10 am
Location: Sydney, Australia
Has thanked: 30 times
Been thanked: 30 times

Re: Flood of CHN_DISCONNECTED events from USB Serial

Postby FXCoder » Thu Sep 13, 2018 8:58 am

Perhaps.
1. obnotify
Given that obnotify is the callback from obqueue then it would seem reasonable to catch a lack of a buffer being available with an assert.
The lack of a buffer being there from from a hal_buffers callback would indicate something is wrong in hal_buffers.
i.e. the posting and the callback are carried out within obqPostFullBufferS(...) so maybe this is a moot point.

2. sduDataReceived
This is called at ISR level from the USB driver and is a callback (setup in a USBEndpointConfig in usb_cfg).
I guess this error could be caught at runtime by a flag/state change in hal_serial_usb.
I'm not sure what the appropriate action would be.
Perhaps a chnAddFlagsI(sdup, SD_QUEUE_FULL_ERROR) should be added as a minimum.
I have seen a thread lock/race condition happen during USB RX which may be related to the silent loss of incoming data.
Certainly increasing the SERIAL_USB_BUFFERS_NUMBER from 2 to 4 appears to have quelled that issue for me.

Thoughts?

Bob


Return to “Bug Reports”

Who is online

Users browsing this forum: No registered users and 4 guests