Leonardo fails to send exactly 64 bytes.

[embmicro]

When using the virtual serial port on the Leonardo I found that if I send exactly 64 bytes it doesn't send until another byte is sent. Below is a demo to show this problem.

void setup() {
  // put your setup code here, to run once:

}

void loop() {
  if (Serial) {
    if (Serial.available() > 0) {
      Serial.read();
      char buff[65] = "D_ENDPOINT(USB_ENDPOINT_IN (CDC_ENDPOINT_IN ),USB_ENDPOINT_TYPE";
      Serial.write(buff,64);
      Serial.flush();
    }
  }
}

After sending a character, nothing appears in the terminal. However, sending another then prints all 128 characters. This happens regardless of the serial port monitor I've tried. I've also tried on Linux and Windows 10 with slightly different, but similar results. This is with 1.6.5.

[shiftleftplusone]

to my experience, the reading part of your code is supposed to work better this way:

byte buf[65], i;

void loop() {
  if (Serial) {
    i=0;
    while (Serial.available() <64); // wait for 64 bytes in the buffer
    while (Serial.available() && (i<=64) ) {  // get them!
      buf[i]=Serial.read();
      ++i;
    }
   //...

  }
}

next, you can't asign a string directly to a pointer (actually buf[] is a pointer).
But you can assign it this way:

strcpy(buf, "D_ENDPOINT(USB_ENDPOINT_IN (CDC_ENDPOINT_IN ),USB_ENDPOINT_TYPE");
buf[64]='\0';

whatever this USB_/CD _ stuff might be and whatever the sense of this assignement might be.

anyway, the Serial documentation is very poor, and I'm not sure if Serial allows a 64 byte buffer size (maybe try it using 32 instead in case it fails)

@arduino developers:
please provide better examples and tutorials for fail-safe Serial usage!

[Chris--A]

@vogonjeltz

In the provided example buff is an array, not a pointer. And so is the string it is initialized with. This method of initialization is fine.

@embmicro is simply using the single read to ensure they have control over the loop (will only send the string when a character is sent to the Arduino), otherwise the loop would send the string continuously and you might not notice the first copy isn't actually sent.

[shiftleftplusone]

in C an array is always a pointer!
char buf[] is identical to char * buf !

and initializing at variable declaration
buf[]= "D_ENDPOINT(USB_ENDPOINT_IN (CDC_ENDPOINT_IN ),USB_ENDPOINT_TYPE";
works, sure,
but not inside the code
buf[65]= "D_ENDPOINT(USB_ENDPOINT_IN (CDC_ENDPOINT_IN ),USB_ENDPOINT_TYPE";

but of course I stand corrected in case I'm wrong and I must admit that I don't understand at all what
"D_ENDPOINT(USB_ENDPOINT_IN (CDC_ENDPOINT_IN ),USB_ENDPOINT_TYPE"
actually means or what it's all about it.

[NicoHood]

I guess the 32u4 misses to send a Zero Length package (ZLP).

The EP Size for the 32u4 is 64 (USB_EP_SIZE). If you send exactly 64 bytes you need to end the session with a ZLP. Otherwise the host expects more bytes. The solution for this is to a) send a ZLP or b) send not more than USB_EPSIZE - 1 bytes.

[NicoHood]

Tearing down the issue:

size_t Serial_::write(const uint8_t *buffer, size_t size)
{
    /* only try to send bytes if the high-level CDC connection itself 
     is open (not just the pipe) - the OS should set lineState when the port
     is opened and clear lineState when the port is closed.
     bytes sent before the user opens the connection or after
     the connection is closed are lost - just like with a UART. */

    // TODO - ZE - check behavior on different OSes and test what happens if an
    // open connection isn't broken cleanly (cable is yanked out, host dies
    // or locks up, or host virtual serial port hangs)
    if (_usbLineInfo.lineState > 0) {
        int r = USB_Send(CDC_TX,buffer,size);
        if (r > 0) {
            return r;
        } else {
            setWriteError();
            return 0;
        }
    }
    setWriteError();
    return 0;
}

//  Blocking Send of data to an endpoint
int USB_Send(u8 ep, const void* d, int len)
{
    if (!_usbConfiguration)
        return -1;

    int r = len;
    const u8* data = (const u8*)d;
    u8 timeout = 250;       // 250ms timeout on send? TODO
    while (len)
    {
        u8 n = USB_SendSpace(ep);
        if (n == 0)
        {
            if (!(--timeout))
                return -1;
            delay(1);
            continue;
        }

        if (n > len)
            n = len;
        {
            LockEP lock(ep);
            // Frame may have been released by the SOF interrupt handler
            if (!ReadWriteAllowed())
                continue;
            len -= n;
            if (ep & TRANSFER_ZERO)
            {
                while (n--)
                    Send8(0);
            }
            else if (ep & TRANSFER_PGM)
            {
                while (n--)
                    Send8(pgm_read_byte(data++));
            }
            else
            {
                while (n--)
                    Send8(*data++);
            }
            if (!ReadWriteAllowed() || ((len == 0) && (ep & TRANSFER_RELEASE))) // Release full buffer
                ReleaseTX();
        }
    }
    TXLED1;                 // light the TX LED
    TxLEDPulse = TX_RX_LED_PULSE_MS;
    return r;
}

