Bug? - additional zero when Serial0 not begun

Using Arduino Programming Questions
1

Hey all,
I am unsure if this is a bug or if I trigger a latent weirdness.
Background: Arduino Mega to control multiple devices via 3 hardware serial ports (1-3) via an MAX3243.

I set up a simple wrapper class around the serial ports and predefine the command arrays for the device control in my class header file in the init area as const static to save sram. The wrapper class initializes its according port and write various commands to it. Now the weird part.

My commands are stored in a char array:
const static char cmd_info_stat[] = {0x7E,0x30,0x30,0x31,0x35,0x30,0x20,0x31,0x0d};
This is important so that I can terminate with a \r char. The command is then sent via the serial write (I also tried .print(xxx), but same issue):
(*projector).write(cmd_info_stat);

If only serial1 or higher is initialized in the main ino file via a wrapper class, the serial sent output has an added 0 in:
~00150 01
If, however, in the main function I begin serial0 (you know, "Serial") BEFORE initializing the wrapper class, the correct output is sent via serial1:
~00150 1
I can not wrap (pun intended) my head around it. On what behaviour do I hit here?

2

In order for print to know where to stop, the array needs to be terminated with a null character.

const static char cmd_info_stat[] = {0x7E,0x30,0x30,0x31,0x35,0x30,0x20,0x31,0x0d, 0x00};

That null won't be sent to the device. It's just there to mark the end of the character string for the print command.

Whatever you were doing with the other serial line obviously ended putting a zero in the memory slot behind that array, which made it appear to work. In the other configuration there was a 0x30 there and it printed '0'.

3

Another option if you want to keep the array defined like this is to use the version of write that takes two arguments. The second one is the number of bytes to send.

Serial.write(cmd_info_stat, sizeof(cmd_info_stat));

4

^^^ this.

.write WONT STOP at the null character unless you write that logic, which is intrinsic to the .print function.

5

Thanks for all the answers. However, I am not using mainly .print, I was only testing ALSO with it. I used normally only write. Also, the 0 was not inserted at the end of the slot (I would have expected then some kind of termination stuff), but IN the string, before the 1 at the end. I will try out the second write function with the length tomorrow and see how it handles..

6

But should write not also stop writing at the end of the input array?

7

Unless you tell it how many bytes are in the array it has no idea of knowing where the end is.

8

You only showed us a couple of lines of code. You may have a buffer overrun or a NPE or any other kind of thing going on. If you want more specific answers then post a piece of code that compiles and exhibits the indicated behavior so we can try it for ourselves and try to see what's wrong.

9

How about posting the complete code rather than all the hand waiving?

1 Like
10

@gfvalvo @Delta_G
This is the complete code.
Main ino:

#include "Projector_wrapper.h"
#include <string.h>


Projector_wrapper proj(1);

void setup() {
  pinMode(LED_BUILTIN, OUTPUT);
  bool blink = false;
  Serial.begin(9600);
  while (!proj.exists()) {
    delay(1000);
    if(blink) {digitalWrite(LED_BUILTIN, HIGH);}
    else {digitalWrite(LED_BUILTIN, LOW);}
    blink = !blink;
    //Serial.println(proj.get_buffer());
  }
  proj.proj_pwr_on();
  digitalWrite(LED_BUILTIN, HIGH);
  while (!proj.ready_for_signal()) {
      delay(50);
  }
  delay(5000);
  proj.proj_pwr_off();
  digitalWrite(LED_BUILTIN, LOW);
}

void loop() {
  // put your main code here, to run repeatedly:

}

Projector_wrapper.h

#ifndef Projector_wrapper_h
#define Projector_wrapper_h
#define buffer_size 18

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

