Creating Servo class broken? or missing something

ive got a sketch written to control a servo with 2 buttons , works good.
here it is

#include <Servo.h>


const int  buttonServoInput1 = 4;            // Create Switch and assign to pin
const int  buttonServoInput2 = 3;            // Create Switch and assign to pin
const int  buttonServoOutput = 6;             // Create Servo digital output pin and assign
unsigned long previousbuttonServoMillis = 0;
int buttonServoInterval = 5;
int angle = 90;
int moveDeg = 1;
Servo buttonServo;

unsigned long currentMillis;

void setup() {
  pinMode(buttonServoInput1, INPUT_PULLUP);          // initialize the pins
  pinMode(buttonServoInput2, INPUT_PULLUP);          // initialize the pins

  buttonServo.attach(buttonServoOutput);             // attaches the servo on pin to the servo object
 
}

void loop() {
  currentMillis = millis();
  controlbuttonServo();

}
void controlbuttonServo()
{
  if (currentMillis - previousbuttonServoMillis >= buttonServoInterval) {
    if ( digitalRead(buttonServoInput1) == LOW) {  //reads pin contact to ground. button push plus+ increment
      angle = angle + moveDeg;               //changes servo angle by increment until button released.
    }
    if ( digitalRead(buttonServoInput2) == LOW) { //reads pin contact to ground. button push Negetive- increment
      angle = angle - moveDeg;              //changes servo angle by increment until button released.
    }
    angle = constrain(angle, 0, 180);
    buttonServo.write(angle);
    previousbuttonServoMillis = currentMillis;
  }
} //end read

Ive been trying to create a servo class with the above sketch
it compiles and powers the servo but i get no response from the buttons.
im not seeing where the problems at, without errors its hard to guess

#include <Servo.h>

class ButtonServo
{

    int  buttonServoInput1;            // Create Switch and assign to pin
    int  buttonServoInput2;            // Create Switch and assign to pin
    int  buttonServoOutput;             // Create Servo digital output pin and assign
    unsigned long previousbuttonServoMillis;
    int buttonServoInterval;
    int angle;
    int moveDeg;
    Servo buttonServo;
    unsigned long currentMillis;
    
  public:
    ButtonServo(int interval)
    {
      buttonServoInterval = interval;
      pinMode(buttonServoInput1, INPUT_PULLUP);
      pinMode(buttonServoInput2, INPUT_PULLUP);
      pinMode(buttonServoOutput, OUTPUT);
    }
    void Attach(int buttonServoOutput, int buttonServoInput1, int buttonServoInput2)
    {
      buttonServo.attach(buttonServoOutput, buttonServoInput1, buttonServoInput2);
    }

    void Update()
    {
      unsigned long currentMillis = millis();
      //previousbuttonServoMillis;
      angle = 90;
      moveDeg = 1;

      if (currentMillis - previousbuttonServoMillis >= buttonServoInterval) {
        if ( digitalRead(buttonServoInput1) == LOW) {  //reads pin contact to ground. button push plus+ increment
          angle = angle + moveDeg;               //changes servo angle by increment until button released.
        }
        if ( digitalRead(buttonServoInput2) == LOW) { //reads pin contact to ground. button push Negetive- increment
          angle = angle - moveDeg;              //changes servo angle by increment until button released.
        }
        angle = constrain(angle, 0, 180);
        buttonServo.write(angle);
        previousbuttonServoMillis = currentMillis;
      }
    }
};

ButtonServo buttonServo1(10);
//ButtonServo buttonServo2(10);

void setup()
{
  buttonServo1.Attach(6, 3, 4);
  //buttonServo1.Attach(7, 9, 8);
}
void loop()
{
  buttonServo1.Update();
  //buttonServo1.Update();

}
      pinMode(buttonServoInput1, INPUT_PULLUP);
      pinMode(buttonServoInput2, INPUT_PULLUP);
      pinMode(buttonServoOutput, OUTPUT);

I don't think these lines in the constructor are going to work since you don't assign values to the various pin numbers until you call .Attach(). Move them to .Attach(). Also, since the "buttonServoOutput" pin is used with Servo.attach() you don't need to set the mode. The Servo library will do that.

      buttonServo.attach(buttonServoOutput, buttonServoInput1, buttonServoInput2);

Servo.attach() doesn't take three pin numbers! The first pin number is fine but the other two numbers are being used for... THE MIN AND MAX PULSE WIDTH IN MICROSECONDS. The defaults are 544 and 2400, WAY different than your pin number values.

