Hi, I've been using Arduino for 8 months now and I'm really enjoying learning programming and prototyping. I've built a tracked vehicle with a pan and tilt turret that can hold and fire estes model rockets 4 total but is capable of firing up to 8 and is all controlled over xbee pros'. This is my first post and it would be good if someone could look over my code and give me some guidance on cleaning it up or maybe ways I could do something better, the code compiles and works fine on the platform. Any help would be appreciated, thank you.
This is my code:
#include <DualVNH5019MotorShield.h>
#include <Shifter.h>
#include <ServoTimer2.h>
#define SER_Pin 16 //SER_IN
#define RCLK_Pin 17 //L_CLOCK
#define SRCLK_Pin 18 //CLOCK
#define azPin 3
#define elPin 5
#define NUM_REGISTERS 1 //how many registers are in the chain
ServoTimer2 servoaz;
ServoTimer2 servoel;
int incomingByte = 0;
Shifter shifter(SER_Pin, RCLK_Pin, SRCLK_Pin, NUM_REGISTERS);
DualVNH5019MotorShield md;
void setup()
{
Serial.begin(57600);
shifter.clear(); //set all pins on the shift register chain to LOW
shifter.write(); //send changes to the chain and display them
md.init(); // Init the Motor Shield md
servoaz.attach(azPin);
servoel.attach(elPin);
servoaz.write(1600);
delay(10);
servoel.write(1400);
delay(10);
}
void loop(void) {
while (Serial.available() > 0) { // send data only when you receive data
incomingByte = Serial.read(); // read the incoming byte
switch(incomingByte){
case 'w':
forward();
break;
case 'x':
backwards();
break;
case 's':
halt();
break;
case 'a':
pivotleft();
break;
case 'd':
pivotright();
break;
case 'q':
neutralleft();
break;
case 'e':
neutralright();
break;
case 'z'://Neutral Left Backwards
nleftb();
break;
case 'c'://Neutral Right Backwards
nrightb();
break;
case ',':
traverseleft();
break;
case '.':
traverseright();
break;
case 'l':
gunfront();
break;
case 'b':
zeroel();
break;
case 'n':
maxrange();
break;
case 'm':
maxel();
break;
case '1':
fox1();
break;
case '2':
fox2();
break;
case '3':
fox3();
break;
case '4':
fox4();
break;
case '8':
salvo();
break;
default:
break;
}
}
}
void forward() {
md.setSpeeds (300,300);
}
void backwards() {
md.setSpeeds (-300,-300);
}
void pivotleft() {
md.setSpeeds (-255,255);
}
void pivotright() {
md.setSpeeds (255,-255);
}
void neutralleft() {
md.setSpeeds (0,255);
}
void neutralright() {
md.setSpeeds (255,0);
}
void nleftb() {
md.setSpeeds (0,-255);
}
void nrightb() {
md.setSpeeds (-255,0);
}
void halt() {
md.setBrakes (0,0);
}
void traverseright()
{
servoaz.write(3000);
delay(10);
}
void traverseleft()
{
servoaz.write(0);
delay(10);
}
void gunfront()
{
servoaz.write(1600);
delay(10);
}
void zeroel()
{
servoel.write(1500);
delay(10);
}
void maxrange()
{
servoel.write(1200);
delay(10);
}
void maxel()
{
servoel.write(900);
delay(10);
}
void fox1()
{
shifter.setPin(0, HIGH);
shifter.write();
delay(1000);
shifter.setPin(0, LOW);
shifter.write();
}
void fox2()
{
shifter.setPin(1, HIGH);
shifter.write();
delay(1000);
shifter.setPin(1, LOW);
shifter.write();
}
void fox3()
{
shifter.setPin(2, HIGH);
shifter.write();
delay(1000);
shifter.setPin(2, LOW);
shifter.write();
}
void fox4()
{
shifter.setPin(3, HIGH);
shifter.write();
delay(1000);
shifter.setPin(3, LOW);
shifter.write();
}
void salvo()
{
Serial.print("On the way!: ");
shifter.setPin(0, HIGH);
shifter.write();
delay(1000);
shifter.setPin(0, LOW);
shifter.write();
shifter.setPin(1, HIGH);
shifter.write();
delay(1000);
shifter.setPin(1, LOW);
shifter.write();
shifter.setPin(2, HIGH);
shifter.write();
delay(1000);
shifter.setPin(2, LOW);
shifter.write();
shifter.setPin(3, HIGH);
shifter.write();
delay(1000);
shifter.setPin(3, LOW);
shifter.write();
shifter.setPin(4, HIGH);
shifter.write();
delay(1000);
shifter.setPin(4, LOW);
shifter.write();
}
The fox functions only differ by which pin to trigger. You could use one fox function and pass the pin number. The salvo only needs a string of 'foxes'. Also every 1 or 2 line function that is only called from one place.. why not just leave those inline rather than having to go find the actual code down below?
I can't see any good reason why "incomingByte" should have global scope.
Generally-speaking, you should give variables the minimum possible scope, in this case, limited to the code block inside the while (Serial.available() > 0) (which should really be an "if")
What they said about not using "delay".
Thanks for the help, I'm new to programming never even tried it before until Arduino, so how do I create one function that can set different pins high then low depending on input from my laptop without giving them there own individual functions, and the delay is there because that's the duration I need to ignite the rocket igniters reliably. Thanks for the quick reply
ok I get making a function for the different foxes, so just make the function then just call it up for each fox, so I will still have to have the fox cases but just call the function underneath then apply it to that pin, correct?
Thanks everyone for the advice applied the changes to my code and compiles fine, is there any problems with the delays? or was it just because you were unaware of the application?
Roger that, please disregard the pin numbers advice.
All of your steering and move functions that are likewise just different parameters can be melded as well.
At some point take the time to learn what this blog teaches, it will open doors and show you that delay is ugly.
If you put a radio in a rocket and generated a constant tone from it then when it takes off, the doppler in a ground receiver could tell you speed and acceleration. But only if it goes straight away from the receiver. For total analysis you need at least 3 receivers and somewhat complex math.
Generally, we don't like them, but probably in this case, they're OK (unless you decided, after the first launch, you didn't want a salvo after all - with the code as it is written, you can't cancel it)
I've been trying to figure that out for a couple of months, how to abort a salvo if I need to. Any ideas for me?
The short answer is "lose the delays".
The slightly longer answer is "look at the blink without delay example, without delay"
The even longer answer is "Google "Finite state machine" "
thanks for all the help, this is the problem I'm having now, because I'm using the shifter library and using shifter.write to call up those pins attached to the shift register how do i assign them a constant when I'm using shifter.write to access them? i apologise if this is a stupid question
1 included the firepin code and it compiles fine, uploaded it to my project but no joy, i think when i added the byte line its trying to set pins 0,1,2,3 of the arduino, not pins 0,1,2,3 of the shift register