Linear Actuator Position Control Reed Sensor

Hello
I have what I am sure is a simple problem but I have been going in circles for days and have found I am in need of some assistance.
I have a 36v linear actuator with a built in reed sensor with which I am counting pulses to feedback position. The actuator direction is controlled via two active LOW, solid state relays. I am using an Arduino Mega. Rather than switches or buttons I am using the serial monitor to communicate with the Mega.
My problem is that while my sketch knows what 'goals' I am setting for pulses counted/stroke length, the relays will not switch once those goals are reached.
Here is an excerpt of the code:

if (incomingByte == 'A') {
    goalPosition = 50;
    if (goalPosition > CurrentPosition) {
       Retracting = false;
       Extending = true;
       actuatorExtend ();
       Serial.println("AExtending"); 
            }

Once a count of 50 pulses is reached the code will not switch one of the relays to stop the actuator and the actuator will continue to extends on it's way past the goalPosition of 50; however if I type 'A' into the serial monitor once the count is past the goalPosition of 50 the actuator will retract.
I modified a sketch I found here;

http://learn.robotgeek.com/demo-code/123-arduino-linear-actuator-tutorial-preset-position-button-control.html

This actuator in this example however uses a potentiometer for positional feedback rather than a reed switch and push buttons rather than the serial monitor.
Here is my full code:

const int relay1 = 11;
const int relay2 = 12;
const int reedSwitch = 2; //interupt 

int counter = 0;
int counterState = 0;         // current state of the button
int lastButtonState = 0;     // previous state of the button

long lastDebounce0 = 0;
long debounceDelay = 3;	// Ignore bounces under 3MS

int CurrentPosition = 0; 
int goalPosition = 0;

boolean Extending = false;
boolean Retracting = false;

int incomingByte;      // a variable to read incoming serial data into

void actuatorExtend(){
      digitalWrite(relay1, HIGH);
      digitalWrite(relay2, LOW);
}
void actuatorRetract(){
      digitalWrite(relay1, LOW);
      digitalWrite(relay2, HIGH);
}
void actuatorStop() {
    digitalWrite(relay2, HIGH);
  digitalWrite(relay1, HIGH);
}

void setup() {
  // initialize serial communication:
  Serial.begin(9600);
  // initialize the LED pin as an output:
  pinMode(reedSwitch, INPUT);        // reedSwitch is an input

  attachInterrupt(0, trigger0, RISING);
  Serial.println("Repetition counter");
  Serial.print("Start");
  Serial.print("\t");
  Serial.println("End");
  
  pinMode(relay1, OUTPUT);
  pinMode(relay2, OUTPUT);
  digitalWrite(relay2, HIGH);
  digitalWrite(relay1, HIGH);

}

void loop() {
  // see if there's incoming serial data:
if (Serial.available() > 0) {   
    // read the oldest byte in the serial buffer:
    incomingByte = Serial.read();

 if (incomingByte == 'A') {
    goalPosition = 50;
    if (goalPosition > CurrentPosition) {
       Retracting = false;
       Extending = true;
       actuatorExtend ();
       Serial.println("AExtending"); 
            }      
     else if (goalPosition < CurrentPosition) {
        Retracting = true;
        Extending = false;
        actuatorRetract();
        Serial.println("ARetracting");  
             }
              }
                  
 if (incomingByte == 'B') { 
    goalPosition = 15; 
     if (CurrentPosition < goalPosition ) {
        Retracting = false;
        Extending = true;
        actuatorExtend ();
        Serial.println("BExtending");   
            }  
     else if (goalPosition < CurrentPosition) {
         Retracting = true;
         Extending = false;
         actuatorRetract();
         Serial.println("BRetracting");            
              } 
               }
  
       if (incomingByte == 'S') {
           actuatorStop();
           Serial.println("Stop"); 
                 }

 if (Extending == true && CurrentPosition > goalPosition ){    
         //we have reached our goal, shut the relays off
      actuatorStop();
      boolean Extending = false;
      Serial.println("IDLE");  
                 }
  
 if (Retracting == true && CurrentPosition < goalPosition){
          //we have reached our goal, shut the relay off
       actuatorStop();
       boolean Retracting = false;
       Serial.println("DELI");  
              }
                }
                  }

void trigger0() {
    if( (millis() - lastDebounce0) > debounceDelay && Extending == true){
      counter++;
        Serial.print(counter);
        Serial.println(" : ");
         CurrentPosition = counter;
         lastDebounce0 = millis();
            }
            
   if( (millis() - lastDebounce0) > debounceDelay && Retracting == true){
      counter--;
        Serial.println(counter);   
        Serial.print(" : ");
         CurrentPosition = counter;
         lastDebounce0 = millis();
          }       
           }

I am sure the problem is something simple that, being inexperienced as I am with this I have overlooked. Any assistance would be greatly appreciated.

Do you have any documentation or links for the actuator ?

Superjack Harl 3624+. 36v 24" Stroke. 36 pulses per inch.
https://www.sadoun.com/Sat/Products/S/HARL-3624-TD-24-Moteck-Actuator.htm

This actuator in this example however uses a potentiometer for positional feedback rather than a reed switch and push buttons rather than the serial monitor.
Here is my full code:

Does your code address this difference ?

Yes. Attach interrupt with debounce, per the code I posted above.

Sorry, my answer was a little abrupt. I am using an attachInterupt with denounce to count the pulses from the reed switch rather than using the potentiometer feedback that the example sketch from robotgeek.

You don't understand the difference between assignment of an existing
variable and declaring a new variable:

 if (Extending == true && CurrentPosition > goalPosition ){    
         //we have reached our goal, shut the relays off
      actuatorStop();
      boolean Extending = false;  ///////// WRONG!!!!!!
      Serial.println("IDLE");  
                 }

Declaring a new variable of the same name will do nothing. Assignment
(ie changing) an existing variable is just = ;

Specifying a type and a name is a declaration - creating a new separate variable.

So change those lines to :

      Extending = false;

I also note a problem in your motor control routines:

void actuatorExtend(){
      digitalWrite(relay1, HIGH);
      digitalWrite(relay2, LOW);
}
void actuatorRetract(){
      digitalWrite(relay1, LOW);   // turn on one relay before
      digitalWrite(relay2, HIGH);  // turning off the other!!
}
void actuatorStop() {
    digitalWrite(relay2, HIGH);
  digitalWrite(relay1, HIGH);
}

You say the relays are active low - one drives forward, the other backward - thus
its presumably an error (short circuit?) if both are driven LOW.

So you always need to set one pin HIGH before the other goes low:

void actuatorRetract(){
      digitalWrite(relay2, HIGH);  // turn off before
      digitalWrite(relay1, LOW);   // turning on
}

You also may have the issue that both relays are on at power-up before the Arduino
gets going. If this does cause a hardware issue you should add physical pull-up
resistors to both relay inputs if they don't already have them.

Thank you for pointing out the problem with my motor control routine. That was not something that I would have considered with my current experience level. I change the code as per your suggestion.

So change those lines to :
Code:
Extending = false;

That hasn't changed the behavior of the actuator. It still moves past the goalPosition without stopping. It seems to be acting like a While loop. While (incomingByte == 'B') // nothing else will happen until another Byte is registered on the serial monitor, be it 'A' or 'S' or another 'B'. Or could there still be an error I've missed around the code you've adjusted for me? Thank you for that by the way.

 if (Extending == true && CurrentPosition > goalPosition ){    
         //we have reached our goal, shut the relays off
      actuatorStop();
       Extending = false;
      Serial.println("IDLE");  
                 }

I just can't see it though. Frustrating.

Do you know that the currentPosition variable is being updated properly, i.e. that the pulses are being registered? Do you see the appropriate activity on your serial monitor showing the pulses?

Also, your debounce code looks faulty. Consider what happens when your debounce code skips the first half of a bounce but records the second half due to timing coincidence.

A possibly much bigger problem is that you are writing to Serial from inside an interrupt handler! This is a huge no-no and can cause corruption within the serial driver and/or elsewhere. Your interrupt handler should ONLY debounce the signal and make changes to the currentPosition variable. For change detection, debugging, etc, you have an oldPosition variable that you compare with currentPosition inside loop() and if they differ, you do stuff like Serial.print (and relay output actions) there before overwriting oldPosition with currentPosition.

The last two conditionals in loop() are still inside the Serial.available()
test, so they will never be noticed except if a character comes in off the
Serial port.

You need to move them up a level so they always get tested round loop()

Also loop() is simply too long to read - put the various parts of it in subfunctions
so its readable... Always a good idea to increase readability and maintainability
of code - if a function is larger than a page, its probably too big.

Are you running the actuator on 36V?

MarkT.

The last two conditionals in loop() are still inside the Serial.available()
test, so they will never be noticed except if a character comes in off the
Serial port.

You need to move them up a level so they always get tested round loop()

That was the problem. I moved them up and it is working as I intended. Thank you. I am not sure how to mark this post as solved? The first message in this string was my very first post, ever, anywhere.
I have taken on board your suggestion that the loop() is too long. That means though I am going to have to seriously rethink the sketch that this snippet is going to be a part of.

raschemmel, yes I am using a 36V transformer on this. Thank you for your probing questions.

polyglot. I started to reply to you before trying the suggestion from Mark and for the benefit of anyone else looking to use a linear actuator with a reed switch for feed back here is the reply I was intending, as redundant some of it may now be:

Do you know that the currentPosition variable is being updated properly, i.e. that the pulses are being registered? Do you see the appropriate activity on your serial monitor showing the pulses

The currentPosition is definitely being updated correctly. I am saying that because as described above, if the goalPosition is 50, while the actuator extends past 50 on writing 'A' to serial, it will immediately retract on a second write to serial of 'A'. The same goes for a serial write of 'B'. It will extend if CurrentPosition > goalPosition and retract if goalPosition < CurrentPosition. Pulses are registering well on the serial monitor. I did have some initial problems with switch bounce but found this site;
http://robotgestation.com/SensorForce.html
and used the circuit shown in the "SwitchDebounce" image for hardware debounce which uses a 74hc14 schmitt trigger. It works very well but after doing the calcs Tc = C * R, rather than using a 10K resister shown in the example I chose to use a 5k resister. I went from an unfiltered switch bounce like in image "Unfiltered" to a signal shown in image "Circuit 1A" using the "Let's add a capacitor!" example from the webpage to the final signal shown in "Switch Debounce A" image, which according to my tired old CRO looks pretty clean. I hope the images are attached properly as I am intending them to be.

Thank you all for your help. I can finally go to bed without laying awake trying to figure this out. Simple in the end, as I knew it would be, being as green as I am.

SwitchDebounce.jpg

The last two conditionals in loop() are still inside the Serial.available()
test, so they will never be noticed except if a character comes in off the
Serial port.

You need to move them up a level so they always get tested round loop()

@MarkT,
Are these the two IF statements you were referring to :

		if (Extending == true && CurrentPosition > goalPosition )
        	   {    
   	 	   //we have reached our goal, shut the relays off
   	 	   actuatorStop();
  	 	   boolean Extending = false;
          	  Serial.println("IDLE");  
          	   }
  
 		if (Retracting == true && CurrentPosition < goalPosition)
        	   {
        	     //we have reached our goal, shut the relay off
  	 	    actuatorStop();
   	 	    boolean Retracting = false;
   	 	    Serial.println("DELI");  
         	   }

raschemmel
I don't know if it's bad form in a forum to answer a question directed to someone else but yes, those were the two if statements MarkT was referring to.
Mike W.

Not only is it not bad form, it is more like the norm . Joe blow asks question of John Doe and Tom ,Dick & Harry all throw in their answers too. That's like about the norm here so don't worry about it and thanks for the answer. I was trying to follow along for learning purposes.

This problem was solved.

That doesn't tell us much. . Why are you not explaining how you solved it ? Don't you think we would want to know if we responded to that post ?

Could you please post the working code? as I'm doing something similar. Thank you.

Could someone tell me how to connect position sensor that's included in this actuator? I've been trying to figure it out for days and haven't succeeded.