4
\$\begingroup\$

Background:

I'm a member of my high school's robotics club (and in charge of the programming team). The robot is going to be written in C++, but I'm mostly a Python coder and want to make sure I don't have any fundamental gaps in my knowledge.

In preparation for the start of the competition in just a few days, I've quickly written up the bare minimum required for a robot to drive around (included below). I was hoping to get feedback and criticism at this stage to nip any problems/misconceptions I have before they get serious.

As previously stated, I'm unfamiliar with C++ so don't know many of its best practices/idioms/etc. What I'm looking for is a bit vague, but in essence, how can I make my code more professional/better? Am I making any errors?

Any feedback would be welcome.

There are three files:

  • MainRobot.h
  • MainRobot.cpp
  • typedefs.h

MainRobot.h

#ifndef MAINROBOT_H_
#define MAINROBOT_H_

// 3rd-party modules
#include "WPILib.h"

// Modules created by us
#include "typedefs.h"

class MainRobot : public SimpleRobot
{
public:
    // Required methods (for robot to run)
    MainRobot(void);
    void Autonomous(void);
    void OperatorControl(void);

    // Objects (provided by WPILib.h)
    RobotDrive robotDrive;
    Joystick *leftStick;
    Joystick *rightStick;
    Timer timer;

    // Motor ports (hardware)
    static const UINT32 kLeftFrontMotor = kPWMPort_1;
    static const UINT32 kRightFrontMotor = kPWMPort_2;
    static const UINT32 kLeftBackMotor = kPWMPort_3;
    static const UINT32 kRightBackMotor = kPWMPort_4;

    // Joystick ports (hardware)
    static const UINT32 kRightJoystickPort = kUSBPort_1;
    static const UINT32 kLeftJoystickPort = kUSBPort_2;

    // Delay to prevent spamming motors with input
    static const float kDelayValue = 0.01;
};

#endif /*MAINROBOT_H_*/

MainRobot.cpp

#include "MainRobot.h"

/**
 * The constructor.
 *
 * Initializes the motors, drivetrain, joysticks, safety devices, etc.
 */
MainRobot::MainRobot(void):
    robotDrive(kLeftFrontMotor,
               kRightFrontMotor,
               kLeftBackMotor,
               kRightBackMotor)
{
    MainRobot::GetWatchdog().SetExpiration(0.1);    // In seconds

    MainRobot::leftStick = new Joystick(kLeftJoystickPort);
    MainRobot::rightStick = new Joystick(kRightJoystickPort);

    return;
}

/**
 * Initializes the autonomous period when called
 * (robot performs a task without human intervention)
 * 
 * Currently left blank.
 */
void MainRobot::Autonomous(void) {
    while(MainRobot::IsAutonomous()) {
        MainRobot::GetWatchdog().Feed();
        Wait(kDelayValue);
        // Do nothing.
    }
    return;
}

/**
 * Initializes the teleoperated period when called.
 * (robot is driven by humans using two joysticks)
 */
void MainRobot::OperatorControl(void) {
    MainRobot::GetWatchdog().SetEnabled(true);

    MainRobot::timer.Reset();
    MainRobot::timer.Start();

    while(MainRobot::IsOperatorControl()) {
        MainRobot::robotDrive.TankDrive(MainRobot::leftStick, 
                                        MainRobot::rightStick);
        MainRobot::GetWatchdog().Feed();
        Wait(kDelayValue);
    }
    return;
}

typedefs.h

#ifndef TYPE_DEFS_H_
#define TYPE_DEFS_H_

// The available ports for any motors
typedef enum {
    kPWMPort_1 = 1,
    kPWMPort_2 = 2,
    kPWMPort_3 = 3,
    kPWMPort_4 = 4,
    kPWMPort_5 = 5,
    kPWMPort_6 = 6,
    kPWMPort_7 = 7,
    kPWMPort_8 = 8,
    kPWMPort_9 = 9,
    kPWMPort_10 = 10
} PWMPorts;

// The available ports for USB drives (for joysticks, etc)
typedef enum {
    kUSBPort_1 = 1,
    kUSBPort_2 = 2,
    kUSBPort_3 = 3,
    kUSBPort_4 = 4
} USBPorts;


#endif
\$\endgroup\$
3
\$\begingroup\$

Do you plan to replace the Joystick object?

Joystick *leftStick;
Joystick *rightStick;

If not then declare them as

Joystick  leftStick;
Joystick  rightStick;

If you are going to replace them then you should be using smart pointers rather than a RAW pointer (if you want to do it manually then learn the rule of three) But I would go with smart pointers (but that is a second option to normal objects).

If you must have pointer use smart pointers:

std::unique_ptr<Joystick>  leftStick;
std::unique_ptr<Joystick>  rightStick;

There is no need declare these as static (unless you want to get the address or something) so just use enum:

// Motor ports (hardware)
static const UINT32 kLeftFrontMotor = kPWMPort_1;
static const UINT32 kRightFrontMotor = kPWMPort_2;
static const UINT32 kLeftBackMotor = kPWMPort_3;
static const UINT32 kRightBackMotor = kPWMPort_4;

Prefer:

enum { kLeftFrontMotor = kPWMPort_1, kRightFrontMotor = kPWMPort_2, kLeftBackMotor = kPWMPort_3, kRightBackMotor = kPWMPort_4};

No need to explicitly return from a constructor:

MainRobot::MainRobot(void)
{
    ...

    return;  // No need
}

There is nothing wrong with prefixing functions with the class name but personally I would not.

while(MainRobot::IsAutonomous()) {
    MainRobot::GetWatchdog().Feed();
    Wait(kDelayValue);
    // Do nothing.
}

Easier to write:

while(IsAutonomous())
{
    GetWatchdog().Feed();
    Wait(kDelayValue);
    // Do nothing.
}

The file name "typedefs.h" is relatively common. You may want to avoid it.

\$\endgroup\$
1
\$\begingroup\$

If this is C++, you do not need to typedef enums; enum USBPorts { /* values */ }; will do. There's also no need to place return; at the end of a function that returns void, or void in the parameter list of functions that take no parameters.

As Loki Astari remarked, you should not new the joysticks unless you have a good reason to; even if you do, though, and don't use smart pointers, you should make a destructor that deletes the two pointers, and should either implement or make private the copy-constructor and assignment operator.

\$\endgroup\$
1
\$\begingroup\$

It's better style, whenever possible, to initialize members like leftStick and rightStick in the constructor's comma-separated initializer list before the body of the constructor vs. assigning inside the constructor.

If you decide to switch to Joystick objects rather than Joystick pointers, this may be anywhere from slightly more efficient to the only legal option, depending on what Joystick defines for constructor(s) and operators.

If you do make that change, I don't know how your compiler will express it's requirement for pointers in the call to:

    robotDrive.TankDrive(&leftStick, &rightStick);

but it will almost certainly complain somehow, and the syntax shown here will be what it wants.

And, yes, really, you don't want to habitually use prefixes like 'MainRobot::'. Such prefixing is usually optional and meaningless, except in a few cases:

  • It associates member definitions (usually functions, but sometimes static data members) with their defining class, so you have to use it there. This is the only one of these cases that applies to your code above.

  • When a class is being used as a namespace-like container for types and enums and statics AND these definitions need to be referenced from OUTSIDE the class' methods, they must be prefixed like this.

  • Important! when such a prefix is applied to an object method (virtual member function) call, it means "call the method non-virtually" which is not what you normally want or expect. This is normally used to "chain up" from a member function refinement to its counterpart in a base class -- rather than infinitely recurse. Python has a similar idiom for this:

    BaseClassName.methodname(self, arguments) 
    

to make a non-virtual call to the specified class' implementation in place of the normal virtual-by-default-because-it's-Python syntax:

    self.methodname(arguments)

Finally, you might want to take better advantage of the implementation hiding features of C++.

I was planning to advise you to declare "private:" as much of your class content as possible, but I've reconsidered. I think that the best way to hide the implementation details of the MainRobot class may be to move as much of the class content as possible into the .cpp file. You might be able to move the whole class and eliminate the .h file.

At a minimum, I don't see any benefit (though you might be able to anticipate benefits as your implementation grows) for the static class members (or enums) to be defined inside the class vs. module-scoped static non-class-members at the top of the MainRobot.cpp -- unless they are needed in other .cpp files.

There must be some entry point that creates one or more MainRobot objects, and the details of how this works is likely to have been determined by the WPILib / SimpleRobot framework providers -- do you get to define "main" or some special initialization callback function? If you can implement any such hooks within MainRobot.cpp, there may not be sufficient justification for a separate header file. If you need to implement helper objects for MainRobot, it may make more sense to have MainRobot call out to them through the methods in THEIR header files rather than have them call into MainRobot. There's an attractive aspect to having each of them focus on a slice of the problem and not need visibility to the "bigger picture" represented by MainRobot.

Once you move a class completely into its .cpp file, so long as you reasonably limit the content of that .cpp file to class members and possibly a minimal "main" function, there's not a lot of point to the public/private distinction -- the callers are restricted by the strictly local scope of the class definition.

If this is too radical, then certainly fall back to using "private:" with as many class members as you can -- any that aren't needed by the MainRobot creation code path or by MainRobot helper objects.

The desire to have a MainRobot-based class hierarchy would be another reason to keep the .h separate. In that case, you might want to use "protected" instead of private but only for the minimum content that the derived classes need to call directly. You should still consider hiding as much implementation detail as possible in the .cpp

You might consider using protected or private inheritance depending on whether anything outside of MainRobot needs to know that a MainRobot is a SimpleRobot. That's a useful idiom if not a very popular one.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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