I want help condensing my code

Hello all,
I know this is not really a question, and I may be posting this in the wrong place, but I am working on a project where the supply voltage will be on and off and the STM32 will turn on-and-off, so I wanted to minimize the execution speed by condensing the code. Is there someone who can condense this code? I am not worried about the functions though. Thanks for the help!

#include <EEPROM.h> //Import the EEPROM library for using the EEPROM commands

//-------------- Variable Definitions --------------

float volts; //Create variable to store voltage value

int ADCValue; //Create variable to store the ADC value (raw data coming in)
int firstReading = 100; //Create variable to store whether the MCU just turned on or if it is a subsequent value
int address; //Create variable to store current address

byte data; //Create variable to act as a buffer for the written EEPROM value

bool lock = 0; //Create variable to make sure that the read process does not run twice

//-------------- Function Definitions --------------

int FindNextAddress(int startAddress) {
  for(int i=startAddress; i<EEPROM.length(); i++) { //Start stepping through each EEPROM address
    if(EEPROM.read(i) == 0) { //If the byte has not been written
      address = i;
      return address; //Return the address
    }
  }
}

float ADC_to_Volts(int adc_val, float vbat, int ref) {
  float results = adc_val * (vbat / ref); //Formula for ADC value to voltage
  return results; //Return the calculated voltage
}

//-------------- Main Code --------------

void setup() {
    Serial.begin(9600); //Begin the USART connection at 9600 baud
    pinMode(A0, INPUT); //Declare the ADC pin as an input
    pinMode(2, INPUT); //Declare the jumper pin as an input
    
    address = FindNextAddress(0); //Set the address to the next available address
}

void loop() {
  //------ Initial Readings ------
  ADCValue = analogRead(A0); //Read the raw ADC value
  volts = ADC_to_Volts(ADCValue, 5.0, 1023); //Convert raw ADC value to voltage reading
  data = firstReading + (volts * 10); //Assemble byte to be uploaded to EEPROM
  //------ Logic for Write ------
  if(digitalRead(2) == LOW) { //Check if jumper is unplugged
    EEPROM.write(address, (int)data); //Write the data assembled from earlier
    address++; //Go to the next address
    firstReading = 0; //Declare that this is no longer the first reading and is a subsequent value
    delay(500); //delay by 500 ms to make sure entire EEPROM is not written in a short time by using a delay 
  }
  //------ Logic for Read ------
  else { //If the jumper is plugged in
    if(!lock) { //If the value is not locked
      for(int i=0; i<EEPROM.length(); i++) { //Step through every address in the EEPROM
        Serial.println(EEPROM.read(i)); //Print each value in the EEPROM
      }
      lock = 1; //Lock the read function so it dosen't read recursivly
    }
  }
}

Thank you so much!!

That makes no sense.

Do you want the code to take as little time as possible? Seems like maximizing the speed would be the thing.

Anyway, does the code you posted do what you want except for some time considerations?

a7

Something you can do to speed up your program is to avoid the use of floating point variables and floating point operations.

In your code, you convert integer values (digitalRead) to float (volt) and then back to integer. You can save CPU cycles here. Also, each every time you call

float ADC_to_Volts(int adc_val, float vbat, int ref) {
  float results = adc_val * (vbat / ref); //Formula for ADC value to voltage
  return results; //Return the calculated voltage
}

you are forcing the evaluation of the same floating point division (5.0/1023). You can compute a fixed factor just once in the setup and use it here repeatedly.

What is your code supposed to do?

You have to review your code carefully and decide, for example, what firstReading is for, how you will prevent the process from writing beyond the EEPROM capacity, or how you will manage to unplug the jumper before the EEPROM is full, and finally, how come you need more speed and then include this in your code:

    delay(500); //delay by 500 ms to make sure entire EEPROM is not written in a short time by using a delay 

Thank you for the response. First question. This code is for a small (40x40x40mm) cube. The plan is to make use of Faraday's Law to generate electricity by shaking a magnet through a copper coil rapidly. The voltage genration will go through logic to only let the voltage through when it is suitable enough for powering an MCU. This should write the voltage generated by measuring it with an analog pin. Second question. firstReading is for indicating whether or not the byte of data is a subsequent value or not. This will let us know if the MCU turned on and off rapidly, or if it was able to stably stay on. Fourth question. Thank you for the catch, that could have ended badly. Would something like: if((digitalRead(2) == LOW) && (address<EEPROM.length())) instead of if((digitalRead(2) == LOW)) (line 47) work? Fifth question. Would we need to be careful about this once we fix line 47 (answer to 4th question)? Sixth question. I need speed so that there are actually values being printed to the EEPROM. I want enough time to measure, encode, and write all in a short pulse of power. Last question. The delay is in just in case the power to the MCU is stable, we will be able to record long times, so the entire EEPROM does not get written in a matter of seconds. Thanks for all the help, and sorry if my answers are not what you were looking for.

You can do better than that. If you make it a constant, it will be computed once during compilation, and never again.

const float ADC_TO_VOLTS = 5.0/1024;

Notice I used 1024 not 1023 which is not the correct scaling factor.

Or, since you have already made a complete function to do the conversion you could:

float ADC_to_Volts(int adc_val) {
  float results = adc_val * 5.0/1024; //Formula for ADC value to voltage
  return results; //Return the calculated voltage
}

Compiler optimization will ensure that the division is performed only at compile time, just as with the const variable.

I notice you convert the ADC to floating point and then back to integer. Isn't that inefficient? Why do you do it?

Schematic?

@ScienceMax
Check if the following version is a condensed form of your original sketch.

#include <EEPROM.h> //Import the EEPROM library for using the EEPROM commands
int firstReading = 100; //Create variable to store whether the MCU just turned on or if it is a subsequent value
int address; //Create variable to store current address
bool lock = 0; //Create variable to make sure that the read process does not run twice

void setup()
{
  Serial.begin(9600); //Begin the USART connection at 9600 baud
  pinMode(A0, INPUT); //Declare the ADC pin as an input
  pinMode(2, INPUT_PULLUP); //Declare the jumper pin as an input
  address = FindNextAddress(0); //Set the address to the next available address
}

void loop()
{
  int ADCValue = analogRead(A0); //Read the raw ADC value
  float volts = ADCValue * (5.0 / 1023); //32-bit binary32 format = 4-byte
  float data = (float)firstReading + (volts * 10.0); //Assemble byte to be uploaded to EEPROM

  if (digitalRead(2) == LOW)
  {
    EEPROM.put(address, data);
    firstReading = 0;
  }
  else
  {
    if (!lock)
    {
      float y;
      EEPROM.get(address, y);
      Serial.println(y, 2);
      lock = 1;
    }
  }
}

int FindNextAddress(int startAddress)
{
  for (int i = startAddress; i < EEPROM.length(); i++)
  { //Start stepping through each EEPROM address
    if (EEPROM.read(i) == 0)
    { //If the byte has not been written
      address = i;
      return address; //Return the address
    }
  }
}

youre doing an unnecessary conversion from into to float, which just wastes time, and gives a value in the range 0.0 to 4.995
then a float mult by 10, gives a value in the range 0.0 to 49.95
then losing all your 12 bit resolution by converting back to an INT.

I'd recommend you use int throughout, which you can do by working in millivolts.

ADCValue = analogRead(A0); //Read the raw ADC value
long int mVolts = (ADCValue * 50000) /1024;  //including your *10 factor
..
data = firstReading + mVolts; //Assemble byte to be uploaded to EEPROM

One must test to determine efficiency. While code reviews do have some effect they are only thought experiments and can be wildly wrong.
I used to teach and had one class with a group of fun guys who delighted in showing how smart they were to the point of disrupting the flow of the lessons and making the other students feel inadequate.

One day the the lesson degenerated over statements that the lesson code was inefficient. I responded that perhaps we should have a lesson on efficiency which suggestion was well received.

The next day I brought in a PC and access to two different mainframes. I put simple code on the board with two ways of performing the same action and asked which was faster. I always told the responder that they were wrong before running the code. Then I selected which machine ran which version and yes those students were always wrong. When the other students started volunteering answers, told them they were right and they were.
Machines and compilers will do things differently and those differences can add up. Code reviews only go far.

One needs to test. Code profilers show you were your code consumes time and prevent spending days working on speed improvements to code that has little effect on total time.
For example in pseudo code
Array[900][900];
'For(i=1,900){
' For(j=1,900){
' Array[i][j];
' }
'}

Using i then j will usually be quite different from j then i in the brackets. Smart compilers may fix the issue. Change machines, software versions or compilers and the results may change. You need to test.

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