calculating speed, result differs from expected value

Hello everybody!

I'm working on a vehicle controller, witch includes a spedometer/odometer, but the results i'm getting are not what i'm expecting.

Ive been pulling my hair since i noticed the error, but i cannot find the reason, so i'm hoping that you guys and gals can help me.

I'm using a online speed calculator, found HERE:

and i'm using https://www.tinkercad.com to emulate my hardware.

So instead of the hall trigger for wheel rotation detection, i'm using a function generator, outputting a sine wave of X Hz.

When i set the function generator to 1 Hz, the "measured" speed is 7.20 km/h while the online speed calculator says it should be 7.6104 km/h, given a wheel circumference of 2114mm.

As i increase the function generator frequency, the error grows by 0,4104 * frequency.
So the measured speed at 5Hz is 36,0km/h, but should be 38,052, a difference of 2.052km/h
At 8Hz, measured speed is 57,6km/h, should be 60,8832km/h, with a difference of 3,2832km/h

Just to show how i'm calculating things:
distance_travelled = wheelCirqumference_in_cm * current_Hz
I then enter distance_travelled in the online calculator above, chose length=cm, time = 1 second and hit calculate.

The actual precision of the spedometer isn't really a deal-breaker for my project, but my autistic side demands at least a reason for this error, if not a solution..

There are 3 functions that are involved;
the "speedISR" witch gets called by the interrupt,
the "doSpeedCalc" which should get triggered when the number of rotations exceeds the minimum count (think of it as debounce), and finally
the "printSpeed" function, to display the current (=most recently calculated values of) speed and distance.

// definitions (should be all of them)
uint16_t rotCount = 0;
uint32_t rotTime = 0;
uint8_t numRotToMeasure = 3;
uint32_t odometer = 0;
float speed = 0.0;
float tripLength = 0.0;
uint32_t lastSpdRep = 0;
uint32_t spdRepInterval = 1500;
uint32_t cirq = 2114; // mm
uint32_t NOW = 0;
#define spdPin 2

bool checkElapsed(uint32_t last, uint32_t limit) { // Returns true if now - last is more than limit.
  bool result = false;
  if (NOW - last > limit) {
    result = true;
  }
  return result;
}

void setup() {
  pinMode(SpdPin, INPUT);
  attachInterrupt(digitalPinToInterrupt(SpdPin), speedISR, RISING);
}
void loop() {
  NOW = millis();
  doSpeedCalc();
}
void speedISR() {  
  if (!rotTime) {
  // if (!rotCount) {
    rotTime = NOW; // NOW = millis();
  }
  else {
    rotCount++;
  }
  odometer++;
}
void doSpeedCalc(void) {
  if (rotCount) { // speed timeout, in case speed is too low to be meaningful..
    if (checkElapsed(rotTime,(3000 * rotCount))) {
      rotCount = 0;
      rotTime = 0;
      speed = 0.0;
    }
  }
  if (rotCount >= numRotToMeasure) {
    uint32_t elapsedTime = 0;
    uint32_t distance = (cirq * (rotCount));
    elapsedTime = (NOW - rotTime);
    // speed = distance / time
    speed = (distance / elapsedTime) * 3.6; // speed = (mm/mS) * 3.6 (to convert to km/h)
    rotCount = 0;
    rotTime = 0;
  }
  if (checkElapsed(lastSpdRep,spdRepInterval)) {
    lastSpdRep = NOW;
    printSpeed();  
  }
}
void printSpeed(void) {
  tripLength = (odometer * cirq) / 1000.0;
  writeVarSerial(STR_SPD_SPEED,speed,STR_SPD_KMH);
  printLongVarLCD(STR_SPD_SPEED,speed,STR_SPD_KMH);
  if (tripLength > 1000) {
    tripLength = tripLength / 1000.0;
    writeVarSerial(STR_SPD_ODO,tripLength,STR_SPD_KM);
    printLongVarLCD2(STR_SPD_ODO,tripLength,STR_SPD_KM);
  }
  else {
    writeVarSerial(STR_SPD_ODO,tripLength,STR_SPD_M);
    printLongVarLCD2(STR_SPD_ODO,tripLength,STR_SPD_M);
  }
}

There are some macro´s in the code above, as well as strings defined elsewhere but i belive you can easily spot the macros and guesstimate their functions.
And since theyre not the source of the frustration here, i have chosen not to include them.

rotCounter, rotTime and odometer should be qualified as volatile, and accesses to them NOT in interrupt context should be with interrupts disabled., ie disable interrupts, take a copy of these variables, re-enable interrupts and do the calculation using the copies.

I changed the rotCounter, rotTime and odometer to volatile, but the error is still there.

