Way to simplify code/arrange it so it looks nice? Need help.

Hi guys, I’ve been working on some auto gate code for a while, and it looks like this.

#include <elapsedMillis.h>
#include "Wire.h" // for I2C bus
#define I2C_ADDR 0x20
int button2 = 5; // Button 2 on controller
int button = 2; // Button 1 on controller
int gc = 3; // Gate closed limit switch
int go = 4; // Gate open limit switch
static int autoclose; // Triggers the auto closing sequence - stopping
static int fault; // Counts number of potential faults
static int faultdet; // Fault detected
static int milicode; // Triggers the auto closing sequence - shut gate
elapsedMillis timer0; // Fault timer - Clear by resetting unit
elapsedMillis timer1; // Gate open/closing timer
#define interval1 300000 // Gate open/closing timer value
#define interval 1800000 // Fault reset timer value

void setup()
{
  Serial.begin (9600); // Set startup values and begin services
 autoclose = LOW;
 fault = 0;
 faultdet = LOW;
 timer1 = 0;
 timer0 = 0;
 milicode = LOW;
 Wire.begin(); // Wake up I2C bus
  // Set I/O bank A to outputs
 Wire.beginTransmission(I2C_ADDR);
 Wire.write(0x00);
 Wire.write(0x00);
 Wire.endTransmission();
}
void loop() {
  if (digitalRead(button2) == HIGH && digitalRead(gc) == HIGH && digitalRead(go) == LOW) // Button 2 has auto close, sets autoclose HIGH to start delayed closing timer
 {
 autoclose = HIGH;
 Wire.beginTransmission(I2C_ADDR);
 Wire.write(0x12);
 Wire.write(1);
 Wire.endTransmission();
 delay(2000); // Gate needs to be off any limit switches before we read any more data.
 }
if (digitalRead(button) == HIGH && digitalRead(gc) == HIGH && digitalRead(go) == LOW) // Button 1 keeps gate open
 { // OPEN CODE
 Wire.beginTransmission(I2C_ADDR);
 Wire.write(0x12);
 Wire.write(1);
 Wire.endTransmission();
 delay(2000); // Gate needs to be off any limit switches before we read any more data.
 }
   if (digitalRead(button) == HIGH && digitalRead(gc) == LOW && digitalRead(go) == HIGH) // Manually shut gate on button 1
 { // CLOSE CODE
 Wire.beginTransmission(I2C_ADDR);
 Wire.write(0x12);
 Wire.write(2);
 Wire.endTransmission();
 delay(2000); // Gate needs to be off any limit switches before we read any more data.
 }
   if (digitalRead(gc) == HIGH && autoclose == LOW) // Stopping when limit is hit. No delays as this code will run constantly run when gate is closed
 { Wire.beginTransmission(I2C_ADDR);
 Wire.write(0x12);
 Wire.write(0);
 Wire.endTransmission();
 }
  if (digitalRead(go) == HIGH && autoclose == LOW) // Stopping when limit is hit. No delays as this code will run constantly run when gate is open
 { Wire.beginTransmission(I2C_ADDR);
 Wire.write(0x12);
 Wire.write(0);
 Wire.endTransmission();
 }
   if (digitalRead(go) == HIGH && autoclose == HIGH) // Part of the timed gate closing, stops the gate before setting milicode HIGH to start the timer
 {
  autoclose = LOW;
  Wire.beginTransmission(I2C_ADDR);
 Wire.write(0x12);
 Wire.write(0);
 Wire.endTransmission();
 timer1 = 0; // Reset timer to make sure it stays open for at least interval1
milicode = HIGH;
 }
 // mag sensor trigger code below
  
   if (digitalRead(6) == HIGH && digitalRead(gc) == HIGH && digitalRead(go) == LOW && faultdet == LOW) // Reads digitalPin 6, connected to in ground compass,
   //                                                                                                     also makes sure faultdet is low (faultdet will be set high
   //                                                                                                     if gate triggers from compass more then 5 times in 30mins.
 { 
 autoclose = HIGH;
 fault = fault + 1; // Counting number of times opened in 30mins, timer0 resets fault every 30mins to 0
 Wire.beginTransmission(I2C_ADDR);
 Wire.write(0x12);
 Wire.write(1);
 Wire.endTransmission();
 delay(2000); // Gate needs to be off any limit switches before we read any more data.
 }
 if (fault == 5)// Detection if gate opens/closes 5 times in 30mins, shuts off mag by setting faultdet HIGH
 {faultdet = HIGH;
 }
  if (timer0 > interval) { // Resets fault to 0 every interval
    timer0 -= interval; //reset the timer
    fault = 0;
  }
  if (timer1 > interval1 && milicode == HIGH && digitalRead(go) == HIGH) { // Timer to shut gate
     Wire.beginTransmission(I2C_ADDR);
    Wire.write(0x12);
     Wire.write(2);
      Wire.endTransmission();
       delay(2000);
    milicode = LOW;
    
    
    
  }
 if (digitalRead(6)==HIGH) // Resets the shut gate timer until the car has passed and sensor reads LOW.
 {timer1=0;
 }
  Serial.println(timer1);
}

