Odd behaviour of the outputs

Projects Programming
1

I made a code to manage 4 outputs from 8 different inputs.

4 of these inputs are push-on/push-off (on/off switch)
4 of these inputs are just push-on/release off (push button, NO).

At the 4 outputs i connected 4 relays thru 4 transistors. Also i installed LEDs after the transistor to monitor the status of the relay.

My problem is that if i use the 4 on/off switch inoputs everything works as intended, while the temporary inputs act weird. By weird i mean they send a very low voltage signal to the transistors, enought to light up the leds (very dimmed) but not to make the relay click.

I apologize for the long code, the board didn't let me paste it in here so i had to attach it.

Now my problem i assume is where i used digitalWrite. I tried to edit that and using the outstate1, 2, 3 and 4 but that complicates the things even more because it makes the relay click but it only serves as a push on activation (it doesn't turn off).

However if i create a command that turns the states LOW when no input is fed then the push/on-push/off operation messes up because it turns off all the outputs after 1500ms (when the delay for the buzzer ends).

What am i doing wrong?

Thanks

gener.ino (10 KB)

2

Because you did not provide anything like a schematic or wiring diagram, I cannot comment on your issue. I cannot tell whether you used pullup resistors, pulldown resistors, or no resistors.

However,

    if (outstate1 == HIGH)
      outstate1 = HIGH;

Why set outstate1 to HIGH if it is already HIGH?

      outstate1 = HIGH,
      outstate4 = HIGH;

Comma does not do what you seem to think that it does. It works here, but a semicolon would be easier to understand.

int in1State;
int in2State;
int in3State;
int in4State;

When you are tempted to use numbers in the names of variables, it is time to learn about arrays.

  for (int x = 0; x < 80; x++) {   // Wait for 1 second
    delay(1);
  }

Bad comments can be worse than no comments.

void beep(int repeats, int time) {

The parameter repeats is never used. This may be satisfactory but is highly suspicious.

This program has a huge number of if statements. There may be ways to simplify this.

3

What am i doing wrong?

It sounds like you are not using the pinMode() function correctly or at all.

You must declare digital pins to be INPUT, INPUT_PULLUP or OUTPUT.

By weird i mean they send a very low voltage signal to the transistors, enought to light up the leds

That happens when you call digitalWrite(pin, HIGH) on an input pin, which turns on the pullup resistor and makes it appear to be an output pin set to a weak HIGH.

4

Thank you for your reply.
I genuinely didn't think that a schematic could be part of the diagnosis.
So i made one very quickly.
I'm not very good at designing schematics.

vaj4088:

    if (outstate1 == HIGH)

outstate1 = HIGH;




Why set **outstate1** to **HIGH** if it is already **HIGH**?

I can't remember now, i believe it is part of a code i eventually removed.
I'll try to remove it and see if it changes anything.

      outstate1 = HIGH,

outstate4 = HIGH;




Comma does not do what you seem to think that it does. It works here, but a semicolon would be easier to understand.

I've seen it used, i thought it was somehow necessary, i will replace them with semicolon if that's what's needed.

int in1State;

int in2State;
int in3State;
int in4State;




When you are tempted to use numbers in the names of variables, it is time to learn about arrays.

Uhm... ok, i'll look into that.

  for (int x = 0; x < 80; x++) {   // Wait for 1 second

delay(1);
  }




Bad comments can be worse than no comments.

My bad, i started with a bigger value to see if the code worked ok and then adjusted it to fit my needs and forgot to change the instruction. :cold_sweat:

void beep(int repeats, int time) {

The parameter repeats is never used. This may be satisfactory but is highly suspicious.

This program has a huge number of if statements. There may be ways to simplify this.

the beep part of the code was entirely copied from another project found online, i didn't want to mess with it unless it gave me problems.

I would LOVE to semplify this code so bad, but how?

I really appreciate your inputs.

schema.png
schema.png1058×962 13.2 KB

5

jremington:
It sounds like you are not using the pinMode() function correctly or at all.

You must declare digital pins to be INPUT, INPUT_PULLUP or OUTPUT.
That happens when you call digitalWrite(pin, HIGH) on an input pin, which turns on the pullup resistor and makes it appear to be an output pin set to a weak HIGH.

I did, under void setup()

void setup()
{
  pinMode(inPin, INPUT);
  pinMode(in2Pin, INPUT);
  pinMode(in3Pin, INPUT);
  pinMode(in4Pin, INPUT);
  pinMode(in1bisPin, INPUT);
  pinMode(in2bisPin, INPUT);
  pinMode(in3bisPin, INPUT);
  pinMode(in4bisPin, INPUT);
  pinMode(outPin, OUTPUT);
  pinMode(out2Pin, OUTPUT);
  pinMode(out3Pin, OUTPUT);
  pinMode(out4Pin, OUTPUT);
  pinMode(buzzerPin, OUTPUT);

}
6

This is how you could simplify this OP. Not tested, written hurriedly before class starts. I have assumed what your logic is, I haven't actually looked at your sketch but hopefully you get the idea of how it works.

const byte pushButtons[] = {2, 3, 4, 5};
const byte switches[] = {6, 7, 8, 9};
const byte outputs = {10, 11, 12, 13};

void setup() {
  for(byte i = 0; i < 4; i++)
  {
    pinMode(pushButtons[i], INPUT);
    pinMode(switches[i], INPUT);
    pinMode(outputs[i], OUTPUT);
  }
}

void loop() {
  for(byte i = 0; i < 4; i++)
  {
    if(digitalRead(pushButtons[i]) == HIGH || digitalRead(switches[i]) == HIGH)
    {
      digitalWrite(outputs[i], HIGH);
    }
  }
}
7

Obviously, this line

const byte outputs = {10, 11, 12, 13};

should be

const byte outputs[] = {10, 11, 12, 13};

There is also a hidden assumption that all of the arrays are of the same size, and that size is 4 elements. It works for now but will become a maintenance issue if one or more switches are added or removed.

I think that perhaps an else is needed to set outputs LOW when they are not set HIGH.

8

I tried that code, the compiler gave me the error for the missing [] which i corrected right away but the code simply doesn't do anything.

I was wondering, isn't there a better way to do it? like when on audio projects there is the tone.h page attached?
Where i specify all the combinations in a simpler way?

Besides that i still don't understand why my original code doesn't work the way it was intended.

9

vaj4088:
Obviously, this line

const byte outputs = {10, 11, 12, 13};

should be

const byte outputs[] = {10, 11, 12, 13};

There is also a hidden assumption that all of the arrays are of the same size, and that size is 4 elements. It works for now but will become a maintenance issue if one or more switches are added or removed.

I think that perhaps an else is needed to set outputs LOW when they are not set HIGH.

You are correct. I rushed that code out before I had to go to a class and also someone came and talked to me while I was writing it haha.

Like I said, I made an assumption of his logic. This is just to demonstrate the use of arrays and a possible use of logic in this situation. It's not necessarily to be used as an answer, more of an example.

Here it is with those points corrected.

const byte pushButtons[] = {2, 3, 4, 5};
const byte switches[] = {6, 7, 8, 9};
const byte outputs[] = {10, 11, 12, 13};

void setup() {
  for(byte i = 0; i < 4; i++)
  {
    pinMode(pushButtons[i], INPUT);
    pinMode(switches[i], INPUT);
    pinMode(outputs[i], OUTPUT);
  }
}

void loop() {
  for(byte i = 0; i < 4; i++)
  {
    if(digitalRead(pushButtons[i]) == HIGH || digitalRead(switches[i]) == HIGH)
    {
      digitalWrite(outputs[i], HIGH);
    }
    else
    {
      digitalWrite(outputs[i], LOW);
    }
  }
}
10

threepwood85:
I tried that code, the compiler gave me the error for the missing [] which i corrected right away but the code simply doesn't do anything.

I was wondering, isn't there a better way to do it? like when on audio projects there is the tone.h page attached?
Where i specify all the combinations in a simpler way?

Besides that i still don't understand why my original code doesn't work the way it was intended.

I assumed which pins the inputs and outputs were connected to, so if you didn't modify those to match your wiring setup of course it isn't going to work.

I didn't bother looking at your original code because it is very long and your questions was about the buttons, switches and relays.

11

I think your original problem is caused by setting the outputs based on the on/off switches, then immediately afterwards setting them based on the push-button switches. This repeats each time through the loop, so some of the outputs are constantly alternating between high and low states.

If your intent is to set the outputs based on the on/off switches, then have the push-button temporarily override those outputs while the switch is pressed, then it would probably work better to use the state of the on/off switches to set variables, then alter those variables based on the push-button switches, and finally at the end set the outputs based on the variables.

12

Metallor:
You are correct. I rushed that code out before I had to go to a class and also someone came and talked to me while I was writing it haha.

Like I said, I made an assumption of his logic. This is just to demonstrate the use of arrays and a possible use of logic in this situation. It's not necessarily to be used as an answer, more of an example.

Here it is with those points corrected.

const byte pushButtons[] = {2, 3, 4, 5};

const byte switches[] = {6, 7, 8, 9};
const byte outputs[] = {10, 11, 12, 13};

void setup() {
  for(byte i = 0; i < 4; i++)
  {
    pinMode(pushButtons[i], INPUT);
    pinMode(switches[i], INPUT);
    pinMode(outputs[i], OUTPUT);
  }
}

void loop() {
  for(byte i = 0; i < 4; i++)
  {
    if(digitalRead(pushButtons[i]) == HIGH || digitalRead(switches[i]) == HIGH)
    {
      digitalWrite(outputs[i], HIGH);
    }
    else
    {
      digitalWrite(outputs[i], LOW);
    }
  }
}

I gave you too much trust LOL.

I totally missed that.
I corrected that and it works, but only one switch per output, and it's just momentary :(.
Although i am learning something new so i'm happy about that. I appreciate you explaining it to me.

13

david_2018:
I think your original problem is caused by setting the outputs based on the on/off switches, then immediately afterwards setting them based on the push-button switches. This repeats each time through the loop, so some of the outputs are constantly alternating between high and low states.

If your intent is to set the outputs based on the on/off switches, then have the push-button temporarily override those outputs while the switch is pressed, then it would probably work better to use the state of the on/off switches to set variables, then alter those variables based on the push-button switches, and finally at the end set the outputs based on the variables.

Sorry to ask you HOW TO but how do i do that? How can i tell to some inputs to take over some other inputs?

14

threepwood85:
I gave you too much trust LOL.

I totally missed that.
I corrected that and it works, but only one switch per output, and it's just momentary :(.
Although i am learning something new so i'm happy about that. I appreciate you explaining it to me.

You mentioned in your original post that you had push on/push off buttons. Are these like physical switches that keep the contact closed when pressed? Or are these just simple push buttons that you want to act as on/off buttons? Can you please take a photo of your setup?

15

Metallor:
You mentioned in your original post that you had push on/push off buttons. Are these like physical switches that keep the contact closed when pressed? Or are these just simple push buttons that you want to act as on/off buttons? Can you please take a photo of your setup?

at the moment i have all push buttons, i don't think it makes any difference what switch i put on the circuit, it still doesn't act right.

I understand the code is long so i shortened it to narrow down the problem:

int inPin = A5; // switch A push
int in2Pin = A4;  // switch B push
int in1bisPin = A1; // // switch A trigger (rocker)
int in2bisPin = A0; // switch B trigger (rocker)
int outPin = 7;  // A output
int out2Pin = 6;  // B output
int outstate1 = LOW;
int outstate2 = LOW;
int in1State;
int in2State;
int in1bisState;
int in2bisState;
int previous1 = 0;
int previous2 = 0;
long time = 1;         // the last time the output pin was toggled
long debounce = 300;   // the debounce time, increase if the output flickers
long pause = 1500;



void setup()
{
  pinMode(inPin, INPUT);
  pinMode(in2Pin, INPUT);
  pinMode(in1bisPin, INPUT);
  pinMode(in2bisPin, INPUT);
  pinMode(outPin, OUTPUT);
  pinMode(out2Pin, OUTPUT);

}

void loop()
{
  in1State = digitalRead(inPin);
  in2State = digitalRead(in2Pin);
  in1bisState = digitalRead(in1bisPin);
  in2bisState = digitalRead(in2bisPin);




  ///////////////////////////////////////////////////////////////////////////////////
  //////////////////////////////////PUSH ACTIVITY////////////////////////////////////
  ///////////////////////////////////////////////////////////////////////////////////



  if (in1State == HIGH && outstate2 == LOW && previous1 == LOW && millis() - time > debounce) {
    if (outstate1 == HIGH)
      outstate1 = LOW;
    else
      outstate1 = HIGH;
    time = millis();
  }

  if (in1State == HIGH && outstate2 == HIGH && previous1 == HIGH && millis() - time > debounce) {
    if (outstate1 == HIGH)
      outstate1 = HIGH;
    time = millis();
  }





  ///////////////////////////////////////////////////////////////////////////////////
  ////////////////////////////////ROCKER ACTIVITY////////////////////////////////////
  ///////////////////////////////////////////////////////////////////////////////////



  if ((in1bisState == HIGH) && (in2bisState == HIGH)) {


    digitalWrite(outPin, HIGH);
    digitalWrite(out2Pin, HIGH);

  }



  if ((in1bisState == HIGH) && (in2bisState == LOW)) {

    digitalWrite(outPin, HIGH);
    digitalWrite(out2Pin, LOW);
  }


  if ((in1bisState == LOW) && (in2bisState == HIGH)) {

    digitalWrite(outPin, HIGH);
    digitalWrite(out2Pin, HIGH);
  }

  digitalWrite(outPin, outstate1);
  digitalWrite(out2Pin, outstate2);

  previous1 = in1State;
  previous2 = in2State;
}

Now inPin (push on, push off) works as intended making the relay click, while in1bisPin lights up the led but no click on the relay.

16

Haven't tested this, and getting late here, but try this and see how it works:

int inPin = A5; // switch A push
int in2Pin = A4;  // switch B push
int in1bisPin = A1; // // switch A trigger (rocker)
int in2bisPin = A0; // switch B trigger (rocker)
int outPin = 7;  // A output
int out2Pin = 6;  // B output
int outstate1 = LOW;
int outstate2 = LOW;
int in1State;
int in2State;
int in1bisState;
int in2bisState;
int previous1 = 0;
int previous2 = 0;
unsigned long time = 1;         // the last time the output pin was toggled
unsigned long debounce = 300;   // the debounce time, increase if the output flickers
long pause = 1500;



void setup()
{
  pinMode(inPin, INPUT);
  pinMode(in2Pin, INPUT);
  pinMode(in1bisPin, INPUT);
  pinMode(in2bisPin, INPUT);
  pinMode(outPin, OUTPUT);
  pinMode(out2Pin, OUTPUT);

}

void loop()
{
  in1State = digitalRead(inPin);
  in2State = digitalRead(in2Pin);
  in1bisState = digitalRead(in1bisPin);
  in2bisState = digitalRead(in2bisPin);


  ///////////////////////////////////////////////////////////////////////////////////
  ////////////////////////////////ROCKER ACTIVITY////////////////////////////////////
  ///////////////////////////////////////////////////////////////////////////////////



  if ((in1bisState == HIGH) && (in2bisState == HIGH)) {
    outstate1 = HIGH;
    outstate2 = HIGH;
    //  digitalWrite(outPin, HIGH);
    //  digitalWrite(out2Pin, HIGH);

  }



  if ((in1bisState == HIGH) && (in2bisState == LOW)) {
    outstate1 = HIGH;
    outstate2 = LOW;
    //  digitalWrite(outPin, HIGH);
    //  digitalWrite(out2Pin, LOW);
  }


  if ((in1bisState == LOW) && (in2bisState == HIGH)) {
    outstate1 = HIGH;
    outstate2 = HIGH;
    //  digitalWrite(outPin, HIGH);
    //  digitalWrite(out2Pin, HIGH);
  }

  ///////////////////////////////////////////////////////////////////////////////////
  //////////////////////////////////PUSH ACTIVITY////////////////////////////////////
  ///////////////////////////////////////////////////////////////////////////////////



  if (in1State == HIGH && outstate2 == LOW && previous1 == LOW && millis() - time > debounce) {
    if (outstate1 == HIGH)
      outstate1 = LOW;
    else
      outstate1 = HIGH;
    time = millis();
  }

  if (in1State == HIGH && outstate2 == HIGH && previous1 == HIGH && millis() - time > debounce) {
    if (outstate1 == HIGH)
      outstate1 = HIGH;
    time = millis();
  }

  digitalWrite(outPin, outstate1);
  digitalWrite(out2Pin, outstate2);

  previous1 = in1State;
  previous2 = in2State;
}
17

You need a time variable for each button/switch you are debouncing. (Edit: This is not actually true) Also variables related to timing should be UNSIGNED long.

What is your logic? Do you want the relay to come on when either the momentary button OR the switch is HIGH? Do you want pushing the button to change the output to the OPPOSITE of the switch state? I don't understand the need for both a switch and a push button quite yet.

18

Looked at your code a bit more, and noticed a couple of things:

There doesn't appear to be any code for when all four switches are LOW, what would the outputs be in that case?

The logic for the momentary buttons is quite confusing, and a few of your IF statements will never execute (in one case, if a button is pressed, you set the outputs all low, then test in the next IF statement if the same button is pressed AND an output is high). It appears you want to change the output based on the current state of a push button, plus the existing state of one or more outputs. That is going to get very confusing if multiple buttons are pressed at the same time.

19

I'll try the given code asap.

But i would like to add more details about my project.

The relays are going to power 4 different lights.

Each input (or we can say pair of inputs) are directly connected to one specific relay, however there are some rules.

In normal stage (no inputs, startup) all the outputs are LOW
Switch A push turns on/off Relay A
Switch B push turns on/off Relays A and B
Switch C push turns on/off Relay C ONLY if relay B is already ON
Switch D push turns on/off Relays A and D

Switch A trigger turns on Relay A
Switch B trigger turns on Relays A and B
Switch C trigger turns on Relay C ONLY if relay B is already ON
Switch D trigger turns on Relays A and D

In addition to these rules if the push buttons (on/off) are activated then a buzzer goes off to indicate that the module received the command.
If the other regular switches (at the moment i'm using momentary button but will use toggle switches once the project is complete) are used no buzzer is required.

Now to answer to the comment about the absence of a command of all outputs LOW when no inputs... i tried that in the several attempts when i built that code and came to the conclusion that if i used that command i couldn't use the on/off method because it would turn off any relays after the 1500ms buzzer delay.

I know the code is confusing, i'm a newbie here. Please understand. :smiley:

20

david_2018:
Haven't tested this, and getting late here, but try this and see how it works:

int inPin = A5; // switch A push

int in2Pin = A4;  // switch B push
int in1bisPin = A1; // // switch A trigger (rocker)
int in2bisPin = A0; // switch B trigger (rocker)
int outPin = 7;  // A output
int out2Pin = 6;  // B output
int outstate1 = LOW;
int outstate2 = LOW;
int in1State;
int in2State;
int in1bisState;
int in2bisState;
int previous1 = 0;
int previous2 = 0;
unsigned long time = 1;        // the last time the output pin was toggled
unsigned long debounce = 300;  // the debounce time, increase if the output flickers
long pause = 1500;

void setup()
{
  pinMode(inPin, INPUT);
  pinMode(in2Pin, INPUT);
  pinMode(in1bisPin, INPUT);
  pinMode(in2bisPin, INPUT);
  pinMode(outPin, OUTPUT);
  pinMode(out2Pin, OUTPUT);

}

void loop()
{
  in1State = digitalRead(inPin);
  in2State = digitalRead(in2Pin);
  in1bisState = digitalRead(in1bisPin);
  in2bisState = digitalRead(in2bisPin);

///////////////////////////////////////////////////////////////////////////////////
  ////////////////////////////////ROCKER ACTIVITY////////////////////////////////////
  ///////////////////////////////////////////////////////////////////////////////////

if ((in1bisState == HIGH) && (in2bisState == HIGH)) {
    outstate1 = HIGH;
    outstate2 = HIGH;
    //  digitalWrite(outPin, HIGH);
    //  digitalWrite(out2Pin, HIGH);

}

if ((in1bisState == HIGH) && (in2bisState == LOW)) {
    outstate1 = HIGH;
    outstate2 = LOW;
    //  digitalWrite(outPin, HIGH);
    //  digitalWrite(out2Pin, LOW);
  }

if ((in1bisState == LOW) && (in2bisState == HIGH)) {
    outstate1 = HIGH;
    outstate2 = HIGH;
    //  digitalWrite(outPin, HIGH);
    //  digitalWrite(out2Pin, HIGH);
  }

///////////////////////////////////////////////////////////////////////////////////
  //////////////////////////////////PUSH ACTIVITY////////////////////////////////////
  ///////////////////////////////////////////////////////////////////////////////////

if (in1State == HIGH && outstate2 == LOW && previous1 == LOW && millis() - time > debounce) {
    if (outstate1 == HIGH)
      outstate1 = LOW;
    else
      outstate1 = HIGH;
    time = millis();
  }

if (in1State == HIGH && outstate2 == HIGH && previous1 == HIGH && millis() - time > debounce) {
    if (outstate1 == HIGH)
      outstate1 = HIGH;
    time = millis();
  }

digitalWrite(outPin, outstate1);
  digitalWrite(out2Pin, outstate2);

previous1 = in1State;
  previous2 = in2State;
}

in1bis sets output1 HIGH but it stays HIGH even if in1bis gets LOW.
And that's not what i want.

The way this project is intended is that you use either one of the switches. You're not intended to use both the kinds. HOWEVER i want it to be ready to be used like that. Like let's suppose i push the Switch B push (relays A and B turn on) but then i flip the SWITCH C trigger ON i want the relay C to turn ON.

I'm aware of the fact that it seems easy to be because it's in my mind, i'm trying my best to put it into words.