/*const static String cmd_pwr_on = "~0000 1\r";
const static String cmd_pwr_off = "~0000 0\r";
const static String cmd_pwr_stat = "~00124 1\r";
const static String cmd_info_stat = "~00150 1\r";*/
const static char cmd_pwr_on[] =    {0x7E,0x30,0x30,0x30,0x30,0x20,0x31,0x0d};
const static char cmd_pwr_off[] =   {0x7E,0x30,0x30,0x30,0x30,0x20,0x30,0x0d};
const static char cmd_pwr_stat[] =  {0x7E,0x30,0x30,0x31,0x32,0x34,0x20,0x31,0x0d};
const static char cmd_info_stat[] = {0x7E,0x30,0x30,0x31,0x35,0x30,0x20,0x31,0x0d};

class Projector_wrapper{
  public:
    Projector_wrapper(uint8_t portnr);
    bool proj_pwr_on();
    bool proj_pwr_off();
    bool ready_for_signal();
    bool exists();
    String get_buffer();
  private:
    void read_into_rxbuffer();
    void clear_rxbuffer();
    //void write_word(String &txword);
    HardwareSerial* projector = nullptr;
    String rxbuffer = "";
    char txbuffer[buffer_size] = "";
    unsigned int lamp_hours = 0;
};

#endif

Projector_wrapper.cpp

#include "Projector_wrapper.h"

Projector_wrapper::Projector_wrapper(uint8_t portnr){
  switch (portnr){
    case 1:
      Serial1.begin(9600);
      projector = &Serial1;
      break;
    case 2:
      Serial2.begin(9600);
      projector = &Serial2;
      break;
    case 3:
      Serial3.begin(9600);
      projector = &Serial3;
      break;
  }
}

/*void Projector_wrapper::write_word(String &txword){
  
  //txbuffer = txword.c_str();
  for(char x : txword.c_str()){
    (*projector).write(x);
  }
}*/
void Projector_wrapper::clear_rxbuffer(){
  rxbuffer = "";
}

void Projector_wrapper::read_into_rxbuffer(){
  clear_rxbuffer();
  while((*projector).available()){
    rxbuffer.concat((char)(*projector).read());
  }
}

bool Projector_wrapper::proj_pwr_on(){
  (*projector).write(cmd_pwr_on);
  delay(50);
  read_into_rxbuffer();
  if (rxbuffer[0] == 'P'){
    return true;
  }
  return false;
}

bool Projector_wrapper::proj_pwr_off(){
  (*projector).write(cmd_pwr_off);
  delay(50);
  read_into_rxbuffer();
  if(rxbuffer[0]=='P'){
    return true;
  }
  return false;
}

bool Projector_wrapper::ready_for_signal(){
  (*projector).write(cmd_info_stat);
  delay(50);
  read_into_rxbuffer();
  if(rxbuffer[2]=='1'){
    return true;
  }
  return false;
}

bool Projector_wrapper::exists() {
    (*projector).write(cmd_info_stat);
    delay(50);
    read_into_rxbuffer();
    if (rxbuffer[0] == 'O') {
        return true;
    }
    return false;
}

String Projector_wrapper::get_buffer(){
    while ((*projector).available()) {
        rxbuffer.concat((char)(*projector).read());
    }
    return(rxbuffer);
}
11

Just started looking at it, but right off the bat, don't call Serialx.begin() from the class's constructor. Constructors for global objects run before the hardware is initialized by init(). Results are unpredictable if you set up the UARTS before init(). Instead, provide a separate begin() function in your class that's called from setup() and calls Serialx.begin().

12

As already pointed out, that overload of write() continues to send data until if finds a null character ('\0'). It's just dumb luck if it happens to stop at the end of your array as intended rather than continuing on accessing unrelated memory.

13

Both of those #include(s) are unnecessary given #include <Arduino.h>

14

  1. Why did you abandon using string literals and switch to the less readable hex notation?

  2. Why are these character strings global instead of being encapsulated into the class?

  3. Even if you wanted them to be global, variable definitions don't belong in .h files. The definitions go into a .cpp and only declarations go in the .h.

So, I'd modify the .h file:

