2
\$\begingroup\$

The code in question is in GitHub here. arm.h, armconstants.h and arm.cpp. Here are the key passages:

arm.h

class Arm {

public:
    Arm();
    void setup(ros::NodeHandle& nh);
    String getState();
    void armCommand(String command);
    void armCommand(String command, float arg);
    void loop();

private:
    Adafruit_PWMServoDriver ARM;
    float iteration_time;
    float iteration_interval;

    int destination_shoulder;
    int destination_wrist;
    int destination_elbow;

    float wristposition;
    float elbowposition;
    float shoulderposition;

    float shoulderDelta = 0;
    float elbowDelta = 0;
    float wristDelta = 0;


    float currentClaw = CLAWPARKDEG;
    float currentWrist = WRISTPARKDEG;
    float currentElbow = ELBOWPARKDEG;
    float currentShoulder = SHOULDERPARKDEG;

    float iterations = 0;

    String state;
    ros::NodeHandle* nodeHandle;

    void elbow(float deg);
    void shoulder(float deg);
    void wrist(float deg);
    void elbow(int deg);
    void shoulder(int deg);
    void wrist(int deg);

    void openClaw();
    void closeClaw();
    void calculateIterationDeltas();
    int move();

    void traceOut(String msg);
};

arm.cpp

    void Arm::elbow(float deg)
{
    int deglen = (deg + EL_DEGOFFSET) * EL_DEGSCALE; // pulselen of commanded degrees
    ARM.setPWM(ELBOW, 0, deglen);
}

void Arm::shoulder(float deg)
{
    int deglen = (deg + SH_DEGOFFSET) * SH_DEGSCALE; // pulselen of commanded degrees
    ARM.setPWM(SHOULDER, 0, deglen);
}

void Arm::wrist(float deg)
{
    int deglen = (deg + WR_DEGOFFSET) * WR_DEGSCALE; // pulselen of commanded degrees
    ARM.setPWM(WRIST, 0, deglen);
}

    void Arm::armCommand(String command)
{
    if (command == "park")
    {
        destination_shoulder = SHOULDERPARKDEG;
        destination_wrist = WRISTPARKDEG;
        destination_elbow = ELBOWPARKDEG;
        calculateIterationDeltas();
        state = "move";
        return;
    }
    if (command == "floor")
    {
        destination_shoulder = SHOULDERFLOORDEG;
        destination_wrist = WRISTFLOORDEG;
        destination_elbow = ELBOWFLOORDEG;
        calculateIterationDeltas();
        state = "move";
    }
}

The code is for the control of a robot arm with four servos. It runs on a teensy which is just like an Arduino. Each of the robots have a variety of parameters which are different. Also the API allows access to one servo at a time or sometimes all of them at once. The result is code that is somewhat repetitive in places.

I am open to any and all suggestions about C++ usage. But in particular for ideas about how to remove the repetition. I don't want to build an elaborate structure just for elegance sake. But I think the repetition opens things up for bugs, as well as maintenance headaches.

One idea is to treat each of the four servos with four structs containing all the key constants, give each servo a number. e.g. 0, 1, 2, 3 and create an array of four elements pointing to those structs.

Another idea is to create am inheritance structure for servos. Or to use function pointers.

It all feels a bit overkill. But what do you think?

\$\endgroup\$
1
  • \$\begingroup\$ Ahoy! I changed the title so that it describes what the code does per site goals: "State what your code does in your title, not your main concerns about it.". Feel free to edit and give it a different title if there is something more appropriate. \$\endgroup\$ Jan 22 at 23:47

2 Answers 2

2
\$\begingroup\$

arm.h, armconstants.h and arm.cpp.

I find it amusing (and ambiguous) that these are unrelated to the ARM processor architecture.

reference hardware

I am sad that you didn't tell me what part number(s) you bought, or how you made your robot arm. I am especially sad that you didn't suggest a particular software construct as a faithful, or at least good-ish, model of the targeted hardware. I guess we can only test in the hardware lab? And not by spinning up a ROS VM with a simulator?

conventional notation

    int destination_wrist;
    ...
    float wristposition;
    ...
    float wristDelta = 0;
    ...
    float currentWrist = WRISTPARKDEG;

Oh_my_god! Pickaconventionalready. ItDoesntMatterWhichOne.

Though giving us a clue_about wordSeparators would certainly be welcome advice.

    void wrist(float deg);
    ...
    void wrist(int deg);

I'm skeptical that the compiler needs that much hand holding. I would expect it to just coerce integer --> float. (Aaannndddd, it looks like we have a convention of preferring float to double, ok.)

elbow movement

#define EL_DEGOFFSET 43
    void Arm::elbow(float deg)
{
    int deglen = (deg + EL_DEGOFFSET) * EL_DEGSCALE; // pulselen of commanded degrees
    ARM.setPWM(ELBOW, 0, deglen);
}

I am not pleased.

Sooo, we have {EL, SH, WR} prefixes as abbreviations for {ELbow, SHoulder, WRist}, I suppose that's straightforward enough. But what do any of those numbers even mean?

// elbow pulse length  = (degrees + 65.3)*2.83
// elbow degrees = pulselenth /2.83 - 65.3
#define ELBOWPARK 340        // parallel to ground old 340
#define ELBOWVERTICAL 340    // parallel to shoulder old 340
...
#define ELBOWPARKDEG 90

We're doing pulse width modulation, but are we commanding motors ("spin for 100 msec!"), or servos ("spin to vertical!") ? "Parallel to ground" sounds more like "spin to horizontal!" to me. You didn't tell us about the platform, whether we have optical position encoders, or really anything about the physical setup. Perhaps the phrase "commanded degrees" was supposed to tip me off that it's a servo, being commanded to absolute rather than relative degrees.

Parking "parallel to ground" sounds like a helpful comment, until I realize it is identical to "parallel to shoulder", which sounds ambiguous.

No idea what units "old 340" might have.

It's clear we have a 65.3° offset. But then units on that 2.83 conversion factor would have been useful.

The identifier you chose, (roughly) degree_length, does not seem helpful.

#define EL_DEGSCALE 2.5
...
#define SH_DEGSCALE 1.77
...
#define WR_DEGSCALE 1.77

Yeah, I'm sure there's something useful going on there. I just don't know what it is, and the source code chooses not to tell me. Maybe "scale" implies a "stepper motor pulses per degree" conversion factor?

missing parameters

void Arm::calculateIterationDeltas()

Maybe this is some convention that I don't use on a daily basis? But it seems like an undesirable signature.

Tell me that you're accepting destination_{shoulder,elbow,wrist} as input parameters. (And list them in that "natural" order, rather than random orders.)

missing internal documentation

The Doxygen (or other) comments for calculateIterationDeltas() are just terrible. Because there aren't any. And you really need some.

You should explain to the Gentle Reader that the whole raison d'être for this function is to schedule simultaneous motion along several axes, with dominant axis dictating total time and other axes moving more slowly so all will finish together.

I am a little surprised that there is no motion planning, so there must be some really big (undocumented!) assumptions about valid plans. Typically we would worry about self-intersection, even when there's no nearby objects to knock over. We wouldn't want to command claw motion that goes through our own body or limb.

language choice

There's no processor-intensive loops in this code, so it's unclear why you chose to go the C++ route, rather than taking advantage of convenient python bindings.


ideas about how to remove the repetition

This codebase is so opaque, that DRY concerns just don't even begin to rise to the surface.

There are some obvious things one could do with Enums for the three or four joints. But given that it's opaque whether the currently implemented functions do the Right Thing, and the OP submission contained no automated tests, I would have no idea if after refactoring our "joint index" functions are doing the Right Thing or not. In a word, this code base is "unmaintainable". We should strive to improve that aspect, prior to any adventurous refactoring efforts.

Start by putting a ReadMe.md at the root directory, or some convenient subdirectory, and go from there.

Recruit someone to collaborate with you on this code. There will be some onboarding overhead. Pay attention to it. Make the new recruit edit the ReadMe, or create a new document, which describes the concerns they experienced and how they overcame them.

Give that document to a another new recruit, and see how effective it is at quickly onboarding them with few questions directed at colleagues.


This codebase possibly achieves some of its design goals, but it is hard to tell to what extent it accomplishes that, in part because the goals are not transparently revealed.

I would not be willing to delegate or accept maintenance tasks on this code.

\$\endgroup\$
2
  • \$\begingroup\$ Some good points, I will make some changes. But this is not done and not meant as a showcase. Of course I am not asking you about maintaining it for me, nor whether I picked the right language. What about my actual question, "But in particular for ideas about how to remove the repetition. I don't want to build an elaborate structure just for elegance sake. But I think the repetition opens things up for bugs, as well as maintenance headaches." Maybe the code is just to awful for you to think about that. \$\endgroup\$
    – pitosalas
    Jan 23 at 1:10
  • \$\begingroup\$ The standard pair of questions, when a feature branch is submitted for PR code review to decide whether it is yet ready for merging to main, are: (1.) is it correct (no obvious bugs), and (2.) is it maintainable? Colleagues participating in a PR discussion are examining whether a person's code artifact is ready to become a team artifact which all team members could maintain (fix bugs, add features). // "too awful to think about", yes, the deficiencies were frankly distracting. Best to fix them first. \$\endgroup\$
    – J_H
    Jan 23 at 1:28
1
\$\begingroup\$

In addition to J_H's remarks:

Separate concerns

Try to follow the principle of separation of concerns. Your class Arm is doing too many things at once: it is both controlling the robot arm, and it is interpreting commands that come from somewhere. Ideally, those things should be split: have class Arm just be in charge of the movement of the robot arm, and have something else interpret the commands and call member functions of Arm to make the arm move.

Create classes for the components of the arm

I don't want to build an elaborate structure just for elegance sake. But I think the repetition opens things up for bugs, as well as maintenance headaches.

Indeed, repetition is something you should strongly avoid. Don't build elaborate structures, just add structures that will reduce repetition. Keep things simple where possible.

I see two types of actuators: things that rotate, like the shoulder, elbow and wrist, and something that can grab things, like the claw. So I would add two classes:

class Rotator {
public:
    void set_destination(float degrees);

private:
    // Any state that this rotator needs to keep track of.
    …
};

class Grabber {
public:
    void open();
    void close();

private:
    …
};    

Then your Arm becomes simply:

class Arm {
public:
    Rotator shoulder;
    Rotator wrist;
    Rotator elbow;
    Grabber claw;

private:
    // Any other data necessary for the whole arm
    …
};

Then you could have a function that processes arm commands that looks like:

void process_command(String command, Arm& arm) {
    if (command == "park") {
        arm.shoulder.set_destination(SHOULDERPARKDEG);
        arm.wrist.set_destination(WRISTPARKDEG);
        arm.elbow.set_destination(ELBOWPARKDEG);
        …
    }
    …
}

Note how you don't need arrays, inheritance or function pointers. It's just a way of grouping related data and functions together.

Create helper functions where necessary

In the command processing function, you also have a lot of code repetition. Here you could just make a simple helper function that sets all the angles and starts the movement:

void start_move(Arm& arm, float destination_shoulder,
                float destination_wrist, float destination_elbow) {
    arm.shoulder.set_destination(destination_shoulder);
    arm.wrist.set_destination(destination_wrist);
    arm.elbow.set_destination(destination_angle);
}

void process_command(String command, Arm& arm) {
    if (command == "park") {
        start_move(arm, SHOULDERPARKDEG, WRISTPARKDEG, ELBOWPARKDEG);
    } else if (command == "floor") {
        start_move(arm, SHOULDERFLOORDEG, WRISTFLOORDEG, ELBOWFLOORDEG);
    }
}

Continue simplifying and reducing duplication.

Once you have improved your code by creating new classes and functions, look at the code again and see if you can do another round of simplifications and code reductions. For example, if you have to pass three angles around a lot, maybe make a struct that holds those angles.

Indeed, simple and non-repetitive code is easier to understand, easier to maintain, and has less risk of containing bugs. It's definitely not overkill to spend time doing this.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.