Problems on serial.read()

Hi fellows,

I am new in Arduino world, and Im trying to make my final project in the plataform Arduino UNO. But I am having a kind of problem that I am not being able to solve. That´s why I am here to ask the community help.

So, what I am trying to do is make two hops with three Arduinos. In a nutshell, I´m trying to send by Xbee Shield a packet in this format
(2#1#0#) → (destinationHost, sourceHost, hop). To you understand, my 3 arduinos, have an identification such as 1, 2 and 3.
If I tell you that I am sending a packet from (1) to (2), I wold use this (2#1#0#).

But as I told, I am having problem in a method of my code that is displayed bellow:

Beacon Service::recebeBeacon(short id) {

Beacon beacon;
String msg;

while(Serial.available() > 0) {
byte temp = Serial.read();
msg = msg + (char) temp;
}
Serial.flush();

if (msg.length() > 0) {
beacon = converteStringParaBeacon(msg);
delay(1000);
Serial.flush();

if (verificaMensagemBeacon(beacon,id)) {
return beacon;
}
}
return beacon;
}

My arduino (1) just sends the packet like this (2#1#0#), and it is ok!
void loop() {

//Send beacon com Dest(2), Source(1), Hop(0)
beacon = Beacon(2,1,0);
service.enviaBeacon(beacon);
delay(2000);

}
But, when I get the message with Arduino (2), I get a lot of garbage, however the message comes two. Look at the picture attached and
you´ll understand. Please, does anyone knows what is happening???
void loop() {

recebe = service.recebeBeacon(ard_8);
delay(2000);

//recebe.getDhost(), recebe.getShost(), recebe.getSALTO() are short types
beacon = Beacon(recebe.getDhost(), recebe.getShost(), recebe.getSALTO());
Serial.println(service.converteBeaconParaString(beacon));

delay(2000);
}

Am I being clear, guys?

Thank you so muck for your attention.

  1. The String class is a memory killer, you better use char array’s . A bit more work but they work better.
  2. do you synchronize your packets somehow? begin/end of the message (tip use a special chat like < and > )
  3. you are not posting the whole code that makes it harder to analyze

and if you post please use the # button (above the smileys) to get code proper tagged. Makes it more readable.

  1. char array´s.
    How can I do it, please? Can you give a tip?

I think if you see the whole code it´ll get clear;
This is my Beacon.cpp

#include "Beacon.h"
#include <stdio.h>
#include <string.h>

Beacon::Beacon() {
}

Beacon::Beacon(short meu_id) {
	this->dhost = 0; // host de destino
	this->shost = 0; // host de origem
	this->salto = 0; // (metrica SALTO, numero de saltos para cada no)
	//this->meu_id = meu_id;
}

Beacon::Beacon(short dhost, short shost, short salto) {
	this->dhost = dhost; // destination host 
	this->shost = shost; // source host
	this->salto = salto; // hops
	
}

This is my Service.cpp

//send beacon
void Service::enviaBeacon(Beacon beacon) {
	if(beacon.getDhost() != 0){
	  String msg = converteBeaconParaString(beacon);
	  Serial.println(msg);
	}

}
//receive beacon
Beacon Service::recebeBeacon(short id) {

	Beacon beacon;
	String msg;
	
	while(Serial.available() > 0) {
		byte temp = Serial.read();
		msg = msg + (char) temp;	
				
	}
	Serial.flush();
	
	
	if (msg.length() > 0) {
		beacon = converteStringParaBeacon(msg);
		delay(1000);
		Serial.flush();
		
		
		if (verificaMensagemBeacon(beacon,id)) {
			return beacon;
		}
	}
	return beacon;
}
//convert Beacon to String
String Service::converteBeaconParaString(Beacon beacon) {
	char buf[10];
		sprintf(buf, "%d#%d#%d#", beacon.getDhost(), beacon.getShost(), beacon.getSALTO());
	return buf;
}

//convert  String to Beacon
Beacon Service::converteStringParaBeacon(String msg) {
	int c, i;
	short vet[2];
	Beacon beacon;
	for (c = i = 0; c < 2; c++) {
		vet[c] = (short) msg.substring(i, msg.indexOf('#', i)).toInt();
		i = msg.indexOf("#", i) + 1;
	}

	beacon.setDhost(vet[0]);
	beacon.setShost(vet[1]);
	beacon.setSALTO(vet[2]);
		
	return beacon;
}

Clear now? Any suggestion?

Thank a lot.

There is a little bug at “converteStringParaBeacon” method.

Beacon Service::converteStringParaBeacon(String msg) {
	int c, i;
	short vet[2];   // must be vet[3]: you are using three elements at [0], [1] and [2] below 
	Beacon beacon;
	for (c = i = 0; c < 2; c++) {  
              // 'c' will be 0 and 1 values only. The third element (index [2])will not be read.
              // because when 'c=2' then 'c<2' is false.
		vet[c] = (short) msg.substring(i, msg.indexOf('#', i)).toInt();
		i = msg.indexOf("#", i) + 1;
	}

	beacon.setDhost(vet[0]);
	beacon.setShost(vet[1]);
	beacon.setSALTO(vet[2]);   // vet[2] POINTS TO A INVALID LOCATION you have only 2 elements...
		
	return beacon;
}

Fix this and see if works.

Sergio Figueiredo
http://blog.devdelver.com/archives/category/arduino

I have seen a more serious problem with your converteStringParaBeacon() method: It returns a local object reference (beacon). This may cause a "Undefined behavior". You can see more here: http://stackoverflow.com/questions/10930509/scope-of-a-local-object

EDITED: I am wrong and your code is valid. You are not returning a reference to a object. The C++ will make a copy of the local object to return it.

@ Sergio

I appreciate your attention. The bug is already corrected I've put

short vet [3];

@smarcelo Thanks too. I didnt understand, but can you help me solve it? Do you know how to solve it? I'd appreciate.

I was wrong about return local objects. The problem of ‘undefined behavior’ only happens if you are return references to a local object. A local object is always destroyed at the end of function/method. But, in your case, you are return a object by value. So, the C++ will make a safe copy before destroy it.

Are you corrected the ‘for’ loop too?

I think must be like this to get the ‘salto’ value from string:

for (c = i = 0; c < 3; c++) {
Beacon Service::converteStringParaBeacon(String msg) {
	int c, i;
	short vet[3];
	Beacon beacon;
	for (c = i = 0; c < 3; c++) {         [b]  // If I put c<3 and not c<2 it get worst [/b]
		vet[c] = (short) msg.substring(i, msg.indexOf('#', i)).toInt();
		i = msg.indexOf("#", i) + 1;
	}

	beacon.setDhost(vet[0]);
	beacon.setShost(vet[1]);
	beacon.setSALTO(vet[2]);
		
	return beacon;
}

Look at the picture!!! At leat with C<2 I still get in the midle of garbage 2#1#0# ahuahuahah

But, I thank you so much for help. Please, stay with me! :slight_smile:

Beacon Service::converteStringParaBeacon(String msg) { int c, i;

c and i can probably be of the type uint8_t (byte) as the values will not exceed 255 (or do they?)

    Serial.flush();

Block until all pending serial data has been sent. How is this useful?

Hey, about the 255 code I've put this, but it doesnt function!!!

    Beacon beacon;
    String msg;

    while(Serial.available() > 0) {
        byte temp = Serial.read();
        if (temp != 255){
        msg = msg + (char) temp;    
        }

    }

Do you have more suggestion?

Thanks folks,

AugustoSantos: Hey, about the 255 code I've put this, but it doesnt function!!!

Oh, it functions. How it functions may not be congruent with how you're expecting it to function, and unless you tell us what you're expecting it to do, and what it is doing, it's hard to determine what is causing the difference.

Ok Ok, I agree with your comment!!! I really need that when the arduino get the packet, it doen not come with so much garbage. I think the problem is with the "recebe" method. But I really do not know what is happening. One told me to handle XCTU for xbee, I'm looking for information about this. What do you think??

thanks

But I really do not know what is happening.

Until you do, you shouldn't be shooting yourself in the foot with the String class.

By the way, snippets-r-us.com is down the internet a ways. Here, we expect to see ALL of your code.

Ok, here is my whole code:
What suggestion you give me with String class??
thanks

#include "Beacon.h"

#include <stdio.h>
#include <string.h>


Beacon::Beacon() {
  
}

Beacon::Beacon(short meu_id) {
	this->dhost = 0; // host de destino
	this->shost = 0; // host de origem
	this->salto = 0; 
	
}

Beacon::Beacon(short dhost, short shost, short salto) {
	this->dhost = dhost; 
	this->shost = shost;
	this->salto = salto;

}

Beacon::~Beacon() {/*nothing to destruct*/
}


short Beacon::getDhost() const {
	return dhost;
}

short Beacon::getShost() const {
	return shost;
}

short Beacon::getSALTO() const {
	return salto;
}

void Beacon::setDhost(short dhost) {
	this->dhost = dhost;
}

void Beacon::setShost(short shost) {
	this->shost = shost;
}

void Beacon::setSALTO(short salto) {
	this->salto = salto;
}
#ifndef BEACON_H_
#define BEACON_H_

#include <Arduino.h>
#include <string.h>

class Beacon {
public:
	Beacon();
	Beacon(short meu_id);
	Beacon(short dhost,short shost, short salto);
	~Beacon();	
    short getDhost() const;
    short getShost() const;
    short getSALTO() const;
    void setDhost(short dhost);
    void setShost(short shost);
    void setSALTO(short salto);
    
private:
	short dhost;
	short shost;
	short salto;
};

#endif
#include "Service.h"

#include "Beacon.h"
#include <stdio.h>
#include <string.h>


short meu_id; //identificação do no raiz

//Estrutura do pacote de controle
Service::Service() {
}

Service::~Service() {/*nothing to destruct*/
}

void Service::enviaBeacon(Beacon beacon) {
	if(beacon.getDhost() != 0){
	  String msg = converteBeaconParaString(beacon);
	  Serial.println(msg);
	}

}

Beacon Service::recebeBeacon(short id) {

	Beacon beacon;
	String msg;
	
	while(Serial.available() > 0) {
		byte temp = Serial.read();
		if (temp != 255){
		msg = msg + (char) temp;	
		}
				
	}
	if (msg.length() > 0) {
	  beacon = converteStringParaBeacon(msg);
	  delay(1000);
	  Serial.flush();
		
		if (verificaMensagemBeacon(beacon,id)) {
			return beacon;
		}
	}
	return beacon;
}

// Funcao que converte inteiro para String. Arduino trata somente string
String Service::converteBeaconParaString(Beacon beacon) {
	char buf[10];
	sprintf(buf, "%d#%d#%d#", beacon.getDhost(), beacon.getShost(), beacon.getSALTO());
	return buf;
}


Beacon Service::converteStringParaBeacon(String msg) {
	int c, i;
	short vet[3];
	Beacon beacon;
	for (c = i = 0; c < 3; c++) {
		vet[c] = (short) msg.substring(i, msg.indexOf('#', i)).toInt();
		i = msg.indexOf("#", i) + 1;
	}

	beacon.setDhost(vet[0]);
	beacon.setShost(vet[1]);
	beacon.setSALTO(vet[2]);
		
	return beacon;
}


bool Service::verificaMensagemBeacon(Beacon beacon, short id) {
	
      //Verifica se a mensagem é para ele.
	if (beacon.getDhost() == id) {
	  return true;
	}
		
	return false;
}
#ifndef SERVICE_H_
#define SERVICE_H_

#include <Arduino.h>
#include <string.h>
#include <Beacon.h>


class Service {
public:
	Service();
	~Service();
    String converteBeaconParaString(Beacon beacon);
    Beacon converteStringParaBeacon(String msg);
    void enviaBeacon(Beacon beacon);
    Beacon recebeBeacon(short id);
    bool verificaMensagemBeacon(Beacon beacon,short id);

};

#endif

//Arduino that sends the packet

#include <Service.h>
#include <Beacon.h>

Beacon beacon;
Service service;

short ard_8 = 1; //Arduino 08
short ard_5 = 2; //Arduino 05



void setup() {     
   Serial.begin(9600);  
}


void loop() {
          
           //Enviar beacon com D(ard_5), S(ard_8), 0
           beacon = Beacon(ard_5, ard_8, 0);
           service.enviaBeacon(beacon);
           delay(2000);
               
}

//Arduino that receives packet (The problem is here)

#include <Beacon.h>
#include <Service.h>

Beacon beacon;
Beacon recebe;
Service service;

short ard_8 = 1; //Arduino 08
short ard_5 = 2; //Arduino 05



void setup() {     
   Serial.begin(9600);  
}


void loop() {
  
           recebe = service.recebeBeacon(ard_8);
           
           delay(2000); 
           
           //recebe.getSALTO() ta dando problema no metodo recebe que nao sei qual e. Isso ta afetando o getSALTO()
            beacon = Beacon(recebe.getDhost(), recebe.getShost(), recebe.getSALTO());         
           Serial.println(service.converteBeaconParaString(beacon));

AugustoSantos: What suggestion you give me with String class??

Don't use it.

And use what?? You didnt tell me the suggestion!!! :) Array of bytes, char... ???

And the reason to not use it. Can you explain me and give me a tip about how to use what do you think is better in my code?

And use what?

Isn't that obvious? What are you storing in the String? Hint: It's not doubles, floats, longs, booleans, bytes, or instances of classes.

And the reason to not use it.

Because it has a destructor. Now, normally having a destructor is a good thing. But, the destructor calls free() to return the memory that it has allocated, and free() has a bug that causes the destruction of memory.

String Service::converteBeaconParaString(Beacon beacon) {
    char buf[10];
    sprintf(buf, "%d#%d#%d#", beacon.getDhost(), beacon.getShost(), beacon.getSALTO());
    return buf;
}

buf is NOT a String.

buf is a local variable that is going out of scope as soon as the function ends. Returning buf by any means is a bad idea.

The String class suffers from memory management problems and memory leaks.

Use 'C' strings instead i.e. null terminated char arrays. There is a rich library of functions in the C runtime library for dealing with strings, and you can also manipulate them simply by accessing the array elements directly.