 # Need help with code logic

Hello! I’m relatively new to programming and I’m working on a project for which I’m having trouble coming up with logic for a part of the code.

Here’s what I want to achieve:
an odometer and speedometer for my cycle.

A hall sensor detects when my bike wheel makes one revolution. a variable NUMREV is incremented with each revolution (and then multiplied with circumference to give distance). the distance is then displayed on a oled display.

now, when a button is pressed, the screen is changed to show speed, instead of showing distance.

the distance part, I achieved. the speed part I was not able to achieve and I need your expert help.

here’s the problematic part of the code ( the entire code, should you need it, is at the end.)

when the button is pressed, SCRSTATE value changes from 0 to 1 and the programmer starts to calculate speed and displays it.

``````  else if (SCRSTATE == 1) //button is pressed, screen is switched to display speed
{
PREVNUMREV = NUMREV;  //store the current number of revolutions
PREVMILLIS = millis();

if(millis() - PREVMILLIS == 1000);  //one second later
{
NUMREVDELTA = NUMREV - PREVNUMREV;   //difference in number of revolutions
DISTDELTA = (WHEELCIRC*NUMREVDELTA); //distance that was covered in one second
SPEED = DISTDELTA;  //speed in meters per second
display.clearDisplay();
display.setCursor(20,20);
display.setTextSize(3);
display.setTextColor(SSD1306_WHITE);
display.print(SPEED);                              //print speed on display
display.display();
}
``````

now I know what the problem is. the if statement is evaluating to false. so the Arduino skips this step.
but I don’t know a solution

when the button is pressed, I need the Arduino to store current number of revolutions, then continue counting the number of revolutions (do nothing else) , then one second later, subtract the number of revolutions for previous number of revolutions.

I’m very sorry if this is confusing/a pain in the ass. but I could really use some help and I’d appreciate it.

thanks in advance!! FULL CODE;

``````#include <SPI.h>
#include <Wire.h>

#define SCREEN_WIDTH 128 // OLED display width, in pixels
#define SCREEN_HEIGHT 64 // OLED display height, in pixels
#define HALLPIN 10

#define OLED_RESET     4 // Reset pin #

float WHEELCIRC = 2.00; // wheel circumference in meters
int NUMREV = 0;
int PREVNUMREV,NUMREVDELTA;
float DIST = 0.00 , DISTDELTA = 0.00, SPEED = 0.00;
byte HALLSTATE;
byte State1 = 0;
bool State2 = false;
byte SCRSTATE = 0;
byte SCRSTATE_MAX = 1; //always remember to subtract 1 from number of screens required as SCRSTATE starts from 0
unsigned long PREVMILLIS;
void setup() {
Serial.begin(9600);

// SSD1306_SWITCHCAPVCC = generate display voltage from 3.3V internally
if(!display.begin(SSD1306_SWITCHCAPVCC, 0x3C)) { // Address 0x3D for 128x64
Serial.println(F("SSD1306 allocation failed"));
for(;;); // Don't proceed, loop forever
}

// Show initial display buffer contents on the screen --
// the library initializes this with an Adafruit splash screen.
display.display();
delay(2000); // Pause for 2 seconds

// Clear the buffer
display.clearDisplay();
}

void loop() {

if(digitalRead(9) == 1 && State2 == false)
{
if(SCRSTATE < SCRSTATE_MAX)
{
SCRSTATE++;
}
else if (SCRSTATE == SCRSTATE_MAX)
{
SCRSTATE = 0;
}
State2 = true;
}
{
State2 = false;
}

if(HALLSTATE == 1 && State1 == 0)
{
NUMREV++;
State1 = 1;
}
else if(HALLSTATE == 0)
{
State1 = 0;
}

DIST = NUMREV * WHEELCIRC;
Serial.println(NUMREV);
if(SCRSTATE == 0)
{
display.clearDisplay();
display.setCursor(40,20);
display.setTextSize(3);
display.setTextColor(SSD1306_WHITE);
display.print(DIST);
display.display();
}

else if (SCRSTATE == 1)
{
PREVNUMREV = NUMREV;
PREVMILLIS = millis();

if(millis() - PREVMILLIS == 1000);
{
NUMREVDELTA = NUMREV - PREVNUMREV;
DISTDELTA = (WHEELCIRC*NUMREVDELTA);
SPEED = DISTDELTA;  //speed in meters per second
display.clearDisplay();
display.setCursor(20,20);
display.setTextSize(3);
display.setTextColor(SSD1306_WHITE);
display.print(SPEED);
display.display();
}
}

}
``````

Is pin 9 your button? Is this undocumented code some kind of state change detection? The variable names are not very specific, e.g. ‘State2’:

``````  if(digitalRead(9) == 1 && State2 == false)
{
if(SCRSTATE < SCRSTATE_MAX)
{
SCRSTATE++;
}
else if (SCRSTATE == SCRSTATE_MAX)
{
SCRSTATE = 0;
}
State2 = true;
}
{
State2 = false;
}
``````

I assume the button is on pin 9? How exactly is it connected? Pull-up or pull-down resistor? What value?

From what I can see SCRSTATE can only be 0 or 1 so you don't need the if. A plain else will do.

You really should check the result of digitalRead()s against LOW/HIGH not 0/1.

Steve

slipstick: I assume the button is on pin 9?

...which you would not have to do, if it was defined as were these other variables:

``````#define HALLPIN 10
#define OLED_RESET     4 // Reset pin #
``````

I already pointed out that it isn't documented either. Although, it's somewhat evident from its function.

Hi! yes. I apologise for that. the button is on pin 9 and State2 is just a flag variable to make sure that if you press and hold the button it doesn't keep switching.

slipstick: How exactly is it connected?

A pull down resistor. 6.8Kohm. (the button press is being successfully detected, so the issue isn't from there)

slipstick: From what I can see SCRSTATE can only be 0 or 1 so you don't need the if. A plain else will do.

Yes, but in future releases, I might add more options (heart rate, rpm etc) so I just kept it this way so I can simply change "SCRSTAT_MAX" to change the maximum number of screens possible.

slipstick: You really should check the result of digitalRead()s against LOW/HIGH not 0/1.

Thanks! I'll keep that in mind next time :D

Bicycle wheels are relatively slow and I think it would be more effective to measure the time between pulses rather than counting the number of pulses in a second.

Variable names are usually in lowercase or camelCase and UPPERCASE is usually reserved for constants. The compiler won't care but text in UPPERCASE is surprisingly hard to read.

...R

``````  else if (SCRSTATE == 1)
{
PREVNUMREV = NUMREV;
PREVMILLIS = millis();// <<< Because this gets set every time...

if(millis() - PREVMILLIS == 1000);  //one second later <<<   ...This Never happens
{
``````

PREVMILLIS must be set to equal millis() only once when SCRSTATE becomes true.

Robin2: Bicycle wheels are relatively slow and I think it would be more effective to measure the time between pulses rather than counting the number of pulses in a second.

It's a problem of accuracy. Your pulse sampling window is fixed and it is short- one second. The pulses don't arrive in discrete one second intervals. So for example, if your wheel actually rotated 6.423 times in one second, you will count 6 pulses. The error in percent is 6.4233/6*100-100 = 7%. Obviously, the more pulses per sample window, the greater the accuracy.

The method of measuring pulse duration compares 1/6.423 second with whatever the timing resolution is, for example to the nearest millisecond. There is also truncation, but it is no more than 1 ms. In that case, the accuracy is (1/6.423+0.001)/(1/6.423)*100-100 = 0.6423%

That is a tenfold improvement in accuracy.