next topic »

[on:]

I have not programmed since playing with the original IBM basic. I trying to write a sketch to control the rolling roof on my observatory. I have a motor on both ends and reverse voltage to the motor at the opposite end to the direction of travel to minimize drag. I'm using one of those 8 relay boards you can get on ebay, two switch bricks, three pushbutton bricks and three LED bricks. Voltage reversal is done in how the relays are wired. I'd just like some expert to critique my work. The sketch is at www.alfiesty.com/sketch. Thanks

Jim

[Reply #1 on:]

Unfortunately I get a 403  (forbidden, no permission) - error while trying to get access.

[Reply #2 on:]

Sorry, I did not post the entire URL. It is www.alfiesty.com/sketch/sketch_roof_control.

Jim

[Reply #3 on:]

Same error.

______
Rob

[Reply #4 on:]

Sorry, This is the first time I have put a file up on the Internet without help and I screwed it up! Try www.alfiesty.com/sketch/sketch_roof_control/sketch_roof_control.pde

[Reply #5 on:]

Code:

if (bstop = HIGH)

This is an assignment, not a comparison; it will always be true.

Code:

if (bismoving = true)//

Ditto.

I'd say if anything, the program is over-commented - this:

Set up Constants
     Digital Pins 0-8 are outputs for relay drivers
     Pin 0 = Out motor 1 out plus
     Pin 1 = Out motor 1 out minus
     Pin 2 = Out motor 2 in plus
     Pin 3 = Out motor 2 in minus

could just as well be put in the declarations of the pins, whilst this:

Code:

digitalWrite(openmotoropenplus, LOW);      // sets the digital pin as output

is simply stating the obvious incorrectly.

Generally, it is better to qualify things like pin numbers with "const".

[Reply #6 on:]

Code:

 if (bstop = HIGH)

This is an assignment, not a comparison; it will always be true.
Code:

 if (bismoving = true)//

Ditto.

I assume it should be
if (bbstop == HIGH)

I'd say if anything, the program is over-commented - this:

All the comments is a matter of style!

Quote
Set up Constants
     Digital Pins 0-8 are outputs for relay drivers
     Pin 0 = Out motor 1 out plus
     Pin 1 = Out motor 1 out minus
     Pin 2 = Out motor 2 in plus
     Pin 3 = Out motor 2 in minus
could just as well be put in the declarations of the pins, whilst this:
Code:

 digitalWrite(openmotoropenplus, LOW);      // sets the digital pin as output

is simply stating the obvious incorrectly.

How? Example showing correct would help.
Generally, it is better to qualify things like pin numbers with "const".
done

Thank for looking, this is my first programming in close to 25 years., I have to say the help provided by the arduino site is great!

Jim (8000 feet above the average)

[Reply #7 on:]

digitalWrite(openmotoropenplus, LOW);      // sets the digital pin as output

is simply stating the obvious incorrectly.

How? Example showing correct would help.

Well, for a start, it doesn't make the pin an output, it was made an output with a "pinMode".
Your comment might have read "Set the pin low." or "write zero to the pin", but that doesn't describe what that actually achieves, which is what should be commented, if a comment is needed.

[Reply #8 on:]

Just a stylistic thing, I find long variable names like this

openmotoropenplus

very hard to read, especially when they are in large blocks. I would suggest camel case

OpenMotorOpenPlus

or maybe underscores

open_motor_open_plus

______
Rob

[Reply #9 on:]

Thanks Rob, thats a very valid observation. I'll do immediately!

Jim