3
\$\begingroup\$

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/(max(max(this->p,this->d),1)); 
}

// destructor
BrushPID::~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;
}
\$\endgroup\$
3
  • \$\begingroup\$ I personally find all this this-> business visually distracting. If you're coding in an IDE that can't highlight class members in a different color, I find that a simple leading underscore (naming convention) is enough to call out member data. I know this is a somewhat religious point, though. \$\endgroup\$
    – Nate
    Commented May 24, 2013 at 3:15
  • \$\begingroup\$ @Nate Agreed, except prefer trailing underscores, not leading underscores. Each name that contains a double underscore __ or begins with an underscore followed by an uppercase letter is reserved to the implementation for any use. - It's easy to forget this and get it wrong (I have a number of times in recent memory). Basically, use a simple rule: no __ anywhere, and don't start anything with _. \$\endgroup\$
    – Yuushi
    Commented May 24, 2013 at 4:31
  • \$\begingroup\$ @Yuushi, I find it extremely difficult to get that wrong. Typing an underscore is already relatively difficult. Typing two in a row is a mistake I've never made in about 17 years of coding. You shouldn't be referencing "the implementation's" variables anyway (the ones that start with __), so there should be no confusion. If your code had a mixture of variables starting with __, and _, then there would be a source of confusion. But, you should only have ones starting with _ (single) in application layer code. Systems programmers would have different coding standards. \$\endgroup\$
    – Nate
    Commented May 24, 2013 at 5:09

1 Answer 1

3
\$\begingroup\$

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 (=, <, << etc) and after keywords (if, while, etc), and because of single-line expressions (better to use braces and multi-line). The function caclulateNextOutput is unreadable without adding spaces.

  • 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)) in the header

  • 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.

\$\endgroup\$
1
  • \$\begingroup\$ we were taught Hungarian at uni as a uni convention, I guess you shouldn't believe everything you're taught. Thanks for the feedback, I've taken note and adjusted the library \$\endgroup\$ Commented May 25, 2013 at 13:09

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.