I started thinking that this might be some int overflow issue, so i changed the wheel circumference (cirq) to 1000, and now the simulator reports expected speeds.

I then tried to increase the cirq to 1500 and the simulator then started to output incorrect speeds again..
Reported speed is 25.2 when it should be 27.0 at 5Hz.
Reported speed at 1Hz is 3,6km/h but should be 5,4km/h.

When i changed the cirq to 2000 the sim reports expected speeds again, so it does seem to me that my calculations are correct and there´s no overflow, but instead some rounding issue somewhere in the calculation.

But i cant seem to find it..?

----------------- EDIT -----------------
Ive made a simplified circuit and made it public, so feel free to check it out at the link below.

TinkerCad Simulation

Ive also modified the code from the original post, included the changes suggested and included missing elements.
The code below is working in case you want to test it on another emulator/real arduino.

// bikeSys_SPD, By H.Jonsson. dev@liggist.se

// definitions (should be all of them)
volatile uint16_t rotCount = 0;
volatile uint32_t rotTime = 0;
uint8_t numRotToMeasure = 3;
volatile uint32_t odometer = 0;
float speed = 0.0;
float tripLength = 0.0;
uint32_t lastSpdRep = 0;
uint32_t spdRepInterval = 1500;
uint32_t cirq = 2114; // mm
uint32_t NOW = 0;
#define SpdPin 2
#define writeVarSerial(prefix,value,sufix) Serial.println(STR_BLANK); Serial.print(prefix); Serial.print(value); Serial.println(sufix);
#define printLongVarLCD(name,val,suffix) ;
#define printLongVarLCD2(name,val,suffix) ;

#define STR_BLANK " "
#define STR_SPD_SPEED "Spd: "
#define STR_SPD_KMH " Km/h"
#define STR_SPD_ODO "Trip: "
#define STR_SPD_M " m"
#define STR_SPD_KM " Km"

bool checkElapsed(uint32_t last, uint32_t limit) { // Returns true if now - last is more than limit.
  bool result = false;
  if (NOW - last > limit) {
    result = true;
  }
  return result;
}

void setup() {
  pinMode(SpdPin, INPUT);
  attachInterrupt(digitalPinToInterrupt(SpdPin), speedISR, RISING);
  Serial.begin(9600);
  writeVarSerial("BikeSys_SPD"," By H.Jonsson"," dev@liggist.se");
}
void loop() {
  NOW = millis();
  doSpeedCalc();
}

void speedISR() {  
  if (!rotTime) {
  // if (!rotCount) {
    rotTime = NOW; // NOW = millis();
  }
  else {
    rotCount++;
  }
  odometer++;
}

void doSpeedCalc(void) {
  noInterrupts();
  uint16_t _rotCount = rotCount;
  uint32_t _rotTime = rotTime;
  interrupts();
  if (_rotCount) {
    if (checkElapsed(_rotTime,(3000 * _rotCount))) {
      rotCount = 0;
      rotTime = 0;
      speed = 0.0;
    }
  }
  if (_rotCount >= numRotToMeasure) {
    uint32_t elapsedTime = 0;
    uint32_t distance = (cirq * (_rotCount));
    elapsedTime = (NOW - _rotTime);
    // speed = distance / time
    float speedmmms = (distance / elapsedTime); // speed = mm/mS
    speed = speedmmms * 3.6;
    rotCount = 0;
    rotTime = 0;
  }
  if (checkElapsed(lastSpdRep,spdRepInterval)) {
    lastSpdRep = NOW;
    printSpeed();  
  }
}

void printSpeed(void) {
  noInterrupts();
  uint16_t _odometer = odometer;
  interrupts();
  tripLength = (_odometer * cirq) / 1000.0;
  writeVarSerial(STR_SPD_SPEED,speed,STR_SPD_KMH);
  printLongVarLCD(STR_SPD_SPEED,speed,STR_SPD_KMH);
  if (tripLength > 1000) { // if trip more than 1km, display kilometers
    tripLength = tripLength / 1000.0;
    writeVarSerial(STR_SPD_ODO,tripLength,STR_SPD_KM);
    printLongVarLCD2(STR_SPD_ODO,tripLength,STR_SPD_KM);
  }
  else {  // if trip is less than 1km, display meters
    writeVarSerial(STR_SPD_ODO,tripLength,STR_SPD_M);
    printLongVarLCD2(STR_SPD_ODO,tripLength,STR_SPD_M);
  }
}

You use a critical section to read rotCount and rotTime, but fail to do so when writing them - this will cause random errors. So noInterrupts()/interrupts() round all the reads and writes to the volatile variables is required - worth testing to see if that fixes it.

Also you don't seem to realize how simple/concise it is in C to handle boolean expressions:

You can rewrite:

bool checkElapsed(uint32_t last, uint32_t limit) { // Returns true if now - last is more than limit.
  bool result = false;
  if (NOW - last > limit) {
    result = true;
  }
  return result;
}

as

bool checkElapsed(uint32_t last, uint32_t limit)
{
  return NOW - last > limit ;
}

You might want to use floats for the final division calculating the speed, integer division truncates to an integer.

My other thought is the code looks over complicated. Surely you just need to record a count and a time, then at a later point record them again, take differences and divide. No need to reset any of the variables either, that's adding complexity and scope for error I think.

MarkT:
..you don't seem to realize how simple/concise it is in C to handle boolean expressions:

You can rewrite:
....

bool checkElapsed(uint32_t last, uint32_t limit)

{
 return NOW - last > limit ;
}

First off: thank you very much for that hint regarding bool expressions! :smiley:

MarkT:
You might want to use floats for the final division calculating the speed, integer division truncates to an integer.

second: ive been over my code again and again but i fail to see why you would suggest that i use float for final division, since i believe im already doing exactly that.. (see current code below)

MarkT:
My other thought is the code looks over complicated. Surely you just need to record a count and a time, then at a later point record them again, take differences and divide. No need to reset any of the variables either, that's adding complexity and scope for error I think.

Third: i do agree that i often write overly complicated code and that is a potential source of error.
However, i do want current speed, not average speed since power on, meaning i will have to reset the counters and timers each time ive calculated a new current speed.
I might be wrong here tho, but my logic seems sound to me. :slight_smile:

// global variables:
float speed = 0.0;

void speedISR() {  
  if (!rotTime) {
  // if (!rotCount) {
    rotTime = NOW;
  }
  else {
    rotCount++;
  }
  odometer++;
}

void doSpeedCalc(void) {
  noInterrupts();
  uint16_t _rotCount = rotCount;
  uint32_t _rotTime = rotTime;
  interrupts();
  if (_rotCount) {
    if (checkElapsed(_rotTime,(3000 * _rotCount))) {
      noInterrupts();
      rotCount = 0;
      rotTime = 0;
      speed = 0.0;
      interrupts();
    }
  }
  if (_rotCount >= numRotToMeasure) {
    uint32_t elapsedTime = 0;
    uint32_t distance = (cirq * (_rotCount));
    elapsedTime = (NOW - _rotTime);
    // speed = distance / time
    speed = ((distance / elapsedTime) * 3.6);
    noInterrupts();
    rotCount = 0;
    rotTime = 0;
    interrupts();
  }
  if (checkElapsed(lastSpdRep,spdRepInterval)) {
    lastSpdRep = NOW;
    printSpeed();  
  }
}

And yes, the simulator is still giving me wrong speeds, unless im using even thousands as cirq (cirq=2000).

It turned out to be so simple..
Ive known for quite some time that floats works somewhat oddly on the arduino and this is yet another case of floats gone bad.. :stuck_out_tongue:

I got a reply on the simulator forum and a very helpful fellow named Mehdi Hassan Apollo replied with the solution.

it was as simple as adding (float(value)) to the equation.
previous code:
speed = ((distance / elapsedTime) * 3.6);

Updated code:
speed = ((float(distance) / float(elapsedTime)) * 3.6);

Both variables were always declared as floats, but apparently the arduino needed a reminder..

I still get the odd speed result thats a bit off, like if there´s something that messes up the elapsed time, but i can live with that since the error is rare and quite small (761km/h was presented as 758km/h).

so the correct doSpeedCalc function looks like this now:

void doSpeedCalc(void) {
  noInterrupts();
  uint16_t _rotCount = rotCount;
  uint32_t _rotTime = rotTime;
  interrupts();
  if (_rotCount) {
    if (checkElapsed(_rotTime,(3000 * _rotCount))) {
      noInterrupts();
      rotCount = 0;
      rotTime = 0;
      speed = 0.0;
      interrupts();
    }
  }
  if (_rotCount >= numRotToMeasure) {
    noInterrupts();
    rotCount = 0;
    rotTime = 0;
    interrupts();
    uint32_t elapsedTime = 0;
    uint32_t distance = (cirq * (_rotCount));
    elapsedTime = (NOW - _rotTime);
    // speed = distance / time
    speed = ((float(distance) / float(elapsedTime)) * 3.6);
  }
  if (checkElapsed(lastSpdRep,spdRepInterval)) {
    lastSpdRep = NOW;
    printSpeed();  
  }
}

Both variables were always declared as floats, but apparently the arduino needed a reminder..

Nonsense.
Everywhere on this page they are uint32_t.

You are indeed correct AWOL, apparently i should have more coffe before i start typing on forums.. :slight_smile: