Code optimization / Hacky code

Projects Programming
1

Hi!
I have a project, that uses a module which has variable output. The module communicates using RS485. To interface with it, I use an LCD and 2 buttons. One button is of digit selection, and the other is for value (+1).

A slight issue I have is that the screen is slightly visibly flickering.

I am in no way a good programmer, so I also want some comments on my way of execution. I feel that the way I change the module with variables is a bit hacky, so I am open for suggestions.

#include <LiquidCrystal.h>

// Module command
char command[12] = "#010+00.000\r";

//Temporary storage for conversions.
char writetemp;
char readtemp;
int readtemp_int;

// Pin declarations
const int sw = 7;
const int sw2 = 8;
const int rs = 12, en = 11, d4 = 5, d5 = 4, d6 = 3, d7 = 2;

// Switch counters
int digitselect = 5;


//Display
LiquidCrystal lcd(rs, en, d4, d5, d6, d7);

void setup() {
  Serial.begin(9600);
  lcd.begin(16, 2);
  pinMode(8, INPUT_PULLUP);
  pinMode(7, INPUT_PULLUP);

}

void loop() {
  // Reads button state
  int sw_state = digitalRead(sw);
  int sw2_state = digitalRead(sw2);

  //Enables blinking on lcd (To know which digit is selected).
  lcd.blink();

  // DIGIT SELECTION
  if (sw_state == 0) {
    delay(100);
    digitselect++;
    if (digitselect-5 == 2) { digitselect++;}
    if (digitselect > 10) { digitselect = 5;}
  }
  // Clears the previous value.
  delay(50);
  lcd.clear();

  // Print value on lcd
  for (int b = 5;b > 4 && b < 11; b++) {
    lcd.print(command[b]);
  } 

  lcd.print(" mA");



  // Sets the cursor on the selected digit.
  lcd.setCursor(digitselect-5, 0);


  //Reads selected digit from command array.
  readtemp = command[digitselect];

  //Converts character to integer
  readtemp_int = readtemp - '0';

  // If button is pressed, increases integer by 1,
  // in case integer is > 9 resets to 0.
  if (sw2_state == 0) {
    delay(100);
    readtemp_int++;
    if (readtemp_int > 9) {
      readtemp_int = 0;
    }
    // Converts integer back to character.
    writetemp =  '0' + readtemp_int;
    //Changes digit in character array.
    command[digitselect] = writetemp;
  }
  //Sends command to module.
  Serial.write(command);
  
}
2 
  // Clears the previous value.
  delay(50);
  lcd.clear();
  // Print value on lcd

Why clear the LCD and write to it every time through loop() even if there has been no change ?

3

That is a really good point, how would I detect a change?

4

Keep track of the previous temp
when the new temp does not equal the previous temp print it.

Also change only what changes. :stuck_out_tongue_winking_eye:
determine the maximum space for the temp and only clear that space(by printing spaces) and then print only the changed temp.
This eliminates flickering.

5

as others mentions, delays may cause flicker.

consider
simplified code, removing unnecessary variables and lines making the code easier to read
reorganized code so that value is updated and then displayed
not sure about blink, does it need to be call each time in loop() or once in setup()?

# include <LiquidCrystal.h>

const int sw = 7;
const int sw2 = 8;

// LiquidCrystal lcd (rs, en, d4, d5, d6, d7);
LiquidCrystal lcd    (12, 11,  5,  4,  3,  2);

// display string
char command [] = "#010+00.000";

int digitselect = 5;
int digit;

// -----------------------------------------------------------------------------
void setup () {
    Serial.begin (9600);
    lcd.begin (16, 2);
    pinMode (sw,  INPUT_PULLUP);
    pinMode (sw2, INPUT_PULLUP);
}

// -----------------------------------------------------------------------------
void loop ()
{
    // DIGIT SELECTION
    if (LOW ==  digitalRead (sw)) {
        digitselect++;
        if (digitselect-5 == 2)
            digitselect++;
        if (digitselect > 10)
            digitselect = 5;
    }

    // increment value
    if (LOW == digitalRead (sw2))  {
        digit = command [digitselect] - '0';
        digit = 9 > digit ? digit+1 : 0;
        command [digitselect] = digit + '0';
    }

    // update display
    lcd.clear ();
    lcd.print (& command [5]);
    lcd.print (" mA");

    lcd.blink ();
    lcd.setCursor (digitselect-5, 0);

    Serial.write (command);
    Serial.write ('\r');
    delay (100);
}
6

