Any suggestions on this working linear actuator code to make it better?

I'm intending to use this code to control a linear actuator that controls the reverse buckets on a jet boat. The buckets are either raised (full forward power), or lowered (full reverse power), Neutral (equal parts forward and reverse thrust), or gradients between those positions.

One 0-5v hall sensor will be attached to the control lever in the helm to read the lever's position, and another hall sensor is attached to the lever on the transom of the boat that controls the bucket position. The linear actuator moves the transom lever to raise or lower the bucket.

The idea is that when the control lever is moved to a position in the helm, the pot gives a value, and then the actuator moves the bucket until the pot on the bucket matches the value of the control lever pot. I'm using the multimap function to be able to calibrate different neutral values for the control lever and the bucket since the physical neutral positions might not translate linearly between the two.

I'm using a separate MC circuit for each bucket on the boat (there are two), which are ruggeduinos with a MegamotoPlus on each one to control the actuators. The entire system will run through two 80a relays so as a backup I can switch to direct rocker switch control of the buckets in case the MCs freeze or fail.

The code I've pieced together so far works well in tinkercad, and I'm about to set up a bench test with one actuator this week. Does anyone have any suggestions or feedback to improve the code? Because the Megamoto can monitor load, I was thinking about adding code to watch for amperage spikes and force the actuator to rest for a short period if a serious spike occurs. Also, ignore all of the serial prints : ) Thanks in advance for any feedback!

const int reverseBucket = A0; //potentiometer from reverse bucket actuator
const int shiftLever = A1; //potentiometer from throttle

const int enable = 8;
const int PWMA = 11;
const int PWMB = 10;

int reverseMax = 1023;
int reverseMin = 0;//range of reverse bucket actuator pot
int shiftLeverMin = 0;
int shiftLeverMax = 1023; //range of shift lever pot

//these values are for the multimap fuction

int bucketNeutral = 511; //neutral position value for the bucket
int shiftLeverNeutral = 511; //neutral position value for the shift lever
int in[] = {shiftLeverMin,shiftLeverNeutral,shiftLeverMax};
int out[] = {reverseMin,bucketNeutral,reverseMax};

//end multimap fuction values


int speedMin = 100;
int speedMax = 225; //speed range of actuator when moving


int precision = 2;//how close to final value to get

int rawbucketPosition = 0;
int bucketPosition = 0;
int rawdestination = 0;
int destination = 0;
int difference = 0;//values for knowing location

void setup()
{
  pinMode(reverseBucket, INPUT);//input from reverse bucket pot
  pinMode(shiftLever, INPUT);//input from reverse shift lever pot
  pinMode(enable, OUTPUT);
  pinMode(PWMA, OUTPUT);//actuator motor outputs
  pinMode(PWMB, OUTPUT);
  digitalWrite(enable,HIGH);
  Serial.begin(9600);
}

void loop()
{   
    destination = getDestination(); //See where shift lever is
    bucketPosition = analogRead(reverseBucket);//check where reverse bucket is
    Serial.print("Bucket Position    ");
    Serial.println(analogRead(reverseBucket));
    Serial.print("Lever Position    ");
    Serial.println(analogRead(shiftLever));
    difference = destination - bucketPosition;//find out how far you are from the destination
    Serial.print("Difference    ");
    Serial.println(difference);
    if (bucketPosition > destination) pullActuatorUntilStop(destination);// choose what action to take
    else if (bucketPosition < destination) pushActuatorUntilStop(destination);
    else if (difference < precision && difference > -precision) stopActuator();
}//end void loop

int getDestination()
{
    int val = analogRead(shiftLever);//read the shift lever potentiometer to get the destination
    destination = multiMap(val,in,out,3);//convert the shift lever values to match the bucket and calibrate neutral
    return(destination);
}//end getDestination

