Flood of CHN_DISCONNECTED events from USB Serial Topic is solved

Report here problems in any of ChibiOS components. This forum is NOT for support.
User avatar
FXCoder
Posts: 384
Joined: Sun Jun 12, 2016 4:10 am
Location: Sydney, Australia
Has thanked: 180 times
Been thanked: 130 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: 384
Joined: Sun Jun 12, 2016 4:10 am
Location: Sydney, Australia
Has thanked: 180 times
Been thanked: 130 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: 14444
Joined: Wed May 27, 2009 8:48 am
Location: Salerno, Italy
Has thanked: 1074 times
Been thanked: 921 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: 384
Joined: Sun Jun 12, 2016 4:10 am
Location: Sydney, Australia
Has thanked: 180 times
Been thanked: 130 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

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

Re: Flood of CHN_DISCONNECTED events from USB Serial

Postby Giovanni » Mon Dec 31, 2018 10:59 am

bump

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

Re: Flood of CHN_DISCONNECTED events from USB Serial

Postby FXCoder » Mon Dec 31, 2018 2:43 pm

Hi,
I left the code which blocks additional CHN_DISCONNNECTED events by checking the state of the queue.
This looks to be a suitable means of stopping "unbalanced" DISCONNECT/CONNECT events happening.

WRT receive buffers I've increased the SERIAL_USB_BUFFERS_NUMBER from from 2 to 3 (per SDU) in halconf.h
This seems quite stable and tolerates lots of plugging and unplugging generally without drama.

There are occasional USB hang ups with a shell handler (console system) which happen and are still TBI.
Not really a priority though.

So here is the patch (along with a typo fix)...

Code: Select all

Index: hal_serial_usb.c
===================================================================
--- hal_serial_usb.c   (revision 12499)
+++ 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);
@@ -513,7 +515,7 @@
 }
 
 /**
- * @brief   Default data received callback.
+ * @brief   Default data transmitted callback.
  * @details The application must use this function as callback for the IN
  *          interrupt endpoint.
  *


--
Bob

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

Re: Flood of CHN_DISCONNECTED events from USB Serial

Postby Giovanni » Mon Dec 31, 2018 6:20 pm

Hi,

I added the suspend check but looking at onotify() I don't remember how a buffer can be not found there (the NULL case).

That callback is called after posting a buffer in the queue (from obqPostFullBufferS()), so it should not be possible to get NULL from obqGetFullBufferI() (still there is a check for that there...).

Instead of an "if", it should be an assertion as you proposed, it should simply not happen, right now I am failing to see how it does actually happens.

Giovanni

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

Re: Flood of CHN_DISCONNECTED events from USB Serial

Postby FXCoder » Tue Jan 01, 2019 12:08 pm

Hi,
Agreed that obnotify() should never be called from hal_buffers without a buffer being available.
So buf being NULL should never be the case normally.

The same applies to sduSOFHookI().
If a partially filled buffer is queued then obqGetFullBufferI() cannot return NULL.

There is an assert already in place in sduSOFHookI().
Given an assert would only happen on memory corruption of the queue object should they be there in both functions or not at all?

--
Bob

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

Re: Flood of CHN_DISCONNECTED events from USB Serial  Topic is solved

Postby Giovanni » Tue Jan 01, 2019 1:07 pm

I will put the assertion and remove the "if".

Giovanni

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

Re: Flood of CHN_DISCONNECTED events from USB Serial

Postby Giovanni » Sat Jan 05, 2019 4:28 pm

Done, please let me know if you ever see that assertion triggered.

Code: Select all

  /* Checking if there is already a transaction ongoing on the endpoint.*/
  if (!usbGetTransmitStatusI(sdup->config->usbp, sdup->config->bulk_in)) {
    /* Getting a full buffer, a buffer is available for sure because this
       callback is invoked when one has been inserted.*/
    uint8_t *buf = obqGetFullBufferI(&sdup->obqueue, &n);
    osalDbgAssert(buf != NULL, "buffer not found");
    usbStartTransmitI(sdup->config->usbp, sdup->config->bulk_in, buf, n);
  }


Giovanni


Return to “Bug Reports”

Who is online

Users browsing this forum: No registered users and 12 guests