Track if a USB ZLP is needed on flush #4138
Conversation
Also, wait for write access before sending ZLP
sandeepmistry
added
Architecture: AVR
|
This looks good but I propose changing the line where you assign _sendZlp to the following so that if the TRANSFER_RELEASE flag is set you don't set _sendZlp. That is unless I have a misunderstanding on what TRANSFER_RELEASE is supposed to do. |
|
@embmicro thanks for reviewing! As for your suggestion, if |
|
Can you build this? I want to try. I am just not sure if ZLPs are okay for other devices, such as HID or MIDI. Have you checked that first? |
|
@ArduinoBot build this please |
|
Can't you just add the while then like the following? _sendZlp[ep & USB_ENDPOINTS_MASK] = !ReadWriteAllowed() && (len == 0) && !(ep & TRANSFER_RELEASE);
if (!ReadWriteAllowed()) // Release full buffer
ReleaseTX();
if ((len == 0) && (ep & TRANSFER_RELEASE)) {
while(!ReadWriteAllowed());
ReleaseTX();
} |
as suggested by @embmicro
|
@embmicro agreed, your suggestion is much cleaner. Initially I thought the r/w check was only needed if a ZLP needs to be sent when the I've pushed your suggestion in sandeepmistry@bc80824 |
|
@ArduinoBot build this please |
|
I've run into a problem with this patch. Take the following example. void setup() {
// put your setup code here, to run once:
}
void loop() {
if (Serial) {
if (Serial.read() == 'h'){
Serial.write("Hello ");
Serial.flush();
Serial.write("World!\r\n");
Serial.flush();
}
}
}With this sketch I expect "Hello World!" to be printed every time I send an 'h' to the board. However, on my Windows 10 machine, I typically only get "Hello ". Sending another 'h' will then show "Hello World!\r\nHello " Removing the calls to flush makes it work as expected, but that isn't much of a solution. On my Linux machine it works perfectly fine with and without flush. |
|
Sounds like an useful option if it works on linux. +1 from me ;) For the windows users a better fix might be to not send USB_EP_SIZE bytes. Also as described above, I dont think every EP should use the ZLP mechanism, since other devices work different. |
|
I don't know if this is ideal, but I modified USB_Send to never send a full packet and it seems to be working with all my tests. 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;
}
n--; // leave one byte free
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++);
}
// Release full buffer
if (!ReadWriteAllowed() || USB_SendSpace(ep) == 1 || ((len == 0) && (ep & TRANSFER_RELEASE)) )
ReleaseTX();
}
}I also reverted flush back to it's original form. Still calling flush a lot like in my previous example causes trouble. However, flush doesn't seem to be needed at all so maybe it should be an empty function. |
|
does someone knows how this is going on ? I think I maybe have a related problem with 32u4 (?) can I just try to exchange the original USB_Send() with this last one from @embmicro without changing any other code segments in other places/files ? maybe Paul Stoffregen's approach would be a better Solution ? |
|
I'm closing this in favour of #4864. Which is a similar approach to @embmicro's suggestion in #4138 (comment). However, it will spin with a delay and timeout if buffer space is EP size - 1. |
Resolves #3946. This patch tracks if a USB ZLP (zero length packet) is needed to flush data.
A ZLP is needed to indicate end of transfer, if the size of data sent is a multiple of
USB_EP_SIZE(64 bytes on AVR).If a ZLP is needed,
USB_Flushwaits for the current endpoint's FIFO bank to be available before doing a release.