Everything’s getting a little more complex as of late, and I have more magnetic sensors and long range RFID on the cards.

Am wondering if multiple loops is possible? Just have gate open, and gate closed statements in seperate loops, then just use the main loop to goto those loops when the if statements are met?

Can you “goto” code? And how do you go about setting up multiple loops? Or should I be looking at another way?

Ive heard what I’m doing is wrong (rewriting the code every time a condition is met).

Just trying to learn some commands and practices to clean things up before they get too complicated :slight_smile:

Many thanks, Jake.

Hi,

You need to tidy up the spacings. Goto TOOLS and select Auto Format. That will make the indents even and relevant to your { } blocks.

Tom... :)

Use functions. GOTO is not used in c or c++. GOTO reminds me of my TI-82 programmable calculator...

here is an example:

void myfunction(char a){
    Serial.println(a);
}

This function will print a. you can call it by simply adding it to your loop code like this:

void loop(){
    myfunction('a');
    delay(2000);
}

you can also return a value with your function:

int square(int b){
  int result = b*b;  
  return result;
}

void loop(
   Serial.println(square(5));
   delay(2000);
}

So whenever your code repeats, you can use a function instead.

I also recommend that you read up about classes and learn to use c++ in an object oriented way. It's very useful and I find that programming arduino with it is simply amazing. Google it, you'll see. It's not all that complicated.

int button2 = 5; // Button 2 on controller
int button = 2; // Button 1 on controller

elapsedMillis timer0; // Fault timer - Clear by resetting unit
elapsedMillis timer1; // Gate open/closing timer

#define interval1 300000 // Gate open/closing timer value
#define interval 1800000 // Fault reset timer value

Consistent numbering schemes are better than random stuff.

Wire.beginTransmission(I2C_ADDR);
 Wire.write(0x12);
 Wire.write(1);
 Wire.endTransmission();
 delay(2000); // Gate needs to be off any limit switches before we read any more data.

This should be in a function, called here. With a decent name, we would not have to guess what this does.

Wire.beginTransmission(I2C_ADDR);
 Wire.write(0x12);
 Wire.write(1);
 Wire.endTransmission();
 delay(2000); // Gate needs to be off any limit switches before we read any more data.

Haven't we seen this code before? Haven't we seen this code before? Haven't we seen this code before?

I hate seeing duplicate code.

// CLOSE CODE
 Wire.beginTransmission(I2C_ADDR);
 Wire.write(0x12);
 Wire.write(2);
 Wire.endTransmission();
 delay(2000); // Gate needs to be off any limit switches before we read any more data.

At least here there is a comment... The only difference between this code and the other code is the value in the 2nd call to write().

A function that takes an argument (open, close, or stop) would be far better than duplicating nearly identical code.

Wire.beginTransmission(I2C_ADDR);
 Wire.write(0x12);
 Wire.write(0);
 Wire.endTransmission();

More nearly identical code.

I HATE this commenting and layout style.

  if (digitalRead(6) == HIGH && digitalRead(gc) == HIGH && digitalRead(go) == LOW && faultdet == LOW) // Reads digitalPin 6, connected to in ground compass,
   //                                                                                                     also makes sure faultdet is low (faultdet will be set high
   //                                                                                                     if gate triggers from compass more then 5 times in 30mins.

This does the samething, but looks much better, to me:

// Read digitalPin 6, connected to in ground compass,
// make sure faultdet is low (faultdet will be set high
// if gate triggers from compass more then 5 times in 30mins.
   if (digitalRead(6) == HIGH && 
       digitalRead(gc) == HIGH &&
       digitalRead(go) == LOW &&
       faultdet == LOW)
   {
 if (timer1 > interval1 && milicode == HIGH && digitalRead(go) == HIGH) { // Timer to shut gate
     Wire.beginTransmission(I2C_ADDR);
    Wire.write(0x12);
     Wire.write(2);
      Wire.endTransmission();
       delay(2000);
    milicode = LOW;
    
    
    
  }

This just about made

me barf.

if (digitalRead(6)==HIGH) // Resets the shut gate timer until the car has passed and sensor reads LOW.
 {timer1=0;
 }

If you worked for me, and EVER put code on the same line as the {, you'd be doing documentation for a month instead of coding.

Haha, thanks Paul. I didnt realise you could put your if statement on multiple lines. I'll sit down and try and sort it out, it does look much better the way you've shown.

As for putting my code into a fuction with an arguement, how would I go about that?

Wire.beginTransmission(I2C_ADDR);
 Wire.write(0x12);
 Wire.write(0);
 Wire.endTransmission();

This code is always the same, except write() will either be 0, 1 or 2.

So could I write these in three seperate functions, and then just call open, close and stop?

How do you call a function? I will look into this but am just wondering.

I'm definitly not a programmer, I'm an electrician, as I'm sure we've all figured out. But as with everything, I am always trying to incorporate good practices :)

void fire()
{
    Wire.beginTransmission(I2C_ADDR);
    Wire.write(0x12);
    Wire.write(travel);
    Wire.endTransmission();
    delay(2000);
  
}

If I made this a function, and used a static int called travel, and then in every if statement set travel to 0, 1 or 2, then fire the relay, would that be a good way to do it?

Or would three seperate functions, each labeled open, close and stop be a better way?

How do you call a function?

The same way you call all the other functions you're calling.

Why not simply provide "travel" as a parameter?

Because I've never used or even understand parameters and how to apply them. How would I create a parameter for an arguement? :/

Because I've never used or even understand parameters and how to apply them.

Wire.beginTransmission(I2C_ADDR);

Well, you certainly know how to apply them.

How to copy and paste them.

Dont worry, I'll go figure it out.

void fire(byte mode)
{
    Wire.beginTransmission(I2C_ADDR);
    Wire.write(0x12);
    Wire.write(mode);
    Wire.endTransmission();
    delay(2000);
}
   fire(2);  // stop
   fire(1);  // open
   fire(0);  // close

The values for open, close, and stop may not be right, but they will be when you call the functio.

Thanks Paul, +Karma.

I’ve entered this into my code and I get “fire was not declared in this scope”?

#include <elapsedMillis.h>
#include "Wire.h" // for I2C bus

int button2 = 5; // Button 2 on controller
int button = 2; // Button 1 on controller
int gc = 3; // Gate closed limit switch
int go = 4; // Gate open limit switch

static int autoclose; // Triggers the auto closing sequence - stopping
static int fault; // Counts number of potential faults
static int faultdet; // Fault detected
static int milicode; // Triggers the auto closing sequence - shut gate

elapsedMillis timer0; // Fault timer - Clear by resetting unit
elapsedMillis timer1; // Gate open/closing timer

#define interval1 300000 // Gate open/closing timer value
#define interval 1800000 // Fault reset timer value
#define I2C_ADDR 0x20

void setup()
{
  Serial.begin (9600); // Set startup values and begin services

  autoclose = LOW;
  faultdet = LOW;
  milicode = LOW;

  fault = 0;
  timer1 = 0;
  timer0 = 0;

  Wire.begin(); // Wake up I2C bus
  // Set I/O bank A to outputs
  Wire.beginTransmission(I2C_ADDR);
  Wire.write(0x00);
  Wire.write(0x00);
  Wire.endTransmission();
}
void loop() {
  if (digitalRead(button2) == HIGH &&
      digitalRead(gc) == HIGH &&
      digitalRead(go) == LOW) // Button 2 has auto close, sets autoclose HIGH to start delayed closing timer
  {
    autoclose = HIGH;
    fire(1);
  }


  if (digitalRead(button) == HIGH &&
      digitalRead(gc) == HIGH &&
      digitalRead(go) == LOW) // Button 1 keeps gate open
  { // OPEN CODE
    fire(1);
  }


  if (digitalRead(button) == HIGH &&
      digitalRead(gc) == LOW &&
      digitalRead(go) == HIGH) // Manually shut gate on button 1
  { // CLOSE CODE
    fire(2);
  }


  if (digitalRead(gc) == HIGH &&
      autoclose == LOW) // Stopping when limit is hit. No delays as this code will run constantly run when gate is closed
  {
    firestop;
  }


  if (digitalRead(go) == HIGH &&
      autoclose == LOW) // Stopping when limit is hit. No delays as this code will run constantly run when gate is open
  {
    firestop;
  }


  if (digitalRead(go) == HIGH &&
      autoclose == HIGH) // Part of the timed gate closing, stops the gate before setting milicode HIGH to start the timer
  {
    autoclose = LOW;
    firestop;
    timer1 = 0; // Reset timer to make sure it stays open for at least interval1
    milicode = HIGH;
  }

  // mag sensor trigger code below
  // Reads digitalPin 6, connected to in ground compass,
  // also makes sure faultdet is low (faultdet will be set high
  // if gate triggers from compass more then 5 times in 30mins.
  if (digitalRead(6) == HIGH &&
      digitalRead(gc) == HIGH &&
      digitalRead(go) == LOW &&
      faultdet == LOW)
  {
    autoclose = HIGH;
    fault = fault + 1; // Counting number of times opened in 30mins, timer0 resets fault every 30mins to 0
    fire(1);
  }


  if (fault == 5)// Detection if gate opens/closes 5 times in 30mins, shuts off mag by setting faultdet HIGH
  {
    faultdet = HIGH;
  }


  if (timer0 > interval) { // Resets fault to 0 every interval
    timer0 -= interval; //reset the timer
    fault = 0;
  }


  if (timer1 > interval1 && milicode == HIGH && digitalRead(go) == HIGH) { // Timer to shut gate
    fire(2);
    milicode = LOW;
  }


  if (digitalRead(6) == HIGH) // Resets the shut gate timer until the car has passed and sensor reads LOW.
  {
    timer1 = 0;
  }
  void fire(byte mode)
  {
    Wire.beginTransmission(I2C_ADDR);
    Wire.write(0x12);
    Wire.write(mode);
    Wire.endTransmission();
    delay(2000);
  }

  void firestop()
  {
    Wire.beginTransmission(I2C_ADDR);
    Wire.write(0x12);
    Wire.write(0);
    Wire.endTransmission();
  }

  Serial.println(timer1);
}

Is what it looks like at the moment, a lot neater! Thank you

I got it, I put my fire code in the loop! All fixed. Thanks a lot guys! :)

I put my fire code in the loop!

That doesn't make sense, You can't put a function in a function.

    firestop;

What do you think this is doing?

By the way, fire() is a lousy name for a function that moves a gate.

jakeoo: Because I've never used or even understand parameters and how to apply them. How would I create a parameter for an arguement? :/

If I may suggest without giving offence? You will proceed a lot faster if you make your way through a beginner's C/C++ tutorial.

if you read my post you'd know how to call a function and even how to pass arguments...