Simplifying harder statements to easier more understandable code

frasercanavan · November 12, 2021, 1:32am

For one of my projects I have to use code to program 2 595n Shift registers, and I was going to use EEPROM code and BYTEs to program it. My issue is that I don't have the full knowledge about EEPROM and BYTE, and I was wondering if there were another other types of code that could be used in the same working function as EEPROM, but easier to understand/read.
This is my code right now and I want to simplify it by using easier code to understand instead of EEPROM.

#include <EEPROM.h>
byte letter_one = 0;
byte letter_two = 0;
uint16_t letter_one_binary = 0;
uint16_t letter_two_binary = 0;
uint16_t segmentletters[] = {
  0b1110110010001000,  //A
  0b1111001010100000,  //B
  0b1001110000000000,  //C
  0b1111001000100000,  //D
  0b1001110010001000,  //E
  0b1000110010001000,  //F
  0b1011110010000000,  //G
  0b0110110010001000,  //H
  0b1001001000100000,  //I
  0b0111100000000000,  //J
  0b0000110101001000,  //K
  0b0001110000000000,  //L
  0b0110110100000100,  //M
  0b0110110001000100,  //N
  0b1111110000000000,  //O
  0b1100110010001000,  //P
  0b1111110001000000,  //Q
  0b1100110011001000,  //R
  0b1011010010001000,  //S
  0b1000001000100000,  //T
  0b0111110000000000,  //U
  0b0000110100010000,  //V
  0b0110110001010000,  //W
  0b0000000101010100,  //X
  0b0000000100100100,  //Y
  0b1001000100010000   //Z
};
uint8_t clockPin = 11;// define the pin usage for the MBv3
uint8_t clockPin1 = 5;
uint8_t dataPin = 9;         //
uint8_t latchPin = 10;        //
uint8_t outputEnablePin = 8;
uint8_t sizeMap = sizeof(segmentletters) >> 1; //number of entries in the LUT



void setup() {

  Serial.begin(9600);
  for (uint8_t i = 0; i <= sizeMap; i++) {  //iterate through the LUT array
    EEPROM.write('A' + i << 1, lowByte(segmentletters[i])); //  writing the segment data to EEPROM
    EEPROM.write(('A' + i << 1) + 1, highByte(segmentletters[i]));
  }

  pinMode(clockPin, OUTPUT);
  pinMode(clockPin1, OUTPUT);
  pinMode(dataPin, OUTPUT);
  pinMode(latchPin, OUTPUT);
  pinMode(outputEnablePin, OUTPUT);
  digitalWrite(latchPin, LOW);
}

void loop() {
  Serial.println("Write two letters");
  while (Serial.available() == 0);
  byte letter_one = Serial.read();
  if (letter_one >= 'A' && letter_one <= 'Z' ) {
    letter_one = letter_one - 'A';
    letter_one_binary = segmentletters[letter_one];
    Serial.println(letter_one_binary, BIN);
Serial.println(letter_one);
  }


  delay(1);
  if (Serial.available() >= 1) {
    byte letter_two = Serial.read();
    if (letter_two >= 'A' && letter_two <= 'Z' ) {
      letter_two = letter_two - 65;
      letter_two_binary = (segmentletters[letter_two]);
      Serial.println(letter_two_binary, BIN);
      Serial.println(letter_two);
    }
    delay(1);
    while (Serial.available() >= 1) \

      //Serial.println(Serial.available());
      Serial.read();
    }
  }
  while (Serial.available() == 0) {
    digitalWrite(clockPin1, HIGH);
    digitalWrite(latchPin, LOW);
    shiftOut(dataPin, clockPin, LSBFIRST, letter_one_binary);
    shiftOut(dataPin, clockPin, LSBFIRST, letter_one_binary >> 8);
    digitalWrite(latchPin, HIGH);
    delay(1);
 digitalWrite(latchPin, LOW);
    shiftOut(dataPin, clockPin, LSBFIRST, 00000000);
    shiftOut(dataPin, clockPin, LSBFIRST, 00000000);
    digitalWrite(latchPin, HIGH);    
    digitalWrite(clockPin1, LOW);
    digitalWrite(latchPin, LOW);
    shiftOut(dataPin, clockPin, LSBFIRST, letter_two_binary);
    shiftOut(dataPin, clockPin, LSBFIRST, letter_two_binary >> 8);
    digitalWrite(latchPin, HIGH);
    delay(1);
     digitalWrite(latchPin, LOW);
    shiftOut(dataPin, clockPin, LSBFIRST, 00000000);
    shiftOut(dataPin, clockPin, LSBFIRST, 00000000);
    digitalWrite(latchPin, HIGH);    
  }

}

