I would like some advice on good implementation strategies/general practices for C, 8bit micro.

for example i have in one of my project a functions that ramps the speed according to a defined hertz/sec

what would be the best way to reset the accumulator variable, make the accumulator variable global and reset that way, or as i have currently implemented i: i call the function but without a return variable and set a boolean to reset it to my value of choosing.

like this:

case start:
                LED_MOTOR_SetHigh();
                ramp(halfADC,true);
                enablePWM();
                enableCLC();
                chargeBootstrap();
                PIE1bits.TMR2IE = true;
                //while(!softStart());
                mainState = run;
                break;

uint16_t ramp(uint16_t value, bool reset){
    static uint32_t rampAccumulator = fixedFactorHalfADC;
    uint32_t target;
    uint16_t result;
    target = value;
    target <<=8;
    if(reset){
        rampAccumulator = target;
        return 0;
    }
    if(rampAccumulator < target){;
        rampAccumulator += rampAccelerateValue;
        if(rampAccumulator > target){  
            rampAccumulator = target;
        }
    }
    else if(rampAccumulator > target){
        if(rampAccumulator >= rampDecelerateValue){
            rampAccumulator -= rampDecelerateValue;  
        }
        else{
            rampAccumulator = target;
        }
        if(rampAccumulator < target){ 
                rampAccumulator = target;
        }
    }
    if(rampAccumulator > rampAccumulatorMax ){
        rampAccumulator = rampAccumulatorMax ;
    }
    result = rampAccumulator>>8;
    return result; 

For these kinds of variables I will typically define them in a C source file outside of the functions that use them, and create one or more init functions as required to initialize them.

These variables are in some sense global, but are visible only to code in this C source file. They are not visible to code in other source files unless you use an extern declaration.

In a crude way this emulates a C++ object with related variables and methods, relying on C scope rules for encapsulation.

Okay that makes sense!

i would declare it outside the function as:

uint32_t rampAccumulator;

and use it like this?

void setRampAccumulator(uint16_t value){
    uint16_t target = value;
    target <<=8;
    rampAccumulator = target
}

Yes!

You might eliminate the local variable target and explicitly cast:

rampAccumulator = ((uint32_t) value) << 8;

Hi, 

Anytime we put a static variable inside a function, we are creating two issues:

• A possible thread safety issue, if the method/function was to ever be used in threads or ISRs, from multiple contexts
• A unit testing issue. It is difficult (impossible) to reset/refresh the static internal state for testing purposes.

I would recommend moving to a more object oriented style. Doesn't have to be C++, but that is certainly my preference. My examples/changes below are all for C code.

I started with the provided sample code (and added what I thought was missing), which may be found here on godbolt:

https://godbolt.org/z/9rh4Zo

After reviewing the provided sample, I then extracted and defined three functions:

RampContextT RampInit(uint32_t fixedFactorHalfADC, 
                      uint32_t rampAccelerateValue,
                      uint32_t rampDecelerateValue,
                      uint32_t rampAccumulatorMax);

uint16_t RampCalc(RampContextT* context, uint16_t value);

void     RampReset(RampContextT* context, uint16_t value);

These functions now each operate on a RampContext struct which looks like this:

typedef struct RampContext
{
    const uint32_t rampAccelerateValue;
    const uint32_t rampDecelerateValue;
    const uint32_t rampAccumulatorMax;
    uint32_t rampAccumulator;
} RampContextT;

This gives benefits such as:

• We can now Init() and Reset() and any other new functionality that needs to operate on the same "context".
• We can now maintain multiple contexts if needed in the future
• We can now easily reset the state for unit testing or similar purposes
• We now have a more flexible interface to help handle the unknown future a bit better.

Ultimately I ended up with code like this:

https://godbolt.org/z/MYNdfT

Also note that I separated out the 'reset' functionality into its own function. It is better to keep each function as focused as possible on its primary purpose. Easier on our human brains I think!  

I hope that helps!

Best regards,

Matthew

https://covemountainsoftware.com/services/source-code-review/

PS: I didn't analyze the code/algorithm itself and simply tried to preserve it as delivered.

Thanks for this super helpful post!

I have a very poor concept of Object Oriented programming for C.

Do you have any good references to start from scratch with OOP?

You might find Miro Samek’s free e-book helpful. It covers OOP design principles but with a focus on C

'waydan' beat me to it, that is exactly what I was going to refer you to too... my example code would be nicely aligned to Samek's section 1 on Encapsulation.

Good luck!

Matthew