Streamline my button function, passing array data.

edit: I scaled the question down to a more understandable size, thanks for your input. please see the 4th post.

Initial post: Hi All,

I started a sketch, which is working 100% for me, but i feel i can simplyfy the code alot. I feel i need to use to may variables and i need to write to much duplicate code to create another button. I think the function "DigitalUserInput_stopPoll" is sort of completly redundant, but somehow i was not able to pass data directly into the "real" button checking function. At this specific point i would like some guidence.

the goal of this sketch is to create an button library/class that will later on drive multiple stepperdrivers. I want to avoid any delay functions because this would prevent my steppers later on to run smooth.

The following is just a setup of a approach to create an flexible button library that will easily setup a button every time. Button, can be toggle, momentary long pressed and later maybe double pressed(which i don't care for personally) I understand there are ready library's available, but i see this as an exercise.

disclaimer, i have about a week or 2 programming experience, but a life time of logic thinking. Please if you see very unlogical approaches i would be happy if they will be pointed out. I really feel like why! did i not started doing this earlier, this is fun!

I understand this code should be jarred in some sort of class later on but i'm not there yet, or actually i would love to understand that!, maybe you boys and girls can help me out.

debouncing is basically done by slowing down the program, this is now a delay, but otherwise could be all the stepper motor calls and other calculations. For this to become a class, maybe i should add a timed debouncing, something like, only operate when 5millis has been passed since last poll.

int DigitalUserInput_stopPin = 10;//digital input on pin 10
int DigitalUserInput_stop[5] = {DigitalUserInput_stopPin, 0, 1, 1000, 0};//sets array for pushbutton STOP
// place 0 = pin, 
// 1 = 0 = state of button, 0 = off, 1 = on, 2 = undefined, 3 = is pressed longer than "holdtime", 4 = pressing the button after holdtime(state 3).by default should be 0
// 2 = type = 0 for momentairy button, 1 = for switch on off button, 2 = for a toggle switch(cancells longpress options)
// 3 = holdtime = the time before a long pressed is registered in millisecond 1000 is 1 sec.
// 4 = reset = to let the program know if the button is cleared for a next step.
unsigned long DigitalUserInput_stopTimeStamp = 0;//defines a variable for a timestamp. i wanted this in teh array, but the unsigned long conflicted with the INT's.
unsigned long currentMillis = 0;// declares the reference timer
int pin = 0;//variable later used by the "checkDigitalUserInput" function
int state = 0;//variable later used by the "checkDigitalUserInput" function
int type = 0;//variable later used by the "checkDigitalUserInput" function
unsigned long Stime = 0;//variable later used by the "checkDigitalUserInput" function
unsigned long Etime = 0;//variable later used by the "checkDigitalUserInput" function
int holdtime = 0;//variable later used by the "checkDigitalUserInput" function
int reset = 0;//variable later used by the "checkDigitalUserInput" function
int timSerial = 0;//variable used to have a timed serial output rather than delaying the program.

void setup() {
pinMode(DigitalUserInput_stopPin, INPUT);//declares pin 8 to be a stopPin
Serial.begin(9600);
}
void DigitalUserInput_stopPoll(){
  checkDigitalUserInput(DigitalUserInput_stop[0], DigitalUserInput_stop[1], DigitalUserInput_stop[2], DigitalUserInput_stop[3], DigitalUserInput_stop[4], DigitalUserInput_stopTimeStamp);
  DigitalUserInput_stop[0] = pin; 
  DigitalUserInput_stop[1] = state; 
  DigitalUserInput_stop[2] = type; 
  DigitalUserInput_stop[3] = holdtime; 
  DigitalUserInput_stop[4] = reset; 
  DigitalUserInput_stopTimeStamp = Stime; 
  Stime = 0;
}
void checkDigitalUserInput(int pinF, int stateF, int typeF, int holdtimeF, int resetF, unsigned long StimeF)
{
  currentMillis = millis();
  //Serial.println(pinF);
  pin = pinF;//this gives me the possibility to rember the old state
  state = stateF;//this gives me the possibility to rember the old state
  type = typeF;//this gives me the possibility to rember the old state
  Stime = StimeF;//this gives me the possibility to rember the old state 
  reset = resetF;//this gives me the possibility to rember the old state
  if(state == 4){// when pressed after 3, state 4 occurs, only needed once. so clear to state unpressed
    state = 0;
    reset = 0;
    }
  if(typeF == 0 || typeF == 2)//if a momentairy switch/button or toggle switch, do this
  {
    if(digitalRead(pinF) == HIGH && stateF == 0 && resetF == 0)//reset is a variable that tells if the button is "ready" for a new state. 
    {
      if(typeF == 0){Stime = currentMillis;}//button is pressed, change its state, start the timer, here hardware toggle switches get excluded from timing.
      state = 1;
      reset = 1;
      Etime = 0;
    }
    if(digitalRead(pinF) == LOW && stateF == 1 && resetF == 1)//button released after being pressed
    {
      //Etime = (currentMillis - Stime);// no need for this as the timer will run aslong as the button is NOT released.
      state = 0;
      reset = 0;//sets the button to available 
      Stime = 0;//clears start time
    }
  }
  if(typeF == 1)//if the momentairy button should act as a switch, (press once for on, press again for off) do this:
  {
    if(digitalRead(pinF) == HIGH && resetF == 0)
    {
      Stime = currentMillis;//start timer
      reset = 1;//reset high(button busy)
      if(stateF == 0)
      {
        state = 1;
      }
      if(stateF == 1)//if state is high
      {
        state = 0;//set o
      }
    }
    if(digitalRead(pinF) == LOW && resetF == 1)//if low and reset is high, theoretically this part should come first.....
    {
      //Etime = (currentMillis - Stime);//print time
      reset = 0;// make button ready for a state change
      Stime = 0;//stop timing, button has been released.
    }
    }
    if(Stime != 0)// this is to prevent to set Etime as currentMillis, which always will be bigger than the proposed holdTime
    {
    Etime = (currentMillis - Stime);//setting an elapsed time if any.
    }
    if(Etime > holdtimeF)//if time is larger than hold time
    {
      state = 3;//set 3
      Stime = 0;//clear starttime
      reset = 1;//sets reset high as to prevent it from looping the timer again.
      Etime = 0;//clears time bank
    }
    if(digitalRead(pinF) == LOW && stateF == 3 && resetF == 1)// if button released clear the button by reset = 0
    {
      //Stime = currentMillis;
      //state = 3;
      reset = 0;
      //Etime = 0;
    }
    if(digitalRead(pinF) == HIGH && stateF == 3 && resetF == 0)
    {
      Stime = 0;// this should not result in a long press again.
      state = 4;// will be reset every time this complete function is called, so 1 time use only :)
      reset = 1;
      //Etime = 0;
    }
    pin = pinF;// give back variables, is this nescesairy, probably not
    type = typeF;// give back variables, is this nescesairy, probably not
    holdtime = holdtimeF;// give back variables, is this nescesairy, probably not
}
void loop() {
   DigitalUserInput_stopPoll();//check status
   delay(100);//dont kill the serial monitor
   Serial.println(DigitalUserInput_stop[0]);//show pin
   Serial.println(DigitalUserInput_stop[1]);//show state
   Serial.println(DigitalUserInput_stop[2]);// show type
   Serial.println(DigitalUserInput_stop[3]);//show holdtime
   Serial.println(DigitalUserInput_stop[4]);//show reset
   Serial.println(DigitalUserInput_stopTimeStamp);//show timestamp
}

Thank you!

That code is just too hard to read. Comments need to be separated from the code;

int var; // Some useless comment NOT int var;//Some useless comment

Comments need to be verbose IF they do not follow a statement.

// This can be a verbose comment, stretching // over many, many lines.

Comments need to be brief, to the point, AND useful if they are on the same line as a statement.

if(someCondition) // Need to deal with the case of...

void setup() {
pinMode(DigitalUserInput_stopPin, INPUT);//declares pin 8 to be a stopPin
Serial.begin(9600);
}

Code in a function needs to be indented.

  checkDigitalUserInput(DigitalUserInput_stop[0], DigitalUserInput_stop[1], DigitalUserInput_stop[2], DigitalUserInput_stop[3], DigitalUserInput_stop[4], DigitalUserInput_stopTimeStamp);

It is not only possible to spread this across multiple lines, it makes it easier to read.

  checkDigitalUserInput(DigitalUserInput_stop[0],
                                          DigitalUserInput_stop[1],
                                          DigitalUserInput_stop[2],
                                          DigitalUserInput_stop[3],
                                          DigitalUserInput_stop[4],
                                          DigitalUserInput_stopTimeStamp);

Those really, really long names are annoying.

Using an array to hold values, then passing one element at a time to a function defeats the purpose of the array.

That was as far as I could manage to read. Sorry about that.

Thank you PaulS,

ill make the code more clear, and repost it.

how about like this? :

int button_stopPin = 10;
int button_stop[5] = {button_stopPin, 0, 1, 1000, 0};
// sets array for pushbutton STOP
// place 0 = pin, 
// 1 = 0 = state of button, 0 = off, 1 = on, 2 = undefined, 3 = is pressed longer than "holdtime", 4 = pressing the button after holdtime(state 3).by default should be 0
// 2 = type = 0 for momentairy button, 1 = for switch on off button, 2 = for a toggle switch(cancells longpress options)
// 3 = holdtime = the time before a long pressed is registered in millisecond 1000 is 1 sec.

unsigned long button_stopTimeStamp = 0;
unsigned long currentMillis = 0;
int pin = 0;
int state = 0;
int type = 0;
unsigned long Stime = 0;
unsigned long Etime = 0;
int holdtime = 0;
int reset = 0;
int timSerial = 0;
int temp_array[5] = {0, 0, 0, 0, 0};
int TimeStamp = 0;

void setup(){
   pinMode(button_stopPin, INPUT);
   Serial.begin(9600);
 }


void button_stopPoll(){
  checkbutton(button_stop[0], 
              button_stop[1], 
              button_stop[2], 
              button_stop[3], 
              button_stop[4], 
              button_stopTimeStamp);
  button_stop[0] = pin; 
  button_stop[1] = state; 
  button_stop[2] = type; 
  button_stop[3] = holdtime; 
  button_stop[4] = reset; 
  button_stopTimeStamp = Stime; 
  Stime = 0;
}


void checkbutton(int pinF, 
                 int stateF, 
                 int typeF, 
                 int holdtimeF, 
                 int resetF, 
                 unsigned long StimeF){
  currentMillis = millis();
  pin = pinF;
  state = stateF;
  type = typeF;
  Stime = StimeF; 
  reset = resetF;
  if(state == 4)    //if being pressed after longPress, reset to 0
  {
    state = 0;
    reset = 0;
    }
    
    
  if(typeF == 0 || typeF == 2)    //if a momentairy switch/button or toggle switch, do this
  {
    if(digitalRead(pinF) == HIGH && stateF == 0 && resetF == 0)
    {
      if(typeF == 0)    //button is pressed, change its state, start the timer, here hardware toggle switches get excluded from timing.
      {
        Stime = currentMillis;
      }
      state = 1;
      reset = 1;
      Etime = 0;
    }
    
    if(digitalRead(pinF) == LOW && stateF == 1 && resetF == 1)    //button released after being pressed
    {
      state = 0;
      reset = 0;    //sets the button to available 
      Stime = 0;    //clears start time
    }
  }
  
  

  if(typeF == 1)    //if the momentairy button should act as a switch, (press once for on, press again for off) do this:
  {
    if(digitalRead(pinF) == HIGH && resetF == 0)
    {
      Stime = currentMillis;
      reset = 1;
      if(stateF == 0)
      {
        state = 1;
      }
      if(stateF == 1)
      {
        state = 0;
      }
    }
    
    if(digitalRead(pinF) == LOW && resetF == 1)    //register release of button
    {
      reset = 0;
      Stime = 0;    //stop timing, button has been released.
    }
  }
  
  
  
  if(Stime != 0)    //this is to prevent to set Etime(ellepsed time) as currentMillis
  {
    Etime = (currentMillis - Stime);
  }
  
  
  if(Etime > holdtimeF)    //longpress is true
  {
    state = 3;
    Stime = 0;
    reset = 1;
    Etime = 0;
  }
  
  
  if(digitalRead(pinF) == LOW && stateF == 3 && resetF == 1)     // register button release
  {
    reset = 0;
  }
  
    
  if(digitalRead(pinF) == HIGH && stateF == 3 && resetF == 0)    // sets special state 4
  {
    Stime = 0;
    state = 4;  
    reset = 1;
  }
  
  
  pin = pinF;
  type = typeF;
  holdtime = holdtimeF;
}


void loop() {
   button_stopPoll();
   delay(50);
   Serial.println(button_stop[0]);
   Serial.println(button_stop[1]);
   Serial.println(button_stop[2]);
   Serial.println(button_stop[3]);
   Serial.println(button_stop[4]);
   Serial.println(button_stopTimeStamp);
}

edit: added space to IF statements. ok, i see how this starts to clear thing up.. :)

Where you have a series of IFs and ELSEs divide them up with some white space so it is easy to see where each piece begins and ends. My brain is no good at finding the corresponding closing or opening bracket (unlike the editor I use - but it has no idea what the code does).

...R

I thought i should maybe scale the problem down to a more conceptual level.

I wanted to create an function for having buttons.

My approuch was the folowing, having a data array, to hold vital information such as previous state, timestamps and others, for each button i make.
The button function would be passed this array and it would figure out if changes are made etc…

The function part seems to work fine. However passing the data to the function and retrieving it is far from good.
ideally it would work something like this:

  1. declare button array, type, holdtime, status, and other things needed.
    like so: int button_1[5] = {1, 0, 0, 200, 1}; or something like that.

  2. setup the pins sure.

  3. in the loop check for the button status by feeding “button_1” to the buttoncheck function
    like so: pollbutton(button_1);

it would only be necessary to return the state of the button. (i can handle that)

but what i cant handle is some generic way to read and write to the array, without declaring all the individual places in that array.

any suggestions on how to tackle that in a proper way are welcome.

This is how i solved the issue for now:

int button_stopPin = 10;
int button_stop[5] = {button_stopPin, 0, 1, 1000, 0};
void button_stopPoll(){
  checkbutton(button_stop[0], 
              button_stop[1], 
              button_stop[2], 
              button_stop[3], 
              button_stop[4], 
              button_stopTimeStamp);
  button_stop[0] = pin; 
  button_stop[1] = state; 
  button_stop[2] = type; 
  button_stop[3] = holdtime; 
  button_stop[4] = reset; 
  button_stopTimeStamp = Stime; 
  Stime = 0;
}
void checkbutton(int pinF, 
                 int stateF, 
                 int typeF, 
                 int holdtimeF, 
                 int resetF, 
                 unsigned long StimeF){

Lots of other code here that deals with the variables..
and updates them to the new states.

}

in loop to update the button:

button_stopPoll();

My approuch was the folowing, having a data array, to hold vital information such as previous state, timestamps and others, for each button i make.

The stuff you want to put in your array is not all of the same type. Time and state are not the same things. You need a struct (or a class), not an array.

You do NOT want a methodology to deal, yet, with "for each button...". You want something that works for ONE [u]switch[/u]. Buttons belong on shirts.

The button function would be passed this array and it would figure out if changes are made etc..

No. The [u]switch[/u] function would be passed an struct full of data.

I indeed found out that the array only holds same type of data, so i passed the time separately.

I'm trying the struct. thank you i did not find that yet.

next class. but i feel that is yet complicated.

thanx for your help.

I know some of the others will disagree with me but on a device with the limited memory of an Arduino I just use global variables and don't generally bother with passing parameters (especially as C/C++ is fussy about types).

...R

I know some of the others will disagree with me but on a device with the limited memory of an Arduino I just use global variables and don’t generally bother with passing parameters (especially as C/C++ is fussy about types).

C/C++ is only fussy about types when you don’t use them right. I can understand the use of global variables, to some extent. Sometimes, using a global variable to avoid the need to pass an array makes sense. But to try to circumvent type processing does not.

PaulS: But to try to circumvent type processing does not.

Of course it doesn't make sense. I wouldn't even know how to try. And I suspect that variable typing is essential on a system with limited SRAM.

But on the PC I program in Ruby which doesn't require variables to be typed and which makes parameter passing much easier.

...R

But on the PC I program in Ruby which doesn't require variables to be typed and which makes parameter passing much easier.

Until it's idea on how to handle some data differs from how you think it will. That leads to hair-loss.

PaulS:
Until it’s idea on how to handle some data differs from how you think it will. That leads to hair-loss.

I haven’t had a problem yet. I’ve a good head of hair. Unlike C/C++ Ruby was designed from the ground up as an object oriented language. Everything is an object. Of course Ruby is utterly unsuitable for the limited memory of an Arduino.

What makes me lose hair is the silly *& system C/C++ has for identifying pointers. I can never remember which is which so it takes me ages to figure out the intention of code that contains them. The concept of pointers is child’s play. It’s just the referencing system that causes me a problem.

…R

What makes me lose hair is the silly *& system C/C++ has for identifying pointers.

An & is how a reference variable is defined, not a pointer. A * is how a pointer is defined.

PaulS:

What makes me lose hair is the silly *& system C/C++ has for identifying pointers.

An & is how a reference variable is defined, not a pointer. A * is how a pointer is defined.

QED

...R