Code behaves weirdly

C4lculated · March 23, 2019, 10:34am

Hello everyone, I can't figure out why my sketch gives strange results.
Hardware info: I'm using a cheap Pro Mini clone (ATmega328p 16Mhz) connected to a 4 pin I2C 128x64 OLED LCD with an SSD1306 controller (SCL & SDA connected to A5 & A4).

This is my entire sketch. It requires the U8g2 library to be installed:

#include <Arduino.h>
#include <U8g2lib.h>

#define REVERSE 1
#define ALIGNRIGHT 2

//Functions
void reverseheb(unsigned char *string);
void printheb(uint8_t xpos, uint8_t y, char ch[], uint8_t flags);

#ifdef U8X8_HAVE_HW_SPI
#include <SPI.h>
#endif
#ifdef U8X8_HAVE_HW_I2C
#include <Wire.h>
#endif

U8G2_SSD1306_128X64_NONAME_F_HW_I2C u8g2(U8G2_R0, U8X8_PIN_NONE);

void setup(void) {
  u8g2.begin();
  Serial.begin(57600);
}

void loop(void) {
  u8g2.setFont(u8g2_font_ncenB10_tr);
  u8g2.clearBuffer();
  printheb(0, 20, "This is just a simple test.", REVERSE | ALIGNRIGHT);
  u8g2.sendBuffer();
  delay(5000);
}


void reverseheb(unsigned char *string)
{
    int i,j;
    char *temp;
    //
    j = strlen(string);
    temp = (char*)malloc(j+1);
    temp[j] = 0;
    for(i=0 ; string[i] ; i++)
    {
      if(string[i] == 0xD7)
      {
        temp[j-i-1] = string[i+1];
        temp[j-i-2] = 0xD7;
        i++;
      }
      else
      {
        if((string[i] < '0' || string[i] > '9') && (string[i] < 'A' || string[i] > 'Z') && (string[i] < 'a' || string[i] > 'z')) temp[j-i-1] = string[i];
        else
        {
          int m,k;
          k = i;
          while((string[i] >= '0' && string[i] <= '9') || (string[i] >= 'A' && string[i] <= 'Z') || (string[i] >= 'a' && string[i] <= 'z'))
          {
            i++;
          }
          i--;
          for(m=0 ; m<(i-k+1) ; m++)
          {
            temp[j-(k+m)-1] = string[i-m];
          }
        }
      }
    }
    strcpy(string,temp);
    free(temp);
}

void printheb(uint8_t xpos, uint8_t y, char ch[], uint8_t flags)
{
   int i,j,k;
   char tmp;
   char *temp;
   //
   if(!flags) // if not REVERSE and not ALIGNRIGHT
   {
     u8g2.drawUTF8(xpos, y, ch);
     return;
   }
   if(((flags & 1) == 1) && ((flags & 2) == 0) ) // (Line 84) if REVERSE but not ALIGNRIGHT
   //if(flags  == 1) // (Line 85)
   {
     temp = (char *)malloc(strlen(ch) + 1);
     strcpy(temp, ch);
     reverseheb(temp); // (Line 89)
     Serial.println("Alert");
     u8g2.drawUTF8(xpos, y, temp);
   }
   if((flags & 2) == 0) return;
   
   k = 0;
   for(i=0,j=0 ; ch[i] ;)
   {
     j++;
     if(ch[i] == (char)0xD7) i+=2;
       else i++;
     if(j == (  (  (u8g2.getDisplayWidth() - xpos)/u8g2.getMaxCharWidth())  )  || !ch[i])
     {
       tmp = ch[i];
       ch[i] = 0;
       temp = (char *)malloc(strlen(&ch[k]) + 1);
       strcpy(temp, &ch[k]);
       if((flags & 1) == 1) // if REVERSE
       {
         reverseheb(temp);
       }
       u8g2.drawUTF8((u8g2.getDisplayWidth() - (u8g2.getMaxCharWidth() * j) - xpos), y, temp);
       free(temp);
       y+=u8g2.getMaxCharHeight();
       ch[i] = tmp;
       if(ch[i] == 0) break;
       k = i;
       j = 0;
     }
   }
}

The problem: The output is incorrect. Debugging the code shows that the cause is at the if block at line 84 (I have commented the line number in the sketch).

If the block is removed, the output is correct.
If line 84 is commented out and line 85 is uncommented the output is correct.
If line 89 is commented out, the output is correct.

Note: Nothing is printed to the serial monitor, so the condition in line 84 is not met but despite that somehow it still affects the output and I don't understand why.

lesept · March 23, 2019, 11:05am

Have you tried to print the temp variable on the serial monitor before line 89?

MorganS · March 23, 2019, 12:02pm

That is a very long way of saying "it doesnt work".

When the output is not correct, what does it actually do?

Is this just an exercise to practise the use of malloc() and free()? Because you could get the same output without using dynamic memory.

system · March 23, 2019, 1:09pm

(deleted)

system · March 23, 2019, 1:15pm

spycatcher2k:
This looks wrong
for(i=0,j=0 ; ch[i] ;)

It looks bad, but not wrong.
The increments are in the body of the loop.

Comments or better variable names throughout would help legibility.

C4lculated · March 24, 2019, 2:13am

lesept:
Have you tried to print the temp variable on the serial monitor before line 89?

I have not. I'll try it now.

MorganS:
Is this just an exercise to practise the use of malloc() and free()? Because you could get the same output without using dynamic memory.

Exercise? No, this a function to reverse and right-align Hebrew glyphs in Unicode font (Hebrew is a right-to-left language, like Arabic).
I changed the example string to English just to make it more legible for you guys.
I'm using dynamic allocation because I don't know the length of the string.

MorganS:
When the output is not correct, what does it actually do?

It's hard to understand what the output is because it's not designed to work with English, but for the given example text I've made mock-up images:

Incorrect(no change to the if condition):

Correct:

But the thing is, the output shouldn't be relevant because the main issue here is that if an if block is not executed it should not be able to change ANYTHING (other than the program storage space).

AWOL:
Comments or better variable names throughout would help legibility.

Yeah, I know it's a mess, this is just how I work. I always tell myself that when I'm done I'll overhaul everything but to this day that has never happened.

C4lculated · March 24, 2019, 2:45am

C4lculated:
I have not. I'll try it now.

Printing temp before line 89 doesn't do anything because temp is only allocated later, inside the for loop. However, printing temp after line 105 (strcpy(temp, &ch[k])) gives the following results:

Incorrect(line 84 uncommented, line 85 commented out):

This is just a simple test.
just a simple test.
imple test.
st.

Correct(line 85 uncommented, line 84 commented out):

This is 
just a s
imple te
st.

(4 lines because the for loop runs 4 times in this example)

C4lculated · March 25, 2019, 7:12am

Thanks to lesept's suggestion, I have greatly simplified the sketch and now no external components are necessary (LCD) and neither are any libraries (Ug82).

#include <Arduino.h>

#define REVERSE 1
#define ALIGNRIGHT 2

//Functions
void reverseheb(unsigned char *string);
void printheb(uint8_t xpos, uint8_t y, char ch[], uint8_t flags);

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

void loop(void) {
  printheb(0, 20, "This is just a simple test.", REVERSE | ALIGNRIGHT);
  delay(5000);
}


void reverseheb(unsigned char *string)
{
    int i,j;
    char *temp;
    //
    j = strlen(string);
    temp = (char*)malloc(j+1);
    temp[j] = 0;
    for(i=0 ; string[i] ; i++)
    {
      if(string[i] == 0xD7)
      {
        temp[j-i-1] = string[i+1];
        temp[j-i-2] = 0xD7;
        i++;
      }
      else
      {
        if((string[i] < '0' || string[i] > '9') && (string[i] < 'A' || string[i] > 'Z') && (string[i] < 'a' || string[i] > 'z')) temp[j-i-1] = string[i];
        else
        {
          int m,k;
          k = i;
          while((string[i] >= '0' && string[i] <= '9') || (string[i] >= 'A' && string[i] <= 'Z') || (string[i] >= 'a' && string[i] <= 'z'))
          {
            i++;
          }
          i--;
          for(m=0 ; m<(i-k+1) ; m++)
          {
            temp[j-(k+m)-1] = string[i-m];
          }
        }
      }
    }
    strcpy(string,temp);
    free(temp);
}

void printheb(uint8_t xpos, uint8_t y, char ch[], uint8_t flags)
{
   int i,j,k;
   char tmp;
   char *temp;
   //
   if(!flags) // if not REVERSE and not ALIGNRIGHT
   {
     Serial.println(ch);
     return;
   }
   if(((flags & 1) == 1) && ((flags & 2) == 0) ) // (Line 70) if REVERSE but not ALIGNRIGHT
   //if(flags  == 1) // (Line 71)
   {
     temp = (char *)malloc(strlen(ch) + 1);
     strcpy(temp, ch);
     reverseheb(temp); // (Line 75)
     Serial.println("This is never executed.");
   }
   if((flags & 2) == 0) return;
   k = 0;
   for(i=0,j=0 ; ch[i] ;)
   {
     j++;
     if(ch[i] == (char)0xD7) i+=2;
       else i++;
     if(j == (  (  (128 - xpos)/6)  )  || !ch[i])
     {
       tmp = ch[i];
       ch[i] = 0;
       temp = (char *)malloc(strlen(&ch[k]) + 1);
       strcpy(temp, &ch[k]);
       if((flags & 1) == 1) // if REVERSE
       {
         reverseheb(temp);
       }
       Serial.println(temp);
       free(temp);
       y+=13;
       ch[i] = tmp;
       if(ch[i] == 0) break;
       k = i;
       j = 0;
     }
   }
}

So, to reproduce the problem all you need to do is:

Upload the sketch to an Arduino board (I only tested with a Pro Mini but my hunch is that any board will do)
View the output on the serial monitor
Make one of the following changes (doesn't matter which one):

Remove the if block that starts at line 70 and ends at line 77
Comment out line 70 and uncomment line 71
Comment out line 75

View the output on the serial monitor again

Result: The output on the serial monitor should be different.

I'd sure love to know why this is happening.

pjrc · March 30, 2019, 12:59pm

C4lculated:
I only tested with a Pro Mini but my hunch is that any board will do

No, in fact this fails on all ARM boards, due to 2 mistakes.

1: Your printheb() function modifies the string, but you're calling it with a read-only string constant.

2: You're mixing pointers to unsigned char and regular char. That's not legal C++ syntax. Recommend you use File > Preferences to turn on compiler warnings.

Regarding your original question, I looked at your code only briefly. It's a huge mess. But my guess is the confusion is you're calling reverseheb() in 2 places. Indeed the call from line 75 is never executed. However, the call to that function is executed TWICE from line 93.

Maybe if you run this, things will become clearer.

#include <Arduino.h>

#define REVERSE 1
#define ALIGNRIGHT 2

//Functions
void reverseheb(unsigned char *string);
void printheb(uint8_t xpos, uint8_t y, char ch[], uint8_t flags);

void setup(void) {
  Serial.begin(57600);
  while (!Serial) ;
}

void loop(void) {
  char mystr[] = "This is just a simple test.";
  printheb(0, 20, mystr, REVERSE | ALIGNRIGHT);
  delay(5000);
}


void reverseheb(unsigned char *string, const char *called_from)
{
    int i,j;
    char *temp;
    //
    Serial.print("reverseheb called from: ");
    Serial.println(called_from);
    j = strlen(string);
    temp = (char*)malloc(j+1);
    temp[j] = 0;
    for(i=0 ; string[i] ; i++)
    {
      if(string[i] == 0xD7)
      {
        temp[j-i-1] = string[i+1];
        temp[j-i-2] = 0xD7;
        i++;
      }
      else
      {
        if((string[i] < '0' || string[i] > '9') && (string[i] < 'A' || string[i] > 'Z') && (string[i] < 'a' || string[i] > 'z')) temp[j-i-1] = string[i];
        else
        {
          int m,k;
          k = i;
          while((string[i] >= '0' && string[i] <= '9') || (string[i] >= 'A' && string[i] <= 'Z') || (string[i] >= 'a' && string[i] <= 'z'))
          {
            i++;
          }
          i--;
          for(m=0 ; m<(i-k+1) ; m++)
          {
            temp[j-(k+m)-1] = string[i-m];
          }
        }
      }
    }
    strcpy(string,temp);
    free(temp);
}

void printheb(uint8_t xpos, uint8_t y, char ch[], uint8_t flags)
{
   int i,j,k;
   char tmp;
   char *temp;
   //
   if(!flags) // if not REVERSE and not ALIGNRIGHT
   {
     Serial.println(ch);
     return;
   }
   if(((flags & 1) == 1) && ((flags & 2) == 0) ) // (Line 70) if REVERSE but not ALIGNRIGHT
   //if(flags  == 1) // (Line 71)
   {
     temp = (char *)malloc(strlen(ch) + 1);
     strcpy(temp, ch);
     reverseheb(temp, "first location"); // (Line 75)
     Serial.println("This is never executed.");
   }
   if((flags & 2) == 0) return;
   k = 0;
   for(i=0,j=0 ; ch[i] ;)
   {
     j++;
     if(ch[i] == (char)0xD7) i+=2;
       else i++;
     if(j == (  (  (128 - xpos)/6)  )  || !ch[i])
     {
       tmp = ch[i];
       ch[i] = 0;
       temp = (char *)malloc(strlen(&ch[k]) + 1);
       strcpy(temp, &ch[k]);
       if((flags & 1) == 1) // if REVERSE
       {
         reverseheb(temp, "second location");
       }
       Serial.println(temp);
       free(temp);
       y+=13;
       ch[i] = tmp;
       if(ch[i] == 0) break;
       k = i;
       j = 0;
     }
   }
}

Of course, everything would be much clearer if you used more conventional programming style.

There may also be a bug where the same buffer will be freed twice if the REVERSE flag is not set. Again, a simple coding style where you call malloc() and free() from the same function would make these sorts of problems much less likely.

Likewise for the checking of 4 possible cases of those 2 flags. A switch/case block, or 4 if-else checks gives you a code structure that makes the cases much more apparent. While your style may be technically ok syntax (ignoring the compiler warnings and writing to read-only strings), this sort of style is terrible practice. It almost always leads to mistakes, because your code is so difficult to read.

C4lculated · April 6, 2019, 6:01am

pjrc:
No, in fact this fails on all ARM boards, due to 2 mistakes.

1: Your printheb() function modifies the string, but you're calling it with a read-only string constant.

2: You're mixing pointers to unsigned char and regular char. That's not legal C++ syntax. Recommend you use File > Preferences to turn on compiler warnings.

Thank you Paul, I corrected these 2 problems and now everything seems to be working.
To be honest, I did know I had many little mistakes in the code but I just didn't think they would affect the issue that I was seeing.

(Fixed program, just for reference)

#include <Arduino.h>

#define REVERSE 1
#define ALIGNRIGHT 2

//Functions
void reverseheb(char *string);
void printheb(uint8_t xpos, uint8_t y, char ch[], uint8_t flags);

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

void loop(void) {
  printheb(0, 20, "This is just a simple test.", REVERSE | ALIGNRIGHT);
  delay(5000);
}


void reverseheb(char *string)
{
    int i,j;
    char *temp;
    //
    j = strlen(string);
    temp = (char*)malloc(j+1);
    temp[j] = 0;
    for(i=0 ; string[i] ; i++)
    {
      if(string[i] == (char)0xD7)
      {
        temp[j-i-1] = string[i+1];
        temp[j-i-2] = (char)0xD7;
        i++;
      }
      else
      {
        if((string[i] < '0' || string[i] > '9') && (string[i] < 'A' || string[i] > 'Z') && (string[i] < 'a' || string[i] > 'z')) temp[j-i-1] = string[i];
        else
        {
          int m,k;
          k = i;
          while((string[i] >= '0' && string[i] <= '9') || (string[i] >= 'A' && string[i] <= 'Z') || (string[i] >= 'a' && string[i] <= 'z'))
          {
            i++;
          }
          i--;
          for(m=0 ; m<(i-k+1) ; m++)
          {
            temp[j-(k+m)-1] = string[i-m];
          }
        }
      }
    }
    strcpy(string,temp);
    free(temp);
}

void printheb(uint8_t xpos, uint8_t y, char ch[], uint8_t flags)
{
   int i,j,k;
   char *temp;
   //
   if(!flags) // if not REVERSE and not ALIGNRIGHT
   {
     Serial.println(ch);
     return;
   }
   if(((flags & 1) == 1) && ((flags & 2) == 0) ) // (Line 70) if REVERSE but not ALIGNRIGHT
   //if(flags  == 1) // (Line 71)
   {
     temp = (char *)malloc(strlen(ch) + 1);
     strcpy(temp, ch);
     reverseheb(temp); // (Line 75)
     Serial.println("This is never executed.");
     free(temp);
   }
   if((flags & 2) == 0) return;
   k = 0;
   for(i=0,j=0 ; ch[i] ;)
   {
     j++;
     if(ch[i] == (char)0xD7) i+=2;
       else i++;
     if(j == (  (  (128 - xpos)/6)  )  || !ch[i])
     {
       temp = (char *)malloc((i-k) + 1);
       strncpy(temp, &ch[k], (i-k));
       temp[i-k] = 0;
       if((flags & 1) == 1) // if REVERSE
       {
         reverseheb(temp);
       }
       Serial.println(temp);
       free(temp);
       y+=13;
       if(ch[i] == 0) break;
       k = i;
       j = 0;
     }
   }
}

pjrc:
...you're calling reverseheb() in 2 places.

pjrc:
...the call to that function is executed TWICE from line 93.

That is intended behavior. It is probably impossible to understand the function of the original program because I stripped all the code that was not necessary to this issue, and heavily changed the remaining code.

pjrc:
There may also be a bug where the same buffer will be freed twice if the REVERSE flag is not set. Again, a simple coding style where you call malloc() and free() from the same function would make these sorts of problems much less likely.

I thought I was calling malloc() and free() from the same function... Maybe I don't understand what you mean...

Anyway, thanks again and sorry for my bad coding. I know it's terrible, I guess there is just a certain amount of time someone can put into a hobby.

system · May 5, 2021, 11:43pm