Skip to content

ESP32-S2 USB CDC On Boot - Serial input drops incoming data sent quickly from the PC #5727

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

Closed
ghost opened this issue Oct 2, 2021 · 15 comments · Fixed by #6413
Closed

ESP32-S2 USB CDC On Boot - Serial input drops incoming data sent quickly from the PC #5727

ghost opened this issue Oct 2, 2021 · 15 comments · Fixed by #6413
Assignees

Comments

@ghost
Copy link

ghost commented Oct 2, 2021

Hardware:

Board: ESP32-S2 Saola 1R Dev Kit featuring ESP32-S2 WROVER
Core Installation version: https://raw.githubusercontent.com/espressif/arduino-esp32/gh-pages/package_esp32_dev_index.json
IDE name: Arduino IDE 1.8.15
Flash Frequency: 80Mhz
PSRAM enabled: no
Upload Speed: 921600
Computer OS: Windows 7 x64

Description:

When using the "USB CDC On Boot = Enabled" Serial port, it will lose incoming characters (received by the ESP32-S2) unless the PC slows down the sent data by using smaller writes with delays between each write. The example sketch below sets the receive buffer to 2K, however even if I only write 1024 bytes in one Windows API call to WriteFile() I will often only see ~800 bytes arrive.

The sketch runs a tight loop reading and throwing away characters, but it keeps a count. Once there is a delay of 2 seconds when the incoming data stops, it reports how many characters it saw. Of course, this is a bare minimum sketch, which shows the larger problem I'm having in my project.

To try the sketch, configure the USB-COM port for the device to be COM1 (single digit COM ports can be used from the command line) and open the device from PuTTY. Type a few characters, wait a couple of seconds, it will report back how many characters were typed.

The device is now primed (DTR & RTS has been configured by PuTTY) where you can close PuTTY but get it ready to open again (have the PuTTY config window open and ready to connect)... In a DOS console (command prompt), type "copy my1024b.bin \b com1" (the file needs to be created beforehand and should have 1024 bytes in it)... Once it says "1 file(s) copied.", quickly press the Open button in PuTTY and it will tell you how many characters it got.

Sketch:

static int c = 0;
static unsigned long tm;
static char buf [260];

void setup () {

    delay (2500);
    Serial.begin (921600);
    Serial.setRxBufferSize (2048);
}

void loop () {

    if (Serial.available ()) {
        Serial.read ();
        c++;
        tm = micros ();
    }
    if ((c) && (micros () - tm >= 2000000)) { // 2s
        sprintf (buf, "%05d\n", c);
        Serial.write (buf);
        c = 0;
    }
}

Debug Messages:

00825
@me-no-dev
Copy link
Member

try calling Serial.setRxBufferSize before Serial.begin

@ghost
Copy link
Author

ghost commented Oct 2, 2021

I tried that, but it didn't help. I got 00412 as output from a file with 1024 bytes in it.

I made a typo in the copy command earlier (I typed the wrong slash) this is the command to use: "copy my1024b.bin /b com1" but depending on what's in the file, you don't really need the "binary" flag.

Also, just for completeness, changing the COM port number is in "Computer Management"->"Device Manager"->"Ports COM & LPT" double-click your device, select the "Port Settings" tab and press the "Advanced" button. COM Port Number is at the bottom.

@chegewara
Copy link
Contributor

Is there any reason you read 1 byte only with every read call?

@ghost
Copy link
Author

ghost commented Oct 3, 2021

Worth a try, right (it's not how my project does it, but I was keeping the example simple):

static int c = 0;
static unsigned long tm;
static char buf [260];

void setup () {

    delay (2500);
    Serial.setRxBufferSize (2048);
    Serial.begin (921600);
}

void loop () {
    int i;
    int j;

    i = Serial.available ();
    if (i) {
        while (i) {
            if (i > 260)
                j = 260;
            else
                j = i;
            Serial.read (buf, j);
            i -= j;
            c += j;
        }
        tm = micros ();
    }
    if ((c) && (micros () - tm >= 4000000)) { // 4s
        sprintf (buf, "%05d\n", c);
        Serial.write (buf);
        c = 0;
    }
}

Output:

00694

@ghost
Copy link
Author

ghost commented Oct 21, 2021

So, I completed a bunch of other projects and finally got back to this one. The S2 device hasn't been much use to me since I bought it for the USB CDC stuff to replace the ESP8266 in my project and the overclocked serial port of the host device. At 4MHz though, I expect I'm getting 360KB/s out of the ESP8266, and I don't expect (or need) that much out of the S2.. but it just wasn't reliable.

Playing with and looking at the code, I can see you do need to call Serial.setRxBufferSize() first, before Serial.begin().

I found it interesting though, when I called Serial.end() after the setup delay() call, and before Serial.setRxBufferSize(), this all started working properly.

Then I found the Serial.begin() call in core's main.cpp enclosed within #if ARDUINO_USB_CDC_ON_BOOT

Removing the 3 lines makes the original sketch work.

@me-no-dev
Copy link
Member

you can just not enable CDC on boot and do it in the sketch instead.

@VojtechBartoska VojtechBartoska added the Status: In Progress ⚠️ Issue is in progress label Oct 22, 2021
@ghost
Copy link
Author

ghost commented Oct 23, 2021

I've discovered another issue... (Snip unimportant stuff)

With the ESP32-S2, if I limit my application to half-duplex (which, I know, is all the USB port will do anyway - however latencies become an issue with request/response messages on top of the USB ones already there) then it is VERY reliable.

As soon as I try (my project's) full-duplex use-case, the S2 CDC "locks up" if the send-to-S2 data is >64 bytes at a time (the USB packet size). Use-case here is a socket which is meant to be full-duplex capable. Data going in each direction should not affect each other, other than ensuring the S2 receive buffer doesn't overflow (which is where this all started and is handled).

Of course, being USB, you can't just connect a couple of micro-controllers together and use two minimum sketches to show this latest issue, so it's not all that easy to reproduce (unlike the opening post's case).

The best I've come up with is the S2 receiving 128 (of the 2048 RxBuffer) bytes, for each byte it gets, it immediately writes out 16 copies. The host PC writes the 128 bytes and attempts to read back 2048 before looping back and sending in another 128. This works for a while, but eventually the S2's Serial.write() function returns less characters than it's given, at which point it no longer accepts any more (always returns 0 - and I do yield() when this happens, but it doesn't help, the USB CDC has "locked up").

Going half-duplex... Changing the sketch to no longer immediately writing 16 copies, instead it waits for the entire 128 bytes, then it writes out the 2048 in one go, works reliably.

Upping incoming data (from 128 to 256 or 512 etc.) makes it lock up even more quickly. Make it 64 bytes, and it will work reliably. EDIT: it occurs to me, at 64 bytes it's then effectively half-duplex.

@ghost
Copy link
Author

ghost commented Oct 23, 2021

#define SZ 16
#define MIN_CHUNK 1 // Make this the incoming packet size to change to half-duplex

static int t = 0;
static char buf [SZ];

void setup () {
    delay (1500);
    //Take your pick...
    //Serial.end ();
    //Serial.setRxBufferSize (0);
    Serial.setRxBufferSize (2048);
    Serial.begin (921600);
    
    Serial0.begin (115200);
    Serial0.println ("Started");
}

void loop () {
    char ch;
    int i;
    int j;
    int l;

    if (Serial.available () >= MIN_CHUNK) {
        i = MIN_CHUNK;
        while (i) {
            ch = Serial.read ();
            memset (buf, ch, SZ);
            l = SZ;
            while (l) {
                j = Serial.write (buf, l);
                if (j != l) {
                    if (t == 0)
                        Serial0.write ("yield!\n");
                    t = 1;
                    yield ();
                }
                l -= j;
            }
            if (t) {
                t = 0;
                Serial0.write ("back!\n");
            }
            i--;
        }
    }
}

@ghost
Copy link
Author

ghost commented Oct 23, 2021

PC code (sorry for the delay - had to clean it up):

#include <string.h>
#include <stdio.h>
#include <Windows.h>
#include <conio.h>
#include <time.h>

#define COM_PORT_STRING         "\\\\.\\COM1"
#define COM_BAUD_RATE           921600
#define COM_DATA_BYTE_SIZE      8
#define COM_PARITY              NOPARITY
#define COM_STOP_BIT            ONESTOPBIT

//Pick your packet size
//#define SEND_SIZE 64
#define SEND_SIZE 128

#define RC_FACT 16
#define REC_SIZE (SEND_SIZE * RC_FACT)

int main (int argc, char * argv [], char * envp []) {
    int  e;
    int  i;
    int  p;
    int  r;
    char buf [260];
    char b1 [REC_SIZE];
    char b2 [SEND_SIZE];
    unsigned char c;
    DCB dcb;
    COMMTIMEOUTS ct;

    HANDLE hCom = CreateFile (COM_PORT_STRING, GENERIC_READ | GENERIC_WRITE, 0, NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL); 

    if (hCom != INVALID_HANDLE_VALUE) {
        SetupComm (hCom, REC_SIZE, REC_SIZE);

        GetCommState (hCom, &dcb);
        dcb.BaudRate = COM_BAUD_RATE;
        dcb.ByteSize = COM_DATA_BYTE_SIZE;
        dcb.Parity   = COM_PARITY;
        dcb.StopBits = COM_STOP_BIT;
        SetCommState (hCom, &dcb);
        
        GetCommTimeouts (hCom, &ct);
        ct.ReadIntervalTimeout = 4000; // Default was 1ms
        ct.ReadTotalTimeoutMultiplier = 0;
        ct.ReadTotalTimeoutConstant = 0;
        ct.WriteTotalTimeoutMultiplier = 0;
        ct.WriteTotalTimeoutConstant = 0;
        SetCommTimeouts (hCom, &ct);

        EscapeCommFunction (hCom, SETDTR);
        EscapeCommFunction (hCom, SETRTS);

        i = 0;
        c = 'A';
        while (i < SEND_SIZE) {
            b2 [i++] = c++;
            if (c > 'Z')
                c = 'A';
        }

        while (_kbhit () == 0) { // Run until a key is pressed in the console
            p = 0;
            r = SEND_SIZE;
            while (p != SEND_SIZE) {
                i = 0;
                e = WriteFile (hCom, &(b2 [p]), r, &i, NULL);
                if ((e == 0) || (i <= 0))
                    printf ("Oddness detected in write\n");
                p += i;
                r -= i;
            }
            p = 0;
            r = REC_SIZE;
            while (p != REC_SIZE) {
                i = 0;
                e = ReadFile (hCom, &(b1 [p]), r, &i, NULL);
                if ((e == 0) || (i <= 0))
                    printf ("Oddness detected in read\n");
                p += i;
                r -= i;
            }
        }
        CloseHandle (hCom);
    } else {
        buf [0] = 0;
        FormatMessage (FORMAT_MESSAGE_FROM_SYSTEM, NULL, GetLastError (), 0, buf, 260, NULL);
        printf ("Error: %s\n", buf);
        return 1;
    }
    return 0;
}

@ghost
Copy link
Author

ghost commented Oct 23, 2021

BTW: My efforts at cleaning up the code before posting were not great - I had to make a few edits in the posts. (These code files are from a larger test suite, sorry.)

@me-no-dev
Copy link
Member

If you have CDC on boot enabled, Serial.setRxBufferSize (2048); in setup() will not work.

@ghost
Copy link
Author

ghost commented Oct 25, 2021

I said that first, me-no-polite :)

Look for my review of this product at:
https://core-electronics.com.au/esp32-s2-saola-1r-dev-kit-featuring-esp32-s2-wrover.html

@ghost
Copy link
Author

ghost commented Jan 19, 2022

Fix locked mutex when fifo is full: hathach/tinyusb#1286

@kerryland
Copy link

I said that first, me-no-polite :)

Look for my review of this product at: https://core-electronics.com.au/esp32-s2-saola-1r-dev-kit-featuring-esp32-s2-wrover.html

Interesting there are now no reviews listed. Perhaps the trust-worthy retailer doesn't like the negative ones?

@XZCE
Copy link

XZCE commented May 1, 2023

It's still "in the system" via the search, but the jerk has hidden it within the product page:

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants