How can I improve my program?

Hello all, I made a wireless servo system. The goal was to press a button on one board and it would move a servo on another. It all works, but there are a few bugs. For example if you press the button and the servo doesn’t move and you press it again the servo flies to position 2 and then goes back to position 1 and back to position 2 or vise versa.

The reason I have so many write and delay lines it because I needed to slow the servo down and the VarSpeedServo library had conflicts with my other libraries.

I’m fairly new to programming so be gentle. This is also my very first Arduino project.

Here is the receiver code:

#include <VirtualWire.h>
#include <ServoTimer2.h>

//const int ledtest_pin = 9; //LED Test Pin
const int servoPin = 6; //Servo Pin
const int transmit_pin = 12; //Transmit not needed on this board
const int receive_pin = 3;//pin connected between RX module and Arduino
const int transmit_en_pin = 5; //Transmit not needed on this board

ServoTimer2 rangefinder;  

void setup()
{
    //rangefinder.write(1100);
    rangefinder.attach(servoPin);
    rangefinder.write(1500);
    delay(1000);
    //rangefinder.detach();
    vw_set_tx_pin(transmit_pin); //Transmit not needed on this board
    vw_set_rx_pin(receive_pin);
    vw_set_ptt_pin(transmit_en_pin); //Transmit not needed on this board
    vw_set_ptt_inverted(true); //Transmit not needed on this board
    vw_setup(2000);//speed communication bps

    vw_rx_start();       // activate receiving mode

    //pinMode(ledtest_pin, OUTPUT);
    
    //pinMode(LED_BUILTIN, OUTPUT); //Debug
}

void loop()
{
  {
    uint8_t buf[VW_MAX_MESSAGE_LEN];
    uint8_t buflen = VW_MAX_MESSAGE_LEN;
    if (vw_get_message(buf, &buflen))  //verify if any data is received
    { 
      if(buf[0]=='1')//if received 1 turn servo and/or turn on LED(s)
        {
        //digitalWrite(ledtest_pin , HIGH); //Debug
        //digitalWrite(LED_BUILTIN, HIGH); //Debug
        
        rangefinder.attach(servoPin); //Attach and activate servo.
        rangefinder.write(0);
        delay(70);
        rangefinder.write(100);
        delay(70);
        rangefinder.write(200);
        delay(70);
        rangefinder.write(300);
        delay(70);
        rangefinder.write(400);
        delay(70);
        rangefinder.write(500);
        delay(70);
        rangefinder.write(600);
        delay(70);
        rangefinder.write(700);
        delay(70);
        rangefinder.write(800); //Rotate 90 deg
        delay(70);
        rangefinder.write(900);
        delay(70);
        rangefinder.write(1000);
        delay(70);
        rangefinder.write(1100);
        delay(70);
        rangefinder.write(1200);
        delay(70);
        rangefinder.write(1300);
        delay(70);
        rangefinder.write(1400);
        delay(70);
        rangefinder.write(1500);
        delay(500); //Small delay to ensure servo moves. 
        //rangefinder.detach(); //Detach and turn off servo.
        }  
     if(buf[0]=='0')
        {
        //digitalWrite(ledtest_pin , LOW); //Debug
        //igitalWrite(LED_BUILTIN, LOW); //Debug
        
        rangefinder.attach(servoPin); //Attach and activate servo.
        rangefinder.write(1500);
        delay(70);
        rangefinder.write(1400);
        delay(70);
        rangefinder.write(1300);
        delay(70);
        rangefinder.write(1200);
        delay(70);
        rangefinder.write(1100);
        delay(70);
        rangefinder.write(1000);
        delay(70);
        rangefinder.write(900);
        delay(70);
        rangefinder.write(800);
        delay(70);
        rangefinder.write(700); //Rotate 90 deg
        delay(70);
        rangefinder.write(600);
        delay(70);
        rangefinder.write(500);
        delay(70);
        rangefinder.write(400);
        delay(70);
        rangefinder.write(300);
        delay(70);
        rangefinder.write(200);
        delay(70);
        rangefinder.write(100);
        delay(70);
        rangefinder.write(0);
        delay(500); //Small delay to ensure servo moves. 
        //rangefinder.detach(); //Detach and turn off servo.
        }
    }
  }
}

And transmitter:

#include <VirtualWire.h>
const int transmit_pin = 2;//transmit pin connected between TX module and Arduino
//const int receive_pin = 2;
const int transmit_en_pin = 3;
const int buton_sw = 9;
int var_apasare = 0;

void setup()
{
    delay(1000);
    vw_set_tx_pin(transmit_pin);
    //vw_set_rx_pin(receive_pin);
    vw_set_ptt_pin(transmit_en_pin);
    vw_set_ptt_inverted(true);
    vw_setup(2000);       // speed of communication bps
    pinMode(buton_sw, INPUT);
    //Serial.begin(115200);
}

int contor = 1;

void loop()
{
  var_apasare = digitalRead(buton_sw);
  if (var_apasare == HIGH)
  {
    if (contor == 1) {  
      char msg[1] = {'1'};
      //Serial.println("press");
      vw_send((uint8_t *)msg, 1);
      vw_wait_tx(); 
      contor = 0;
      delay (1000);
  }
  else
  {
    if (contor == 0) {   
      char msg[1] = {'0'};
      //Serial.println("press");
      vw_send((uint8_t *)msg, 1);
      vw_wait_tx(); 
      contor = 1;
      delay (1000);
  }
  }
  }
}

You could change this:

        rangefinder.write(0);
        delay(70);
        rangefinder.write(100);
        delay(70);
        rangefinder.write(200);
        delay(70);
        rangefinder.write(300);
        delay(70);
        rangefinder.write(400);
        delay(70);
        rangefinder.write(500);
        delay(70);
        rangefinder.write(600);
        delay(70);
        rangefinder.write(700);
        delay(70);
        rangefinder.write(800); //Rotate 90 deg
        delay(70);
        rangefinder.write(900);
        delay(70);
        rangefinder.write(1000);
        delay(70);
        rangefinder.write(1100);
        delay(70);
        rangefinder.write(1200);
        delay(70);
        rangefinder.write(1300);
        delay(70);
        rangefinder.write(1400);
        delay(70);
        rangefinder.write(1500);
        delay(500); //Small delay to ensure servo moves. 
        //rangefinder.detach(); //Detach and turn off servo.
        }

to this:

   int i;
   for (i = 0; i < 1501; i += 100) {
      rangefinder.write(i);
      delay(70);
   }
   delay(500);

You could do a similar change for the “down-count”, too.

How can I improve my program?

Change the receiver to a state machine. That would entirely eliminate the problem you described.

A hallmark of a complete state machine is an empty setup.

@econjack. If the amount of those delays are critical(probably isn't though), the last delay(500) should be delay(430).

Pete

  • Use consistent variable names. You have established this _pin convention, then you throw in servoPin. That's going to make it harder for you to remember the variable name while you're writing code.
  • Use a consistent indentation size.
  • Don't add extra braces (e.g. line 34 of the first sketch). They make the code more likely to have bugs, cause excessive indentation, and make it hard to read.
  • Use a consistent brace style. You have indented some, which I don't think is very easy to read.
  • Consistent use of whitespace. You have if(buf[0]=='1'), you have if (contor == 0). You have delay(70);, you have delay (1000);.

None of the above affect the functionality of your code so they seem a bit nit picky but I think it really does make a difference for more complex projects, collaboration with others, or sharing code with beginners.

I recommend using the style enforced by Tools > Auto Format in the standard Arduino IDE because that's a useful tool. It is possible to adjust the settings of Auto Format if there's something you really don't like but you will need to redo this every time you install a new version of the Arduino IDE. Auto Format doesn't control everything so there are still some style choices to make. Generally, best practices is to use existing style conventions. For this reason I follow the style established in the official Arduino example sketches. What's most important though is to pick a style and use it consistently.

el_supremo:
@econjack. If the amount of those delays are critical(probably isn't though), the last delay(500) should be delay(430).

Pete

I have no clue. However, if the total delay needs to be 500, then it should be changed to 430 outside the loop.

it should be changed to 430 outside the loop

I was referring to the delay(500) in the replacement code you posted.

Pete

el_supremo:
I was referring to the delay(500) in the replacement code you posted.

Pete

Yes, I know...I was agreeing.

Ohhhhhhhhh. Gotcha.

Pete

Thank you for your suggestion. Unfortunately I can't get past the first step. I've imported the library and studied the examples but when I try to load the states I get an error.

//States
State noop = State(noopUpdate);  //no operation
State On = State(rfDown);
/tmp/550528382/Reciever_v3/Reciever_v3.ino:6:20: error: 'noopUpdate' was not declared in this scope

State noop = State(noopUpdate); //no operation

^

/tmp/550528382/Reciever_v3/Reciever_v3.ino:7:18: error: 'rfDown' was not declared in this scope

State On = State(rfDown);

^

/tmp/550528382/Reciever_v3/Reciever_v3.ino:8:19: error: 'rfUp' was not declared in this scope

State Off = State(rfUp);

^

exit status 1

I'm sure I'm missing something super obvious.

Implementing state machine doesn’t mean you need to implement the State library… A state machine is a programming technique, just like state change detection or using millis() is a technique, which doesn’t require a library but in some cases a library can make things easier. But here I would just suggest to read up on state machines and to implement your own. Just like the use of millis() (/non-blocking timing) it’s a valuable skill to have.

septillion:
Implementing state machine doesn't mean you need to implement the State library... A state machine is a programming technique, just like state change detection or using millis() is a technique, which doesn't require a library but in some cases a library can make things easier. But here I would just suggest to read up on state machines and to implement your own. Just like the use of millis() (/non-blocking timing) it's a valuable skill to have.

I'm sorry I have to knowledge of this topic. Can you point me in the right direction?

The first step is to identify the states in your system (ideally; at a minimum the states in your sketch will have to be identified). Start with your receiver (making the transmitter a state machine is probably not going to gain much). How many states can you identify in setup?

Would I have 2 or 4 states? 'Up and down' or 'Up, moving up, down, moving down'?

I have already done some research and found a few examples but I still am unsure how to implement it into my code.

Those don't look like anything in setup.

How many states can you identify in just setup? (Trust me. It is worth starting there.)

One, right? The initial state of 'up' or rangefinder.write(1500).

"Waiting" is always a state. Whether waiting for time to elapse or a condition to bet met or both.

It is often handy to have an initialization state and a finalization state. For example, when waiting for 1000 milliseconds, an initialization state might save the value from millis to be used in the actual waiting. Having separate initialization and finalization states makes cleaner / simpler / easier to debug code.

So, knowing that "waiting" (delay(1000)) is a state, how many states are needed for setup?

I’ve done some work. I think I’m on the right track. It still doesn’t work as intended though.

#include <VirtualWire.h>
#include <ServoTimer2.h>

//const int ledtest_pin = 13; //LED Test Pin
const int servo_pin = 6; //Servo Pin
const int transmit_pin = 12; //Transmit not needed on this board
const int receive_pin = 3;//pin connected between RX module and Arduino
const int transmit_en_pin = 5; //Transmit not needed on this board
unsigned long current_millis;
unsigned long move_time;
unsigned long task_wait;
byte message_number;
int pos = 1500; //0 is up, 1 is down

ServoTimer2 rangefinder;  

enum States {
  stateStart,
  stateInput,
  stateUp,
  stateMoveDown,
  stateDown,
  stateMoveUp,
  stateMove,
};

States mState = stateStart;

void setup()
{
    Serial.begin(9600);
    move_time = millis();
    task_wait = millis();
    mState = stateStart;
  
    //Initialize  
    rangefinder.attach(servo_pin);
    vw_set_rx_pin(receive_pin);
    vw_setup(2000); //speed communication bps
    vw_rx_start(); // activate receiving mode
    Serial.println("SETUP VARS");
}

void loop()
{
  current_millis  = millis();
  
  //Interpret Signal
  uint8_t buf[VW_MAX_MESSAGE_LEN];
  uint8_t buflen = VW_MAX_MESSAGE_LEN;

  switch (mState)
  {
    //Begin stateStart
    case stateStart:
      {
      //Serial.println("START");
      rangefinder.write(pos);
      if (message_number > 0) 
        {
          message_number = 0;
          mState = stateInput;
        } 
      if (message_number == 0) 
        {
          mState = stateInput;
        }
      }
      break;
  //End stateStart
  
  //Begin stateInput
  case stateInput:
  //Serial.println("stateInput");
    {
      //rangefinder.write(1500);
      Serial.println("Wait For Input");
      if (vw_get_message(buf, &buflen))  //verify if any data is received
        { 
          if(buf[0]=='1')
            {
              Serial.println("Received '1'");
              message_number = 1;
              mState = stateMove;
            }
          if(buf[0]=='0')
            {
              Serial.println("Received '0'");
              message_number = 2;
              mState = stateMove;
            }
        }
    }
    break;
  //End stateInput
  
  //Begin stateMove
  case stateMove:
  Serial.println("MOVE");
  {
    switch(message_number)
    {
    case 1:
        Serial.println("1 RECEIVED");
        int i;
        for (i = 0; i < 1501; i += 100) 
        {
          rangefinder.write(i);
          delay(70);
        }
        pos = 1500;
        mState = stateInput;
        break;
    case 2:
        Serial.println("0 RECEIVED");
        for (i = 1501; i > 0; i -= 100) 
        {
          rangefinder.write(i);
          delay(70);
        }
        pos = 0;
        mState = stateInput;
        break;
    }
  }
  //End stateMove
  
  //Begin stateUp
  case stateUp:
    Serial.println("UP");
    mState = stateStart; //advance to the next state
    break;
  //End stateUp
  
  /*
  //Begin stateMoveDown
  case stateMoveDown: 
  Serial.println("MOVE DOWN");
    {
      int i;
      for (i = 0; i < 1501; i += 100) 
      {
        rangefinder.write(i);
        delay(70);
      }
      pos = 1500;
      mState = stateDown; //advance to the next state
      break;
    }
  //End stateMoveDown
  */
  
  //Begin stateDown
  case stateDown:
    Serial.println("DOWN");
    mState = stateInput; //advance to the next state
    break;
    
  //End stateDown
  
  /*
  //Begin stateMoveUp
  case stateMoveUp:
  Serial.println("MOVE UP");
    {
      int i;
      for (i = 1501; i > 0; i -= 100) 
      {
        rangefinder.write(i);
        delay(70);
      }
      pos = 0;
      mState = stateInput;
      break;
    } 
  //End stateMoveUp
  */
  }
}

Okay, so I did a lot of work. Rewrote the transmitter to just send a signal and then the receiver deciphers what to do with it. It more or less fixes the problem where it would try and go up or down twice. Here is my new code:

#include <VirtualWire.h>
#include <ServoTimer2.h>

const int servo_pin = 6; //Servo Pin
const int receive_pin = 3;//pin connected between RX module and Arduino
unsigned long current_millis;
unsigned long move_time;
unsigned long task_wait;
byte message_number = 1; //1 is up and 0 is down
int rf_state = 1; //1 is up and 0 is down

ServoTimer2 rangefinder;  

enum States {
  stateStart,
  stateInput,
  stateUp,
  stateDown,
  stateMove,
};

States mState = stateStart;

void setup()
{
    move_time = millis();
    task_wait = millis();
    mState = stateStart;
    rangefinder.write(1500); 
}

void loop()
{
  current_millis  = millis();
  
  //Interpret Signal
  uint8_t buf[VW_MAX_MESSAGE_LEN];
  uint8_t buflen = VW_MAX_MESSAGE_LEN;
  
  switch (mState)
  {
    //Begin stateStart
    case stateStart:
    
    //Debug
    Serial.begin(9600);
    
    //Debug Message
    Serial.println("****************");
    Serial.println("POWER ON / START");
    Serial.println("****************");
    
    //Set servo to default state (up)
    Serial.println("Reset Servo");
   
    //Initialize vars
    rangefinder.attach(servo_pin);
    vw_set_rx_pin(receive_pin);
    vw_setup(2000); //speed communication bps
    vw_rx_start(); // activate receiving mode
    Serial.println("SETUP VARS");
    
    //Debug Display Ready
    Serial.println("READY");
    Serial.println("");
    
    //Get ready for input
    mState = stateInput;
    break;
    //End stateStart
  
    //Begin stateInput
    case stateInput:
    //Serial.println("Ready for Input");
    if (vw_get_message(buf, &buflen))  //verify if any data is received
      { 
        if (buf[0]=='1') 
          {
            if (rf_state == 0)
            {
              Serial.println("Received '1'");
              message_number = 1;
              mState = stateMove;
            }
            if (rf_state == 1)
            {
              Serial.println("Received '0'");
              message_number = 0;
              mState = stateMove;
            }
            else
              break;
          }
        }
    break;
  //End stateInput
  
  //Begin stateMove
  case stateMove:
    Serial.println("MOVE");
    switch(message_number)
    {
    case 0:
        {
        Serial.println("0 RECEIVED");
        Serial.println("Moving Down");
        int i;
        for (i = 1501; i > 0; i -= 100) 
          {
            rangefinder.write(i);
            delay(90);
          }
        mState = stateDown;
        break;
        }
    case 1:
        {
        Serial.println("1 RECEIVED");
        Serial.println("Moving Up");
        int i;
        for (i = 0; i < 1501; i += 100) 
          {
            rangefinder.write(i);
            delay(90);
          }
        mState = stateUp;
        break;
        }
    }
    break;
  //End stateMove

  //Begin stateDown
  case stateDown:
    rf_state = 0;
    Serial.println("State DOWN (0)");
    Serial.println("");
    //delay(1000);
    mState = stateInput;
  break;
  //End stateDown

  //Begin stateUp
  case stateUp:
    rf_state = 1;
    Serial.println("State UP (1)");
    Serial.println("");
    //delay(1000);
    mState = stateInput;
  break;
  //End stateUp
  }
}