Water Solenoids Project - Want your opinion, please.

Hello,

I have built the hardware and wrote the code to control two latching solenoids with a foot pedal (momentary switch). The code below works, but I’d like to know if you think the code could be written better (faster, smaller) or it’s fine as-is. I’m planning on building a few of these and wanted to make sure I wouldn’t run into any problems in the future.

I’m using an IC H bridge (SN754410). Datasheet can be found here: http://focus.ti.com/lit/ds/symlink/sn754410.pdf

The latching solenoids only require about 30 milliseconds of DC current. One polarity to open, opposite polarity to close.

When the foot pedal (momentary switch, pin 2) is closed, the solenoid opens. When the foot pedal is released, the solenoid closes.

Any constructive criticism will be greatly appreciated. Thanks in advance.

  const int switchPin = 2;    // foot pedal input
  const int solenoid1Pin = 3;    // H-bridge leg 1 (pin 2, 1A)
  const int solenoid2Pin = 4;    // H-bridge leg 2 (pin 7, 2A)
  const int solenoid3Pin = 5;    // H-bridge leg 3 (pin 15, 4A)
  const int solenoid4Pin = 6;    // H-bridge leg 4 (pin 10, 3A)
  const int enablePin = 9;    // H-bridge enable pin
  const int ledPin = 13;      // LED 
  
int val;                        // variable for reading the pin status
int val2;                       // variable for reading the delayed/debounced status
int buttonState;                // variable to hold the foot pedal state


  void setup() {
    // set the foot pedal as an input:
    pinMode(switchPin, INPUT); 

    // set all the other pins you're using as outputs:
    pinMode(solenoid1Pin, OUTPUT); 
    pinMode(solenoid2Pin, OUTPUT); 
    pinMode(solenoid3Pin, OUTPUT); 
    pinMode(solenoid4Pin, OUTPUT); 
    pinMode(enablePin, OUTPUT);
    pinMode(ledPin, OUTPUT);

    // set enablePin high so that solenoid can turn on:
    digitalWrite(enablePin, HIGH); 

    // blink the LED 5 times. This should happen only once.
    // if you see the LED blink five times, it means that the module
    // reset itself,. probably because the solenoid caused a brownout
    // or a short.
    blink(ledPin, 5, 500);
  }

void loop(){
  val = digitalRead(switchPin);      // read input value and store it in val
  delay(10);                         // 10 milliseconds is a good amount of time
  val2 = digitalRead(switchPin);     // read the input again to check for bounces
  if (val == val2) {                 // make sure we got 2 consistant readings!
    if (val != buttonState) {          // the button state has changed!
      if (val == LOW) {                // check if the button is pressed
      digitalWrite(solenoid1Pin, LOW);   // set leg 1 of the H-bridge low
      digitalWrite(solenoid2Pin, HIGH);  // set leg 2 of the H-bridge high
      digitalWrite(solenoid3Pin, LOW);   // set leg 3 of the H-bridge low
      digitalWrite(solenoid4Pin, HIGH);  // set leg 4 of the H-bridge high
        delay(30);                        // delay used for pulse
      digitalWrite(solenoid1Pin, LOW);   // set leg 1 of the H-bridge low
      digitalWrite(solenoid2Pin, LOW);  // set leg 2 of the H-bridge low
      digitalWrite(solenoid3Pin, LOW);   // set leg 3 of the H-bridge low
      digitalWrite(solenoid4Pin, LOW);  // set leg 4 of the H-bridge low
        } else
        if (val == HIGH) {
          if (val != buttonState) {          // the button state has changed!
      digitalWrite(solenoid1Pin, HIGH);   // set leg 1 of the H-bridge low
      digitalWrite(solenoid2Pin, LOW);   // set leg 2 of the H-bridge high
      digitalWrite(solenoid3Pin, HIGH);   // set leg 3 of the H-bridge low
      digitalWrite(solenoid4Pin, LOW);   // set leg 4 of the H-bridge high
        delay(30);                        // delay used for pulse
      digitalWrite(solenoid1Pin, LOW);   // set leg 1 of the H-bridge low
      digitalWrite(solenoid2Pin, LOW);  // set leg 2 of the H-bridge low
      digitalWrite(solenoid3Pin, LOW);   // set leg 3 of the H-bridge low
      digitalWrite(solenoid4Pin, LOW);  // set leg 4 of the H-bridge low
      }
    }
    buttonState = val;                 // save the new state in our variable
  }
 }
}
  /*
    blinks an LED
   */
  void blink(int whatPin, int howManyTimes, int milliSecs) {
    int i = 0;
    for ( i = 0; i < howManyTimes; i++) {
      digitalWrite(whatPin, HIGH);
      delay(milliSecs/2);
      digitalWrite(whatPin, LOW);
      delay(milliSecs/2);
    }
  }

I’d make a pin array and use enums to reference it. That way you can use a for loop to address pins in succession. Or you could use raw access to the pins and write the bits all at once, but that could reduce portability to future versions of android.

Using pin arrays:

enum {
  switchPin = 0,
  solenoid1Pin,
  solenoid2Pin,
 ...
};

const int PinArray[] = { 2, 3, 4, 5, 6, 9, 13 };

void SetHBridge(byte value)
{
  for (int i = solenoid1Pin; i <= solenoid4Pin; i++)
  {
    digitalWrite(PinArray[i], value&0x01);
    value >>= 0x01;
  }
}

...

This will at least make it smaller, but probably not faster. If this will be your only code, I don’t think you are in desperate need of more space; maintaining an easy to understand code flow could be more usefull.

int val;                        // variable for reading the pin status
int val2;                       // variable for reading the delayed/debounced status
int buttonState;                // variable to hold the foot pedal state

More meaningful names, and you wouldn't need the comments. If you are counting things, you go 1, 2, 3..., not yeah ok, 2, 3..., so val and val2 are particularly irritating to me.

      if (val == LOW) {                // check if the button is pressed
        } else
        if (val == HIGH) {
          if (val != buttonState) {

If val isn't LOW, what other values can it hold? There is no reason to compare it to HIGH. If it isn't LOW, it must be HIGH.

The next if test is unnecessary, too. The comparison of val to LOW only happens if val is not equal to buttonState. Since the value in val can not change between the first time it is compared to buttonState, another comparison is not needed.

If you put each { on a new line and each else on a new line, and properly indent all the code in between, you'd see right off that those two if's are not needed.

Typically, when I am coding, when I type if, I create this whole block:

if()
{
}
else
{
}

Then, I go back and fill in the conditional part, and the action sections. If you do the conditional part and the true action first, by the time you get to the else, you can have forgotten what not true means, so you put a lot more stuff in the false block than is needed.

Arrays of pins are good, when the action to be performed on all the pins is the same.

In the case of pins for an H-bridge, though, that is not true. I prefer to use names that describe the purpose of the pin - enable1, enable2, go1, go2, for example. Then, when it comes time to set the state of the pins, it isn't necessary to remember whether the pin in solenoid1Pin is an enable pin or a go pin.

Kidmosey and PaulS,

Thanks for your feedback.

One question. The reason I put in the else, if val == HIGH is because no pulse should be sent, unless the button is high and the button state changes. If I take out the val == HIGH and the val != buttonState out, if the first if statements are not met, wouldn't it send a pulse to close the solenoid?

 if (val == LOW) {                // check if the button is pressed
        } else
        if (val == HIGH) {
          if (val != buttonState) {

As this program runs, most of the time no conditions will be met to cause a pulse to be sent. Only on button down and button up. I'm trying to think through the logic. Am I missing something?

Thanks for your help.

terex: One question. The reason I put in the else, if val == HIGH is because no pulse should be sent, unless the button is high and the button state changes. If I take out the val == HIGH and the val != buttonState out, if the first if statements are not met, wouldn't it send a pulse to close the solenoid?

If you restructure it as Paul suggested, it makes a bit more sense

  if (val == val2)
  {
    if ([color=red]val != buttonState[/color])
    {
      if (val == LOW)
      {
        digitalWrite(...);
        digitalWrite(...);
        digitalWrite(...);
        ...
      }
      else
//  { // Imagine this is here
        if (val == HIGH)
        {
          if ([color=red]val != buttonState[/color])
          {
            digitalWrite(...);
            digitalWrite(...);
            digitalWrite(...);
            ...
          }
        }
//   } // and this
  }

Indenting your code properly makes it much easier to see what is happening. Here you can see that you've checked val != buttonState twice. Also, you could combine the two first if statments into one, and as Paul mentioned, remove the val == HIGH if statement, as it is always true if you hit that block of code.

This is what you get.

  if (val == val2 && val != buttonState)
  {
    if (val == LOW)
    {
      digitalWrite(...);
      digitalWrite(...);
      digitalWrite(...);
      ...
    }
    else
    {
      digitalWrite(...);
      digitalWrite(...);
      digitalWrite(...);
      ...
    }
  }

If ain't broke, keep messing with it 'til it is!