groundFungus · November 12, 2021, 1:40am

Read the forum guidelines to see how to properly post code and some hints on how to get the most from this forum.
Use the IDE autoformat tool (ctrl-t or Tools, Auto format) before posting code in code tags.

What parts are you having trouble with?

The way that you read serial input could use improvement. Have a look at the serial input basics tutorial.

guix · November 12, 2021, 6:30am

Hello

I don't understand why you use EEPROM in your case. Do you need it? Why are you writing the same thing in EEPROM every time the arduino is reset ?

This for loop will read outside of the array :
for (uint8_t i = 0; i <= sizeMap; i++)
It should be :
for (uint8_t i = 0; i < sizeMap; i++)

frasercanavan · November 12, 2021, 1:51am

For one of my projects for school we created a breadboard connected to a dual display and the function of the code is to whenever in the serial monitor I type in two letters it will display them on the dual display. I have the code it is just my issue is that my code may not be properly understood as I use statements like EEPROM that have not been taught. I want to know if there is an easier way to simplify my code so it becomes more readable and understandable. Here is my code for this project.

#include <EEPROM.h>
byte letter_one = 0;
byte letter_two = 0;
uint16_t letter_one_binary = 0;
uint16_t letter_two_binary = 0;
uint16_t segmentletters[] = {
  0b1110110010001000,  //A
  0b1111001010100000,  //B
  0b1001110000000000,  //C
  0b1111001000100000,  //D
  0b1001110010001000,  //E
  0b1000110010001000,  //F
  0b1011110010000000,  //G
  0b0110110010001000,  //H
  0b1001001000100000,  //I
  0b0111100000000000,  //J
  0b0000110101001000,  //K
  0b0001110000000000,  //L
  0b0110110100000100,  //M
  0b0110110001000100,  //N
  0b1111110000000000,  //O
  0b1100110010001000,  //P
  0b1111110001000000,  //Q
  0b1100110011001000,  //R
  0b1011010010001000,  //S
  0b1000001000100000,  //T
  0b0111110000000000,  //U
  0b0000110100010000,  //V
  0b0110110001010000,  //W
  0b0000000101010100,  //X
  0b0000000100100100,  //Y
  0b1001000100010000   //Z
};
uint8_t clockPin = 11;// define the pin usage for the MBv3
uint8_t clockPin1 = 5;
uint8_t dataPin = 9;         //
uint8_t latchPin = 10;        //
uint8_t outputEnablePin = 8;
uint8_t sizeMap = sizeof(segmentletters) >> 1; //number of entries in the LUT



void setup() {

  Serial.begin(9600);
  for (uint8_t i = 0; i <= sizeMap; i++) {  //iterate through the LUT array
    EEPROM.write('A' + i << 1, lowByte(segmentletters[i])); //  writing the segment data to EEPROM
    EEPROM.write(('A' + i << 1) + 1, highByte(segmentletters[i]));
  }

  pinMode(clockPin, OUTPUT);
  pinMode(clockPin1, OUTPUT);
  pinMode(dataPin, OUTPUT);
  pinMode(latchPin, OUTPUT);
  pinMode(outputEnablePin, OUTPUT);
  digitalWrite(latchPin, LOW);
}

