PID Controller library

I'm trying to implement a PID without floating point operations on a micro controller. I appreciate all feedback I can get.

Header file:

#ifndef BRUSHPID_H
#define BRUSHPID_H

#if defined(ARDUINO) && (ARDUINO >= 100) // Arduino Library
#include "Arduino.h"
#else
#include "WProgram.h"
#endif

#define KU_CONSTANT 2

#define DT  100         //  in milliseconds, descrete time step size chose to be 0.1[s]=100[ms]
#define PERIOD 6000     //  in milliseconds, measured as 6[s]=6000[ms]

// Ziegler Algorithem Constants
#define ZIEGLER_P 0
#define ZIEGLER_PI 1
#define ZIEGLER_CLASSIC_PID 2
#define ZIEGLER_PESSEN_INTEGAL_RULE 3
#define ZIEGLER_SOME_OVERSHOOT 4
#define ZIEGLER_NO_OVERSHOOT 5

// Tuning Type
#define TUNING_TYPE ZIEGLER_P

// bit extension to avoid float operations
#define LONG_SHIFT 10

class BrushPID{
private:
    unsigned long p;
    unsigned long i;
    unsigned long d;
    signed long lCurrentError;
    signed long lLastError;
    signed long lTotalError;
    signed long lMaxError;
    signed long lMaxTotalError;
    signed long caclulateNextOutput(void);
    unsigned long ulLastTimeStamp;
    void zieglerNicholasTuning(signed int iTuningMethod);
    void resetTotalError(void);
public:
    BrushPID(void);
    ~BrushPID(void);
    signed long getNextOutput(signed long *newError);
};

#endif BRUSHPID_H

Source File:

```
#include "BrushPID.h"
#include "../BrushCommon/BrushCommon.h"

BrushPID::BrushPID(void) {

// init local object data
this->lCurrentError=0;
this->lLastError=0;
this->lTotalError=0;
this->ulLastTimeStamp=millis(); // timestamp since arduino boot in milliseconds
this->p=KU_CONSTANT;
this->i=0;
this->d=0;

//init PID paramaters according to chosen tuning type
this->zieglerNicholasTuning(TUNING_TYPE);

// calculate overflow boundry
this->lMaxTotalError=MAX_SIGNED_LONG-MAX_SIGNED_LONG/(max(this->i*DT,1)); // max to prevent division by 0
this->lMaxError=MAX_SIGNED_LONG-MAX_SIGNED_LONG/(ma

I know nothing of PID controllers, so I'll restrict comments to the code itself.

Comments on the header:

-
A #endif doesn't take a tag (BRUSHPID_H)

-
Use of the 'signed' keyword is unusual. Do you have a good reason for its use?

-
Hungarian notation (your variable type-prefixes) is a really bad idea

-
Tuning methods would be better defined as an enum rather than as #defines and a signed int parameter (to zieglerNicholasTuning).

-
OVERSHOOT defines are not used.

-
pass newError into getNextOutput by reference ?

Comments of the source file:

-
It is better to define the include path in the Makefile instead of the
source file. So the Makefile would have something like

-I $(PROJECT)/BrushCommon/

as a part of the compilation rule.

-
Some of the code is hard to read because of a lack of spacing around
operators (=,

-
The default tuning type should perhaps be a parameter (with default) to the
constructor.

-
lMaxError, lMaxTotalError seem to be constants. And why use
MAX_SIGNED_LONG when LONG_MAX is available from limits.h

-
According to the Wiki reference, ZEIGLER_PI 'p' value should be (* 10 / 22),
the ZIEGLER_PESSEN_INTEGAL_RULE 'i' value looks wrong and the
ZIEGLER_PESSEN_INTEGAL_RULE 'd' value has a hard-coded time constant (100)

-
In zieglerNicholasTuning, I'd call shifted_constant just ku and make
it const. But then again,... in the BrushPID constructor, 'p' is
initialised to KU_CONSTANT whereas in zieglerNicholasTuning, KU_CONSTANT
is shifted by LONG_SHIFT. This seems wrong. The initial, wrong, value is
unused, but to avoid this sort of error, I would probably define KU_CONSTANT
as (2

-
LONG_SHIFT is, to me, an unhelpful name.

-
DT better as DELTA_T ?

-
The while loop in getNextOutput` would be better with explicit braces :

while ((millis() - this->ulLastTimeStamp) < DT) {
    // busy-wait
}

I guess you have no other way of waiting and nothing else going on that
needs CPU time, so this is probably ok. But don't do it at interrupt
level. In general, when there is an alternative, this sort of loop is
best avoided.

StackExchange Code Review Q#26551, answer score: 3