A few comments on your original code:

char command[12] = "#010+00.000\r";

command is too small, the text has 12 characters, you need to add an additional char to store the terminating NULL. gcjr's code solves this by using command[] which lets the compiler set the size based on what is needed.

    if (digitselect-5 == 2) { digitselect++;}

Looks a bit convoluted, (digitselect == 7) is easier to read, but defining decimalPosition as 7 would let you use (digitselect == decimalPositon) so someone (including yourself a year later) will know the reason for the comparison. SImilarly 5 and 10 could be given names such as firstDigit and lastDigit.
Makes the code easier to understand, and eliminates the actual numbers from the code, which will make it much easier if you ever want to change the position or length of the number.

  // Print value on lcd
  for (int b = 5;b > 4 && b < 11; b++) {
    lcd.print(command[b]);
  }

Checking for b > 4 is unnecessary, b starts at 5 and will never go below that.

  //Reads selected digit from command array.
  readtemp = command[digitselect];

  //Converts character to integer
  readtemp_int = readtemp - '0';

  // If button is pressed, increases integer by 1,
  // in case integer is > 9 resets to 0.
  if (sw2_state == 0) {
    delay(100);
    readtemp_int++;
    if (readtemp_int > 9) {
      readtemp_int = 0;
    }
    // Converts integer back to character.
    writetemp =  '0' + readtemp_int;
    //Changes digit in character array.
    command[digitselect] = writetemp;
    command[digitselect]++;
  }

readtemp is not needed, should be able to increment command[digitselect] directly, as long as you change the comparison from > 9 to > '9'.

  lcd.clear();

  // Print value on lcd
  for (int b = 5;b > 4 && b < 11; b++) {
    lcd.print(command[b]);
  } 

  lcd.print(" mA");

Clearing the display is completely unnecessary, you are always writing the same number of characters at the same position, so all that is needed is overwriting the previous characters. The " mA" does not need to be written repeatedly, once in setup would be sufficient if you never clear the display. Have not test it, but lcd.write(&command[5], 6) would be simpler than the for loop. (write 6 characters beginning at command[5]).

7

Hi! Thank you everyone for the help.
Sorry for the late response. This is how I've changed the code:

#include <LiquidCrystal.h>

// Module command
char command[] = "#010+00.000";


// Pin declarations
const int sw = 7;
const int sw2 = 8;

// Display
LiquidCrystal lcd    (12, 11,  5,  4,  3,  2);

// Variables for detecting change
int lastsw;
int lastsw2;


int digitselect = 5;
int digit;
int channel;


void setup() {
  Serial.begin(9600);
  lcd.begin(16, 2);
  lcd.blink();

  pinMode(sw, INPUT_PULLUP);
  pinMode(sw2, INPUT_PULLUP);

//Print the starting values
  lcd.print("00.000 mA");
  lcd.setCursor(0,1);
  lcd.print("CHN: 0");
  lcd.setCursor(0, 0);
}

void loop() {
  lastsw = (digitalRead(sw));
  lastsw2 = (digitalRead(sw2));
  // DIGIT SELECTION
  if (digitalRead(sw) == 0) {
    delay(100);
    digitselect++;
    if (digitselect-5 == 2) { digitselect++;}
    if (digitselect > 11) { digitselect = 5;}
  }

  if (digitselect < 11) {
    //VALUE INCREMENTATION
    if (digitalRead(sw2) == 0) {
    delay(100);
    digit = command [digitselect] - '0';
    digit = 9 > digit ? digit+1 : 0;
    command [digitselect] = digit + '0';
    }
  }

  else if (digitselect == 11){
    lcd.setCursor(5, 1);
    // CHANNEL SELECTION
    if (digitalRead(sw2) == LOW) {
    delay(200);
    channel = command [3] - '0';
    digit = 1 > digit ? digit+1 : 0;
    command [3] = digit + '0';
    }
  }
  if (lastsw != digitalRead(sw) || lastsw2 != digitalRead(sw2)) {
  // Print value on lcd
  lcd.clear();
  lcd.print (& command [5]);
  lcd.print (" mA");
  lcd.setCursor(0, 1);
  lcd.print("CHN: ");
  lcd.print(command[3]);

  // Sets the cursor on the selected digit.
  lcd.setCursor(digitselect-5, 0);
  
  //Sends command to module.
  Serial.write(command);
  Serial.write('\r');
  }
}

But I've run into an issue. The digit selection / value incimentation are too fast. And It's generally unstable, to get the desired input, I have to really quickly press the button. i've been thinking about this...

8

You need to detect when the button [color = red]becomes[/color] pressed rather than when it [color = red]is[/color] pressed
See the StateChangeDetection example in the IDE

9

you have various delays at odd spots in code.

replace all of them with just one 100 msec delay at the end of loop() which will slow things down

but as @UKHeliBob suggested, it would be more reliable to check for button presses, not that they are being pressed by tracking the last position and only checking if the button is pressed when there is a change in state

    byte but = (digitalRead(sw));
    if (swLst != but)  {
        swLst = but;
        if (LOW == but) {
            ...

and similar or sw2

10

Thank you everyone!
Sorry for a late update.
I've finished the code and I've eliminated all my previous issues.

#include <LiquidCrystal.h>
/*
  Changelog from VER.2
  Fixed LCD flicker, by only printing to display, if there is a change.
  Added 2 channel switching.

  Finished - REV.3
  Orlando Dlugoš Čeh 30.11.2022
*/

// Module command
char command[] = "#010+00.000";


// Pin declarations
const int sw = 7;
const int sw2 = 8;

// Display
LiquidCrystal lcd    (12, 11,  5,  4,  3,  2);

int lastsw;
int lastsw2;

int sw_store = 0;
int sw2_store = 0;


int digitselect = 5;
int digit;
int channel;


void setup() {
  Serial.begin(9600);
  lcd.begin(16, 2);
  lcd.blink();

  pinMode(sw, INPUT_PULLUP);
  pinMode(sw2, INPUT_PULLUP);

  lcd.print("00.000 mA");
  lcd.setCursor(0,1);
  lcd.print("CHN: 0");
  lcd.setCursor(0, 0);
}

void loop() {

int sw_detect = digitalRead(sw);
int sw2_detect = digitalRead(sw2);

  // DIGIT SELECTION
  if (sw_detect != sw_store) {
      if (sw_detect == LOW) {
      digitselect++;
      if (digitselect-5 == 2) { digitselect++;}
      if (digitselect > 11) { digitselect = 5;}
      }
  }
  sw_store = (sw_detect);

  //VALUE INCREMENTATION
  if (digitselect < 11) {
    if (sw2_detect != sw2_store) {
      if (sw2_detect == LOW) {
        digit = command [digitselect] - '0';
        digit = 9 > digit ? digit+1 : 0;
        command [digitselect] = digit + '0';
      }
    }
     sw2_store = (sw2_detect);
  }
  else if (digitselect == 11){
    lcd.setCursor(5, 1);
    // CHANNEL SELECTION
    if (sw2_detect != sw2_store) {
      if (sw2_detect == LOW) {
      channel = command [3] - '0';
      digit = 1 > digit ? digit+1 : 0;
      command [3] = digit + '0';
      }
    }
  }
  
   if (sw_detect == LOW || sw2_detect == LOW) {
      // Print value on lcd
      lcd.clear();
      lcd.print (& command [5]);
      lcd.print (" mA");
      lcd.setCursor(0, 1);
      lcd.print("CHN: ");
      lcd.print(command[3]);

      // Sets the cursor on the selected digit.
      lcd.setCursor(digitselect-5, 0);
      
      //Sends command to module.
      Serial.write(command);
      Serial.write('\r');
   }

   delay(100);
  }

This is the code, I consider this project finished.
Thanks again everyone for the help.