void loop() {
  Serial.println("Write two letters");
  while (Serial.available() == 0);
  byte letter_one = Serial.read();
  if (letter_one >= 'A' && letter_one <= 'Z' ) {
    letter_one = letter_one - 'A';
    letter_one_binary = segmentletters[letter_one];
    Serial.println(letter_one_binary, BIN);
Serial.println(letter_one);
  }


  delay(1);
  if (Serial.available() >= 1) {
    byte letter_two = Serial.read();
    if (letter_two >= 'A' && letter_two <= 'Z' ) {
      letter_two = letter_two - 65;
      letter_two_binary = (segmentletters[letter_two]);
      Serial.println(letter_two_binary, BIN);
      Serial.println(letter_two);
    }
    delay(1);
    while (Serial.available() >= 1) \

      //Serial.println(Serial.available());
      Serial.read();
    }
  }
  while (Serial.available() == 0) {
    digitalWrite(clockPin1, HIGH);
    digitalWrite(latchPin, LOW);
    shiftOut(dataPin, clockPin, LSBFIRST, letter_one_binary);
    shiftOut(dataPin, clockPin, LSBFIRST, letter_one_binary >> 8);
    digitalWrite(latchPin, HIGH);
    delay(1);
 digitalWrite(latchPin, LOW);
    shiftOut(dataPin, clockPin, LSBFIRST, 00000000);
    shiftOut(dataPin, clockPin, LSBFIRST, 00000000);
    digitalWrite(latchPin, HIGH);    
    digitalWrite(clockPin1, LOW);
    digitalWrite(latchPin, LOW);
    shiftOut(dataPin, clockPin, LSBFIRST, letter_two_binary);
    shiftOut(dataPin, clockPin, LSBFIRST, letter_two_binary >> 8);
    digitalWrite(latchPin, HIGH);
    delay(1);
     digitalWrite(latchPin, LOW);
    shiftOut(dataPin, clockPin, LSBFIRST, 00000000);
    shiftOut(dataPin, clockPin, LSBFIRST, 00000000);
    digitalWrite(latchPin, HIGH);    
  }

}

Koepel · November 12, 2021, 2:26am

Hi, welcome to the forum.

I can paint a vague picture in which direction to go. Let me know if it is useful.

Don't try to be smart. Follow the KISS principle this is a link, you can click on it. The code should show in the most obvious way what it is doing.

That means no trickery like this: uint8_t sizeMap = sizeof(segmentletters) >> 1;
The common way is: uint8_t sizeMap = sizeof(segmentletters)/sizeof(segementletters[0]);

Read the online reference of the functions that you use.
For example: https://www.arduino.cc/en/Reference/EEPROMWrite.

Again, please no trickery. How about this:

int index = (int) 'A';       // start with a offset
for (uint8_t i = 0; i <= sizeMap; i++) 
{
  EEPROM.write(index++, lowByte(segmentletters[i]));
  EEPROM.write(index++, highByte(segmentletters[i]));
}

By the way, I don't understand the offset.

Some are very anxious about losing a byte here and there. They use uint8_t and byte. I prefer the default 'int', and let the compiler do the optimizations. It is possible that the compiler does not even use a memory location for a simple variable.

A lookup table should be in Flash memory. See the PROGMEM explanation: https://www.arduino.cc/reference/en/language/variables/utilities/progmem/

Press Ctrl+T in the Arduino IDE and you will see immediate that your curly brackets and indents are wrong.

Try to avoid to wait forever, such as here: while (Serial.available() == 0);.
Instead, let the loop() run over and over again as fast as possible and do something if that action is needed.
For example:

void loop()
{
  if( Serial.available() > 0)
  {
    do something
  }
}

Do you see what is going on here:

if (letter_two >= 'A' && letter_two <= 'Z' ) {
      letter_two = letter_two - 65;

The 'A' and 'Z' are letters, but 65 is a number. The variable name letter_two is for a letter, not a index.
To show what is going on, you could do this:

if (letter_two >= 'A' && letter_two <= 'Z' ) {
  int index_two = letter-two - 'A';

There is not much you can do to simplify the sketch. Trying to reduce the number of code lines will make it harder to read.

To make a code more readable, I like to make it open and spacious. But that is just my personal preference. You can see if you like this: millis_and_finite_state_machine.ino

You could read this: https://blog.wokwi.com/how-to-write-clean-arduino-code/

david_2018 · November 12, 2021, 2:33am

As far as the EEPROM is concerned, you can completely remove all of that, because you are writing data to EEPROM but never using it anywhere else.

6v6gt · November 12, 2021, 3:11am

The code appears to have been written by someone who knew what they were doing, but wrote it in a deliberately obscure way to provide all the elements of a solution, but not to provide the working solution itself. That is, the student must assemble a working solution using the techniques demonstrated in that code.

For a start, the segment letters table need only be 8 bits wide, not 16, since the low order bits are never used. The working program would be half that size.

If we saw the schematic, it would probably also exhibit an unnecessary complexity with redundant shift registers etc.

SteveMann · November 12, 2021, 3:27am

Comments?

Blackfin · November 12, 2021, 4:08am

I suspect you lifted a significant portion of the "complex" code from here:

github.com

rsgcaces/AVRFoundations/blob/master/B_BASIC_SKILLS/Write14SegUpperCaseCharacters/Write14SegUpperCaseCharacters.ino

// PROJECT  :Write14SegUpperCaseCharacters
// PURPOSE  :Writes a lookup table of 14 segments (word) for the uppercase ASCII
//          :characters to EEPROM 
// COURSE   :ICS3U
// AUTHOR   :C. D'Arcy
// DATE     :2019 10 19
// MCU      :328p
// STATUS   :Working
// REFERENCE:http://darcy.rsgc.on.ca/ACES/TEI3M/1920/Tasks.html#PoV

#include <EEPROM.h>       //requires the support of this Arduino library

// Define the 14-Segment Uppercase Letter LookUp Table...
uint16_t segmentMap[] = {
  //ABCDEFGHJKLMNP__ :Segment order
  0b1110110010001000,  //A or 0xEC88 or O166210
  0b1111001010100000,  //B or 0xF2A0 or O?
  0b1001110000000000,  //C or 0x9C00 or O
  0b1111001000100000,  //D or 0xF220 or O
  0b1001110010001000,  //E or 0x9C88 or O

This file has been truncated. show original

but weren't really sure what it was doing. As @david_2018 noted, you included the EEPROM stuff but the part of the code you wrote doesn't actually use it.

Credit the source of the code you lifted, remove that which you don't understand/use and the rest will fall into place.

6v6gt · November 12, 2021, 4:48am

Ah yes. Following that link in the code found by @Blackfin I found the schematic. I wrongly assumed a seven segment display consisting of two independent elements. There is less scope for optimization than I thought.

Edit

Try commenting out these sections. They appear unnecessary for multiplexing the display.

UKHeliBob · November 12, 2021, 8:08am

Duplicate topics merged

Please do NOT cross post / duplicate as it wastes peoples time and efforts to have more than one post for a single topic.

Continued cross posting could result in a time out from the forum.

Could you also take a few moments to Learn How To Use The Forum.

Other general help and troubleshooting advice can be found here.
It will help you get the best out of the forum in the future.

david_2018 · November 12, 2021, 9:11am

6v6gt:

frasercanavan:
>     digitalWrite(latchPin, LOW);
    shiftOut(dataPin, clockPin, LSBFIRST, 00000000);
    shiftOut(dataPin, clockPin, LSBFIRST, 00000000);
    digitalWrite(latchPin, HIGH); 
> ```
Try commenting out these sections. They appear unnecessary for multiplexing the display.

Actually that does need to be in the code. Common (and necessary) technique when switching from one character to another on a multiplexed display. The OP can try removing it temporarily to see why it is needed.

Using a function for shifting out the uint16_t would make the code more readable, as sell as using variable names such as leftDigit and rightDigit for the writes to clockPin1instead of HIGH and LOW. clockPin1 is itself a poor choice of names for a pin that switches between the displays.

6v6gt · November 12, 2021, 9:36am

I'd have certainly used a delay of about 4ms instead of the 1ms so there is less switching and possibly less ghosting. Anyway, even if you are right, there is still scope to trim 14 zeros out of that section of code without impacting it.

david_2018 · November 12, 2021, 10:00am

The delay does seem very short, with only two digits it could be much longer before the display starts to flash noticeably. A much more serious problem is the blocking code, that leaves the display completely off while waiting for serial data to arrive.

johnwasser · November 12, 2021, 1:43pm

You don't use the data written into EEPROM so the best way to simplify the EEPROM part is to remove it entirely:

uint16_t letter_one_binary = 0;
uint16_t letter_two_binary = 0;
const uint16_t segmentletters[] =
{
  0b1110110010001000,  //A
  0b1111001010100000,  //B
  0b1001110000000000,  //C
  0b1111001000100000,  //D
  0b1001110010001000,  //E
  0b1000110010001000,  //F
  0b1011110010000000,  //G
  0b0110110010001000,  //H
  0b1001001000100000,  //I
  0b0111100000000000,  //J
  0b0000110101001000,  //K
  0b0001110000000000,  //L
  0b0110110100000100,  //M
  0b0110110001000100,  //N
  0b1111110000000000,  //O
  0b1100110010001000,  //P
  0b1111110001000000,  //Q
  0b1100110011001000,  //R
  0b1011010010001000,  //S
  0b1000001000100000,  //T
  0b0111110000000000,  //U
  0b0000110100010000,  //V
  0b0110110001010000,  //W
  0b0000000101010100,  //X
  0b0000000100100100,  //Y
  0b1001000100010000   //Z
};
uint8_t clockPin = 11;// define the pin usage for the MBv3
uint8_t DisplaySelectPin = 5;
uint8_t dataPin = 9;
uint8_t latchPin = 10;
// uint8_t outputEnablePin = 8; // ??? unused ???
uint8_t sizeMap = sizeof(segmentletters) >> 1; //number of entries in the LUT



void setup()
{

  Serial.begin(9600);

  pinMode(clockPin, OUTPUT);
  pinMode(DisplaySelectPin, OUTPUT);
  pinMode(dataPin, OUTPUT);
  pinMode(latchPin, OUTPUT);
  // pinMode(outputEnablePin, OUTPUT); // ??? unused ???
  digitalWrite(latchPin, LOW);
}

void loop()
{
  if (Serial.available())
  {
    byte letter = Serial.read();
    if (letter >= 'A' && letter <= 'Z' )
    {
      // Valid letter!
      letter_one_binary = letter_two_binary;  // Shift letters left
      letter_two_binary = segmentletters[letter - 'A'];
      Serial.print(letter);
    }
  }

  // Display the latest two valid characters
  
  digitalWrite(DisplaySelectPin, HIGH);  // Select Left Display

  digitalWrite(latchPin, LOW);
  shiftOut(dataPin, clockPin, LSBFIRST, letter_one_binary);
  shiftOut(dataPin, clockPin, LSBFIRST, letter_one_binary >> 8);
  digitalWrite(latchPin, HIGH);
  delay(4);
  digitalWrite(latchPin, LOW);
  shiftOut(dataPin, clockPin, LSBFIRST, 00000000);
  shiftOut(dataPin, clockPin, LSBFIRST, 00000000);
  digitalWrite(latchPin, HIGH);

  digitalWrite(DisplaySelectPin, LOW);  // Select Right Display

  digitalWrite(latchPin, LOW);
  shiftOut(dataPin, clockPin, LSBFIRST, letter_two_binary);
  shiftOut(dataPin, clockPin, LSBFIRST, letter_two_binary >> 8);
  digitalWrite(latchPin, HIGH);
  delay(4);
  digitalWrite(latchPin, LOW);
  shiftOut(dataPin, clockPin, LSBFIRST, 00000000);
  shiftOut(dataPin, clockPin, LSBFIRST, 00000000);
  digitalWrite(latchPin, HIGH);
}

system · May 11, 2022, 1:44pm

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