Skip to content

00.3 Coding Conventions

rkeller1 edited this page May 20, 2021 · 18 revisions

Setup STM32CubeIDE

Import the code formatter from Filr: .\Filr\Net Folders\MV BFFT\Templates\CodingStyleTemplate_STM32CubeIDE\BFFT_VCU_CodingStyleTemplateSTMIDE.xml

How to:

  • Start STM32CubeIDE
  • In the ribbon is a tab called Window
  • Select Preferences in this tab.
  • Navigate to C / C ++ -> Code Style -> Formatter in the opened window
  • Select import and choose the template from Filr

Currently not to take in account, causes many errors which are from third party source code like HAL and FreeRTOS

Warnings must be treated like errors.

How to:

  • Right click the solution in the project explorer
  • Select "Properties" (last element)
  • Navigate to C/C++ Build->Settings
  • On the right side open the tab "Tool Settings"
  • In MCU GCC/G++ Compiler select warnings and check the third entry (Treat warnings as errors(-Werror))

How to work with GitHub

  • Create a branch for every task (Trellocard) you are working on. If you need more than one branch for your task, atomize your trellocard
  • Commit your changes to the branch (Use summary and description for documentation f.e. function, intention ...)
  • The commit description should contain the link to the Trellocard (Trellocard --> Share --> Copy/Paste link [f.e. https://trello.com/c/2n955Eap])
  • During respectively after working on the branch publish it to GitHub and create a Pull-Request
  • Invite reviewers
  • It might also be useful to write in the software_dev channel that a task has been completed and whether someone could take a look
  • As a reviewer you should verify that everything should be working as expected and follows the coding conventions
  • Is something mandatory missing
  • Some names are not self explanatory
  • Just be critical and discusse
  • Write comments to specific lines that might cause problems
  • If everything is okay approve the Pull-Request that it can be merged with the master

General coding conventions

Source code has to be written in english. This applies, for examples, comments, variables and function names.

// User has to enter the max. value
int32_t s32MaxUserValue;
// Check against limits
bool bIsMaxValue(int32_t s32EnteredValue);

Do not use internet idioms without citation and date of access. Be skeptical! Idioms are often prone to errors.

// bad example
int8_t s8Val = -42;
s8Val  = (s8Val^(s8Val>>8u))-(s8Val>>8u);

// Problems:
// Source? --> Documentation is missing
// What is the function? --> GetABS
// What happens for signed char minimum (-128)? --> -128
// Shift of signed variables? --> behaviour depends on the compiler
// What happens for int16_t, int32_t? Undefined, but definitely not the expected operation

// okay
#define BUFFER_SIZE 15u

int main() 
{
   uint8_t u8Name[BUFFER_SIZE];
   
   // Source (https://www.educative.io/edpresso/how-to-write-regular-expressions-in-c), Date of access 25/04/2021
   // regex -- accepts only letters as an input argument for scanf
   scanf("%[a-zA-Z]", u8Name);
   printf("%s", u8Name);
   return 0;
}

Rather use a self explanatory name than a comment which describes the use/function of the variable/function

// incorrect
int32_t s32Foo(const uint8_t* const cpu8Data); // Send data via CAN bus

// correct
int32_t s32CAN_SendData(const uint8_t* const cpu8Data); 

Formal parameters, that are only used for reading within a function, are to be passed with the keyword const. Use const as often as possible.

bool bTime_IsEqual(const psTime_t const cpsTime, 
    const uint32_t cu32TimeStamp /*, ...*/);  

If possible const variables should be used instead of defines. Of course this cannot be applyed to specify the array size, a define should be used there instead.

// incorrect
#define CAN_TX_ADDRESS 123u
uint8_t au8CAN_RX_BUFFER[8u];

// correct
#define CAN_RX_BUFFER_SIZE 8u
const uint32_t cu32CANTxAddress = 123u;  
uint8_t au8CAN_RX_BUFFER[CAN_RX_BUFFER_SIZE];

Do not use blocking programming outside a thread, only exception is a blocking programming with timeout.

// incorrect
while(!bIsButtonPressed())
{
   //...
}

// okay 
const uint32_t cu32TicksUntilTimeout= 0xFFFu; 
const uint32_t cu32TicksTimeout= HAL_GetTick() + cu32TicksUntilTimeout; 

while((cu32TicksTimeout > HAL_GetTick()) && (false == bIsButtonPressed()))
{
   //...
}

If possible, use references instead of pointers. (f.e. collections are not usable with references)

// not recommended
void vCAN_Test(const CANFrame* const cpCANFrame);

// recommended
void vCAN_Test(const CANFrame& crCANFrame);

Classes, interfaces and polymorphism

Interfaces should have the prefix I

class ICANFrame
{
public: 
   virtual ~ICANFrame() = default; 
   virtual uint32_t u32GetID() = 0; 
};

Whenever a virtual method is used (virtual as well as pure virtual) a virtual destructor has to be implemented

// not okay
class ICANFrame
{
private: 
   uint32_t _u32FrameID; 
public: 
   ~ICANFrame();
   virtual void vSetID(const uint32_t cu32ID);
   virtual uint32_t u32GetID() = 0;
};

// okay
class ICANFrame
{
private: 
   uint32_t _u32FrameID; 
public: 
   virtual ~ICANFrame();
   virtual void vSetID(const uint32_t cu32ID);
   virtual uint32_t u32GetID() = 0;
};

The keyword override has to be used when overwritting a derived (pure) virtual method

// not okay
class CANFrame : public ICANFrame
{
private: 
   uint32_t _u32FrameID; 
public: 
   ~CANFrame();
   void vSetID(const uint32_t cu32ID);
   uint32_t u32GetID();
};

// okay
class CANFrame : public ICANFrame
{
private: 
   uint32_t _u32FrameID; 
public: 
   ~CANFrame();
   void vSetID(const uint32_t cu32ID) override;
   uint32_t u32GetID() override;
};

private member names should begin with an underscore

class CANFrame
{
private: 
   uint32_t _u32ID; 
   bool _bIsIDValid();
};

Implicite inline is prohibited. For each class a .hpp/.h and *.cpp has to be implemented. Only exception are template classes

// not okay 
class CANFrame
{
private: 
   uint32_t _u32ID; 
public: 
   uint32_t u32GetID()
   {
      return _u32ID;
   }
};

// okay 
class CANFrame
{
private: 
   uint32_t _u32ID; 
public: 
   uint32_t u32GetID(); // implemented in *.cpp
};

template<typename T>
class CANFrame
{
private: 
   T _xID;
public: 
   const T& xGetID()
   {
      return _xID; 
   }
};

The keyword auto should be used like other datatypes. This means, if you expect a pointer as datatype you should use auto*, if you expect a reference you should use auto&.

auto xValue = 10u; 
auto* pxValue = &xValue; 
auto** ppxValue = &pxValue; 
auto& rxValue = xValue; 

Code format

The opening { and closing } curly bracket always begin in a new line.

// incorrect
void vSayHello(void){
    std::cout << "Hello World!" << std::endl;
}  

// correct
void vSayHello(void)
{
    std::cout << "Hello World!" << std::endl;
} 

Only one variable/constant should be declared/defined per line.

// incorrect
uint32_t u32Val1 = 0u, u32Val2 = 0u; 

// correct
uint32_t u32Val1 = 0u; 
uint32_t u32Val2 = 0u; 

Global variables

Global project variables must be defined in a *.c file (f.e. globals.c). Rather use modul global variables with setter and getter methods for better debuggability. The keyword volatile has to be used on both (global and modul global) variables to prevent from optimization.

//globals.c
volatile uint32_t u32CycleCounter; 

//better
volatile static uint32_t u32CycleCounter; 

uint32_t u32GetCycleCounter(void)
{
   return u32CylceCounter; 
}

void u32SetCycleCounter(const uint32_t cu32CycleCounter)
{
    u32CycleCounter = cu32CycleCounter;
}

The use of variables with global project or module validity should be minimized. The key word to prevent unwanted optimizations is volatile.

// may be optimized
uint32_t u32CycleCounter; 

// correct
volatile uint32_t u32ActiveThread; 
volatile static uint32_t u32CurrentBuffer; 

Logical expressions

Every logical expression must be bracketed. Implicit assumption of operator priorities should be avoided.

// incorrect
if(NULL != pu8Data && NULL != pu8Buffer)   
{
    //...
}
 
// correct
if((NULL != pu8Data) && (NULL != pu8Buffer))   
{
    //...
}

Inequality or equality are to be checked in such a way that the constant is on the left side of the operator.

//correct
if(NULL != pu8Data)
if(nullptr != pu8Data)
if(0u != u32ElementSize)

There must be no implicit knowledge of the operator priorities. Expressions with different operators have to be bracketed.

// okay 
if(bIsPressed && (u8Count > 10u))
if((true == bIsPressed) && (u8Count > 10u))

// not okay
if(true == bIsPressed && u8Count > 10u)

Signed and unsigned variables/constants are not allowed to mix in a statement/condition.

// incorrect
if(s32Val < u32Val)
{
    //...
}

Naming conventions

The short identifier should be used as a prefix for variable names.

int unsigned float other
int8_t s8 uint8_t u8 f32_t f32 bool b
int16_t s16 uint16_t u16 f64_t f64 unknown x
int32_t s32 uint32_t u32 f96_t f96
int64_t s64 uint64_t u64
int8_t s8Val = 0; 
int32_t s32Val = 0; 
uint8_t u8Val = 0u; 
uint32_t u32Val = 0u; 
f32_t f32Val = 0.0f; 
bool bIsInitialized = false;

A c must be placed in front of constants. For pointers is p and for arrays is to put an a in front of the prefix. A void pointer is a pv as a complete prefix to be placed in front. For references a r must be placed in front.

Keyword/Type prefix
const c
pointers p
arrays a
void pointers pv
references r
const uint32_t cu32Val = 314u; 
uint8_t* pu8Data = NULL; 
void* pvGeneric = NULL; 
uint8_t au8Buffer[BUFFER_SIZE]; 
const uint32_t& cru32Val = cu32Val; 

long constants are with a L, unsigned constants with an U and unsigned long constants must be terminated with an UL. This has to be done in order to increase the independence from the compiler.

uint8_t u8State = 0u; 
uint64_t u64MaxVal = 0UL;
int64_t s32MaxVal = 0L;

Variables of complex data types enum, union, struct, bitfield are named with a prefix:

Complex data type prefix
enum (class) e
union u
struct s
bitfields bf
enum States eStates = Start; 
union Converter uConverter; 
struct Time sTime; 
struct Bitfield bfRegister; 

// better
eStates_t eStates = Start; 
uConvert_t uConverter; 
sTime_t sTime; 
bfBitfield_t bfRegister; 

Pointers should have a p as the prefix.

uint8_t* pu8Data = &u8Val; 
uint8_t** ppu8Data = &pu8Data; 

Own type definitions (typedef) must be followed by a _t as a postfix.

typedef float f32_t; 
typedef double f64_t; 

Self-explanatory function names are to be used. F.e. the function name broken down as follows: {Short code return value}{module name}_{verb}{other specifying words}. This convention is not applied for class member functions! There is no module name mandatory.

int32_t s32CAN_SendBuffer(const uint8_t* const cpu8Buffer); 

#defines are capitalized.

#define CAN_BUFFER_SIZE 10u 

A typedef should always be used for function pointers.

typedef f64_t* (*pf64_ptf_t)(void); 
//...
pf64_ptf_t xptf = pf64DoStuff;

Sanity

Implicit casts (casts by compiler) during assignments are to be avoided! Use explicit casts to show that it is intended.

int32_t s32Val = u32Val; 
uint16_t u16Val = u32Val; 

// correct
int32_t s32Val = static_cast<int32_t>(u32Val);
uint16_t u16Val = static_cast<uint16_t>(u32Val);

Pointers must be initialized with NULL/nullptr before they are used for the first time. This prevents that the pointer is not assigned. --> sanity checks can be used to increase safety. For auto this is not possible and not mendatory.

// incorrect
uint8_t* pu8Data; 

// correct
uint8_t* pu8Data = NULL; 
uint8_t* pu8Buffer = nullptr; 
auto* au32Val = new uint32_t[MAX_ARRAY_SIZE]; 

After a free/delete, the pointer must be set to NULL/nullptr in order to mark them as invalid.

uint8_t* pu8Buffer = nullptr; 
pu8Buffer = new uint8_t[SIZE]; 
delete[] pu8Buffer; 
pu8Buffer = nullptr; 

auto* pxBuffer = = new uint32_t[SIZE]; 
delete[] pxBuffer;
pxBuffer = nullptr; 

uint8_t* pu8Buffer = NULL; 
pu8Buffer = (uint8_t*) malloc(sizeof(uint8_t)*SIZE); 
free(pu8Buffer); 
pu8Buffer = NULL; 

The bit-oriented operators (>> and <<) should not be used on signed variables.

// incorrect
int32_t s32Val = 10; 
s32Val = s32Val << 10u; 

When declaring an enumeration, the first element has to be initialized with the start value even if it is 0 (default). Use enum class instead of enum (C++)! --> Why is enum class preferred over enum?

typedef enum class FSMStates
{
    Z1 = 0, 
    Z2, 
    //...
}eFSMStates_t; 
typedef eFSMStates_t* peFSMStates_t; 

Complex datatypes (struct, class, ...) should never be passed by value. Otherwise they require too much memory on the stack. Use pointers or references instead.

// incorrect 
void vPassingByCopy(const sTime_t csTime);

// correct
void vPassingByReference(const sTime_t& crTime);
void vPassingByReference(const psTime_t const cpsTime);

Functions that are only used within a module (c-file) should be declared as static in the .c/.cpp file (encapsulation principle).

static void vLocalDispatcher(const uint8_t cu8Event); 

If pointers are passed to a function, they must be validated before use (NULL != cpX [sanity check]). For failure an error handling has to be implemented.

int32_t s32CAN_SendBuffer(const uint8_t* const cpu8Data)
{
    int32_t s32ErrorHandle = CAN_ERROR; 
    
    if(nullptr != cpu8Data) // if(NULL != cpu8Data)
    {
        //...
        s32ErrorHandle = CAN_OKAY;
    }
    
    return s32ErrorHandle; 
}

Prohibited

break is only allowed in switch/case structures. Do not use fallthrough without good documentation!

// okay 
switch(eEvent)
{
    case Event1: 
        //...
        break; 
    default: 
        break; 
}

// not okay
while(1)
{
    if(HAL_PIN_SET == HAL_GPIO_ReadPin(/*...*/))
    {
        break; 
    }
}

Jumps with goto are not allowed.

goto Label1; //not okay  

Omitting parts of the loop body using continue is not permitted.

//not okay
while (iStatus < 10)
{
    iStatus = GetStatus();
    
    if (iStatus == 4)
    {
        continue; // not okay
    }
    // other statements
}  

using namespace xyz; is not allowed

// not okay
using namespace std; 
//...
cout << "Hello World! Are you ready to rumble?" << endl; 

// okay
using std::string; 
std::cout << "Hello World! Are you ready to rumble?" << std::endl; 

Reference