Update CDC.cpp - Remove delay in operator bool()

[collin80]

Remove 10ms blocking wait from bool() operator in CDC code. Instead, set a timestamp and wait 10ms before returning true.

[matthijskooijman]

Thanks for picking this up!

This code introduces a race condition, since CDC_Setup is run in ISR context, so the timestamp might be changed halfway through reading it. To fix it, copy the timestamp into a local variable while interrupts are disabled (ATOMIC_BLOCK(ATOMIC_RESTORESTATE) { } from avr/atomic.h is good for this) first.

Also, the "Update CDC.cpp" from the commit message isn't very helpful, the first summary line should explain what actually happened (git can tell me what file it changed). The first sentence from your extended description actually looks fine as the summary line to me :-)

[collin80]

This code was only changed for the Due though. It's native 32 bit so the timestamp code should be atomic isn't it?

I've changed the title summary to better describe this pull request.

[matthijskooijman]

Oh, didn't realize that the Due's CDC.cpp was so similar. I think that on the Due, it should already be atomic indeed. I guess it would be best to apply the same change to AVR as well, though.

Regarding the summary - I was referring to the actual commit message, not just the pullrequest description. However, to edit you would need to commit --amend or rebase using a git client, and I suspect you just edited the file through github? If so, just keep this in mind for future contributions :-)

[collin80]

Yeah, what happened is that I edited the file locally on my machine to make
sure it worked. Once I was satisfied I had to find a way to issue a pull
request so I just quickly went to github and did the edits right there. I
didn't really take enough time to be careful here so I suppose the commit
message wasn't too helpful. I'll try to take more care in doing this in the
future.

On Thu, Jul 2, 2015 at 8:12 AM, Matthijs Kooijman notifications@github.com
wrote:

Oh, didn't realize that the Due's CDC.cpp was so similar. I think that on
the Due, it should already be atomic indeed. I guess it would be best to
apply the same change to AVR as well, though.

Regarding the summary - I was referring to the actual commit message, not
just the pullrequest description. However, to edit you would need to commit
--amend or rebase using a git client, and I suspect you just edited the
file through github? If so, just keep this in mind for future contributions
:-)

—
Reply to this email directly or view it on GitHub
#3437 (comment).

[NicoHood]

Why is there a 10 ms delay anyways? To wait a bit more before sending new bytes with the CDC?

However I would definitely NOT go for this fix on AVR. 4 bytes of ram vs 10ms at startup, and ONLY if this function (the bool operator) is used. For due, I have no idea how much ram it has and I dont use it anyways :P

[matthijskooijman]

@NicoHood, the goal is not to save 10ms at startup, the goal is to save 10ms whenever checking the serial status after startup. Here's the thread where the discussion about this PR started: https://groups.google.com/a/arduino.cc/forum/#!topic/developers/SQEvifbj0S4

[NicoHood]

Doesnt #3343 solve this? You can check the DTR and RTS states yourself and keep the delay for its simple "nooby" usage. I would not add 4 more bytes to ram just for such a simple thing, now that we DO have dtr() and rts() functions.

[matthijskooijman]

@NicoHood, AFAIU, the problem is that shortly after dtr is asserted, writing data to the host still fails, so essentially dtr is sort-of lying for a short while after the port is opened. Using the newly introduced dtr() would allow a sketch to keep track of this and prevent problems, but that would mean a lot of overhead in all sketches, for something that, I think, should just work.

[collin80]

Yes, and so I'm still unsure of any other fix than the one I suggested
where we take up 4 bytes of RAM to store a timestamp and don't return that
it is OK to begin using the USB port until 10ms have elapsed. Any other
solution seems to either waste huge amounts of time if you call it in a
loop or not properly protect against this issue. Though, I am unaware of
anyone actually confirming that this is still an issue. Who had the issue,
when, and on what operating system? I'm wondering if it was a very specific
hardware problem that happens to nearly no one or if it is more wide spread?

On Sun, Aug 16, 2015 at 6:19 AM, Matthijs Kooijman <notifications@github.com

wrote:

@NicoHood https://github.com/NicoHood, AFAIU, the problem is that
shortly after dtr is asserted, writing data to the host still fails, so
essentially dtr is sort-of lying for a short while after the port is
opened. Using the newly introduced dtr() would allow a sketch to keep
track of this and prevent problems, but that would mean a lot of overhead
in all sketches, for something that, I think, should just work.

—
Reply to this email directly or view it on GitHub
#3437 (comment).

[matthijskooijman]

#3624 contains a different approach, needing only 1 byte of RAM (at the cost of a bit of delay even when not strictly required, but for your loop usecase this would only lead to 10ms once, not on every loop invocation).

As for reproducing the issue, I haven't tried, nor have I seen anyone else say something about this. The original commit message suggested that @ffissore noticed this bug, but IIRC I highlighted him before without a response. Trying again now :-)

[collin80]

You know what, I like that other solution better. It saves memory, still
delays, and only has the delay when it is really necessary. Everyone should
forget my pull request and go with that one. It truly does seem like the
correct solution to the problem.

On Sun, Aug 16, 2015 at 10:34 AM, Matthijs Kooijman <
notifications@github.com> wrote:

#3624 #3624 contains a different
approach, needing only 1 byte of RAM (at the cost of a bit of delay even
when not strictly required, but for your loop usecase this would only lead
to 10ms once, not on every loop invocation).

As for reproducing the issue, I haven't tried, nor have I seen anyone else
say something about this. The original commit message suggested that
@ffissore https://github.com/ffissore noticed this bug, but IIRC I
highlighted him before without a response. Trying again now :-)

—
Reply to this email directly or view it on GitHub
#3437 (comment).

[NicoHood]

Yep the other PR looks better. You could close it then.
I still do not understand the break API from @matthijskooijman , could it help here??

But the same issue is for HID. The issue is still opened somewhere from me. And there is no fix yet for this. I dont know if this is really an OS only problem or if we can somehow check if the endpoint is ready to transmit data somehow. If it is a general USB problem, cant we add a fix for the whole USB thing, not only CDC but HID and MIDI as well?

[collin80]

PR 3624 is a better idea and better implementation of this. Use that one instead. I'm closing my PR because the other one is better.

[matthijskooijman]

I still do not understand the break API from @matthijskooijman , could it help here??

No, it could not and is completely unrelated. See https://en.wikipedia.org/wiki/Universal_asynchronous_receiver/transmitter#Break_condition

But the same issue is for HID. The issue is still opened somewhere from me. And there is no fix yet for this. I dont know if this is really an OS only problem or if we can somehow check if the endpoint is ready to transmit data somehow. If it is a general USB problem, cant we add a fix for the whole USB thing, not only CDC but HID and MIDI as well?

I don't think there is a generic "endpoint opened" flag in generic USB. For CDC, the DTR flag is used for this since usually the flag is asserted only while the port is opened, but even that is not necessarily true. I'm not sure if HID and MIDI have a similar feature, you'd have to check the specs for those protocols (and it's certainly off-topic for this PR).