Bug in my code and I cannot find it

The code below was updated by “septillion” I think working from a less elegant set of code by me. I have just had cause to reload my boards (ATMega328) with it and the behaviour seems to have changed; probably because I may have changed something.

Simply put, this code moves a servo from one position to another based on a signal on a pin being present or not. The movement from 75 to 105 works fine, but removing the signal the return to 75 is almost instant without the slow movement.

I suspect this is very simple but I cannot see it so I would appreciate a little help

Thank you

David

#include <Servo.h>

//constants
const byte InputPins[] = {11, 12, 13, A0, A1, A2, A3, A4};
const byte NrServos = sizeof(InputPins);
const byte ServoPins[NrServos] = {3, 4, 5, 6, 7, 8, 9, 10};
const byte EndPositions[2] = {75, 105};
const byte BounceAngles[] = {5, 0, 2, 0, 1};
const byte MovementDelay = 30;
const byte BounceDelay = 120; 

//Variables
Servo servo;
bool previousButtonStates[NrServos];

void setup() {
  for (byte i = 0; i < NrServos; i++) {
    pinMode(InputPins[i], INPUT);             // set pins to INPUT
    servo.attach(ServoPins[i]);
    servo.write(EndPositions[0]);             // move servo to DOWN position
    delay(2 * BounceDelay);
    servo.detach();
  }
}

void loop() {
  for (byte i = 0; i < NrServos; i++) {
    const bool State = digitalRead(InputPins[i]);
    if (State != previousButtonStates[i]) {
      previousButtonStates[i] = State;
      servo.attach(ServoPins[i]);
      
      for(byte pos = EndPositions[!State]; 
          pos <= EndPositions[State]; 
          (State ? pos++ : pos--))
      {
        servo.write(pos);                     // tell servo to go to position in variable 'pos'
        delay(MovementDelay);                 // waits 'MovementDelay' for the servo to reach the position
      }
                                              //make the servo 'bounce'
      for(byte i = 0; i < sizeof(BounceAngles); i++){
        servo.write(EndPositions[State] + (State ? -1: +1) * BounceAngles[i]);
        delay(BounceDelay);
      }
                                              //And fix at end position
      servo.write(EndPositions[State]);
      delay(BounceDelay);
      servo.detach();
    }
  }
}

What is the point of attaching one servo to 8 different pins? You don’t REALLY have the servo physically wired to 8 different pins, do you?

    const bool State = digitalRead(InputPins[i]);

What was the point of declaring State to be const?

      for(byte pos = EndPositions[!State];
          pos <= EndPositions[State];
          (State ? pos++ : pos--))

Using true or false as array indices does not make sense. Clearly, some other type is more appropriate.

I really can not understand what you are trying to do. You WILL have to post a schematic.

There are up to 8 servos attached to the board using pins 3 - 10. They all need to work in exactly the same way so the loop detects whether any one of them has changed.

const bool State = digitalRead(InputPins*);*
What was the point of declaring State to be const?
I have no idea, I didn’t code this as I stated above
Using true or false as array indices does not make sense. Clearly, some other type is more appropriate.
Again, I didn’t write this so I don’t know why this was done this way.
What am I trying to do?
These servos are attached to semaphore signals on a model railway. They are controlled by a on/off single pole switch which puts 5vdc onto the appropriate pin (or removes it) causing the servo to rotate either clockwise or counter clockwise. I do have a working version of this code but as I said, it was enhanced by another board member as a suggestion to replace tedious code with loops.
The problem is manifesting itself by the servo rotating slowly in one direction but instantaneously in the other.
I’ve attached a diagram of the set up - please don’t comment on the wiring or the resistors etc, it works with my old code, I just want to use the more elegant version.

There are up to 8 servos attached to the board using pins 3 - 10.

Then you need 8 instances of the Servo class, not one.

PaulS:
What was the point of declaring State to be const

What was the point of NOT doing this? There is a convention that says that if the variable is not supposed to be changed - declare it const. Personally, I am not a fan of this convention, but it has its followers. And it makes sense.

PaulS:
Using true or false as array indices does not make sense. Clearly, some other type is more appropriate.

Actually, it kinda does. In C++ language, bool is an integer type, one of many other integer types. Its values have “strange” names, yet they are perfectly usable in integer contexts, acting as 0 and 1. One can no longer apply -- and ++ to bool objects though, but this code doesn’t.

PaulS:
Then you need 8 instances of the Servo class, not one.

That is actually an interesting question. The author of the code actually attempts a clever thing: managing multiple servos with one Servo object, by detaching and reattaching it to different pins.

Is this supposed to work? When Servo object is detached from its pin, does the pin maintain its last PWM setting?

Judging by the source code of Servo class, the PWM signal is not maintained and the pin just goes to low. I would expect the servo to just stay put in this case, assuming it is not mechanically loaded. So, it should sorta/kinda work… Precarious, possibly jittery at reattachment, but it should work, if done properly.

for (byte i = 0; i < NrServos; i++) {


                                        //make the servo ‘bounce’
     for(byte i = 0; i < sizeof(BounceAngles); i++){

This creates two different variables called i. Inside the inner loop, you cannot access the value of the outer i.

It is traditional to use i as a for-loop variable. But that does not make it a good idea. Give these two variables distinct names like servoNum and currentBounceAngleIndex.

Is it part of the specification that only one servo can move at a time and during the movement all buttons are ignored? Because you used delay() this is a major impediment to proper function.

MorganS:
This creates two different variables called i. Inside the inner loop, you cannot access the value of the outer i.

It is traditional to use i as a for-loop variable. But that does not make it a good idea. Give these two variables distinct names like servoNum and currentBounceAngleIndex.

Is it part of the specification that only one servo can move at a time and during the movement all buttons are ignored? Because you used delay() this is a major impediment to proper function.

MorganS:
This creates two different variables called i. Inside the inner loop, you cannot access the value of the outer i.

It is traditional to use i as a for-loop variable. But that does not make it a good idea. Give these two variables distinct names like servoNum and currentBounceAngleIndex.

Is it part of the specification that only one servo can move at a time and during the movement all buttons are ignored? Because you used delay() this is a major impediment to proper function.

Point taken on the variable i - I can change that but curious because that part of the code works - even though the return does not recognise the delay, the bounce does work.

The fact that only one servo can be processed at one time is not a problem, the loop detects a change to any one and then acts on it then continues with the loop checking other pins. This is fine, not likely to change two signals at once and time for the loop is minimal.

   for(byte pos = EndPositions[!State]; 

pos <= EndPositions[State];
          (State ? pos++ : pos–))
      {

Change <= to !=

MorganS:
Change <= to !=

Really?? I’m not a C++ programmer but in other languages I was always taught never to use equal (or not equal) to terminate a loop. Please explain why this might be causing my problem and further my education

rynd2it:
Really?? I’m not a C++ programmer but in other languages I was always taught never to use equal (or not equal) to terminate a loop. Please explain why this might be causing my problem and further my education

MorganS is absolutely right.

You need to move your servo either forward from 75 to 105, or backward from 105 to 75. When you are moving backward, you start from pos is 105 and your EndPositions[State] is 75. Your cycle condition pos <= EndPositions[State] will never be true. Your servos will never move backward.

Since you are using such a bi-directional cycle, pos != EndPositions[State] is completely appropriate here.

If you don’t like this bi-directional cycle, you might want to rewrite it as two separate uni-directional cycles. One will move forward and have pos <= EndPositions[State] as its condition. Another will move backward and have pos >= EndPositions[State] as its condition.

Secondly, I don’t know who taught you to “never to use equal (or not equal) to terminate a loop”, but this sounds like something that might be appropriate for freshman students. (Only to be told at the beginning of their second year to forget all they learned in the first year.) One can actually argue in favor of !=.

Montmorency:
MorganS is absolutely right.

You need to move your servo either forward from 75 to 105, or backward from 105 to 75. When you are moving backward, you start from pos is 105 and your EndPositions[State] is 75. Your cycle condition pos <= EndPositions[State] will never be true. Your servos will never move backward.

Since you are using such a bi-directional cycle, pos != EndPositions[State] is completely appropriate here.

If you don’t like this bi-directional cycle, you might want to rewrite it as two separate uni-directional cycles. One will move forward and have pos <= EndPositions[State] as its condition. Another will move backward and have pos >= EndPositions[State] as its condition.

But the servos DO move back to position 75, just without the delays.

I won’t argue programming - been doing it since the 60’s and old habits die hard :wink:

rynd2it:
But the servos DO move back to position 75, just without the delays.

Because you do servo.write(EndPositions[State]); right after the non-functional loop.

MorganS:
Because you do servo.write(EndPositions[State]); right after the non-functional loop.

OK, I think I got it. Now to change the code and try and upload it - whole other story.

I’ll report back

Thanks

OK, thanks for the help - it now works and the key was the change from <= to !=

Cheers

David