Weird little math problem

I'm building an automotive tach signal simulator and I had weird little problem and I narrowed it down to one line of code. This expression was not evaluating properly , target_rpm = (8000*potvalue)/1023 + 1000; it comes out as 1000 no matter what potvalue is. I played around with it a bit and what I discovered confused me even more specifically, this almost identical expression target_rpm = (8000*potvalue)/1000 + 1000; evaluates properly. I found an easy work around for the problem but for future reference, I'd still like to know why this happens.

Almost surely an integer math problem, but you need to supply the declaration of variables to be sure.

an unsigned int holds up to 65535, so if potvalue is 8 or more you will overflow. try making target_rpm and all the other variables unsigned long and see if the problem persists.

We like to see full a sketch. In the ideal world, you would have made a test sketch that shows the problem. ;)

I can spot a few problems. Are they all integers ? and potvalue is the raw value from analogRead ? You can use floating point to prevent problems. Use more parentheses, force the constants to be float (1023.0 instead of 1023), add comment.

When using integers, you have to be sure if it will fit into an integer. The maximum is 32767, and 8000 * 1023 is too high. I assume you would like a range of 1000 to 9000. I think it can not be done with 16-bit integers. You have to scale up to 32-bit integers, and convert the result back to 16-bit integers.

Do you know the map() function ? http://arduino.cc/en/reference/map I think the map() function does not check for overflow, so it could have the same result as using 16-bit integers and 8000*1023 is too much.

// example for working but bad code with float.
int potvalue = analogRead(A0);
float target_rpm = ((8000.0 * (float) potvalue) / 1023.0 ) + 1000.0;
// example for working and good code with float.
int potvalue = analogRead(A0);   // read pot, this is raw ADC value
float voltage = (float) potvalue * 5.0 / 1023.0;  // voltage of pin, not used
float pot = (float) potvalue / 1023.0;   // pot setting with range of 0...1
float target_rpm = (8000.0 * pot) + 1000.0; // gain and offset for rpm
// example for geeks using integers.
int potvalue = analogRead(A0);   // read pot, this is raw ADC value
// The calculation is: potvalue / 1023 * 8000 + 1000.
// To prevent loosing bits, the potvalue is first multiplied by 8000.
// This will not fit into an 16-bit integer, so long integer is used.
long temporary = (((long) potvalue * 8000L) / 1023L) + 1000L;
int target_rpm = (int) temporary;

Okay, looks like I've got to pay more attention to the order of operations. Reordering the expression to this target_rpm = ((8000/1023)*potvalue) + 1000; worked.

I had originally considered that there might be an issue with the size of the integer values, as I've run across that before however, in this case, what made me discount that possibility was the fact that target_rpm = (8000*potvalue)/1000 + 1000; did NOT fail, it would seem to me it should have suffered from the same issues, any ideas why it worked?

Anyways, I now have a better appreciation of how the size on an int value can affect the way an expression is evaluated, I'll be paying more attention this in the future.

target_rpm = ((8000/1023)*potvalue) + 1000;

That's not a good solution because 8000/1023 is 7 so it reduces to 7*potvalue+1000 If you want better precision you have to do a long multiplication. I think this will do it:

target_rpm = ((8000L*potvalue)/1023 + 1000;

Pete

target_rpm = (8000*potvalue)/1000 + 1000

My guess is that this works because the compiler optimises the expression to

target_rpm = (8 * (potvalue/1000) ) + 1000

However, looking at this potvalue/1000 will always be 0 (assuming raw pot value fro Arduino 0..1023) except for the few cases between 1000..1023, so the expression will only evaluate to 1000 or 1008. Do you get a range of values or just 2?

When the divisor was 1000, the expression evaluated through the full range of values, and the circuit worked exactly as planned however when the divisor was 1023, it always evaluated to 1000.

PETE, thanks for the observation about rounding error.

Anyways, if anybody’s interested, here’s the latest version of the full code. It seems to work as intended.

/* Tach signal simulator

Simulates a tach signal from a 4 cylinder engine with 2 tach pulses per engine revolution and tsch pulse width equivslant to 30 degrees of crank rotation. The output is varied from 1000 to 9000 rpm by turning a pot connected to an analog input pin.

*/

int tachout = 12; // select the pin for the tach out signal
int sensorPin = A0; // select the input pin for the potentiometer
int potvalue = 0; // variable to store the value coming from the sensor
int target_rpm = 0;
long int period = 0;
long int pulse_width = 0;
float last_pulse = 0;

void setup()
{
// declare the tachout as an OUTPUT:
pinMode(tachout, OUTPUT);
}

void loop()
{
potvalue = analogRead(sensorPin);
target_rpm = ((8000L*potvalue)/1023) + 1000;

period = 60000000/(2*target_rpm); // calculate time between tach pulses
pulse_width = period/6;
if (micros() < ( last_pulse + pulse_width))
{
digitalWrite(tachout, HIGH);
}
else
{
digitalWrite(tachout, LOW);
}
if (micros() > ( last_pulse + period))
{
last_pulse = micros();
}
}

My guess is that this works because the compiler optimises the expression to

No. The 8 should be a 7.

My guess is that this works because the compiler optimises the expression to

Code:

target_rpm = (8 * (potvalue/1000) ) + 1000

Well close, but no cigar.

8000*potvalue/1000 would optimise to 8*potvalue , not to 8*potvalue/1000

A somewhat related question, how are remainders handled for division of integers with an integer result? Is the resultant quotient rounded off or truncated? In the case of my earlier code, if the division of 8000/1023 is truncated, the result is 7 if it's rounded, the result is 8.

And while I'm at it, is there an online resource somewhere to help the novice programmer navigate the subtleties of integer math?

Quotients don't need rounding off or truncation as they are exact. A quotient is just an integer. There are issues with signed quotients due to the way that C does signed integer division by truncating towards zero (ie trunc(), not floor()).

Anyway back to your actual calculation. The 1023 should be 1024 anyway if you read the specs, so the division can simply be a shift:

target_rpm = ((8000 * potvalue) >> 10) + 1000

To be as accurate as possible the actual conversion from ADC output to voltage would be

  float voltage = (analogRead () + 0.5) * Vcc / 1024.0 ;

But that correction of 1/2 LSB is seldom relevant, and floating point is a lot slower to calculate.

Integer division truncates.

Pete

8000*potvalue/1000 would optimise to 8*potvalue , not to 8*potvalue/1000

Yup. You spotted my deliberate mistake :)