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);
}

 

A few comments would make the program much easier to read.
I don't know what ADC you are using so I cannot comment on your choice of variable types.
My first thought is that you could use a function for the ADC measurements, passing in the ADC name and result multiplier and returning the value. That would clean up a lot of the repetitive code.
When you are labeling the values you are sending to the display, they always follow the value on the same line, so you don't need to specify an absolute location for them. e.g.:
lcd.setCursor(0, 0);
lcd.print(V1, 2);
lcd.print(" V");
That will print the value of V1 followed by one space and "V".
Those changes will shorten and simplify your program a bit.
Good luck,
Keith

 

Yes, the is my problem because i have a lot of repetitive code. I am using the ads1115, unfortunately i am not familiar with functions, and since the code is from scratch and gathered from here and there i did not include any comments, sorry. With the last idea i will look into it and make the code a bit shorter.

 

If you are not confident enough to use functions, you could reduce the number of statements by combining some of the lines:

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;

becomes:

V1 = ((ads.readADC_SingleEnded(1)) * 0.0001875) * 8;
V2 = ((ads.readADC_SingleEnded(2)) * 0.0001875) * 8;
V3 = ((ads.readADC_SingleEnded(3)) * 0.0001875) * 7;
V4 = ((adsa.readADC_SingleEnded(1)) * 0.0001875) * 10;
V5 = ((adsa.readADC_SingleEnded(2)) * 0.0001875) * 10;
V6 = ((adsa.readADC_SingleEnded(3)) * 0.0001875) * 20;

Regards,
Keith

 

if (pulseWidth = 0);

This looks suspicious.

 

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 ?

You are defining a bunch of variables that only get used once. So why do it?

For instance:

  int16_t adc1;
  adc1 = ads.readADC_SingleEnded(1);
  V1 = (adc1 * 0.0001875) * 8;

becomes

  V1 = ads.readADC_SingleEnded(1) * 0.0015;

The following code:

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

SEE NOTE AT END OF POST

Is functionally identical to

  if ( V3 < 28 )
    if (pulseWidth != 255)
       pulseWidth++;

Be sure to understand why this is the case.

or

  if ( V3 < 28 )
       pulseWidth += pulseWidth != 255;

or even

  pulseWidth += ( V3 < 28 )? (pulseWidth != 255) : 0 ;

or

  pulseWidth += ( V3 < 28 ) * (pulseWidth != 255);

These are not equivalent in terms of performance and certainly not in terms of readability. You will need to decide how important performance is over readability.

But that's assuming that the original code is correct to begin with, which it probably isn't.

For instance, what should happen if V3 == 28?

You appear to be using some extended version of C in which functions can be overloaded because you have two versions of the adc.print() function, one with one argument and one with two.

You can probably reduce you code quite a bit, depending on the capabilities of this function.

NOTE: This post was written without spotting the logic error in which the if() test was written as an assignment instead of an equality comparison.

 

Is functionally identical to

Except that if V3>28 then V3 is assigned the value zero.

EDIT: Sorry, V3 isn't assigned zero, pulseWidth is.

 

Except that if V3>28 then V3 is assigned the value zero.

Where does that happen? In the original code? In my alternative? If so, which alternative? It would be nice if you provided a bit more detail.

The only place that I see that V3 is ever assigned a value is higher in the loop() function:

  V3 = (adc3 * 0.0001875) * 7;

The original code near the bottom of that function is:

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

V3 is not changed.

But I think I see what caught your attention, and that is that if V3 > 28 then the test in the else is almost certainly wrong in that it sets pulseWidth equal to 0, instead of asking if it happens to be equal to zero.

This is a very common mistake and easy to overlook (as I did). This is why I generally recommend writing a test that checks for equality to a constant as

if (0 == pulseWidth)

so that if you type

if (0 = pulseWidth)

if gets turned into a syntax error that any compiler will catch, instead of a warning that only some compilers will issue and that many programmers will ignore even if it does throw a warning.

 

Forgive my brainfart, not V3, pulsewidth gets assigned zero.

 

thank you Keith , that will shorten my code quite a lot.
@ WBahn for the first question, the piece of code that reads the adc is from i don't even remember where, so i guess i just made it more...but now with some help it will become less, as for the PWM function, i don't understand all of the alternatives that you gave me, but i will test them all to see which is better, and performance definitely comes first,the V3=28 is just for a test, it can be any number depending on what type of battery i will be using. If V3 will be 28V i want nothing to happen, i don't care, because it will quickly go into another direction.

 

You can put all of these variable declarations/initializations on the same line:

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;

vs.

float V1 = V2 = V3 = V4 = V5 = V6 = en1 = en2 = 0.0;

You can avoid unnecessary if's and braces:

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