void pushActuatorUntilStop(int destination)
{
  destination = getDestination();
  int rawbucketPosition = analogRead(reverseBucket); 
  difference = destination - rawbucketPosition;//check difference to see if continue moving, or stop

  while (difference > precision)
  {
    destination = getDestination();
    rawbucketPosition = analogRead(reverseBucket); //continue checking difference
    difference = destination - rawbucketPosition;
    int Speed = constrain(difference, speedMin, 255);
    Serial.print("Bucket    ");
    Serial.println(rawbucketPosition);
    Serial.print("Lever    ");
    Serial.println(destination);
    pushActuator(Speed);
  }//end while
  
  delay(25);
  stopActuator();
}//end pushActuatorUntilStop

void pullActuatorUntilStop(int destination)
{
  destination = getDestination();
  int rawbucketPosition = analogRead(reverseBucket); //check difference to see if continue moving, or stop
  difference = destination - rawbucketPosition;

  while (difference < -precision)
  {
    destination = getDestination();
    rawbucketPosition = analogRead(reverseBucket); //continue checking difference
    difference = destination - rawbucketPosition;
    int Speed = constrain(-difference, speedMin, 255);
    Serial.print("Bucket    ");
    Serial.println(rawbucketPosition);
    Serial.print("Lever    ");
    Serial.println(destination);
    pullActuator(Speed);
  }//end while
  
  delay(25);
  stopActuator();
}//end pullActuatorUntilStop

void stopActuator()
{
  analogWrite(PWMA,0);
  analogWrite(PWMB,0);
}//end stopActuator

void pushActuator(int Speed)
{ 
  analogWrite(PWMB, Speed);
  analogWrite(PWMA,0);
}//end pushActuator

void pullActuator(int Speed)
{
  analogWrite(PWMB,0);
  analogWrite(PWMA, Speed);
}//end pullActuator


//MultiMap Functionality to set neutral position

int multiMap(int val, int* _in, int* _out, uint8_t size)
{
  // take care the value is within range
  // val = constrain(val, _in[0], _in[size-1]);
  if (val <= _in[0]) return _out[0];
  if (val >= _in[size-1]) return _out[size-1];

  // search right interval
  uint8_t pos = 1;  // _in[0] allready tested
  while(val > _in[pos]) pos++;

  // this will handle all exact "points" in the _in array
  if (val == _in[pos]) return _out[pos];

  // interpolate in the right segment for the rest
  return map(val, _in[pos-1], _in[pos], _out[pos-1], _out[pos]);
}

Your "precision" value won't get used as you want, I think. Only if "bucketPosition" is exactly equal to "destination" will the code get to the else-if where "precision" is compared to "difference" and at that point, "difference" will always be zero.

An interesting project, but once it’s working, review it for fail-safe issues.
If I’m skipping along at 60mph in a twin-jet boat, I certainly don’t want either actuator making up its own mind - same if either propulsion duct stops ‘thrusting’ ... a quick u-turn ?

I can think of about six failure scenarios that don’t include your controller.
All of them could be fatal.

I am also concerned about safety. The more of your code I read, the more errors, potential errors, duplicated code and confused thinking I pick up on. You should treat this project with extreme caution and test extensively. Is there a way to limit the speed to a few knots, until you have completed this testing?

Example of duplicated code:

difference = destination - rawbucketPosition;

I see that line repeated 5 times.

Example of confused code:

void pullActuatorUntilStop(int destination)
{
  destination = getDestination();

This function is passed "destination" as a parameter, which was previously returned by getDestination(). But this function immediately overwrites that value with a new result from getDestination(). This is also repeated multiple times.

I would not use potentiometers in a harsh environment. Especially at the bucket end. Look at using Hall-effect sensors and tiny magnets instead. And an optical encoder on the sending end.

PaulRB:
Your "precision" value won't get used as you want, I think. Only if "bucketPosition" is exactly equal to "destination" will the code get to the else-if where "precision" is compared to "difference" and at that point, "difference" will always be zero.

Thank you, I will look into this

lastchancename:
An interesting project, but once it’s working, review it for fail-safe issues.
If I’m skipping along at 60mph in a twin-jet boat, I certainly don’t want either actuator making up its own mind - same if either propulsion duct stops ‘thrusting’ ... a quick u-turn ?

I can think of about six failure scenarios that don’t include your controller.
All of them could be fatal.

I agree that it should be reviewed for fail-safe issues, that's why I'm coming here to have more eyes on the code, and will extensively test on the bench before taking it out.
This is also why I have factored in a backup system that is direct control of the actuators. When a bucket lowers in a twin jet under speed it isn't a sudden stop — it causes a braking turn in that direction - but it isn't remotely instant, especially not with this actuator speed. If we started to feel that happen the backup system can be activated. Electric rocker switches are used to control electric and hydraulic actuators frequently in this application. The MC is the new part, although there are companies that have used them commercially for this application.

lastchancename:
An interesting project, but once it’s working, review it for fail-safe issues.
If I’m skipping along at 60mph in a twin-jet boat, I certainly don’t want either actuator making up its own mind - same if either propulsion duct stops ‘thrusting’ ... a quick u-turn ?

I can think of about six failure scenarios that don’t include your controller.
All of them could be fatal.

Probably not relevant but, How a Broken Engine Caused this Plane to Suddenly Fall Apart Over Thailand | Testing the Limits.

PaulRB:
I am also concerned about safety. The more of your code I read, the more errors, potential errors, duplicated code and confused thinking I pick up on. You should treat this project with extreme caution and test extensively. Is there a way to limit the speed to a few knots, until you have completed this testing?

Example of duplicated code:

difference = destination - rawbucketPosition;

I see that line repeated 5 times.

Example of confused code:

void pullActuatorUntilStop(int destination)

{
 destination = getDestination();



This function is passed "destination" as a parameter, which was previously returned by getDestination(). But this function immediately overwrites that value with a new result from getDestination(). This is also repeated multiple times.

Thank you for the feedback. I'll look into passing the destination parameter needlessly. The setup will be tested extensively on the bench and won't make it to application if it doesn't perform flawlessly. As mentioned above, there will also be a backup system on board.
As far as the repeated code goes, is that something that makes the code less reliable, or is it just good practice? The code is only repeated because once a function was working, it just got copied.

SteveMann:
I would not use potentiometers in a harsh environment. Especially at the bucket end. Look at using Hall-effect sensors and tiny magnets instead. And an optical encoder on the sending end.

I'm actually using saltwater rated Hall-Effect sensors instead of pots. Sorry for the confusion! I mistakingly interchanged the terms in my description of the project.

iceabenezer:
As far as the repeated code goes, is that something that makes the code less reliable?

Yes, in very general terms: Repeated code means more code. More code means more bugs.

The longer a piece of code, the more difficult it becomes to be able to say that it is 100% bug-free. Even modestly long pieces of code can never be said, with complete confidence, to be entirely bug-free.

So keeping code shorter is almost always better. Of course, it is possible to take things too far and make code so short that it is difficult to read and understand, and therefore difficult to de-bug!

PaulRB:
Yes, in very general terms: Repeated code means more code. More code means more bugs.

The longer a piece of code, the more difficult it becomes to be able to say that it is 100% bug-free. Even modestly long pieces of code can never be said, with complete confidence, to be entirely bug-free.

So keeping code shorter is almost always better. Of course, it is possible to take things too far and make code so short that it is difficult to read and understand, and therefore difficult to de-bug!

Excellent. Thank you, I'll try and make it more efficient. So, for this specific line of code, would the best approach be to store Difference as a variable returned by a function? I'm uncertain how the values will be passed between all of the functions. I assume values like bucketPosition could also be condensed.
I'm just learning the ropes here, so a lot of my results come from trial and error and that's why I'm reaching out for suggestions on how to make this more efficient. Thanks as always for the feedback!

The issue may not be in your code once it’s been written, but something as simple as the choice of an N/O contact, instead of an N/C switch could make the difference...
Or an opto being illuminated vs dark in the default conditions,,,

Your environment is high-vibration and high corrosion, and electrically noisy. Keep those in mind. This is why engineers are regarded as ‘boring killjoys’ by many in society.

In operation, you may have less than a quarter of a second to locate and activate a ‘backup system’. in that time a passenger (or driver) can be thrown away from the controls, or overboard, or the boat’s heading may divert toward that small cluster of rocks.

Sure they’re all hypotheticals, but that’s what engineering and ‘system’ design is all about.

As I said, interesting, and a problem we’d all love to investigate, but don’t forget those invisible dark forces that want you to fail !

lastchancename:
The issue may not be in your code once it’s been written, but something as simple as the choice of an N/O contact, instead of an N/C switch could make the difference...
Or an opto being illuminated vs dark in the default conditions,,,

Your environment is high-vibration and high corrosion, and electrically noisy. Keep those in mind. This is why engineers are regarded as ‘boring killjoys’ by many in society.

In operation, you may have less than a quarter of a second to locate and activate a ‘backup system’. in that time a passenger (or driver) can be thrown away from the controls, or overboard, or the boat’s heading may divert toward that small cluster of rocks.

Sure they’re all hypotheticals, but that’s what engineering and ‘system’ design is all about.

As I said, interesting, and a problem we’d all love to investigate, but don’t forget those invisible dark forces that want you to fail !

Absolutely! And I appreciate your thoughtfulness. I've spent the last 3 months going through hypotheticals and I'm pretty comfortable with where I'm at with this system. If this was controlling the steering system, I wouldn't undertake the project because failure tolerance would be nearly 0, but with the buckets, it's much more forgiving and the plausible worst-case isn't catastrophic. I've trialed failure conditions my other boat with hydraulic buckets and feel comfortable with the reaction window. If the bench tests go very well, we'll see if it moves forward. I'm also not completely committed to this happening — I have to see how it performs and if it seems consistent.
Flipping the kill switch will switch control to the rockers. I'm comfortable using automotive relays to power the switched circuits; they are used in similar applications. The sensors sending the signals are Hall sensors used in industrial equipment to control throttle values on dozers and dumptrucks, so I think they'll perform well here.
Again, I appreciate all of the safety concerns and they will be considered through the process here.

Hi,
Welcome to the forum.

Have you tried your code?
Have to prototyped any of this to check its operation?
Do you have a schematic of your project?
Can you post a link to specs/data of the actuator and controllers?

Thanks.. Tom.. :slight_smile:

TomGeorge:
Hi,
Welcome to the forum.

Have you tried your code?
Have to prototyped any of this to check its operation?
Do you have a schematic of your project?
Can you post a link to specs/data of the actuator and controllers?

Thanks.. Tom.. :slight_smile:

Hey Tom, I'm in the process of prototyping right now. Should have a bench test going next week. The code works great in the emulator — we'll see how it does on the bench. The controllers are MegamotoPlus motor driver shields on top of Ruggeduinos, which are Arduino clones that are fortified quite a bit and have more tolerance to voltage fluctuations. The actuators are GlideForce GF17 actuators with the high-speed gear ratio and 1000lbs of force.
I don't have a schematic other than the one in the emulator, but it's not exactly accurate because of the limitations of the components available in the emulator. If the bench test works repeatedly over the next few weeks and the code works well under repeated tests, I'll move towards mapping the bigger circuit with the backups and relays.

I've had a go at tidying up your code, mainly to show how much shorter and simpler I think it could be, and how much duplication can be removed. I believe it should behave the same as your existing code, but I am unable to test it, so please test it to convince yourself it is performing the same, and let me know if I missed anything!

const int bucketPin = A0; //potentiometer from reverse bucket actuator
const int throttlePin = A1; //potentiometer from throttle

const int enable = 8;
const int PWMA = 11;
const int PWMB = 10;

const int bucketMax = 1023;
const int bucketMin = 0;//range of reverse bucket actuator pot
const int throttleMin = 0;
const int throttleMax = 1023; //range of shift lever pot
const int bucketNeutral = 511; //neutral position value for the bucket
const int throttleNeutral = 511; //neutral position value for the shift lever

const int speedMin = 100;
const int speedMax = 225; //speed range of actuator when moving

const int precision = 2;//how close to final value to get

void setup()
{
  pinMode(bucketPin, INPUT);//input from reverse bucket pot
  pinMode(throttlePin, INPUT);//input from reverse shift lever pot
  pinMode(enable, OUTPUT);
  pinMode(PWMA, OUTPUT);//actuator motor outputs
  pinMode(PWMB, OUTPUT);
  digitalWrite(enable, HIGH);
  Serial.begin(9600);
}

void loop()
{
  //read the throttle potentiometer and calculate the bucketTarget
  int throttle = analogRead(throttlePin);
  Serial.print("Throttle Position    ");
  Serial.println(throttle);
  int bucketTarget;
  if (throttle < throttleNeutral)
    bucketTarget = map(throttle, throttleMin, throttleNeutral, bucketMin, bucketNeutral);
  else
    bucketTarget = map(throttle, throttleNeutral, throttleMax, bucketNeutral, bucketMax);

  //check where reverse bucket is
  int bucket = analogRead(bucketPin);
  Serial.print("Bucket Position    ");
  Serial.println(bucket);

  //find out how far you are from the bucketTarget
  int difference = abs(bucketTarget - bucket);
  Serial.print("Difference    ");
  Serial.println(difference);

  // choose what action to take
  if (difference <= precision) {
    Serial.println("Speed is Zero   ");
    analogWrite(PWMA, 0);
    analogWrite(PWMB, 0);
  }
  else {

    //Calculate required motor speed
    int speed = constrain(difference, speedMin, speedMax);
    Serial.print("Speed    ");
    Serial.println(speed);

    if (bucket > bucketTarget) {
      analogWrite(PWMB, 0);
      analogWrite(PWMA, speed);
    }
    else {
      analogWrite(PWMB, speed);
      analogWrite(PWMA, 0);
    }
  }

  delay(25);
} //end of loop()

This is great! I like how you broke up the forward and backwards positions into two separate entities and then were able to remove the multimap function. I was leery of the multimap because I don't know how reliable it is under many, many iterations.

There are several things I see that could present problems. The first one is trivial. "Throttle" here is misleading because the way this waterjet works, there is a separate control for engine throttle. Engine throttle is tweaked constantly under way and changes the speed of the vessel. The buckets are more like (but not literally) a CVT transmission that linearly operates in reverse, neutral, and forward and all of the gradients between. That's why I called it "bucketLever". Maybe "thrust" would be more appropriate here. Again, trivial.

The other thing I see is potentially not an issue either. Right now, how the code works is that the entire thing has to iterate to calculate the position and set the speed of the actuator. Would a while loop to check for changes in bucketTarget be a better approach?

When positioning the boat at slow speeds, there are frequent changes from forwards to backwards with the bucket lever, feathering it back and forth depending on the movement of the boat. With the code as it is, I worry about lag if the target changes frequently and rapidly especially with the 25ms lag at the end of the loop. I guess I won't know until I bench test it!

Thanks again for the creative reworking of the existing code. I'm stoked to try this out and I feel like I just learned a ton from your code.

iceabenezer:
Would a while loop to check for changes in bucketTarget be a better approach?

Two things you cannot use in "real time" code: "delay()" and "while". Nothing must delay the progress of the main loop() through the various checks and actions.

iceabenezer:
With the code as it is, I worry about lag if the target changes frequently and rapidly especially with the 25ms lag at the end of the loop. I guess I won't know until I bench test it!

Haven't looked at your code (until now) but as I mention, you cannot use "delay()", simple as that.

You know more than me about jetboats, so rename the variables as most appropriate, just be consistent. I noticed in your original code that the word throttle was used in the comments.

Using a while loop to iterate the adjustments is not necessary, because loop() itself is iterated. It gets called by a hidden while loop. As your project develops, you may need the code to monitor more inputs and adjust other outputs. Using while loops for the bucket monitoring would block those other "tasks", unless you build extra code into those while loops to continue those other tasks, which would lead you back towards over-complex, repetitive code.

I left the delay() from your original code, mostly to deliberately slow things down a little for debugging. When the Serial.print()s go, the delay() can also go. If you remove them, you will find that the iteration speed will increase dramatically, because right now your code is spending almost all its time doing those delay()s and Serial.print()s, especially at the relatively slow 9600 baud rate.