Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

There is no stream.available for rx #1930

Closed
jantje opened this issue Mar 12, 2014 · 18 comments
Closed

There is no stream.available for rx #1930

jantje opened this issue Mar 12, 2014 · 18 comments
Milestone

Comments

@jantje
Copy link

jantje commented Mar 12, 2014

As explained in http://arduino.cc/en/Reference/StreamAvailable#.UyDJNvFl7PE
available tells how much data is available for receiving in the stream.
There is however no method how much is available or waiting for sending.
This lack makes it impossible for a sketch to be smart about what it sends.

@PaulStoffregen
Copy link
Contributor

Yes, I've long wanted to add Stream::ready()

@jantje
Copy link
Author

jantje commented Mar 14, 2014

Hi Paul thanks for agreeing on the concern.
Personally I think "availlable" and "ready" are to common and do not specify about which queue ( send queue or receive queue) we are talking.
I modified my code so that available takes a boolean which specifies the queue and which defaults to rx.
However I don't like that as well because it is another parameter which increases program size and it hardly makes things clearer.
I now think of adding 2 methods
availableInSendQueue and availableInReceiveQue (the best I could think of right now). Available like in "How much data is available."
Making available inline non virtual method implemented as { return availableInReceiveQue();} should make for a "hardly no impact at all solution".

Some more background why I need this (and this would be the perfect time for the Arudino core team to make me do the work ;-) )
I log data every x millis in the loop for logging and running my web site to control arduino. I'm talking about debugging so there is quite some data (800+characters per log)
I need to log in 1 go because otherwise I get inconsistent data.
If I use 9600BAUD it takes about 1ms per character to send. With a buffer of maximum 255 (due to uint8_t) it takes at least (800-255=545) ms for sending the data. Which is about half a second my robot is not responding (completely unacceptable for a robot).
Currently I can not check whether the buffer is empty so I may have 800ms unresponsiveness.
Because the logging itself (without the actual write) takes about 55ms that makes a total of 855ms.
I could increase the BAUD rate but unfortunately I have not been able to get "100% reliable communication" yet at higher Baud rates.
But if I can not test the transmission buffer and I have 10 times better transmission it will still take 55ms for processing and 80ms transmission totalling 135ms which is still to much for me as I prefer to log every second.

@jantje
Copy link
Author

jantje commented Mar 21, 2014

I have implemented the previous solution and I didn't like it (again :-( )
I'm now pretty pleased with this code (in stream)

    virtual int available()=0 ;
    int  availableInReceiveQue(){ return available();};
    virtual int  availableInSendQueue() {return -1;};

this is 100% compatible with existing libraries and sketches and allows for extension of availableInSendQueue() with a consistent naming.
I'm still doubting whether availableInReceiveQue() should be virtual or not. I prefer it virtual but there is a program and memory size impact.

my hardware serial class definition looks like

    virtual int  available();
    virtual int  availableInSendQueue();

and the code

int  HardwareSerial::available()
{
    return (unsigned int)(SERIAL_RX_BUFFER_SIZE + _rx_buffer_head - _rx_buffer_tail) % SERIAL_RX_BUFFER_SIZE;
}

int  HardwareSerial::availableInSendQueue()
{
    return (unsigned int)(SERIAL_TX_BUFFER_SIZE + _tx_buffer_head - _tx_buffer_tail) % SERIAL_TX_BUFFER_SIZE;
}

I have an idea about the growth path (getting rid of available) but due to lack of interest I won't bother to explain.

@jantje
Copy link
Author

jantje commented Jul 4, 2014

can this please be taken into consideration?

@PaulStoffregen
Copy link
Contributor

I believe this issue is a duplicate of #1408 and maybe #672

@jantje
Copy link
Author

jantje commented Jul 7, 2014

It is a duplicate but if you only fi it in hardware serial the fix of the duplicate is not good enough for me.
@PaulStoffregen
Please look at the code I came up with and posted above. It handles nearly all concerns posted on the developers list.
It is backwards compatible, allows for the symmetric names for tx and rx and by using -1 as default return value one can test whether the available for tx is implemented.
It only needs a mod in stream and hardware serial.

@bperrybap
Copy link

