Pages: [1]   Go Down
Author Topic: I'm somewhat new would like expert to critique my sketch  (Read 932 times)
0 Members and 1 Guest are viewing this topic.
Buena Vista, CO
Offline Offline
Full Member
***
Karma: 0
Posts: 180
Arduino rocks
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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
Logged

8000ft above the average

Netherlands
Offline Offline
God Member
*****
Karma: 5
Posts: 614
A naughty mind is a joy forever.
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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

Logged

Buena Vista, CO
Offline Offline
Full Member
***
Karma: 0
Posts: 180
Arduino rocks
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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

Jim
Logged

8000ft above the average

nr Bundaberg, Australia
Offline Offline
Tesla Member
***
Karma: 121
Posts: 8458
Scattered showers my arse -- Noah, 2348BC.
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

Same error.

______
Rob
Logged

Rob Gray aka the GRAYnomad www.robgray.com

Buena Vista, CO
Offline Offline
Full Member
***
Karma: 0
Posts: 180
Arduino rocks
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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
Logged

8000ft above the average

Global Moderator
UK
Offline Offline
Brattain Member
*****
Karma: 240
Posts: 24424
I don't think you connected the grounds, Dave.
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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:
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.

Generally, it is better to qualify things like pin numbers with "const".
« Last Edit: April 28, 2011, 12:12:01 pm by AWOL » Logged

"Pete, it's a fool looks for logic in the chambers of the human heart." Ulysses Everett McGill.
Do not send technical questions via personal messaging - they will be ignored.

Buena Vista, CO
Offline Offline
Full Member
***
Karma: 0
Posts: 180
Arduino rocks
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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)
Logged

8000ft above the average

Global Moderator
UK
Offline Offline
Brattain Member
*****
Karma: 240
Posts: 24424
I don't think you connected the grounds, Dave.
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Quote
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.
Logged

"Pete, it's a fool looks for logic in the chambers of the human heart." Ulysses Everett McGill.
Do not send technical questions via personal messaging - they will be ignored.

nr Bundaberg, Australia
Offline Offline
Tesla Member
***
Karma: 121
Posts: 8458
Scattered showers my arse -- Noah, 2348BC.
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

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
Logged

Rob Gray aka the GRAYnomad www.robgray.com

Buena Vista, CO
Offline Offline
Full Member
***
Karma: 0
Posts: 180
Arduino rocks
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

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

Jim
Logged

8000ft above the average

Pages: [1]   Go Up
Jump to: