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 10class 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_HSource File:#include BrushPID.h#include ../BrushCommon/BrushCommon.hBrushPID::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/(max(max(this->p,this->d),1)); }// destructorBrushPID::~BrushPID(void) {    //nothing to do     }// next step of discrete PID long BrushPID::getNextOutput(long *newError){    //prevent overflows    if(lMaxTotalError<abs(this->lTotalError)) this->resetTotalError();    if(lMaxError<abs(*newError)) *newError=this->lLastError;    //check if DT has passed, by comparing current to last registered time stamp    while((millis()-this->ulLastTimeStamp)<DT);    //set new timestamp    this->ulLastTimeStamp=millis();    this->lLastError=this->lCurrentError;    this->lCurrentError=*newError;    this->lTotalError+=*newError*DT;    return caclulateNextOutput();}long BrushPID::caclulateNextOutput(void){    return (this->p*this->lCurrentError+this->i*this->lTotalError+this->d*(this->lCurrentError-this->lLastError)/DT)>>LONG_SHIFT;}/* ***** Setting PID Parameter according to  Ziegler-Nicholas tuning Method* Source: http://en.wikipedia.org/wiki/Ziegler%E2%80%93Nichols_method****/void BrushPID::zieglerNicholasTuning(int iTuningMethod=ZIEGLER_P){    unsigned long shifted_constant=KU_CONSTANT<<LONG_SHIFT;    switch(iTuningMethod){        case(ZIEGLER_P):            this->p=shifted_constant>>1;            this->i=0;            this->d=0;            break;        case(ZIEGLER_PI):            this->p=(shifted_constant*22)/10;            this->i=(this->p*10)/12/max(PERIOD,1);            this->d=0;            break;        case(ZIEGLER_CLASSIC_PID):            this->p=(shifted_constant*6)/10;            this->i=2*this->p/max(PERIOD,1);            this->d=(this->p*PERIOD)>>3;            break;         case(ZIEGLER_PESSEN_INTEGAL_RULE):            this->p=(shifted_constant*7)/10;            this->i=(shifted_constant<<1);            this->d=(shifted_constant*15)/100;        default:            this->p=shifted_constant;            this->i=0;            this->d=0;    }}void BrushPID::resetTotalError(){    this->lTotalError=0;}
PID Controller library
c++;library;arduino
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 ideaTuning 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 thesource 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 aroundoperators (=, <, << etc) and after keywords (if, while, etc), and because ofsingle-line expressions (better to use braces and multi-line). The functioncaclulateNextOutput is unreadable without adding spaces.The default tuning type should perhaps be a parameter (with default) to theconstructor.lMaxError, lMaxTotalError seem to be constants.  And why useMAX_SIGNED_LONG when LONG_MAX is available from limits.hAccording to the Wiki reference, ZEIGLER_PI 'p' value should be (* 10 / 22),the ZIEGLER_PESSEN_INTEGAL_RULE 'i' value looks wrong and theZIEGLER_PESSEN_INTEGAL_RULE 'd' value has a hard-coded time constant (100)In zieglerNicholasTuning, I'd call shifted_constant just ku and makeit const.  But then again,... in the BrushPID constructor, 'p' isinitialised to KU_CONSTANT whereas in zieglerNicholasTuning, KU_CONSTANTis shifted by LONG_SHIFT.  This seems wrong.  The initial, wrong, value isunused, but to avoid this sort of error, I would probably define KU_CONSTANTas (2 << (LONG_SHIFT)) in the headerLONG_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 thatneeds CPU time, so this is probably ok.  But don't do it at interruptlevel.  In general, when there is an alternative, this sort of loop isbest avoided.