Things like pinMode don't belong in the constructor anyway. Constructors often happen before init() gets called to set up the hardware and things like pinMode or digitalWrite or anything else that has to do with the hardware on the chip can have weird effects or just not work at all.

The only thing that should be in the constructor are variable initializations and stuff like that. Put the hardware stuff in a .begin or .init method that you call from setup. Or in your case it could go in the attach method.

so changes to pinMode location as below, into Attach with same results as before. servo active and sets it first position to 90deg then nothing.

now im not sure how to attach the input control pins ?
i have maybe 14 to 20 days into learning arduino,
half of those days were maybe 4 years ago.

im trying to setup a robot arm with multiple servos
some with button control, move while held and stop and hold position when released, the grabber/claw the rotating base
others with pots or a joystick/ pc flight controller.
posted original code here: 8 Servos one UNO Robot Arm - Project Guidance - Arduino Forum
and a piece above

if : buttonServo.attach(buttonServoOutput); only attaches (one pin) the output, "kinda pointless"
how are the controls button1 and 2 attached to this instance of servo in the class
or a pot for that mater
ive never seen any tutorials or examples where a servo was used in a class in this manner.
only thing im finding in servo are:
attach KEYWORD2
detach KEYWORD2
write KEYWORD2
read KEYWORD2
attached KEYWORD2 ______________ havnt looked into yet
writeMicroseconds KEYWORD2
readMicroseconds KEYWORD2

Quite frankly im not sure why this basic function isnt already built into the library.
Michael Margolis,, I got the library update today,, needs a second update :slight_smile:

  public:
    ButtonServo(int interval)
    {
      buttonServoInterval = interval;

    }
    void Attach(int buttonServoOutput) 
 {
      buttonServo.attach(buttonServoOutput);
      pinMode(buttonServoInput1, INPUT_PULLUP);
      pinMode(buttonServoInput2, INPUT_PULLUP);
     
    }

    void Update()

Nothing is connected to the servo class except the pin used by the servo. And if you want to move the servo you call it with a position.

If you want to have manually move it with buttons you have to code that. Why it's not build in? Because it's not all that common to do so :slight_smile: So if you want to use it more often it's perfectly fine to make a library around it :slight_smile: And if you want to keep the original interface to the Servo library, extend it (make your library a child of it), and not use an object in it.

void Attach(int buttonServoOutput)
{
  buttonServo.attach(buttonServoOutput);
  pinMode(buttonServoInput1, INPUT_PULLUP);
  pinMode(buttonServoInput2, INPUT_PULLUP);
}

I don't see a place where you assign values to buttonServoInput1 and buttonServoInput2. You even took those two arguments off ButtonServo::Attach() so there is no way to pass in the values.

If those member variables never get set, that's going to cause problems.

Im a hardware guy, need a rig to drill an 8 inch hole 300 feet into the earth im you guy need a hobby lath converted into a cnc turning machine no problem, need an auto transmission rebuilt no problem.
spending the next few weeks learning all the intricacies of the language to build a library not going to happen.
i just thought it would be nice to clean up some code and leave something others could use.

what i had works, simple, cut and paste change a few digits done.
if any body wants to run with it be my guest.

Johnwasser
I don't see a place where you assign values to buttonServoInput1 and buttonServoInput2. You even took those two arguments off ButtonServo::Attach() so there is no way to pass in the values.

Yep.
that is what i was talking about, i couldn't figure out how to pass the control values to the servo either.
if the only way was to run it through setup(); kinda defeats the purpose of a class to control a servo.

if it would do , buttonServo1.Attach(Output, input1, input2); and potServo1.Attach(output, Input);
this conversation wouldn't have gotten this far.

using buttons and pots to physically move servos should be part of the main library for arduino
there is a reason the visual ide's like XOD and Scratch are growing.
hardware is easily available to start projects but a lot of people get stuck dealing with sometimes overly complex code to do a simple thing like call a class plug in a few pins compile , upload and go.
arduino can be a great tool for learning code BUT, if to many people find the code to difficult to get basic hardware to run and work, its going to stop the forward progress of far too many.

is it is i spent a bit over a week, getting all the tidbits together to operate a simple 8 servo robot arm without delays and a stepper driver for a slide rail.

for some they just need to get some hardware going.

Thanks for all the help..

spending the next few weeks learning all the intricacies of the language to build a library not going to happen.
i just thought it would be nice to clean up some code and leave something others could use.

Please just don’t if that’s your attitude. We have enough crappy code written by people who didn’t want to learn to do it right as it is. If you want to share then learn to write good code. If you want to write crap code then please keep it for yourself. Just saying.