Sketch Problems

[ccphoto]

Hi I am new to Arduino and this is my first attempt at producing my own sketch so please be kind.
I am using a windscreen wiper motor 12v to power open a roof of my observatory.
It originally started off being manually controlled with a forward and reverse switch to send the roof back and forward. The switch operated some 12volt relays to control the motor direction with limit switches to signal and stop the motor when at the end of it open or close position.
I have now adapted it to be operated as well by an arduino une with a 4 way relay shield. I have it working initiating it with software push buttons to start and stop the motor. I have a 2nd set of limits to detect fully open or fully closed position of roof (had to do this as uno operates at 5v)
The problem I have is that whilst the sketch I have written works shutting the motor off in the fully open position. The fully closed position does not. It tries to close the relay but only momentarily then starts up again. cycling between stop start. Yet if I use the software button to close the relay it works.
Attached below is the sketch
Any comments on what I have produced would be gratefully received

/*
   Created by ArduinoGetStarted.com

   This example code is in the public domain

   Tutorial page: https://arduinogetstarted.com/tutorials/arduino-button-relay
*/
String data;
char d1;
int CLOSED = 12; //Closed Limit connected to  pin 12
int OPENED = 11; // Opened Limit connected to pin 11
int relay_1 = 4;
int relay_2 = 5;
int Openled = 10; //roof opening
int Closeled = 9; //roof closing
int buttonState = digitalRead(CLOSED); //read new state
int buttonState1 = digitalRead(OPENED); //read new state

void setup() {
  Serial.begin(9600);                // initialize serial
  pinMode(CLOSED, INPUT_PULLUP); // set closed pin to input pull-up mode
  pinMode(OPENED, INPUT_PULLUP); // set opened pin to input pull-up mode
  pinMode(relay_1, OUTPUT);        // set open relay to output mode
  pinMode(relay_2, OUTPUT);        // set close relay to output mode
  pinMode(Openled, OUTPUT); // set closed pin to output mode
  pinMode(Closeled, OUTPUT); // set opened pin to output mode
}

void loop() {

  if (Serial.available()) {
    data = Serial.readString();
    d1 = data.charAt(0);
    if (d1 == 'A') {
      digitalWrite(relay_1, HIGH);
      delay(100);
      digitalWrite(Openled, HIGH);
    }
  }
  else if (d1 == 'a') {
    digitalWrite(relay_1, LOW);
    delay(100);
    digitalWrite(Openled, LOW);
  }

  else if (d1 == 'C') {
    digitalWrite(relay_2, HIGH);
    delay(100);
    digitalWrite(Closeled, HIGH);
  }


  else if (d1 == 'x') {
    digitalWrite(relay_2, LOW);
    delay(100);
    digitalWrite(Closeled, LOW);
  }

  if (digitalRead(CLOSED) == LOW) {
    Serial.println("The button is being pressed");
    delay(100);
    digitalWrite(relay_2, LOW);  // turn off open relay
    delay(100);
    digitalWrite(Closeled, LOW);

  }

  if (digitalRead(OPENED) == LOW) {
    Serial.println("The button is being pressed");
    delay(100);
    digitalWrite(relay_1, LOW);  // turn off closed relay
    delay(100);
    digitalWrite(Openled, LOW);

  }
}

[Idahowalker]

@ccphoto

Yet if I use the software button to close the relay it works.

What is a software button?

pinMode(relay_2, OUTPUT);        // set close relay to output mode

Why not name the thing something that makes sense?

pinMode( closeRelay, OUTPUT);        // set close relay to output mode

How is closed detected? Is there a limit switch that 'says' click, I'm closed stop the motor now?

I take it 'C' means close?

[david_2018]

The value stored in d1 is only updated when there is Serial data available. All of the if statements checking the value of d1, except for (d1 == 'A') (which is inside the if (Serial.available()) ), are being executed every time through loop, causing the relays to respond to the last command after the limit switches have stopped the motor.

[Idahowalker]

d1 needs to be set to '' to stop the same command from repeating after the command has done its thing.

[Idahowalker]

like

@ccphoto