The ZLP can be found here:

            if (!ReadWriteAllowed() || ((len == 0) && (ep & TRANSFER_RELEASE))) // Release full buffer
                ReleaseTX();

To fix this issue you could try to add this to the write() function instead:

int r = USB_Send(CDC_TX | TRANSFER_RELEASE,buffer,size);

The reason why flush does not work here is possibly because the 32u4 switched to the 2nd usb bank which is empty and so it does not flush it. So normally it would be better to fix the flush instead of the write, because the flush is then not really needed if we flush every write.

I have no idea yet how to fix this. Also I am not sure if flushing with every read (the fix above) decreases the speed somehow. If not, the fix should be fine and we can remove the flush from the Interrupt, or remove it in general because its a) not needed anymore and b) useless anyways.

I am also not sure what this really does.

static inline void ReleaseTX()
{
    UEINTX = 0x3A;  // FIFOCON=0 NAKINI=0 RWAL=1 NAKOUTI=1 RXSTPI=1 RXOUTI=0 STALLEDI=1 TXINI=0
}

Comparing to Lufa, I guess it (should!) does this:
https://github.com/abcminiuser/lufa/blob/master/LUFA/Drivers/USB/Core/AVR8/Endpoint_AVR8.h#L452-L460

Can you please try the write() patch I showed above?

@facchinm
@cmaglie

Edit: we should better fix the flush() instead of flushing on every write() which will slow down things again.

[Chris--A]

@vogonjeltz

Unfortunately your assumptions are incorrect.

char buf[] is identical to char * buf !

No it is not, There is nothing in the C, or C++ standard that denotes this. An array can be implicitly converted to a pointer type (pointing to the first element), but that has nothing to do with the original declaration (want proof, use sizeof and you'll see your error!).

Do not forget that in an .ino file, everything is compiled as C++, not C (wrong language assumptions).

but not inside the code
buf[65]= "D_ENDPOINT(USB_ENDPOINT_IN (CDC_ENDPOINT_IN ),USB_ENDPOINT_TYPE";

Well, I do not know where you got that from, but the original code is an initialization, not an assignment! ... Significantly different.

char buff[65] = "D_ENDPOINT(USB_ENDPOINT_IN (CDC_ENDPOINT_IN ),USB_ENDPOINT_TYPE";

From the C standard:

6.3.2.1 Lvalues, arrays, and function designators
...
3 Except when it is the operand of the sizeof operator or the unary & operator, or is a string literal used to initialize an array, an expression that has type ‘‘array of type’’ is converted to an expression with type ‘‘pointer to type’’ that points to the initial element of the array object and is not an lvalue. If the array object has register storage class, the behavior is undefined.

The C++ standard just extends this...

Its better that you get rid of these assumptions sooner than later. All it is doing is causing confusion.

[shiftleftplusone]

yes, char array[] == char * array points to the initial element, it's a pointer to a char .
in C, char arrays can be initialized by
char array[] = "xxxxx";
or
char array[5];
but IMO not by
char array[5]="xxxxx";

wether C++ extends this or not, I don't know, I'm just talking about C.

[Chris--A]

Regardless of what it converts to, an array is not a pointer.
A string literal is an array by definition...

but IMO not by
byte array[5]="xxxxx";

Yup, string literals are null terminated. The literal you provided has 6 chars, not 5. So you have erroneously forgotten to leave enough room for this. It is your own fault, and nothing to do with strings or arrays being pointers.

BTW: forget c,You can only use this if you have actual *.c files. Arduino *ino and *.cpp files are C++ regardless...

[shiftleftplusone]

ok, about terminating '\0' you're right, but that was not the point.

[Chris--A]

ok, about terminating '\0' you're right, but that was not the point

Ok,... What was the point??? The standard clearly states what is what, arguing against that isn't going to convince me. Maybe we should continue this in the forum, keeping the issue tracker for issues....

[NicoHood]

Guys...
Focus on the issue: CDC flush() doesnt work correct.

[Chris--A]

Of course, like I mentioned, I can continue this in the forum if needed, This issue is still not resolved which should be the focus. However there is obviously a discrepancy that needs clarification, and is only going to infect future issues unless rectified.

[embmicro]

@NicoHood

I tried your fix and it didn't work for me. Thanks a lot for looking into this!

[NicoHood]

Didnt work in which way? (I actually havent tried this myself, I just had a quick look though).

[embmicro]

The behavior was the same. Sending 64 bytes would result in nothing being shown, send another and everything shows up.