Assignment due tomorrow, thought it was an easy fix....

Hey guys!

I'm far from an experienced Arduino guy but sometimes it can come in handy with what I do. I got an Assignment due in like 4h and I've been trying to figure out why this is not working.
Short info about the assignment, it's a machine that got a "lift" which have a bottom switch and a top switch. When the bottom switch is switched on (momentarily), a variable says "run lift" until it hits the top. Very simple but I have no idea what I'm doing wrong.
Here is the code that I've been working with, not surprised if there is heaps of misstakes in general but hey, i just want it to run.

(The motors are connected to a transistor etc. but that should be fine)

Belt part at the end is just another part of the machine.... It needs to run for like 2sec.

Would appreciate help a lot =(

EDIT: The problem I mainly had was to change the state of liftState. It is always 1, or HIGH of some reason....

const int liftBottom = 5;
const int liftTop = 6;
const int beltSw = 7;
const int lift = 10;
const int belt = 9;

int liftState = LOW;
int val = 0;
int valTop = 0;
int valBelt = 0;
int valLift = 0;
int tempTest = 0;

void setup() {
  Serial.begin(9600);
  pinMode(liftBottom, INPUT); 
  pinMode(liftTop, INPUT); 
  pinMode(beltSw, INPUT); 
  pinMode(lift, OUTPUT);
  pinMode(belt, OUTPUT);
}

void loop() {
 tempTest = digitalRead(liftState);
  Serial.println(tempTest);
  
    val = digitalRead(liftBottom);
    valTop = digitalRead(liftTop);
////___________Check the state of the lift______
if (val == HIGH) {
//   digitalWrite(liftState, HIGH);
analogWrite(lift, 255);
   //Serial.println("ON");
 }
   else if (valTop == HIGH){
//    digitalWrite(liftState, LOW);
analogWrite(lift, 0);
    //Serial.println("OFF");
   }

   
   ///__________________________LIFT CONTROLLER
  
   valLift = digitalRead(liftState);

   if (valLift == HIGH) {
    analogWrite(lift, 255); 
   }
   else{
     analogWrite(lift, 0);
   }
   
   valBelt = digitalRead(beltSw);
   
   if (valBelt == HIGH) {
     analogWrite(belt, 255); 
   }
   else{
      analogWrite(belt, 0); 
   }

}

Your "LIFT CONTROLLER" part resets the "lift" state in it's else block, undoing what your "Check the state of the lift" section did...

You read liftState as a digital input:

tempTest = digitalRead(liftState);

but you've declared it as LOW:

int liftState = LOW;

which has a value of zero and so you are actually reading pin zero.

You have not said how the top and bottom switches are wired and you declare them as INPUT with no internal pullup.
Your test of liftBottom, for example,

if (val == HIGH) {

appears to assume that the switch is normally LOW (and therefore you have a pulldown resistor on the switch) and the switch is active when it is HIGH. If you haven't got a pulldown resistor on the switch then it will normally read as HIGH and as LOW when it is activated.
You need to figure out how you've wired up the switches and what HIGH and LOW mean with that wiring. If you do not have external pulldown resistors, you should activate the internal pullups on each pin like this:

  pinMode(liftBottom, INPUT_PULLUP);

and change your code so that it works with LOW being activated.
There are more problems and I seriously doubt that you'll have this working in, hmmm, about 3 hours?
Good luck.

Pete

First, don't do this:

const int liftBottom = 5;
const int liftTop = 6;
const int beltSw = 7;
const int lift = 10;
const int belt = 9;

Do this:

#define SW_BOTTOM 5
#define SW_TOP    6
#define SW_BELT   7
#define PIN_LEFT  10
#define PIN_BELT  9

On a microcontroller, you have very limited memory. You don't use much RAM in your sketch, but you should get in the habit of declaring constants with #defines rather than as constant integers. A #define will be substituted with the value you've defined it to before the code is compiled. This uses no RAM. A const int, on the other hand, will require 2 bytes of RAM each. That adds up.

Also, your naming convention could be better. I used "SW_" as a prefix for switch inputs, and "PIN_" as a prefix for output pins. What you use is up to you, provided it's unmistakably clear when you and others read your code. By this line:

tempTest = digitalRead(liftState);

... it looks like you're already guilty of mixing up pin definitions and status variables, so good naming practices serve as a reminder. (Don't sweat it. It happens to everyone.)

As others have said, you need to make sure you understand what your switch inputs are doing at all times. You must use a pull-up / pull-down resistor unless this is done for you by the machine you're controlling, otherwise there will be a time where there is no valid input level -- you'll get random results when you read that pin. The easiest way to do this (again, as has been said already) is to use the internal pull-ups and connect the other side of the switch to ground, so it goes LOW when pressed. Your new setup routine would look something like this:

void setup() {
  Serial.begin(9600);
  
  pinMode(SW_BOTTOM, INPUT_PULLUP);
  pinMode(SW_TOP,    INPUT_PULLUP);
  pinMode(SW_BELT,   INPUT_PULLUP);
  pinMode(PIN_LIFT,  OUTPUT);
  pinMode(PIN_BELT,  OUTPUT);
}

Finally, you need to be doing something to guarantee you're not reading switch contact bounce as discrete states. Usually by measuring once, then waiting a certain number of ms and verifying the state hasn't changed before acting on any further input. Although, if the switches are indeed limit switches, it might be enough to act on the first LOW (pressed) state and ignore the status after that until the state has been reset somehow.

I don't follow exactly what your program flow should be... Would you want the the lift to raise until it hits the top, then lower until it hits the bottom, and repeat indefinitely? If so, is the lift motor direction controlled by HIGH/LOW with no option for "off"? Or should the lift always raise to the top any time it hits the bottom by whatever external means? (In this case, the lift motor could be turned on with a HIGH logic level, and turned off with LOW.) What does the belt do? I see no mention of how that fits in.

One last thing -- I really doubt you should be using analogWrite() here. That's meant to provide a pulse-width modulated (PWM) output. You're setting it to the extremes of 255 and 0, which is effectively the same result you'll get with digitalWrite(). That's a dead giveaway that you should be using digitalWrite() instead.

psst:
SirNickity:
C++ compilers are smarter than that. They know that const don't go into RAM, so a const int is effectively the same as a #define.

When someone is facing a 4 hour deadline, that is not the nit to pick.

4 hours was up a couple hours ago. By now, the matter is settled and it's down to "what would make this better next time." Good point about the compiler optimizations though.

I'm surprised Paul didn't jump on him for trying to get us to do his homework.

KeithRB:
const don't go into RAM

Are you really sure about that? It's true that the storage of const values may be optimised away, but where they aren't they'll be stored in RAM. If you want to ensure your values to remain in PROGMEM you would need to use the PROGMEM attribute.

It depends on how they're referenced in the sketch.
If you ever take the address of a const and use the pointer, you can be pretty sure it'll be put in RAM.
Otherwise, yes, it will be optimized into a literal.