else if (d1 == 'a') {
    digitalWrite(relay_1, LOW);
    delay(100);
    digitalWrite(Openled, LOW);
d1='';
  }

[ccphoto]

The software button is created as a windows app which was generated using visual studio. 'A' is the open button 'C' is Closed 'a' and 'x' are stop buttons in the app. These work. The CLOSED and OPENED are limit switched which are used to drop the relays 1 and 2 on the arduino relay shield.
The OPENED limit works fine but the CLOSED limit tries to stop momentarily then the motor drives again then tries to stop then starts again.
I have to use the stop butten in the app to get the relay to drop out.
The app sends the open and close commands via the serial port.
The hope is i can control the roof opening and closin remotely remotely.
Thanks for the prompt comments

[Idahowalker]

@ccphoto

but the CLOSED limit tries to stop momentarily then the motor drives again then tries to stop then starts again.

when the door is closed does the door momentarily open then try to close again?

Did you try the suggestion form post#5?

[david_2018]

I think you just have a misplace bracket. All of the if statements that check for the various commands should only be processed when a command is received, as it is all except for 'A" are checked every time loop executes, essentially re-executing those commands constantly.

/*
   Created by ArduinoGetStarted.com

   This example code is in the public domain

   Tutorial page: https://arduinogetstarted.com/tutorials/arduino-button-relay
*/
String data;
char d1;
int CLOSED = 12; //Closed Limit connected to  pin 12
int OPENED = 11; // Opened Limit connected to pin 11
int relay_1 = 4;
int relay_2 = 5;
int Openled = 10; //roof opening
int Closeled = 9; //roof closing
int buttonState = digitalRead(CLOSED); //read new state
int buttonState1 = digitalRead(OPENED); //read new state

void setup() {
  Serial.begin(9600);                // initialize serial
  pinMode(CLOSED, INPUT_PULLUP); // set closed pin to input pull-up mode
  pinMode(OPENED, INPUT_PULLUP); // set opened pin to input pull-up mode
  pinMode(relay_1, OUTPUT);        // set open relay to output mode
  pinMode(relay_2, OUTPUT);        // set close relay to output mode
  pinMode(Openled, OUTPUT); // set closed pin to output mode
  pinMode(Closeled, OUTPUT); // set opened pin to output mode
}

void loop() {

  if (Serial.available()) {
    data = Serial.readString();
    d1 = data.charAt(0);
    if (d1 == 'A') {
      digitalWrite(relay_1, HIGH);
      delay(100);
      digitalWrite(Openled, HIGH);
      //} <<<<<<<<<<<<<<<<<<< remove this bracket
    }
    else if (d1 == 'a') {
      digitalWrite(relay_1, LOW);
      delay(100);
      digitalWrite(Openled, LOW);
    }

    else if (d1 == 'C') {
      digitalWrite(relay_2, HIGH);
      delay(100);
      digitalWrite(Closeled, HIGH);
    }


    else if (d1 == 'x') {
      digitalWrite(relay_2, LOW);
      delay(100);
      digitalWrite(Closeled, LOW);
    }
  } // <<<<<<<<<< place bracket here instead

  if (digitalRead(CLOSED) == LOW) {
    Serial.println("The button is being pressed");
    delay(100);
    digitalWrite(relay_2, LOW);  // turn off open relay
    delay(100);
    digitalWrite(Closeled, LOW);

  }

  if (digitalRead(OPENED) == LOW) {
    Serial.println("The button is being pressed");
    delay(100);
    digitalWrite(relay_1, LOW);  // turn off closed relay
    delay(100);
    digitalWrite(Openled, LOW);

  }
}

[dougp]

Don't know if this'll help but, I opened the sketch in IDE 2.0.0-beta.7 and it shows this:

Right clicking on CLOSED with the underlining squigglies and then 'Go to Definition F12' reveals 'No definition found for CLOSED'. There is a definition for OPENED.

[ccphoto]

Many thanks for all your pronpt responses but I can now report that post no8 provided the solution. It was a mis placed bracket so many thanks David_2018.
Every thing working fine now
I' ve learnt something more to my limited knowledge of Arduino programming