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!! :smiley:

FULL CODE;

#include <SPI.h>
#include <Wire.h>
#include <Adafruit_GFX.h>
#include <Adafruit_SSD1306.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 # 
Adafruit_SSD1306 display(SCREEN_WIDTH, SCREEN_HEIGHT, &Wire, OLED_RESET);

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;
  }
  else if (digitalRead(9) == 0)
  {
    State2 = false;
  }

 

  HALLSTATE = digitalRead(HALLPIN);

  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;
  }
  else if (digitalRead(9) == 0)
  {
    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.