I like this solution better than any of the other solutions I've seen so far and MUCH
better than having available() in Stream and then a proprietary writeBufferFree() function that may or may not exist down in a library.
It offers consistency and backward compatibility since all the new code will use
availableInReceiveQue()
and
availableInSendQueue()

@jantje
Copy link
Author

jantje commented Jul 8, 2014

@bperrybap
Thanks.
As you can see I use this and it is the result of many trials and errors.

@cmaglie
Copy link
Member

cmaglie commented Jul 13, 2014

Really interesting!

@jantje
is there a reason to put availableInSendQueue() in Stream instead of Print?
The write method is defined in Print so I'm expecting to find the corresponding availableInSendQueue() there too.

@cmaglie cmaglie added this to the Release 1.5.8 milestone Jul 13, 2014
@jantje
Copy link
Author

jantje commented Jul 13, 2014

This question was also asked by matthijskooijman in #1408
I have no objections in putting the method in Print. However the most logical name to me in that case would be availableInWriteQueue()
In that case I would rename availableInReceiveQue to availableInReadQue
I'm picky on naming but as I'm not responsible for the naming I'm not going to make a fuss of it.

@cmaglie
Copy link
Member

cmaglie commented Jul 13, 2014

This question was also asked by matthijskooijman in #1408
I have no objections in putting the method in Print

Oh, I see, missed that, thanks.

However the most logical name to me in that case would be availableInWriteQueue()
In that case I would rename availableInReceiveQue to availableInReadQue

mmhh, may I ask why you feel the need to specify that there is a "queue"?
I mean, the original method was called available() and not availableInQueue() (the fact that there is a queue was implicit).

Now, since we are going to handle input and output, I expect that available() becomes something like availableRead()/availableWrite() or availableForRead()/availableForWrite().

@bperrybap
Copy link

On 07/13/2014 11:13 AM, Cristian Maglie wrote:

This question was also asked by matthijskooijman in #1408 <https://github.com/arduino/Arduino/issues/1408>
I have no objections in putting the method in Print

Oh, I see, missed that, thanks.

However the most logical name to me in that case would be availableInWriteQueue()
In that case I would rename availableInReceiveQue to availableInReadQue

mmhh, may I ask why you feel the need to specify that there is a "queue"?
I mean, the original method was called |available()| and not |availableInQueue()| (the fact that there is a queue was implicit).

Now, since we are going to handle input and output, I expect that |available()| becomes something like |availableRead()/availableWrite()| or |availableForRead()/availableForWrite()|.


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

I like names being consistent and to have a name that can help
portray the functionality.

I don't have particularly strong feelings on the names
but I'd pick either:
availableInReadQue()/availableInWriteQue()
or
availableForRead()/availableForWrite()

BTW, I realize this next part below is pretty far off topic for the "available" discussion,
but while we are looking at adding a new functions to the Stream/Print APIs,
flush() has somewhat similar confusing issues.
Currently flush() is in Stream and with the exception of flush() all other Stream implemented functions
are for the read direction.
Originally flush() was also for the read side which made sense because Stream only implements
read side functions. (It inherits the write side from print)
When flush changed to being on the write side, things went wonky
and we got things like HardwareSerial and SoftwareSerial implementing it on different
directions having very different functionality.
It isn't obvious from looking at the Stream header what flush() does
since there is no comment where flush() is declared in the class.

Maybe "flush" could use an update as well?

Currently, since flush() is in Stream,
there is no common way to tell a device library that uses Print but not Stream
to force buffered data to be sent to the device
without dragging in Stream which doesn't see right to me.

When might having "flush" without Stream be useful?
Say you had a graphic device like a GLCD which is output only so it only uses Print.
Maybe it does buffered writes in local memory.
flush() could be used to force the dirty local buffer to be pushed to the physical display.

I probably should start a new thread on the developers list for this,
since backward compatibility and naming
issues are just as complex and potentially contentious as with the available() functions.
Especially since there is also the throw the data away vs wait for the i/o to complete
functionality, and names like flush/purge/drain/empty have not been used consistently
across different operating systems, and APIs so it is easy to get confused by names.