vs.

  if (V3 < 28 && pulseWidth != 25) pulseWidth++;
  if (V3 > 28 && pulseWidth = 0) ;

The last statement must be a mistake because it doesn't do anything.

 

You can put all of these variable declarations/initializations on the same line:

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;

vs.

float V1 = V2 = V3 = V4 = V5 = V6 = en1 = en2 = 0.0;

You can avoid unnecessary if's and braces:

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

vs.

  if (V3 < 28 && pulseWidth != 25) pulseWidth++;
  if (V3 > 28 && pulseWidth = 0) ;

The last statement must be a mistake because it doesn't do anything.

That last statement,

if (pulseWidth = 0);​

,does do something, albeit awkwardly. It’s the same as just:

pulsewidth = 0;​

 

thank you Keith , that will shorten my code quite a lot.
@ WBahn for the first question, the piece of code that reads the adc is from i don't even remember where, so i guess i just made it more...but now with some help it will become less, as for the PWM function, i don't understand all of the alternatives that you gave me, but i will test them all to see which is better, and performance definitely comes first,the V3=28 is just for a test, it can be any number depending on what type of battery i will be using. If V3 will be 28V i want nothing to happen, i don't care, because it will quickly go into another direction.

Right now, if the condition that (V3 > 28) is true, nothing happens at all (providing that the equality vs. assignment issue is fixed). What is it that you want to have happen if V3 > 28?

Since you don't care what happens when V3 == 28, you can combine the two if() statements into a single if()-else statement (assuming you actually want something to happen in the else case.

As for the alternatives, sit down with a C reference and figure out what each of them does. It will be a good learning exercise for you.

 

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

vs.

  if (V3 > 28 && pulseWidth = 0) ;

This is not the same thing, in fact it should throw a syntax error. The order of operations for these three operators are, from highest to lowest, {>,&&,=}. So this is the same as

  if ( ((V3 > 28) && pulseWidth) = 0) ;

After comparing V3 and 28 (and producing either 0 or 1) it will logically AND that with the current value pulseWidth, again producing either 0 or 1, and it will then try to set the result of that expression to 0, which is a syntax value.

To get equivalent code, you would could do

  if ( (V3 > 28) && (pulseWidth = 0) ) ;

But note that this relies on guaranteed shortcircuiting for that equivalence and that the following is NOT equivalent

  if ( (pulseWidth = 0) && (V3 > 28) ) ;

 

This is not the same thing, in fact it should throw a syntax error. The order of operations for these three operators are, from highest to lowest, {>,&&,=}. So this is the same as

  if ( ((V3 > 28) && pulseWidth) = 0) ;

After comparing V3 and 28 (and producing either 0 or 1) it will logically AND that with the current value pulseWidth, again producing either 0 or 1, and it will then try to set the result of that expression to 0, which is a syntax value.

To get equivalent code, you would could do

  if ( (V3 > 28) && (pulseWidth = 0) ) ;

But note that this relies on guaranteed shortcircuiting for that equivalence and that the following is NOT equivalent

  if ( (pulseWidth = 0) && (V3 > 28) ) ;

The original code is quite “messy”. It isn’t clear at all what it is intended to do. Hence, it’s unlikely that any suggestions based on this code will be useful.

I know it’s been mentioned before, but can the TS state in natural language what is the intent of this code snippet? WITHOUT any code?

Once we all agree on what is to be done, then and only then can simplified code, that is useful, can be recommended.

I have an idea what the TS wants, but until we can agree on WHAT that is, we’re all just spinning our wheels.

You gotta walk before you run!

 

This is not the same thing, in fact it should throw a syntax error.

No, it's valid C syntax.

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

is equivalent to:

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

 

No, it's valid C syntax.

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

is equivalent to:

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

But that’s not the code that WBahn said should throw a syntax error. He was referring to this line

if (V3 > 28 && pulseWidth = 0) ;

 

No, it's valid C syntax.

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

is equivalent to:

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

I didn't say that those two weren't equivalent. Please go back and read what I wrote. I said that

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

is not the same as

  if (V3 > 28 && pulseWidth = 0) ;

 

I didn't say that those two weren't equivalent. Please go back and read what I wrote. I said that

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

is not the same as

  if (V3 > 28 && pulseWidth = 0) ;

I really don't think this is a big issue. I'm sure he meant to write "if (pulseWidth == 0);". The Arduino compiler would not detect "if (pulseWidth = 0);" as a syntax error. "pulsewidth" would just become "0".
His "if" statements are a bit jumbled but we do not know what he is testing for so there is nothing we can do to help there.
Keith

 

if V3<28V i want my PWM to start, and if V3>28V i want my pwm to stop, that's it, those are my 2 lines of code that i need for my charging. Is there a big problem with that? because everyone seems to not like that thing.