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.

[NicoHood]

try

static inline void ReleaseTX()
{
    UEINTX = ~(1<<TXINI);
}

[embmicro]

After making that change I can't get anything to show up.

[NicoHood]

We have a really bad code style here. (look at the brackets).
Please test the following addition:

        // This if only counts for the next line
        if (n > len)
            n = len;

        // try this patch:
        if(n== USB_EP_SIZE)
               n=USB_EP_SIZE-1;

        // Those brackets are useless
        {
            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();
        }

[embmicro]

I got it to work by changing USB_Send()

        if (!ReadWriteAllowed())    // Release full buffer
            ReleaseTX();

        if ((len == 0) && (ep & TRANSFER_RELEASE))
            ReleaseTX();

The reason your edit doesn't work (I think) is because ReleaseTX() is called once anyways since the endpoint is full (ReadWriteAllowed() will be false). However, we need to call it a second time to send an empty packet. I don't really know USB so forgive me if my terminology or understanding is wrong.

[NicoHood]

I think that flush only flushes the 64 bytes, but does not send a ZLP. You can also try:

void USB_Flush(u8 ep)
{
    SetEP(ep);
    if (FifoByteCount()){
        ReleaseTX();
        ClearIN();
    }
}

This will however not work with the ISR itself (self flushing).

[embmicro]

I believe those brackets are used for the EP lock.

[NicoHood]

No those brackets are useless.
Your idea might be correct. The theory is correct at least.

However this can only work if you modified this and added a release here as well:
int r = USB_Send(CDC_TX,buffer,size);
did you?

[embmicro]

Yea I have it as

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

[embmicro]

My understanding is that the TRANSFER_RELEASE flag should force USB_Send to actually transfer the data. That's what we want since Serial.write is blocking (at least I assumed that's what it should do).

[NicoHood]

This then makes sense, but doesnt solve the problem at all. It will now flush on every sending. making flush() useless, and the ISR as well.

Please try the suggested USB_Flush(u8 ep) fix without any other modifications.

Serial is not blocking, serial is interrupt driven and only blocking on a full buffer (also HW serial). That is speed also. Flushing usb everytime makes it slow and blocking.

[embmicro]

The USB_Flush didn't work. However the following did.

void USB_Flush(u8 ep)
{
    SetEP(ep);
    //if (FifoByteCount())
        ReleaseTX();
}

[NicoHood]

Yep tested that now on my own.
I dont know what those registers really do. The guy who wrote the USBcore should review this. However I think the fix you just wrote is the best one. Not sure if it does not bring up other issues.

[NicoHood]

Someone should try this with a 16 byte buffer on a u2 device. Since this only operates in single bank mode I am wondering if it has the same problem (it shouldnt). But I remember to have those lags there too. let me check if i have a device set up...

[NicoHood]

Its not a bank problem. Its just because it doesnt see that all bytes (64/16) are sent, but a ZLP is missing.

The best patch is just to send a ZLP, not sure if this gives any problems if the host disconnects etc. But host disconnection is not reallt carefully implemented, although it works.

@cmaglie @facchinm how should we continue?

[matthijskooijman]

I believe those brackets are used for the EP lock.

This is correct, the brackets make sure that the lock variable goes out of scope, releasing the lock on the endpoint.

[NicoHood]

Oh i missed that. makes sense. commenting helps...
If we patch this, please add a comment there.

[sandeepmistry]

@embmicro @NicoHood I've proposed a patch to resolve this: #4138