may someone help me verify this code?

can someone please help me? im making something thats basicly a useless box but with more variety for a class and i need someone to look at this code

int on = 1; //detects when machine is on.
int led = 2; //led that turns on when machine is on.
int off = 3; //turns off machine
int up = 4; //open trapdoor that allows off pin to turn off machine.
long randomTrap;
long randomArm;
void setup() {

pinMode(1, INPUT);
pinMode(2, OUTPUT);
pinMode(3, OUTPUT);
pinMode(4, OUTPUT);
Serial.begin(9600);
Serial.println("begin");
}

void loop() {
digitalWrite(0, HIGH);
randomTrap = random(5000);
if (on == HIGH) { //insert code to open trapdoor.
digitalWrite(led, HIGH); //led turns on when machine is on.
delay(randomTrap);
digitalWrite(up, HIGH);
Serial.println("trapdoor is open");
if (up == HIGH){
randomArm = random(5000);
delay(randomArm);
digitalWrite(off, HIGH);
}

}else {
digitalWrite(led, LOW); //led turns off when machine is off.
digitalWrite(up, LOW); //trapdoor closes.
digitalWrite(off, LOW); //turns off off.
}
}

if theres anything wrong, please tell me. ill appreciate all the help you can give.

Does it compile?
Does it work?

Pete

Hey, welcome! Post your code in code </> tags:

so your code looks like this!

For one thing, in void setup() you can use the names instead of numbers in the pinMode() functions. For example, like this:

pinMode(on, INPUT);

Also, it may not be an issue but you might want to connect the ‘on’ pin to something else such as pin 5, because pin 1 is a TX pin. It may not be an issue though.

Have you tested this out yourself?

marco

Only you can tell if things aren't working the way you want.

Some good habits:
Don't use delay()
Keep { and } on a line all by themselves.
Use the smallest 'type' possible. byte led = 2; instead of int led = 2;
If a variable does not need to be modified, use const. ex: const byte led = 2;
Remember D0 and D1 are used for serial, don't use them unless you have to.
digitalWrite(0, HIGH); this writes to D0 which is RXD, since it is an input at power up you are turning on its PULLUP.

.

Hi,
Welcome to the forum.

Please read the first post in any forum entitled how to use this forum.
http://forum.arduino.cc/index.php/topic,148850.0.html then look down to item #7 about how to post your code.
It will be formatted in a scrolling window that makes it easier to read.

Thanks.... Tom.. :slight_smile:

int on = 1; //detects when machine is on.
int led = 2; //led that turns on when machine is on.
int off = 3; //turns off machine
int up = 4; //open trapdoor that allows off pin to turn off machine.

1: For readability it is good to differentiate global variables by capitalizing the first letter
2: For constant values it is good to declare them 'const' so the compiler can protect you better.
3: As mentioned before: DON'T use Pin 1 and Serial in the same sketch.
4: It makes your pin number constants clearer if you put Pin at the end of the name

const int OnPin = 2; //detects when machine is on.
const int LedPin = 3; //led that turns on when machine is on.
const int OffPin = 4; //turns off machine
const int UpPin = 5; //open trapdoor that allows off pin to turn off machine.

The more meaningful the variable names the easier it will be to read the code

long TrapDelay;
long ArmDelay;
void setup() {
  pinMode(OnPin, INPUT);
  pinMode(LedPin, OUTPUT);
  pinMode(OffPin, OUTPUT);
  pinMode(UpPin, OUTPUT);
  Serial.begin(9600);
  Serial.println("begin");
}

Don't define names for pins and then use the number instead!

void loop() {
  digitalWrite(0, HIGH);
  TrapDelay = random(5000);
  if (on == HIGH) { //insert code to open trapdoor.
    digitalWrite(led, HIGH); //led turns on when machine is on.
    delay(randomTrap);
    digitalWrite(up, HIGH);
    Serial.println("trapdoor is open");
    if (up == HIGH) {
      randomArm = random(5000);
      delay(randomArm);
      digitalWrite(off, HIGH);
    }

  } else {
    digitalWrite(led, LOW); //led turns off when machine is off.
    digitalWrite(up, LOW); //trapdoor closes.
    digitalWrite(off, LOW); //turns off off.
  }
}

1: You are using digitalWrite() on Pin 0 without declaring a name, using pinMode() to set it as an output
2: DON'T use Pin 0 and Serial in the same sketch.
3: You can't read an input pin by using its pin number! Use digitalRead(pin).
4: Since HIGH==1==true and LOW==0==false then digitalRead(pin) and (digitalRead(pin) == HIGH) are equivalent.

void loop() {
  // digitalWrite(0, HIGH);  // Don't write to Pin 0 when using Serial
  if (digitalRead(OnPin)) { //insert code to open trapdoor.
    digitalWrite(LedPin, HIGH); //led turns on when machine is on.
    delay(random(5000));
    digitalWrite(UpPin, HIGH);
    Serial.println("trapdoor is open");
    if (digitalRead(UpPin)) {  // Yes, you can read an OUTPUT pin but since you JUST set it HIGH this is always true
      delay(random(5000));
      digitalWrite(OffPin, HIGH);
    }

  } else {
    digitalWrite(LedPin, LOW); //led turns off when machine is off.
    digitalWrite(UpPin, LOW); //trapdoor closes.
    digitalWrite(OffPin, LOW); //turns off off.
  }
}

1: You don't need to store function results in a variable (ArmDelay, TrapDelay) if you use them only once and immediately
2: Even if you are using the results more than once inside a single function you can make your variable local instead of global.

id like to thank you guys for the information, especially JohnWasser for taking the time to write down everything i needed to know. thanks everyone!