SerialUSB boolean operator
35 views
Skip to first unread message
Collin Kidder
Jun 16, 2015, 2:40:12 PM6/16/15
to developers
I found a nasty surprise recently. I had a sketch with a loop that
looked something like this:
void loop()
{
if (SerialUSB)
{
//send things over the serial usb interface
}
}
The goal was to not waste time sending things over the native port if
nobody was there to listen. On the surface it seemed reasonable and
seemed to work fine. But, later on I found that the sketch had serious
performance problems. Eventually this was tracked down to the use of
the serialUSB boolean check. It's *seriously* slow. So, I looked at
the Arduino core source code. Imagine my surprise to find this:
// This operator is a convenient way for a sketch to check whether the
// port has actually been configured and opened by the host (as opposed
// to just being connected to the host). It can be used, for example, in
// setup() before printing to ensure that an application on the host is
// actually ready to receive and display the data.
// We add a short delay before returning to fix a bug observed by Federico
// where the port is configured (lineState != 0) but not quite opened.
Serial_::operator bool()
{
// this is here to avoid spurious opening after upload
if (millis() < 500)
return false;
bool result = false;
if (_usbLineInfo.lineState > 0)
{
result = true;
}
delay(10);
return result;
}
What in the ever loving hell is a 10 millisecond wait doing in there?!
I realize that there is a comment explaining why it's there but is
that really the proper solution? Something like that is a totally
hidden landmine just waiting to be stepped on. That's 840,000
instruction clocks that are wasted just sitting around doing nothing.
I think it's natural to wonder whether the port still has a listener
on the other end in situations other than the initial setup. What if
someone has the board externally powered and jerks out the USB cord?
There is no listener any longer and the sketch should be able to find
that out without incurring a 10ms delay. So, I'm a bit perturbed to
find something like this. Maybe I'm the only one to think of trying to
detect the port status while the sketch is running. In the end I
didn't really need to do it anyway and so I just took that part out.
Now it just constantly tries to overflow the serial buffer even if
nobody is listening.
I guess the question is, who put that there and how can we fix it?
It's there to fix a problem but I just don't feel that phantom delays
hidden in the core library are the proper solution. Perhaps the code
could store the current timestamp when lineState goes non-zero and the
bool operator could return false until at least 10ms has gone by since
that point? That's a bit more complicated and requires 4 more bytes of
RAM used by the core to track the timestamp but it's a hell of a lot
faster than always waiting 10ms in this function.
looked something like this:
void loop()
{
if (SerialUSB)
{
//send things over the serial usb interface
}
}
The goal was to not waste time sending things over the native port if
nobody was there to listen. On the surface it seemed reasonable and
seemed to work fine. But, later on I found that the sketch had serious
performance problems. Eventually this was tracked down to the use of
the serialUSB boolean check. It's *seriously* slow. So, I looked at
the Arduino core source code. Imagine my surprise to find this:
// This operator is a convenient way for a sketch to check whether the
// port has actually been configured and opened by the host (as opposed
// to just being connected to the host). It can be used, for example, in
// setup() before printing to ensure that an application on the host is
// actually ready to receive and display the data.
// We add a short delay before returning to fix a bug observed by Federico
// where the port is configured (lineState != 0) but not quite opened.
Serial_::operator bool()
{
// this is here to avoid spurious opening after upload
if (millis() < 500)
return false;
bool result = false;
if (_usbLineInfo.lineState > 0)
{
result = true;
}
delay(10);
return result;
}
What in the ever loving hell is a 10 millisecond wait doing in there?!
I realize that there is a comment explaining why it's there but is
that really the proper solution? Something like that is a totally
hidden landmine just waiting to be stepped on. That's 840,000
instruction clocks that are wasted just sitting around doing nothing.
I think it's natural to wonder whether the port still has a listener
on the other end in situations other than the initial setup. What if
someone has the board externally powered and jerks out the USB cord?
There is no listener any longer and the sketch should be able to find
that out without incurring a 10ms delay. So, I'm a bit perturbed to
find something like this. Maybe I'm the only one to think of trying to
detect the port status while the sketch is running. In the end I
didn't really need to do it anyway and so I just took that part out.
Now it just constantly tries to overflow the serial buffer even if
nobody is listening.
I guess the question is, who put that there and how can we fix it?
It's there to fix a problem but I just don't feel that phantom delays
hidden in the core library are the proper solution. Perhaps the code
could store the current timestamp when lineState goes non-zero and the
bool operator could return false until at least 10ms has gone by since
that point? That's a bit more complicated and requires 4 more bytes of
RAM used by the core to track the timestamp but it's a hell of a lot
faster than always waiting 10ms in this function.
Matthijs Kooijman
Jun 18, 2015, 7:27:31 AM6/18/15
to devel...@arduino.cc
Hi Collin,
> What in the ever loving hell is a 10 millisecond wait doing in there?!
> I realize that there is a comment explaining why it's there but is
> that really the proper solution? Something like that is a totally
> hidden landmine just waiting to be stepped on. That's 840,000
> instruction clocks that are wasted just sitting around doing nothing.
> I think it's natural to wonder whether the port still has a listener
> on the other end in situations other than the initial setup. What if
> someone has the board externally powered and jerks out the USB cord?
> There is no listener any longer and the sketch should be able to find
> that out without incurring a 10ms delay. So, I'm a bit perturbed to
> find something like this. Maybe I'm the only one to think of trying to
> detect the port status while the sketch is running. In the end I
> didn't really need to do it anyway and so I just took that part out.
> Now it just constantly tries to overflow the serial buffer even if
> nobody is listening.
I guess the most common usecase is while(SerialUSB) /* wait */; in
which case the delay is harmless.
> I guess the question is, who put that there and how can we fix it?
> It's there to fix a problem but I just don't feel that phantom delays
> hidden in the core library are the proper solution. Perhaps the code
> could store the current timestamp when lineState goes non-zero and the
> bool operator could return false until at least 10ms has gone by since
> that point? That's a bit more complicated and requires 4 more bytes of
> RAM used by the core to track the timestamp but it's a hell of a lot
> faster than always waiting 10ms in this function.
It does sound like the proper solution to me, yes.
More obvious would be to wait 10ms before changing lineState, but that
would mean 10ms delay in an ISR, which probably breaks more than it
fixes.
Even better would have been to do:
while (SerialUSB) /* wait */;
delay(10);
but I guess it's a bit too late for that.
Gr.
Matthijs
> What in the ever loving hell is a 10 millisecond wait doing in there?!
> I realize that there is a comment explaining why it's there but is
> that really the proper solution? Something like that is a totally
> hidden landmine just waiting to be stepped on. That's 840,000
> instruction clocks that are wasted just sitting around doing nothing.
> I think it's natural to wonder whether the port still has a listener
> on the other end in situations other than the initial setup. What if
> someone has the board externally powered and jerks out the USB cord?
> There is no listener any longer and the sketch should be able to find
> that out without incurring a 10ms delay. So, I'm a bit perturbed to
> find something like this. Maybe I'm the only one to think of trying to
> detect the port status while the sketch is running. In the end I
> didn't really need to do it anyway and so I just took that part out.
> Now it just constantly tries to overflow the serial buffer even if
> nobody is listening.
which case the delay is harmless.
> I guess the question is, who put that there and how can we fix it?
> It's there to fix a problem but I just don't feel that phantom delays
> hidden in the core library are the proper solution. Perhaps the code
> could store the current timestamp when lineState goes non-zero and the
> bool operator could return false until at least 10ms has gone by since
> that point? That's a bit more complicated and requires 4 more bytes of
> RAM used by the core to track the timestamp but it's a hell of a lot
> faster than always waiting 10ms in this function.
More obvious would be to wait 10ms before changing lineState, but that
would mean 10ms delay in an ISR, which probably breaks more than it
fixes.
Even better would have been to do:
while (SerialUSB) /* wait */;
delay(10);
but I guess it's a bit too late for that.
Gr.
Matthijs
Collin Kidder
Jun 29, 2015, 3:13:03 PM6/29/15
to developers
On Thu, Jun 18, 2015 at 7:27 AM, Matthijs Kooijman <matt...@stdin.nl> wrote:
>> I guess the question is, who put that there and how can we fix it?
>> It's there to fix a problem but I just don't feel that phantom delays
>> hidden in the core library are the proper solution. Perhaps the code
>> could store the current timestamp when lineState goes non-zero and the
>> bool operator could return false until at least 10ms has gone by since
>> that point? That's a bit more complicated and requires 4 more bytes of
>> RAM used by the core to track the timestamp but it's a hell of a lot
>> faster than always waiting 10ms in this function.
> It does sound like the proper solution to me, yes.
>
> More obvious would be to wait 10ms before changing lineState, but that
> would mean 10ms delay in an ISR, which probably breaks more than it
> fixes.
>
I've gone ahead and submitted a pull request for this.
>> I guess the question is, who put that there and how can we fix it?
>> It's there to fix a problem but I just don't feel that phantom delays
>> hidden in the core library are the proper solution. Perhaps the code
>> could store the current timestamp when lineState goes non-zero and the
>> bool operator could return false until at least 10ms has gone by since
>> that point? That's a bit more complicated and requires 4 more bytes of
>> RAM used by the core to track the timestamp but it's a hell of a lot
>> faster than always waiting 10ms in this function.
> It does sound like the proper solution to me, yes.
>
> More obvious would be to wait 10ms before changing lineState, but that
> would mean 10ms delay in an ISR, which probably breaks more than it
> fixes.
>
https://github.com/arduino/Arduino/pull/3437
It's basically exactly what I had suggested - a 32 bit variable that
stores the time stamp when lineState changed and then a check in
operator bool() to make sure that true is not returned until at least
10ms after the lineState changed. I've tested this with a sketch and
it seems to work fine.
It's probably not something tons of people are dying to see but I
found another use case - I wrote a battery managment system sketch
that initializes and runs without needing the USB port to be plugged
into anything. It specifically checks within loop() to see if it has
gotten a connection on the USB port and if this is the first time that
has happened since the sketch started up. If so it outputs a simple
menu of what the user can do and what the current state of the BMS is.
This requires that I'm able to check the current state of the USB port
from within loop. If I were to do that within setup it would either
happen once or I wouldn't be able to run the sketch while I'm waiting
for a USB connection. So, in my case the 10ms delay really was
annoying even though it's a pretty specific use case. So, simple fix
and it seems like it should satisfy everyone so long as nobody misses
the 4 bytes of RAM now taken up. But, the Due has lots of RAM.
-Collin
Andrew Kroll
Jun 29, 2015, 3:17:32 PM6/29/15
to devel...@arduino.cc
The reason the delay is in there I believe is due to a race condition.
The USB serial isn't actually ready for at least 10ms after it claims to actually be ready.
In other words, where the USB serial part says it is ready needs to be positioned to a smarter place where it is actually enumerated AND opened, not just enumerated.The USB serial isn't actually ready for at least 10ms after it claims to actually be ready.
--
You received this message because you are subscribed to the Google Groups "Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to developers+...@arduino.cc.
--
Collin Kidder
Jun 29, 2015, 3:38:13 PM6/29/15
to developers
I just fixed the 10ms delay by making it delay 10ms like it used to
but now the sketch doesn't block during that time. It just returns
false until the 10ms is up. However, this doesn't necessarily fix the
underlying problem because code like the write method directly use
lineState and so will be able to proceed immediately if the sketch is
sending with no regard to whether it is connected. I suppose a better
solution, then, is to actually not change lineState until 10ms is up.
But, that's an interesting challenge as the CDC code does not have any
sort of "loop" functionality that would be called periodically.
Instead I suppose there could be a small method that checks whether
lineState is currently 0 but yet a flag was set with a timestamp that
we need to change the lineState at X time. That method could then wait
the 10ms and change linestate. The method would have to be called from
all other methods that might need to check lineState. It seems that
this is just write and operator bool. Given that these are the only
two functions that really rely on lineState maybe it'd be easiest to
amend my pull request to also bring the 10ms non-blocking delay
functionality directly into the write method. It would introduce a
slight (sub microsecond) delay to each call to write though.
-Collin
but now the sketch doesn't block during that time. It just returns
false until the 10ms is up. However, this doesn't necessarily fix the
underlying problem because code like the write method directly use
lineState and so will be able to proceed immediately if the sketch is
sending with no regard to whether it is connected. I suppose a better
solution, then, is to actually not change lineState until 10ms is up.
But, that's an interesting challenge as the CDC code does not have any
sort of "loop" functionality that would be called periodically.
Instead I suppose there could be a small method that checks whether
lineState is currently 0 but yet a flag was set with a timestamp that
we need to change the lineState at X time. That method could then wait
the 10ms and change linestate. The method would have to be called from
all other methods that might need to check lineState. It seems that
this is just write and operator bool. Given that these are the only
two functions that really rely on lineState maybe it'd be easiest to
amend my pull request to also bring the 10ms non-blocking delay
functionality directly into the write method. It would introduce a
slight (sub microsecond) delay to each call to write though.
-Collin
Matthijs Kooijman
Jun 30, 2015, 6:11:00 AM6/30/15
to devel...@arduino.cc
Hi Andrew,
> In other words, where the USB serial part says it is ready needs to be
> positioned to a smarter place where it is actually enumerated AND opened,
> not just enumerated.
I don't think this is accurate, since the current code already checks
the "linestate", which is essentially the state of the DTR signal, as
indicated by the USB host (so it already checks more than just the USB
enumerated).
I presume that, on some unspecified platform(s), the DTR pin is asserted
before the USB host is actually ready to receive data. If that is indeed
the case, I don't see any better way to fix this, since I don't think
the CDC protocol has any better indications than DTR.
Also, since DTR is supposed to mean Data Terminal Ready, it really seems
like a bug on the USB host side that it isn't ready when it sets DTR
high.
This workaround was added by Zach Eveland in
ade4893f585e3e94fa6cf683620e1d12afc88ecd, but it says it was noticed by
Federico. Federico, do you remember under what OS and conditions this
problem triggered?
Anyone want to try removing the delay and see if the problem still
exists on the supported OSes?
Gr.
Matthijs
> In other words, where the USB serial part says it is ready needs to be
> positioned to a smarter place where it is actually enumerated AND opened,
> not just enumerated.
the "linestate", which is essentially the state of the DTR signal, as
indicated by the USB host (so it already checks more than just the USB
enumerated).
I presume that, on some unspecified platform(s), the DTR pin is asserted
before the USB host is actually ready to receive data. If that is indeed
the case, I don't see any better way to fix this, since I don't think
the CDC protocol has any better indications than DTR.
Also, since DTR is supposed to mean Data Terminal Ready, it really seems
like a bug on the USB host side that it isn't ready when it sets DTR
high.
This workaround was added by Zach Eveland in
ade4893f585e3e94fa6cf683620e1d12afc88ecd, but it says it was noticed by
Federico. Federico, do you remember under what OS and conditions this
problem triggered?
Anyone want to try removing the delay and see if the problem still
exists on the supported OSes?
Gr.
Matthijs
Reply all
Reply to author
Forward
0 new messages