Code analysis.

I made this code to control a dc motor based on the readings of 2 LDR’s, and I would like to know if it is well written or if there’s anyway to simplify it. I guess the code is self-explanatory!

#define InA1 2 // motor pin 1
#define InA2 4 // motor pin 2
#define PWM1 3 // PWM pin
int ldr1 = 1; int ldr2 = 2; // ldr analog pins
// variables to store de ldr reading and calculate the reading difference
int valor1; int valor2; int dif;
// minimium difference necessary to turn the motor on
int mindif = 40;

void setup() {
  pinMode(InA1, OUTPUT);
  pinMode(InA2, OUTPUT);
  pinMode(PWM1, OUTPUT);
}

void loop() {
  valor1 = analogRead(ldr1);
  valor2 = analogRead(ldr2);
  dif = abs(valor1 - valor2);
  if (dif <= mindif) motorP();
  else if (valor1 > valor2) motorH(127);
  else motorAH(127);
}

void motorP() {
  digitalWrite(InA1, LOW);
  digitalWrite(InA2, LOW);
  analogWrite(PWM1, 0);
}

void motorH(int PWM_val) {
  digitalWrite(InA1, HIGH);
  digitalWrite(InA2, LOW);
  analogWrite(PWM1, PWM_val);
}

void motorAH(int PWM_val) {
  digitalWrite(InA1, LOW);
  digitalWrite(InA2, HIGH);
  analogWrite(PWM1, PWM_val);
}

Thanks everyone fot the tips!

Your code looks ok. One issue that might bite you is that the ADC can give innacurate results if you try to read different pins in rapid succession - see Robtillaart's response in this thread: Arduino Forum

The variable names are rather cryptic, as indeed are those of the procedures - wouldn't motorP be better as motorStop?

The only way I can see to make it any simpler is to refactor those three functions and make one called motorControl that takes the three parameters needed and does the necessary pin operations. Then, you could call it directly from your if logic, or make some wrapper functions with better names thus:

void motorStop ()
{
motorControl(LOW,LOW,0);
}

valor1,valor2 and dif don't need to be global, you can declare them in loop.

127 should be a constant, not a reused literal.

This is all nitpicky stuff though - you're fine - apart from the variable names :wink:

Hmm thanks for the advices, but... What does ADC means? As you can see, I'm kinda noob on this things! :blush:

And also... How would the motorControl function look like? I know how to create functions that use numbers as paramenters, but not words like LOW or HIGH... :blush:

And about the function names, I'm brazilian, so all of them make sense in portuguese :wink:

Thanks for the tipos! ^^

int ldr1 = 1; int ldr2 = 2; // ldr analog pins

These are constants, so why not make them constants?

int valor1; int valor2; int dif;

There is no reason for these to have global scope; move them into "loop"

ADC is Analog to Digital Converter. Your calls to analogRead may be problematic, see the solutions in the other thread

LOW and HIGH are defined as numbers - 0 and 1 respectively so you can build the control function just like your other ones:

void motorControl(int A1Setting, int A2Setting, int PWM_val)
{
digitalWrite(InA1, A1Setting);
digitalWrite(InA2, A2Setting);
analogWrite(PWM1, PWM_val);
}

AWOL:

int ldr1 = 1; int ldr2 = 2; // ldr analog pins

These are constants, so why not make them constants?

int valor1; int valor2; int dif;

There is no reason for these to have global scope; move them into "loop"

Should I declare it as const int ldr..... Or could I use the #define function?

And is there any problem in leaving it as global or it is more correct to leave it in the loop as it doesn´t need to be global?

Thanks!

wildbill:
ADC is Analog to Digital Converter. Your calls to analogRead may be problematic, see the solutions in the other thread

LOW and HIGH are defined as numbers - 0 and 1 respectively so you can build the control function just like your other ones:

void motorControl(int A1Setting, int A2Setting, int PWM_val)

{
digitalWrite(InA1, A1Setting);
digitalWrite(InA2, A2Setting);
analogWrite(PWM1, PWM_val);
}

So when calling the motorControl function, I would write for instance motorControl(1, 0, 127) to make it as HIGH, LOW, 50% pwm cycle?

Thanks!!

And is there any problem in leaving it as global or it is more correct to leave it in the loop as it doesn´t need to be global?

No immediate problem,but as your sketches grow, you may find that names get confusing - if a varaible doesn't need global scope (if it doesn't need to hold the same value from "loop" to "loop" or from "setup" to "loop", then keep the scope as local as possible.

AWOL:

And is there any problem in leaving it as global or it is more correct to leave it in the loop as it doesn´t need to be global?

No immediate problem,but as your sketches grow, you may find that names get confusing - if a varaible doesn't need global scope (if it doesn't need to hold the same value from "loop" to "loop" or from "setup" to "loop", then keep the scope as local as possible.

Hmm, and could I declare a variable in the loop and other variable with the same name in another function?

Thanks for the attention

AWOL:
http://arduino.cc/en/Reference/Scope

Hmm thanks a lot! hahahah

Hmm, and could I declare a variable in the loop and other variable with the same name in another function?

Yes, and it's often done, for instance inside if statements (i seems to be used a lot) as the loop counter. However I think it's probably a good pratice to not use same names as years later when you are reading the code you might think it's a global name because you are seeing the same name in several functions defined in the program, when indeed they are local scope variables.

Lefty

Spielwurfel:
So when calling the motorControl function, I would write for instance motorControl(1, 0, 127) to make it as HIGH, LOW, 50% pwm cycle?

You could, but better to use the constants and say motorControl(HIGH, LOW, 127)

nicer still if you use a #define or const to give 127 a meaningful name too.

Ok!! Thanks everyone! =)

You could also build a slightly more complex function that does things for you:

void motorControl( int speed ) {
  bool fwd = speed >= 0; // forward if positive
  speed = constrain( abs( speed ), 0, 255 ); // analogWrite doesn't take numbers > 255 or < 0
  // ternary operators are cool in that they can make code more semantic
  // they replace this:
  // if (a) { b } else { c } with this: a ? b : c
  digitalWrite( InA1, fwd ? HIGH : LOW );
  digitalWrite( InA2, fwd ? LOW : HIGH );
  analogWrite( speed );
  // special case for speed == 0 (braking)
  if ( speed == 0 ) {
    digitalWrite( InA2, LOW );
    digitalWrite( InA2, LOW );
  }
}

However I think it's probably a good practice to not use same names as years later when you are reading the code you might think it's a global name because you are seeing the same name in several functions defined in the program, when indeed they are local scope variables.

I use 1 letter names for loop indices, and other variables that I KNOW are not to be relied on for any period of time, and I never use 1 letter global names, just to avoid this possibility.

Its a good idea to establish some kind of naming convention for variables. The practice that Microsoft recommends (iVar for int, bVar for boolean, fVar for float, cVar for class instance) annoys the hell out of me, but the use of g as the first letter of any global variable, and NOT using g as the first letter of any local variable, makes sense.

dif = abs(valor1 - valor2);
if (dif <= mindif) motorP();
else if (valor1 > valor2) motorH(127);
else motorAH(127);

I have a question with this part of code as the clauses are not necessary disjunct ?

There are cases possible where both (dif <= mindif) and (valor1 > valor2) are true, e.g. valor1 = 1 and valor2 = 0.

Does your program logic do what you want ?

Can you tell me in plain english what is intended with this code?

As I read it, if valor1 and valor 2 are close, the motor should stop. Otherwise, if valor1 > valor2 motor goes in one direction, else the other direction.

OK, makes sense.