I only brought it up here because, timing wise,
if there is going to be changes to Stream & Print,
it might be good to also do changes to "flush" at the same time.
And if updates to "flush" were desired,
it also might be useful to consider all the new function names of both the "available" and
the "flush" functions at the same time
to have some sort of consistency between them.

--- bill

@jantje
Copy link
Author

jantje commented Jul 13, 2014

as I stated before. Available just doesn't say anything.
Finding good names is hard. In my experience longer names make less confusing code.
Rereading the above I think availableInWriteQueue() is not even 60% perfect. It can be understood as "available written data in write queue" (which is the current implementation) and as "available for adding data to write queue".
I agree that Queue is an implementation and is not a good idea to add to the name.

As to availableForRead()/availableForWrite()
availableForWrite is not compliant with the code as the code is exactly the same as the existing available. So it is more StillToWrite(). For my code that works fine as I test for StillToWrite()==0.
But due to the dubious nature of the word "available" one is fully right when disagreeing :-(

There is lots of confusion and doubt on my side. I think the word "available" is part of the problem.
What I think users want as functionality(And that Is not what I implemented) is

  1. How much data is there for me to read.
  2. How much room for data is there for me to write.

Note that that is a "different calculation" on a "different queue". I think this should be reflected in the names:
The best I can come up with right now is:
stillToRead()/availableForWrite()
I hope other people have better ideas.

@bperrybap
Copy link

On 07/13/2014 04:57 PM, jantje wrote:

As to availableForRead()/availableForWrite()
availableForWrite is not compliant with the code as the code is exactly the same as the existing available.

I'm not understanding this.

The existing available() function in Stream is for the read side and
returns how much data is available for reading using read()
read more than that, and currently, you will get a -1 return status.

There is lots of confusion and doubt on my side. I think the word "available" is part of the problem.
What I think users want as functionality(And that Is not what I implemented) is

  1. How much data is there for me to read.
  2. How much room for data is there for me to write.

I thought everyone was in agreement that those 2 functions are what is needed
and I thought that is what your code implemented.
#1 was availableInReceiveQue()
#2 was availableInSendQue()
(Perhaps I misunderstood your code)

And I thought this was the current proposal:

#1 is availableForRead() would be the same as the existing available() and
returns how much data is available for reading using read()
#2 is availableForWrite() returns how much additional data can be written using write() before a block would likely occur.

Those names seem fairly consistent and descriptive to me as both are returning
how much data can be written or read using write()/read() before i/o blocking issues come into play.

I think of it as
{data} Available For reading using read() before blocking issues come into play
{data} Available For writing using write() before blocking issues come into play

@jantje
Copy link
Author

jantje commented Jul 13, 2014

As I stated: I get/got very confused by the dual meaning of the word available in a buffer (available to read versus available to write).
I thought my implementation of availableInSendQueue() returned the buffer available to write to.
But ... When I was rereading I noticed that the code didn't do so. Then I checked my code and saw that I was testing for availableInSendQueue()==0 while I want to test that the buffer is fully available for writing. :-(
Because of the double negative everything works fine ..... So the new code could be:

int  HardwareSerial::[propose a good name]()
{
    return (int)SERIAL_TX_BUFFER_SIZE - (( int)(SERIAL_TX_BUFFER_SIZE + _tx_buffer_head - _tx_buffer_tail) % SERIAL_TX_BUFFER_SIZE);
}

@cmaglie
Copy link
Member

cmaglie commented Jul 14, 2014

When might having "flush" without Stream be useful?
I probably should start a new thread on the developers list for this,

Please do, this question deserves his own thread.

Originally flush() was also for the read side which made sense because Stream only implements read side functions. (It inherits the write side from print) When flush changed to being on the write side, things went wonky

I always asked myself why the flush() method was in Stream instead of Print. Now I know the reason.

@jantje
Copy link
Author

jantje commented Jul 14, 2014

As I stated: I get/got very confused by the dual meaning of the word available in a buffer (available to read versus available to write).

This is again confusingly worded. New try: The two meanings I see for available in one queue
1 "Available space to add data to the queue"
2 "Available data to process from the queue"

@cmaglie
Copy link
Member

cmaglie commented Sep 12, 2014

Fixed with #2193

@cmaglie cmaglie closed this as completed Sep 12, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants