Newbie needs help with arduino code

Oh then let's recheck your conversion code for RPMs, there must be a flaw there...

@chip I'll update this post as I continue to read the code...

1st remark :

the reset of lastencoderValueR/lastencoderValueL is done near the end of loop() and i think you should do it at the end of your RPM calculation block instead, because quite a number of millis will be spent during all the Serial.print in between

Posted by: @chip

Posted by: @zeferby

This looks like a 2 RPM target, not 10 RPM ? 

Yes you are correct although my wheels are actually spinning around 10 RPM and I have to set the target of 2 in my code to get them to spin 10 RPM, if I set it to 10 here the wheels are spinning much faster than 10 RPM.

This is what I can not figure out and I am using the code Bill posted on the Dronebot Workshop for DB1 and I am using the same exact motors with encoders he is using. When the motors are spinning at 10 RPM my code is showing 2 RPM.

Is it possible that there is a mistake in these lines of code?

#define ENC_COUNT_REV 374
rpmR = (float)(lastencoderValueR * 60.0 / ENC_COUNT_REV);
rpmL = (float)(lastencoderValueL * 60.0 / ENC_COUNT_REV); 

@zeferby

I see you are thinking along the same lines as I am ? 

Posted by: @zeferby

1st remark :

the reset of lastencoderValueR/lastencoderValueL is done near the end of loop() and i think you should do it at the end of your RPM calculation block instead, because quite a number of millis will be spent during all the Serial.print in between

Yes I thought about that and changed the code to this .

// Update RPM value every 1/2 second
  currentMillis = millis();
if (currentMillis - previousMillis > RPMinterval) {
previousMillis = currentMillis;
// Calculate RPM of  Motors
rpmR = (float)(lastencoderValueR * 60.0 / ENC_COUNT_REV);
rpmL = (float)(lastencoderValueL * 60.0 / ENC_COUNT_REV); 
lastencoderValueR=0;
lastencoderValueL=0;  
}

@chip

This code is ambiguous:

// Update RPM value every 1/4 second
  currentMillis = millis();
if (currentMillis - previousMillis > RPMinterval) {
previousMillis = currentMillis;

RPMInterval is set for 500ms in the variable declarations i.e. 1/2 second

@chip

I was looking at the code in your first post not the .ino file!

What about something along these lines ?

// Update RPM value every 1/2 second
currentMillis = millis();
if (currentMillis - previousMillis > RPMinterval) {

//first let's keep all current values "safe"
float timeDeltaMinutes = (currentMillis - previousMillis) / 60000.0;
float R = lastencoderValueR;
float L = lastencoderValueL;

//then reset volatile data for next calculation
previousMillis = currentMillis;
lastencoderValueR = 0;
lastencoderValueL = 0;

// Calculate Rounds of Motors during the measuring period (using float !), then divide by period length in minutes
R = R / ENC_COUNT_REV;
L = L / ENC_COUNT_REV;
// Calculate RPM of Motors
rpmR = R / timeDeltaMinutes;
rpmL = L / timeDeltaMinutes;
}

@chip

rpmR = (float)(lastencoderValueR * 60.0 / ENC_COUNT_REV);

This is bad programming style rpmR is already declared a float in the globals.

And mixed variable types in an equation should be avoided as the results can be unpredictable, therefore the following should be adequate.

rpmR = lastencoderValueR * 60 / ENC_COUNT_REV;

@chip @pugwash

And actually the "60" factor would be for a 1 second interval, while we have "approximately" 500ms

Posted by: @zeferby

// Update RPM value every 1/2 second
currentMillis = millis();
if (currentMillis - previousMillis > RPMinterval) {

//first let's keep all current values "safe"
float timeDeltaMinutes = (currentMillis - previousMillis) / 60000.0;
float R = lastencoderValueR;
float L = lastencoderValueL;

//then reset volatile data for next calculation
previousMillis = currentMillis;
lastencoderValueR = 0;
lastencoderValueL = 0;

// Calculate Rounds of Motors during the measuring period (using float !), then divide by period length in minutes
R = R / ENC_COUNT_REV;
L = L / ENC_COUNT_REV;
// Calculate RPM of Motors
rpmR = R / timeDeltaMinutes;
rpmL = L / timeDeltaMinutes;
}

So all this would go in my Void Loop ?

@chip it's how i would write your block of code currently between lines 128 and 137

From this :

Posted by: @zeferby

it's how i would write your block of code currently between lines 128 and 137

From this :

volatile long lastencoderValueR = 0; 
volatile long lastencoderValueL = 0;

So woule I delete these lines in my global ?

@chip absolutely not ! These are the variables declarations+definitions that you need for the encoder interrupt routines, and that we also read in the RPM calculation block (and we save their values into other local variables to play with at our leisure, while the encoders continue to update the "volatile" ones)

The 2 "volatile" variables are "shared" between your code in the loop() function and the code that is executed whenever the encoders required it, irrespective of where your own code execution point currenlty is (the 2 interrupt functions you have provided for your encoders), hence their value is "volatile" because it can change at any time