Can this code be shorter and more tidy ?

Hello everyone, i have a little piece of code which i use for a solar charger, but it seems kinda long and...raw. Could it be shorter and cleaner ?

#include <Wire.h>
#include <LiquidCrystal_I2C.h>
LiquidCrystal_I2C lcd(0x27, 20, 4);
#include <Adafruit_ADS1015.h>
Adafruit_ADS1115 ads(0x4B);
Adafruit_ADS1115 adsa(0x4A);
float V1 = 0.0;
float V2 = 0.0;
float V3 = 0.0;
float V4 = 0.0;
float V5 = 0.0;
float V6 = 0.0;
float en1 = 0.0;
float en2 = 0.0;

void setup(void)
{
  pinMode(9, OUTPUT);
  lcd.init();
  lcd.backlight();
  Serial.begin(9600);
  ads.begin();
  adsa.begin();
}
unsigned long startMillis;
unsigned long currentMillis;
unsigned long elapsedMillis;
byte pulseWidth = 0;

void loop(void)
{
  int16_t adc1;
  adc1 = ads.readADC_SingleEnded(1);
  V1 = (adc1 * 0.0001875) * 8;
  int16_t adc2;
  adc2 = ads.readADC_SingleEnded(2);
  V2 = (adc2 * 0.0001875) * 8;
  int16_t adc3;
  adc3 = ads.readADC_SingleEnded(3);
  V3 = (adc3 * 0.0001875) * 7;
  int16_t adc4;
  adc4 = adsa.readADC_SingleEnded(1);
  V4 = (adc4 * 0.0001875) * 10;
  int16_t adc5;
  adc5 = adsa.readADC_SingleEnded(2);
  V5 = (adc5 * 0.0001875) * 10;
  int16_t adc6;
  adc6 = adsa.readADC_SingleEnded(3);
  V6 = (adc6 * 0.0001875) * 20;

  currentMillis = millis();
  elapsedMillis = (currentMillis - startMillis);
  unsigned long SS = (elapsedMillis / 1000) % 60;
  unsigned long MM = (elapsedMillis / (60000)) % 60;
  unsigned long HH = (elapsedMillis / (3600000));

  en1 = en1 += ((V1 * V4) + (V2 * V5)) * 1 / 36000;
  en2 = en2 += (V3 * V6) * 1 / 36000;
  lcd.setCursor(0, 0);
  lcd.print(V1, 2);
  lcd.setCursor(5, 0);
  lcd.print("V");
  lcd.setCursor(7, 0);
  lcd.print(V2, 2);
  lcd.setCursor(12, 0);
  lcd.print("V");
  lcd.setCursor(14, 0);
  lcd.print(V3, 2);
  lcd.setCursor(19, 0);
  lcd.print("V");

  lcd.setCursor(0, 1);
  lcd.print(V4, 2);
  lcd.setCursor(5, 1);
  lcd.print("A");
  lcd.setCursor(7, 1);
  lcd.print(V5, 2);
  lcd.setCursor(12, 1);
  lcd.print("A");
  lcd.setCursor(14, 1);
  lcd.print(V6, 2);
  lcd.setCursor(19, 1);
  lcd.print("A");

  lcd.setCursor(0, 2);
  lcd.print(V1 * V4, 1);
  lcd.setCursor(5, 2);
  lcd.print("W");
  lcd.setCursor(7, 2);
  lcd.print(V2 * V5, 1);
  lcd.setCursor(12, 2);
  lcd.print("W");
  lcd.setCursor(14, 2);
  lcd.print(V3 * V6, 1);
  lcd.setCursor(19, 2);
  lcd.print("W");

  lcd.setCursor(0, 3);
  lcd.print(en1 , 1);
  lcd.setCursor(7, 3);
  lcd.print(en2 , 1);
  lcd.setCursor(14, 3);
  lcd.print(MM);
  lcd.setCursor(17, 3);
  lcd.print(":");
  lcd.setCursor(19, 3);
  lcd.print(" ");
  lcd.setCursor(18, 3);
  lcd.print(SS);

  if (V3 < 28 )
  {
    if (pulseWidth != 255) pulseWidth++;
  }
  if (V3 > 28 )
  {
    if (pulseWidth = 0);
  }
  analogWrite(9, pulseWidth);
}

Hi bogdan666,

You could look into using "functions".....

Hope this helps!

Zeb

Almost certainly.

But first question, what’s this?

 if (V3 > 28 )
  {
    if (pulseWidth = 0);
  }

using arrays can help avoid duplicating code

#define N   6
#define K   0.0001875
    float scaler [N] = { 8*K, 8*K, 7*K, 10*K, 10*K, 20*K };
    float v [N];
    for (int n = 0; n < N; n++)
        v [n] = scaler [n] * adsa.readADC_SingleEnded ((n%3)+1);

individual lcd command can become tedious. i use sprintf() to format complete strings and have a single function to display all the strings, such as

void lcdDisp (
    const char *s0,
    const char *s2,
    const char *s2,
    const char *s3 )
{
    lcd.setCursor (0, 0);
    lcd.print (s0);
    lcd.setCursor (0, 1);
    lcd.print (s1);
    lcd.setCursor (0, 2);
    lcd.print (s2);
    lcd.setCursor (0, 3);
    lcd.print (s3);
}

the issue with sprintf() on arduino is that is doesn't support floats. dtostr() can be used, but it requires an intermediate string.

in your case this could be awkward but a sub-function could be written to for 4 floats with string labels for each.

void fmt (
    char *s,
    float f0,
    const char *lbl0,
    float f1,
    const char *lbl1,
    float f2,
    const char *lbl2 )
{
    char *s0 [10];
    char *s1 [10];
    char *s2 [10];

    dtostr (f0, 4, 2, s0);
    dtostr (f1, 4, 2, s1);
    dtostr (f2, 4, 2, s2);

    sprintf (s, "%4s %s %4s %s %4s %s", s0, lbl0, s1, lbl1, s2, lbl2 );
}

in the end you may end up with more lines of code but it would be more maintainable.

pcbbc:
Almost certainly.

But first question, what’s this?

 if (V3 > 28 )

{
    if (pulseWidth = 0);
  }

This is the part of the code that prevents an overshoot of voltage, so if the voltage is over the one i set it, 28V in this case, it stops the charging.
@ZebH the link you sent is not working.
Not very familiar with arrays, but will look into it, thanks.

en1 = en1 +=

Since "en1 += " is a shortcut for "en1 = en1 +" this is equivalent to:

en1 = en1 = en1 +

That's TWO redundant assignments. You should use either:

en1 = en1 +

or (preferably) the shortcut:

en1 +=

bogdan666:
This is the part of the code that prevents an overshoot of voltage, so if the voltage is over the one i set it, 28V in this case, it stops the charging.

Yes, I know that. My comment was a suggestion you might perhaps need to brush up on your C syntax:
Single equals = is an assignment.
Double equals == is a comparison.

First mistake: It’s almost universally wrong to use single equals in a “if” or “while” condition.
Second mistake: That “if” doesn’t have an associated logic block. It ends in a semicolon.

This line...

if (pulseWidth = 0);

...is therefore entirely equivalent to...

pulseWidth = 0;

If that is actually what you intended, then that what you should write.

However given the previous matching “if” one could say you perhaps might have intended...

code] if (pulseWidth == 0) pulseWidth--;