Millis() in a while loop

Projects Programming
1

Hi Everyone,

I wanted to sanity check myself on some code I am writing. C++ is far from my strong suit so I wanted to confirm if my understanding is correct. I am writing a function to time how long a button is pressed as part of an arming sequence for the control box of a model rocket project. See code below for the arming sequence. For now it's not linked to any functions but just some print outs to help clarify if it's functioning correctly.

bool isSwitchesOn;
bool threeSecondHold;
bool takeOff;

void readAll(){
  Switch1 = digitalRead(SW1);
  Switch2 = digitalRead(SW2);
  JoyPress1 = digitalRead(SWJ1);
  JoyPress2 = digitalRead(SWJ2);
}


unsigned long timePress(int button){
  unsigned long startTime;
  unsigned long endTime;

  while(button == 0){
    startTime = millis();
  }
  endTime = millis();
  unsigned long duration;
  duration == startTime - endTime;
  Serial.print(duration);
  return duration;
}

void takeOffSequence(){
  readAll();
  if(Switch1 == 1 && Switch2 == 1){
    isSwitchesOn = true;
  }

  if(isSwitchesOn == true){
    display.clearDisplay();
    display.setTextSize(2);
    display.setTextColor(WHITE);
    display.setCursor(0, 0);
    display.println("Switches Armed");
    Serial.println("Switches Armed");
  }

  if(timePress(JoyPress1) > 3000 && timePress(JoyPress2) > 3000){
    threeSecondHold = true;
  }


  if(isSwitchesOn == true && threeSecondHold == true){
    takeOff = true;
  }

  if(takeOff == true){
    display.clearDisplay();
    display.setTextSize(2);
    display.setTextColor(WHITE);
    display.setCursor(0, 20);
    display.println("TAKEOFF!!");
    Serial.println("TAKEOFF!!");
    delay(3000);
  }

I was curious as to how the millis() function behaves in a while loop. Specifically in the timePress() function I've created. When the variable is set to millis() would that record the time when the condition became true or would it continue updating the variable until the condition is no longer true... essentially will this function work to to time the button presses in and in the context I'm attempting to apply it in?

I'm currently awaiting a PCB and lack sufficient jumper cables throw together a breadboard and test so any advice ahead of time would be amazing.

Regards

Aman Basra

2

Hi
This doesn't seem right to me.
duration == startTime - endTime;

== is comparison between values and no attribution.
I believe you wanted to write:
duration = startTime - endTime;

3

Please show a full sketch. I don't know what the parameter button is, is that a pin number ?

4

cheers for pointing that out. Quit right it was meant to be a single =.

5

Hi,

Here is the full code. I didn't include it because it contains a lot of unfinished bits. I tend to jump between functions and leave things half finished so I figured refining it to that section might've been an easier read.

#include <Arduino.h>
#include <SPI.h>
#include <Wire.h>
#include <RF24.h>
#include <nRF24L01.h>
#include <printf.h>
#include <Adafruit_GFX.h>
#include <Adafruit_SSD1306.h>

//RF Init
// byte addresses[6] = ("0");
// bool ConnectionStatus = ("Not Connected");

//OLED Dimensions
#define SCREEN_WIDTH 128
#define SCREEN_HEIGHT 64

//Display Init
Adafruit_SSD1306 display(SCREEN_WIDTH, SCREEN_HEIGHT, &Wire, -1);

//NRFPins
#define CE_PIN  16
#define CSN_PIN 13

//Toggle Switch pins
#define SW1 5
#define SW2 17

//Encoder 1 Pins
#define CLK1 36
#define DT1 39
#define ESW1 34

//Encoder 2 Pins
#define CLK2 33
#define DT2 32
#define ESW2 35

//Potentiometer Pins
#define P1 25
#define P2 26

//Joystick Pins
#define X1 4
#define Y1 2
#define SWJ1 15

#define X2 27
#define Y2 14
#define SWJ2 12

// //Data send struct
// typedef struct{
//   bool sw1_on;
//   bool sw2_on;
//   int p1;
//   int p2;
//   int enc1;
//   bool esw1;
//   int enc2;
//   bool esw2;
//   int x1;
//   int y1;
//   bool jsw1;
//   int x2;
//   int y2;
//   bool jsw2;
// }
// tx_data;
// tx_data data;

int Switch1, Switch2;
int JoyPress1, JoyPress2;

void setup()
 {
  if (!display.begin(SSD1306_SWITCHCAPVCC, 0x3C))
  { // Address 0x3D for 128x64
    Serial.println(F("SSD1306 allocation failed"));
    for (;;)
      ;
  }
  delay(2000);
  display.display();
  delay(2000);
  display.clearDisplay();

  Serial.begin(115200);
  pinMode(SW1, INPUT_PULLUP);
  pinMode(SW2, INPUT_PULLUP);

  pinMode(CLK1, INPUT);
  pinMode(DT1, INPUT);
  pinMode(ESW1, INPUT_PULLUP);

  pinMode(CLK2, INPUT);
  pinMode(DT2, INPUT);
  pinMode(ESW2, INPUT_PULLUP);

  pinMode(X1, INPUT);
  pinMode(Y1, INPUT);
  pinMode(SWJ1, INPUT_PULLUP);

  pinMode(X2, INPUT);
  pinMode(Y2, INPUT);
  pinMode(SWJ2, INPUT_PULLUP);

  display.setTextSize(1);
  display.setTextColor(WHITE);
  display.setCursor(0, 0);
  display.println("Display Operational");
  display.display();
  delay(250);
  display.clearDisplay();
}

bool isSwitchesOn;
bool threeSecondHold;
bool takeOff;

void readAll(){
  Switch1 = digitalRead(SW1);
  Switch2 = digitalRead(SW2);
  JoyPress1 = digitalRead(SWJ1);
  JoyPress2 = digitalRead(SWJ2);
}


unsigned long timePress(int button){
  unsigned long startTime;
  unsigned long endTime;

  while(button == 0){
    startTime = millis();
  }
  endTime = millis();
  unsigned long duration;
  duration = startTime - endTime;
  Serial.print(duration);
  return duration;
}

void takeOffSequence(){
  readAll();
  if(Switch1 == 1 && Switch2 == 1){
    isSwitchesOn = true;
  }

  if(isSwitchesOn == true){
    display.clearDisplay();
    display.setTextSize(2);
    display.setTextColor(WHITE);
    display.setCursor(0, 0);
    display.println("Switches Armed");
    Serial.println("Switches Armed");
  }

  if(timePress(JoyPress1) > 3000 && timePress(JoyPress2) > 3000){
    threeSecondHold = true;
  }


  if(isSwitchesOn == true && threeSecondHold == true){
    takeOff = true;
  }

  if(takeOff == true){
    display.clearDisplay();
    display.setTextSize(2);
    display.setTextColor(WHITE);
    display.setCursor(0, 20);
    display.println("TAKEOFF!!");
    Serial.println("TAKEOFF!!");
    delay(3000);
  }
}

void printAll(){
  int pot1 = analogRead(P1);
  pot1 = map(pot1, 0, 4095, 0, 100);
  int pot2 = analogRead(P2);
  pot2 = map(pot2, 0, 4095, 0, 100);
  Serial.print("Switch 1    - ");
  Serial.print(digitalRead(SW1));
  Serial.print("    | Switch2  - ");
  Serial.println(digitalRead(SW2));
  Serial.print("X1          - ");
  Serial.print(analogRead(X1));
  Serial.print("    | x2       - ");
  Serial.println(analogRead(X2));
  Serial.print("Y1          - ");
  Serial.print(digitalRead(Y1));
  Serial.print("    | X2       - ");
  Serial.println(analogRead(X2));
  Serial.print("Y1          - ");
  Serial.print(analogRead(Y1));
  Serial.print("    | Y2       - ");
  Serial.println(analogRead(Y2));
  Serial.print("JSW1        - ");
  Serial.print(digitalRead(SWJ1));
  Serial.print("    | JSW2     - ");
  Serial.println(digitalRead(SWJ2));
  Serial.print("Enc1        - ");
  Serial.print("TBC");
  Serial.print("      |  Enc2   - ");
  Serial.println("TBC");
  Serial.print("ESW1        - ");
  Serial.print(digitalRead(ESW1));
  Serial.print("      |  ESW2   - ");
  Serial.println(digitalRead(ESW2));
  Serial.print("Pot1        - ");
  Serial.print(analogRead(P1));
  Serial.print("      |  Pot2   - ");
  Serial.println(pot1);
  delay(250);
  Serial.print("[2J");
}


void loop(){
  takeOffSequence();
}

This unit will be used as a Universal controller amongst future projects too so I will be incorporating ESP-NOW Comms, NRF24L01 and BLE for wireless comms hence a few references sprinkled around.

EDIT Any tips for formatting Serial.print()s would be greatly appreciated. The printAll() function is very messy and irritating me!

6

From what I can see, you are making rather gratuitous use of boolean variables. An 'if' statement by itself, is adequate to test a condition and execute some code. You don't have to create a variable for the sole purpose of testing. I highlighted the last example in loop(), I think it's not the only case, there are more... but for example the above code doesn't use 'takeoff' for any other purpose, and so should be expunged because it's non-standard coding.

if(isSwitchesOn == true && threeSecondHold == true) {
    display.clearDisplay();
    display.setTextSize(2);
    display.setTextColor(WHITE);
    display.setCursor(0, 20);
    display.println("TAKEOFF!!");
    Serial.println("TAKEOFF!!");
    delay(3000);
  }

Which does exactly the same thing, but more straightforwardly.

With your approach, there is a risk that re-arrangement of the code sequence could cause the value of 'takeOff' to be mishandled.

7

Nothing changes the value of button in the body of your while loop.

If button is ever 0, your program gets stucked here.

a7

8

Thanks for the advice. You're right, the takeOff bool was useless. I have removed it to streamline the code. I'm almost certain the code is riddled with inefficiencies like this. I intend to get a functional set of functions working then hopefully take the time to dive deeper into C++ and make the code 'Prettier', simpler and more efficient.

9

It's not actually inefficient. The compiler would optimize that to the same target code. It's just not the best approach from the point of view of source code quality.

10

Ahhh, I totally missed this. I've added the readAll(); function ahead of the function which should update the buttons state.

unsigned long timePress(int button){
  readAll();
  unsigned long startTime;
  unsigned long endTime;
  unsigned long duration;

  while(button == 0){
    startTime = millis();
  }
  endTime = millis();
  duration = startTime - endTime;
  Serial.print(duration);
  return duration;
}
11

Another minor point, it's good to name pin states explicitly, like

while(button == LOW){

instead of

while(button == 0){
12

Very basic question. If I've stored the states as int would this work? Does C++ recognise 0 as LOW? For example the variable for the state of the Joystick Button (JoyPress1) is an int so I assumed it would require me to use the binary terms.

13

Yes, you can store an acquired pin state in an 'int' variable. So

if (Switch1 == LOW) {

is perfectly okay.

I just noticed, the definition of Switch1 is missing from your sketch. Please update the sketch so it's complete. Actually, better to re-post in a new message.

14

I've burned a bit more grey matter on this, perhaps logiced my way to insanity with this one. Very long winded but I think this should fundamentally return the length of time the button was pressed for...

unsigned long take2(int button){
  readAll();
  unsigned long startTime;
  unsigned long endTime;
  unsigned long duration;
  if(button == LOW){
    endTime = 0;
    startTime = millis();
  }
  else if(button == HIGH){
    endTime = millis();
  }
  duration = startTime - endTime;
  Serial.print(duration);
  endTime = 0;
  return duration;
}

So the timer should reset so as not to end up with an ever growing end time . I was considering adding previous and current state tracking to trigger the timing but not sure if that's necessary... Does this make any sense or was the use of a while loop more applicable to this?

15

as ageneral advice:

write code for one single thing. Test it throuroughly and if it works reliably add another small function.

By coding this way the number of lines where the bug could sit is almost everytime small.

If you write a lot of code before start testing it there are exponential more places where the bug could sit. And it will take much longer to find the bug.

If you want to use waiting time. Do experiments.

for example write a test code that does check and analyse this:

You might feel "moving forward" by writing all this code without testing it. But in fact you are creating more work to test.

Well if it is irritating you. If you don't write what you ideally expect it to look like How should somebody else make a suggestion?
As a main idea:
put the title to the left and the value to the right.
group lines that belong together
insert a empty line before and after each group

keep the function printall() but use more **sub-**functions where each sub-function prints some data title and value together that belong together.

As you are using standard things like pots and buttons you could test your code with the wokwi-simulator

best regards Stefan

1 Like
16

Something I noticed - it may or may not matter to you at this point. Your button code blocks while the switch is held down. This means nothing else can happen. In your case, it might not be a problem but in future, you may want to keep other parts of the program running while you poll buttons, so you would need to re-think the way you do that.

17

As I tried to provide context for your code in the wokwi simulator, I noticed that

you might create an entire functioning sketch that simply shows your take2() code working or not, like how it will be used in a larger sketch.

It seems odd to call take2 with an int.

How does readAll() update the variable button? Which appears in your code to be employed as the value of an i/o pin, not its number.

So please a nice test sketch! Maybe take3. :expressionless:

a7

18

I'm not completely sure about your case, but often this happens because people start coding before they have a solid plan for how the program should be structured. The code lulls you into the belief that you're planning as you go along. In fact, it's getting in the way.

19

Time for, well, something else. But here's a tip:

//  duration = startTime - endTime;
    duration = endTime - startTime;

millis() is the number of milliseconds your code has been running when you call it, the duration is the larger, later number minus the smaller, earlier number.

It was a late problem for me tinkering with your code: I read past it at least twenty times, so.

a7

20

Don't feel bad, the first time I saw it I thought "What the heck (close) is a millis()?", but the context makes it obvious. Strangest abbreviation for getting time in milliseconds I have come across. But then many would not like my getTimeMs() either, or the homely sisters, getTimeUs(), and getTimeTicks().

My function and variable names are on the long side so I can figure them out later on, in concert with comments of course. I also try to make variables start with an object or noun and my functions start with a verb (or noun used as a verb). /End of unwanted, pedantic, and perceivably smarmy, post./