Function intended to read entire serial input into a char array reads just one char at the time

I did the following function that is intended to read entire serial input into a char array and yet just reads one char at the time!

int serialRead(char *text)
{
    int i = 0;
    for (; i < 16 && Serial.available() > 0; i++) {
        text[i] = (char)Serial.read();
    }
    text[i] = '\0';
    return i;
}

Being a for loop why it breaks for each single char despite a full string being typed?

With that if(), 'i' increases faster than Serial.read() happens. Or Serial.available() is zero simetimes.

What you mean, serial doesn't read one char at each time?

You are most likely reading the data out faster than it can come in, so Serial.available is false and the loop exits quickly.

int serialRead(char *text)
{
    int i = 0;
    while( i < 16)
    {
        while( !Serial.available() )
        {
            delay(1);
        }
        text[i] = (char)Serial.read();
        i++;
    }
    text[i] = '\0';
    return i;
}

There's probably a more efficient way to write this, but I was in a hurry. Also, you should have some kind of timeout mechanism in case the serial output stops for some reason.

1 Like

It does read one character at a time. Serial.available() is often just 1 or 0, as the characters dribble in.

Without seeing how you are calling this, it is hard to make a good suggestion about how to do it better.

Your function could… return zero every time you call it until such time as it has finished taking in data from the serial port, either because it counts to 16 or sees a carriage return or other delimiter, or whatever.

Only then would the function return non-zero, the number of characters waiting in the array where you have been parking them.

Spend some time reading

about all the tricks.

HTH

a7

1 Like

Ok, I see it now, I end up setting 5ms, because 1 millisecond still resulted in broken strings:

int serialRead(char *text)
{
    int i = 0;
    for (; i < 15 && Serial.available() > 0; i++) {
        text[i] = (char)Serial.read();
        delay(5); // Waits for next byte to fill the buffer. 1 baud = 1 byte per second.
    }
    text[i] = '\0';
    return i;
}

You probably used Serial.begin(9600); in your setup, but we'd never know because you didn't share that.

With that setting, incoming characters arrive about every 1.3 milliseconds, presuming the sender is able to transmit seamlessly. Serial Monitor does do so, if you type your string and then hit enter.
There's no need to use 9600. Set your SerialMonitor to 115200, and put that number into the begin statement, and you'd likely not have to use delay() at all.

HOWEVER, that doesn't address the fact that your code may compile and work at that higher speed, but was still poor code. That, the others have addressed, though you seem to have ignored their advice.

1 Like

Ok, about the delimiter, and because I just want the text and not the new line char, here is the adjusted version to also take into account your remarks:

int serialRead(char *text)
{
    int i = 0;
    for (; i < 15 && Serial.available() > 0; i++) {
        text[i] = (char)Serial.read();
        if (text[i] == '\n' || text[i] == '\t' || text[i] == '\r')
            break;
        delay(5); // Waits for next byte to fill the buffer. 1 baud = 1 byte per second.
    }
    text[i] = '\0';
    return i;
}

Here's your code in a simple program:

char buffer[100];

void setup() {
  Serial.begin(9600);
  Serial.println("enter chars: (qwertyuiopasdfghjklzxcvbnmqwertyuiopasdfghjklzxcvbnm is good)");
}

void loop() {
  int i = serialRead(buffer);
  if (i > 0){
    Serial.print(i);
    Serial.print(":");
    Serial.print(buffer);
    Serial.println(':');
  }

  if (false && Serial.available() > 0) {
    delay(1);
    int j = Serial.available();
    Serial.readBytesUntil('\n', buffer, j);
    Serial.print(':');
    Serial.print(buffer);
    Serial.println(':');
  }
}

int serialRead(char *text)
{
    int i = 0;
    for (; i < 16 && Serial.available() > 0; i++) {
        text[i] = (char)Serial.read();
    }
    text[i] = '\0';
    return i;
}

int serialReadCL(char *text)
{
    int i = 0;
    while( i < 16)
    {
        while( !Serial.available() )
        {
            delay(1);
        }
        text[i] = (char)Serial.read();
        i++;
    }
    text[i] = '\0';
    return i;
}

with output from Wokwi:

enter chars: (qwertyuiopasdfghjklzxcvbnmqwertyuiopasdfghjklzxcvbnm is good)
1:q:
1:w:
1:e:
1:r:
1:t:
1:y:
1:u:
1:i:
1:o:
1:p:
1:a:
1:s:
1:d:
2:fg:
7:hjklzxc:
12:vbnmqwertyui:
16:opasdfghjklzxcvb:
3:nm
:

1 Like

You are slowly arriving at the reinvention of readBytesUntik().

Reads to the delimiter or until the buffer provided is full or it times out waiting.

a7

3 Likes

interesting... why separate definition of i iterator?

isn't it simple and better:

  for (int i = 0 ; i < 15 && Serial.available() > 0; i++) {

and even better use byte instead of int

This is very different from what I wrote. But if it works for you, it works.

You know, I thought that I remembered the Arduino serial library had a better way of doing this, but I didn't have the time to research it!

I end up with a version that is slightly better than the one suggested, because I need to split the input into words so just one delimiter is not enough. This way the alternative bellow can process multiple end chars as the end char and not just one like the one suggested. so, here it is:

size_t readSerialUntil(const char *characters, char *buffer, size_t length)
{
    size_t i = 0;
    if (Serial.available() > 0)
    {
        unsigned long last_read_millis = millis();
        do
        {
            if (Serial.available() > 0)
            {
                buffer[i] = (char)Serial.read();
                if (strchr(characters, buffer[i]))
                    break;
                last_read_millis = millis();
                i++;
            }
        } while (millis() - last_read_millis < 3 && i < length - 1);
    }
    buffer[i] = '\0';
    return i;
}

And in my particular case I call it like so:
readSerialUntil("\n\t\r \0", serial_read, 16);

I tested it and it works pretty well.

Not trying to nitpick, but the variable last_read_millis doesn't even need to be initialized due to the loop being a do while one, so, that line can just be:

    ...
        unsigned long last_read_millis;
        ...

This topic was automatically closed 180 days after the last reply. New replies are no longer allowed.