My shade project. Did I do good?

I thought my first arduino code project would be to make my shade go up and down based on a light sensor aimed outside. My shade is heavy so I’m using nema 17 and 3d printed worm gear. I looked at many other projects and did the tutorials but nothing was the same as what I needed so I made my own code. I placed an endstop at the bottom to know when the shade is closed and up works just by number of steps to move. The code is written so that it wont go down if the endstop is triggered already and it wont go up if the endstop isnt triggered.
After lots of playing around everything works as I want but being a newbie I would like to know if I did the code the way it should be and what improvements I can make.
Also I would like to add an override switch where I can move the shade up and down and ignore the light sensor. I think a 3 option switch (up,down,middle) would work but I dont know where to start with the code to implement that.
Thanks and all feedback welcome…

// heavy window shade project
// shade should open and close with daylight

#define dirPin 55
#define stepPin 54
#define X_ENABLE_PIN 38
#define home_switch 3 // For shade close Xmin
#define X_MAX_PIN 2 // to be used as override up switch
#define Y_MAX_PIN 15  // to be used for override down switch
#define lightSensorPin 13
byte HSval;
byte downSwitch = digitalRead(X_MAX_PIN);
byte upSwitch = digitalRead(Y_MAX_PIN);
float steps;

void setup(){
 Serial.begin(9600);
   pinMode(X_MAX_PIN, INPUT_PULLUP);
   pinMode(X_ENABLE_PIN , OUTPUT);
   pinMode(home_switch, INPUT_PULLUP);
   pinMode(stepPin, OUTPUT);
   pinMode(dirPin, OUTPUT);
digitalWrite(X_ENABLE_PIN , LOW);
pinMode (lightSensorPin, INPUT);

}

void loop () {
int lightVal=analogRead(lightSensorPin);    
Serial.println(lightVal);
HSval = digitalRead(home_switch);
if (lightVal > 900 & HSval == HIGH)  // if its dark outside and the endstop isnt triggered (shade is not down)...
    do
    {
      digitalWrite(dirPin, HIGH);  
    digitalWrite(stepPin, HIGH);
    delayMicroseconds(100);                      
    digitalWrite(stepPin, LOW);
    delayMicroseconds(100); 
      HSval = digitalRead(home_switch);
      steps=0; // reset steps to zero so that it can be moved back up when the light increases
    } while (HSval == HIGH);  // move it down until the endstop is triggered

    if (lightVal < 750 & HSval == LOW) { // if its light outside and the endstop is triggered (shade is down)...
      while (steps < 1000000) {    // Move shade up two feet
        digitalWrite(dirPin, LOW);
        digitalWrite(stepPin, HIGH);
        delayMicroseconds(100);
         digitalWrite(stepPin, LOW);
        delayMicroseconds(100);
        steps++;  // Increase the number of steps taken
      }}
}

A minor comment on the code
1)
Fix your indentations; makes it a lot easier to read
2)
You should never have two } on the same line; again readability.

Use tools → autoformat in the IDE to fix both.

This is actually not correct

if (lightVal > 900 & HSval == HIGH)

& is a bitwise AND; in a condition you want a logical AND which is &&

You’re using hardcoded numbers (900, 750, 1000000); it’s better to place them near the top of the code using e.g. #define.

Something like

// heavy window shade project
// shade should open and close with daylight

#define TWOFEET 1000000UL
#define LOWLIGHT 900
#define HIGHLIGHT 750

...
...

If you ever want to adjust, you do not have to dig through the code to find it. Replace where you currently use those numbers by the defined constants.

Further the steps variable should not be a float but an integer type; in this case unsigned long.

=========
If you want to add an override switch, you can wire the common pin to GND and the two other pins to pins on your Arduino. Use pinMode(somePin, INPUT_PULLUP). You can now test if one of the pins is LOW (indicating in which position the switch is); both pins HIGH will indicate that the switch is in the center position and that you want to use the light sensor.

Based on your code, you can modify the lightVal variable when one of the pins is LOW to simulate light / low light.

=========
Lastly I would seriously consider adding a limit switch at the other side of the blind as well; in case your code decides to run away, you can not move further than that.

Sometimes the compiler will not do what we think.

if (lightVal > 900 & HSval == HIGH)  // if its dark outside and the endstop isnt triggered (shade is not down)...
    do
    {

That if () has no { so the compiler sees it as a single line that evaluates a T/F then does nothing, it gets optimized out.

I put my brackets { } on their own lines at the indent level of the control structure (if, else, do, while, for, switch, etc) just so it's easy to see them lined up. Using the IDE Autoformat Tool (ctrl-T) gets the balances checked, use it OFTEN.

Don't use an int where a byte will do. You don't have bytes to waste if you want to do much at all.

You will need to learn a different approach if you want to do more than one thing at a time, like run a motor while watching a switch. During a delay() the motor might keep running but the switch is never read. There's ways around that, the first most important trick is explained simply and well in this blog: http://gammon.com.au/blink

PS, if someone shines a light on the window at night, will the shade open?

The compiler should pick up the do ... and compile accordingly. I agree that adding extra brackets is a good habit.

sterretje: The compiler should pick up the do ... and compile accordingly. I agree that adding extra brackets is a good habit.

I agree using {} after an 'if' even if they are not needed is a good habit. The original code may be correct but then people come along and add further indented statements expecting them to be conditional on the 'if' but they are not. If the code is not properly tested it can be hard to spot later.

Congrats on a working project! Congrats on using code tags on your first post! Karma++

Good comments on your existing code. I want to comment on your future plans.

You want to add the ability to manually position the blinds. You have considered a 3 positions switch. I would suggest something like this: Add a mode selection switch to be able to select automatic mode (which you currently have functioning) and manual mode. Add 2 momentary switches for up and down. When in manual mode, ignore the light sensor. If the user selects up, move up but still keep count of the steps so that you do not let the user run too high. If the user selects down, move down but still respect your limit switch.

Make sense? Without something like this, the system will always be in automatic mode. If your user moves the shade up, the light sensor may indicate the shade should be closed and close it right back up. I know that I would be cheesed off at this. :)

GoForSmoke: PS, if someone shines a light on the window at night, will the shade open?

I thought about this and saw someone else try and write code to avoid this problem. Ill see how it works out for now and if it becomes a problem Ill try and fix the code.

vinceherman: Congrats on a working project! Congrats on using code tags on your first post! Karma++

Good comments on your existing code. I want to comment on your future plans.

You want to add the ability to manually position the blinds. You have considered a 3 positions switch. I would suggest something like this: Add a mode selection switch to be able to select automatic mode (which you currently have functioning) and manual mode. Add 2 momentary switches for up and down. When in manual mode, ignore the light sensor. If the user selects up, move up but still keep count of the steps so that you do not let the user run too high. If the user selects down, move down but still respect your limit switch.

Make sense? Without something like this, the system will always be in automatic mode. If your user moves the shade up, the light sensor may indicate the shade should be closed and close it right back up. I know that I would be cheesed off at this. :)

I saw many others not using codes and being told off, so I knew better.lol

You have the right idea with the switch but I think ill use one 3 way switch and the middle will be what you call auto and either up or down would be manual by default.

Wiring seems easy enough but where do I begin with the code? Am I using the same if command or am I making a completely new if or a new function. I'm really not sure what the right approach is with this. Any tutorials would help also.

Thanks so much

Ah, so the 3 position switch will be Open (all the way), Auto and Close (all the way). I was imagining it as a way to open it a little.

Wiring. It can be done many ways. I like this method that uses a single analog pin.

Or you can do it with 2 digital pins.

vinceherman: Ah, so the 3 position switch will be Open (all the way), Auto and Close (all the way). I was imagining it as a way to open it a little.

I guess I need to check it over with the wife...

You could get a 2-position switch for MANUAL-AUTO and a slider for when it's on manual. Set the slider, throw the switch, flip it back later. I got nice 60mm travel linear slider pots from Futurlec a while back and they are nice.

You can write code with different modes and keep track of what mode to run using a variable.