#ifndef Projector_wrapper_h
#define Projector_wrapper_h
#define buffer_size 18

#include <Arduino.h>

class Projector_wrapper {
  public:
    Projector_wrapper(uint8_t portnr);
    bool proj_pwr_on();
    bool proj_pwr_off();
    bool ready_for_signal();
    bool exists();
    String get_buffer();
  private:
    void read_into_rxbuffer();
    void clear_rxbuffer();
    //void write_word(String &txword);
    HardwareSerial* projector = nullptr;
    String rxbuffer = "";
    char txbuffer[buffer_size] = "";
    unsigned int lamp_hours = 0;

    static constexpr char cmd_pwr_on[] = "~0000 1\r";
    static constexpr char cmd_pwr_off[] = "~0000 0\r";
    static constexpr char cmd_pwr_stat[] = "~00124 1\r";
    static constexpr char cmd_info_stat[] = "~00150 1\r";
};

#endif

And then add to the top of the .cpp file (after the #include):

constexpr char Projector_wrapper::cmd_pwr_on[];
constexpr char Projector_wrapper::cmd_pwr_off[];
constexpr char Projector_wrapper::cmd_pwr_stat[];
constexpr char Projector_wrapper::cmd_info_stat[];
15

@gfvalvo @Delta_G Thank you, I incorporated your recommendations. Especially the fixed write length seems to have made the difference. I tried with a string as well as my previously declared as hex chars character array, both work. I preferred using the hex notation as it felt easier for me to expand it to use control characters which are less common later in case I want to expand it to projector models which use a more exotic control set.
I declared them global in the notation that I use less sram if I declare them static global, so that they are instanciated only once in total. The global namespace was kind of a side effect, so I modified it along your recommendations. For other people, this is the final working code (cycling the projector once as soon as its available):

ino:

#include "Projector_wrapper.h"
#include <string.h>


Projector_wrapper proj(1);

void setup() {
  proj.begin_serial();
  pinMode(LED_BUILTIN, OUTPUT);
  bool blink = false;
  //Serial.begin(9600);
  while (!proj.exists()) {
    delay(1000);
    if(blink) {digitalWrite(LED_BUILTIN, HIGH);}
    else {digitalWrite(LED_BUILTIN, LOW);}
    blink = !blink;
    //Serial.println(proj.get_buffer());
  }
  proj.proj_pwr_on();
  digitalWrite(LED_BUILTIN, HIGH);
  while (!proj.ready_for_signal()) {
      delay(50);
  }
  delay(5000);
  proj.proj_pwr_off();
  digitalWrite(LED_BUILTIN, LOW);
}

void loop() {
  // put your main code here, to run repeatedly:

}

Projector_wrapper.h:

#ifndef Projector_wrapper_h
#define Projector_wrapper_h
#define buffer_size 18

#include <Arduino.h>

/*const static String cmd_pwr_on = "~0000 1\r";
const static String cmd_pwr_off = "~0000 0\r";
const static String cmd_pwr_stat = "~00124 1\r";
const static String cmd_info_stat = "~00150 1\r";*/
/*const static char cmd_pwr_on[] =    {0x7E,0x30,0x30,0x30,0x30,0x20,0x31,0x0d};
const static char cmd_pwr_off[] =   {0x7E,0x30,0x30,0x30,0x30,0x20,0x30,0x0d};
const static char cmd_pwr_stat[] =  {0x7E,0x30,0x30,0x31,0x32,0x34,0x20,0x31,0x0d};
const static char cmd_info_stat[] = {0x7E,0x30,0x30,0x31,0x35,0x30,0x20,0x31,0x0d};*/

class Projector_wrapper{
  public:
    Projector_wrapper(uint8_t x) : portnr(x){};
    bool proj_pwr_on();
    bool proj_pwr_off();
    bool ready_for_signal();
    bool exists();
    String get_buffer();
    void begin_serial();
  private:
    void read_into_rxbuffer();
    void clear_rxbuffer();
    //void write_word(String &txword);
    HardwareSerial* projector = nullptr;
    String rxbuffer = "";
    char txbuffer[buffer_size] = "";
    unsigned int lamp_hours = 0;
    uint8_t portnr = 0;

    /*static constexpr char cmd_pwr_on[] = "~0000 1\r";
    static constexpr char cmd_pwr_off[] = "~0000 0\r";
    static constexpr char cmd_pwr_stat[] = "~00124 1\r";
    static constexpr char cmd_info_stat[] = "~00150 1\r";*/
    constexpr static char cmd_pwr_on[] =    {0x7E,0x30,0x30,0x30,0x30,0x20,0x31,0x0d};
    constexpr static char cmd_pwr_off[] =   {0x7E,0x30,0x30,0x30,0x30,0x20,0x30,0x0d};
    constexpr static char cmd_pwr_stat[] =  {0x7E,0x30,0x30,0x31,0x32,0x34,0x20,0x31,0x0d};
    constexpr static char cmd_info_stat[] = {0x7E,0x30,0x30,0x31,0x35,0x30,0x20,0x31,0x0d};
};

#endif

Projector_wrapper.cpp:

#include "Projector_wrapper.h"

constexpr char Projector_wrapper::cmd_pwr_on[];
constexpr char Projector_wrapper::cmd_pwr_off[];
constexpr char Projector_wrapper::cmd_pwr_stat[];
constexpr char Projector_wrapper::cmd_info_stat[];

void Projector_wrapper::begin_serial(){
  switch (portnr){
    case 1:
      Serial1.begin(9600);
      projector = &Serial1;
      break;
    case 2:
      Serial2.begin(9600);
      projector = &Serial2;
      break;
    case 3:
      Serial3.begin(9600);
      projector = &Serial3;
      break;
  }
}

/*void Projector_wrapper::write_word(String &txword){
  
  //txbuffer = txword.c_str();
  for(char x : txword.c_str()){
    (*projector).write(x);
  }
}*/
void Projector_wrapper::clear_rxbuffer(){
  rxbuffer = "";
}

void Projector_wrapper::read_into_rxbuffer(){
  clear_rxbuffer();
  while((*projector).available()){
    rxbuffer.concat((char)(*projector).read());
  }
}

bool Projector_wrapper::proj_pwr_on(){
  (*projector).write(cmd_pwr_on,8);
  delay(50);
  read_into_rxbuffer();
  if (rxbuffer[0] == 'P'){
    return true;
  }
  return false;
}

bool Projector_wrapper::proj_pwr_off(){
  (*projector).write(cmd_pwr_off,8);
  delay(50);
  read_into_rxbuffer();
  if(rxbuffer[0]=='P'){
    return true;
  }
  return false;
}

bool Projector_wrapper::ready_for_signal(){
  (*projector).write(cmd_info_stat,9);
  delay(50);
  read_into_rxbuffer();
  if(rxbuffer[2]=='1'){
    return true;
  }
  return false;
}

bool Projector_wrapper::exists() {
    (*projector).write(cmd_info_stat,9);
    delay(50);
    read_into_rxbuffer();
    if (rxbuffer[0] == 'O') {
        return true;
    }
    return false;
}

String Projector_wrapper::get_buffer(){
    while ((*projector).available()) {
        rxbuffer.concat((char)(*projector).read());
    }
    return(rxbuffer);
}

Thank you!

16

You're chasing a false economy. Any "exotic" characters can always be entered with an escape sequence with in the ASCII string literal. The literal is also much more readable making it easier to catch mistakes.

Nope. Global and static global use the same amount of memory. And it's the same a static class variables which is where they belong per my code (and your latest).

Statements like that are going to cause you problems because when you change things, the size given to write() could get out of synch with the actual size of the array. Just put a terminating null at the end and leave out the length in the call to write(). That way it will go until it finds the null, but won't send it.

17

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