5
\$\begingroup\$

I'm writing code for embedded systems (IAR compiler) and it should adhere to Misra C++ 2008. This limits the available features of the language quite a bit:

  • no C++1x features
  • use of the standard library is banned as well
  • no dynamic memory (no heap)

We use PC-Lint to check for adherence.

Rule 5-0-15 of Misra states:

Array indexing is the only acceptable form of pointer arithmetic, because it is clearer and hence less error prone than pointer manipulation. This rule bans the explicit calculation of pointer values. Array indexing shall only be applied to objects defined as an array type. [...]

I have marked the sentence which my question is revolving around. I've written a small file system for our external flash memory and the file table itself resides in an EEPROM. The way to access the external EEPROM is a bit awkward. I have to derive the class from CConfigElement and I get handed a byte pointer and a data size and that array of bytes will be synchronized to the EEPROM. The complete system behind config elements and how they are working would result in probably a whole project review.

I have a known maximum number of files, I know the data size is sufficient for the number of file entries I want. My table is basically just an array of file entries. So I cast the byte pointer to a file entry pointer and work on that with array indexing.

That is clearly forbidden by the rule because the table is just a pointer and not an array. Often we are using an exception to the rule and just move on, like it is done in the code below.

A file is identified with a selector. To verify that the file stored in the file system and the requested file are actually the same, the maximum size of the file is used as a verification. The user of the code works on the external flash based on the file information provided by the file system (base address and current file size). The user also has to report back the amount of bytes written, so that the current file size can be updated.

The file system creates a new file if an unknown file is requested and aligns the base address of the new file to the 4 kiB sectors of the flash. Currently only a full reset is supported, not a deletion of single files.

We define our own types in vtypes.h which is included in all compilations:

  • vtypes.h
#ifndef VTypes_H
#define VTypes_H

typedef unsigned  char        VBOOL;

typedef char                  VCHAR;

typedef signed    char        VS8;
typedef unsigned  char        VU8;

typedef signed    short       VS16;
typedef unsigned  short       VU16;

typedef signed    long        VS32;
typedef unsigned  long        VU32;

typedef signed    long long   VS64;
typedef unsigned  long long   VU64;

typedef float                 VFLT32;
typedef double                VDBL64;

static const VS8  VS8_MIN     = -128;
static const VS8  VS8_MAX     = 127;
static const VU8  VU8_MAX     = 255;

static const VS16 VS16_MIN    = -32768;
static const VS16 VS16_MAX    = 32767;
static const VU16 VU16_MAX    = 65535U;


static const VS32 VS32_MIN    = (-2147483647L -1);
static const VS32 VS32_MAX    = 2147483647L;
static const VU32 VU32_MAX    = 4294967295UL;

static const VS64 VS64_MAX    = 9223372036854775807LL;
static const VS64 VS64_MIN    = (-9223372036854775807LL -1);
static const VU64 VU64_MAX    = 18446744073709551615ULL;

static const VFLT32 VFLT32_MIN = 1.18e-38F;
static const VFLT32 VFLT32_MAX = 3.39e38F;

static const VDBL64 VDBL64_MIN = 2.23e-308;
static const VDBL64 VDBL64_MAX = 1.79e308;

#ifdef __cplusplus

   #define NULL 0
   #define VEXTERN_C extern "C"
   #define VEXTERN_C_BEGIN extern "C" {
   #define VEXTERN_C_END }

#undef TRUE
#undef FALSE

   static const VBOOL TRUE  = 1;
   static const VBOOL FALSE = 0;

#else /* __cplusplus */

   #define NULL ((void *)0)
   #define TRUE  1U
   #define FALSE 0U

   #define VEXTERN_C
   #define VEXTERN_C_BEGIN
   #define VEXTERN_C_END

#endif  /* __cplusplus */


#endif

Before

This is the implementation before trying to adhere to all the Misra rules.

  • CConfigElement.h stripped for just the essentials
#ifndef CCONFIGELEMENT_H
#define CCONFIGELEMENT_H

#include "IConfigElement.h"

class CConfigElement : public IConfigElement {

public:
    virtual void SetItsData(VU8* const d, const VU32 size)
    {
        VASSERT(size >= dataSize);
        data = d;
    }

    //lint -esym(1960,data,dataSize) // must be visible in derived classes
protected:
    VU8* data;
    VU32 dataSize;

};
#endif /* CCONFIGELEMENT_H */
  • CCeMiniFileSystem.h
#ifndef CCEMINIFILESYSTEM_H
#define CCEMINIFILESYSTEM_H

#include "CConfigElement.h"
class CConfig;

class CCeMiniFileSystem : public CConfigElement
{
public:
   struct SoftwareImageSelector_t
   {
      StorageLocation_t StorageLocation;
      ControllerIdentifier_t ControllerIdentifier;
      ImageIdentifier_t ImageIdentifier;
   };

   static inline VBOOL operator==(const SoftwareImageSelector_t& lhs, const SoftwareImageSelector_t& rhs)
   {
      return (lhs.StorageLocation == rhs.StorageLocation) && (lhs.ControllerIdentifier == rhs.ControllerIdentifier) &&
             (lhs.ImageIdentifier == rhs.ImageIdentifier);
   }

   static inline VBOOL operator!=(const SoftwareImageSelector_t& lhs, const SoftwareImageSelector_t& rhs)
   {
      return !operator==(lhs, rhs);
   }

public:
    CCeMiniFileSystem();

    virtual ~CCeMiniFileSystem();


    VBOOL GetFileInfo(SoftwareImageSelector_t fileSelector, VU32 fileMaxSize, VBOOL& isNewFile,
                      VU32& baseAddress, VU32& currentSize);

    void UpdateCurrentSize(SoftwareImageSelector_t fileSelector, VU32 fileMaxSize,
                           VU32 additionalBytesWritten);

    void SetConfig(CConfig* const configThisElementIsAttachedTo);

    virtual void SetItsData(VU8* const d, const VU32 size);

    void ResetFileSystem();

private:

    VBOOL FindFile(SoftwareImageSelector_t fileSelector, VU32 fileMaxSize, VBOOL& fileExists,
                   VU32& fileEntryIndex);

#pragma pack(push,1)
    struct FileEntry_t
    {
        SoftwareImageSelector_t selector;
        VU32 baseAddressInFlash;
        VU32 currentSizeInFlash;
        VU32 MaxSize;
    };
#pragma pack(pop)

    const VU32 MAX_NUMBER_OF_FILES;
    FileEntry_t* fileTable;

    CConfig* config;

private:
    CCeMiniFileSystem(const CCeMiniFileSystem&);            // Only declaration -> linker error on usage
    CCeMiniFileSystem& operator=(const CCeMiniFileSystem&); // Only declaration -> linker error on usage
};

#endif /* CCEMINIFILESYSTEM_H_ */

  • CCeMiniFileSystem.cpp
#include "CCeMiniFileSystem.h"
#include "CConfig.h"

CCeMiniFileSystem::CCeMiniFileSystem() : CConfigElement(),
                                         MAX_NUMBER_OF_FILES(7),
                                         fileTable(NULL),
                                         config(NULL)
{
    dataSize = MAX_NUMBER_OF_FILES * sizeof(FileEntry_t);
}

void CCeMiniFileSystem::SetItsData(VU8* const d, const VU32 size)
{
    CConfigElement::SetItsData(d, size);
    fileTable = reinterpret_cast<FileEntry_t*>(data);
}

CCeMiniFileSystem::~CCeMiniFileSystem()
{
}

VBOOL CCeMiniFileSystem::FindFile(SoftwareUpdate::SoftwareImageSelector_t fileSelector, VU32 fileMaxSize,
                                  VBOOL& fileExists, VU32& fileEntryIndex)
{
    fileExists = FALSE;
    for (fileEntryIndex = 0U; fileEntryIndex < MAX_NUMBER_OF_FILES; fileEntryIndex++)
    {
        if (fileTable[fileEntryIndex].selector.StorageLocation != 2U)
        {
            // all files in flash have StorageLocation 2, so no files after this index
            // abort here --> fileEntryIndex is index for new file
            break;
        }
        if (fileTable[fileEntryIndex].selector == fileSelector)
        {
            if (fileTable[fileEntryIndex].MaxSize != fileMaxSize)
            {
                return FALSE; //lint !e904 file exists but has different size
            }
            else
            {
                fileExists = TRUE;
                break;
            }
        }
    }
    return TRUE;
}

VBOOL CCeMiniFileSystem::GetFileInfo(SoftwareUpdate::SoftwareImageSelector_t fileSelector, VU32 fileMaxSize,
                                     VBOOL& isNewFile, VU32& baseAddress, VU32& currentSize)
{
    // Search file
    VBOOL fileExists;
    VU32 fileEntryIndex;
    if (FALSE == FindFile(fileSelector, fileMaxSize, fileExists, fileEntryIndex))
    {
        return FALSE; //lint !e904 file exists but has different size
    }

    FileEntry_t& entryToUse = fileTable[fileEntryIndex];

    if (FALSE == fileExists)
    {
        // new file
        isNewFile                     = TRUE;
        entryToUse.selector           = fileSelector;
        entryToUse.MaxSize            = fileMaxSize;
        entryToUse.currentSizeInFlash = 0U;
        // get new baseAddress
        if (fileEntryIndex == 0U)
        {
            // first file
            entryToUse.baseAddressInFlash = 0U;
        }
        else
        {
            const VU32 indexBefore     = fileEntryIndex - 1U;
            const VU32 endOfFileBefore = fileTable[indexBefore].baseAddressInFlash + fileTable[indexBefore].MaxSize;
            const VU32 newBaseAddress  = (endOfFileBefore / 4096U + 1U) * 4096U; // align to next sector in flash
            entryToUse.baseAddressInFlash = newBaseAddress;
            baseAddress                   = newBaseAddress;
        }
        if (FALSE == config->Synchronize(this)) // write fileTable to EEPROM
        {
            return FALSE; //lint !e904 error synchronizing fileTable to EEPROM
        }
    }
    else
    {
        // existing file
        isNewFile   = FALSE;
        baseAddress = entryToUse.baseAddressInFlash;
        currentSize = entryToUse.currentSizeInFlash;
    }
    return TRUE;
}

void CCeMiniFileSystem::UpdateCurrentSize(SoftwareUpdate::SoftwareImageSelector_t fileSelector, VU32 fileMaxSize,
                                          VU32 additionalBytesWritten)
{
    // Search file
    VBOOL fileExists;
    VU32 fileEntryIndex;
    if (FALSE == FindFile(fileSelector, fileMaxSize, fileExists, fileEntryIndex))
    {
        return; //lint !e904 file exists but has different size
    }

    FileEntry_t& entryToUse = fileTable[fileEntryIndex];

    entryToUse.currentSizeInFlash += additionalBytesWritten;
    config->Synchronize(this); // write fileTable to EEPROM
}

void CCeMiniFileSystem::SetConfig(CConfig* const configThisElementIsAttachedTo)
{
    config = configThisElementIsAttachedTo;
}

void CCeMiniFileSystem::ResetFileSystem()
{
    for (VU32 i = 0; i < dataSize; i++)
    {
        data[i] = 0U;
    }
    config->Synchronize(this); // write fileTable to EEPROM
}


The offending lines are all those containing fileTable[fileEntryIndex] and usually in this case we would probably just add a //lint !e1960 5-0-15 comment with a justification behind it.

But I asked myself: Can I do better - can I write code that complies with the rule?

So I changed the simple pointer to FileEntry_t to a pointer to an array of known number of elements. This required to change the cast when I get the data pointer and it requires that I now dereference the pointer before I can access the element of the array. And while I was at it, I changed the loop in FindFile to have just a single break (another Misra rule).

After

  • CCeMiniFileSystem.h
#ifndef CCEMINIFILESYSTEM_H
#define CCEMINIFILESYSTEM_H

#include "CConfigElement.h"
class CConfig;

class CCeMiniFileSystem : public CConfigElement
{
public:
   struct SoftwareImageSelector_t
   {
      StorageLocation_t StorageLocation;
      ControllerIdentifier_t ControllerIdentifier;
      ImageIdentifier_t ImageIdentifier;
   };

   static inline VBOOL operator==(const SoftwareImageSelector_t& lhs, const SoftwareImageSelector_t& rhs)
   {
      return (lhs.StorageLocation == rhs.StorageLocation) && (lhs.ControllerIdentifier == rhs.ControllerIdentifier) &&
             (lhs.ImageIdentifier == rhs.ImageIdentifier);
   }

   static inline VBOOL operator!=(const SoftwareImageSelector_t& lhs, const SoftwareImageSelector_t& rhs)
   {
      return !operator==(lhs, rhs);
   }

public:
    CCeMiniFileSystem();

    virtual ~CCeMiniFileSystem();


    VBOOL GetFileInfo(SoftwareImageSelector_t fileSelector, VU32 fileMaxSize, VBOOL& isNewFile,
                      VU32& baseAddress, VU32& currentSize);

    void UpdateCurrentSize(SoftwareImageSelector_t fileSelector, VU32 fileMaxSize,
                           VU32 additionalBytesWritten);

    void SetConfig(CConfig* const configThisElementIsAttachedTo);

    virtual void SetItsData(VU8* const d, const VU32 size);

    void ResetFileSystem();

private:

    VBOOL FindFile(SoftwareImageSelector_t fileSelector, VU32 fileMaxSize, VBOOL& fileExists,
                   VU32& fileEntryIndex);

#pragma pack(push,1)
    struct FileEntry_t
    {
        SoftwareImageSelector_t selector;
        VU32 baseAddressInFlash;
        VU32 currentSizeInFlash;
        VU32 MaxSize;
    };
#pragma pack(pop)

    static const VU32 MAX_NUMBER_OF_FILES = 7U;
    FileEntry_t (* fileTable)[MAX_NUMBER_OF_FILES]; //Pointer to a FileEntry_t-array with MAX_NUMBER_OF_FILES Elements

    CConfig* config;

private:
    CCeMiniFileSystem(const CCeMiniFileSystem&);            // Only declaration -> linker error on usage
    CCeMiniFileSystem& operator=(const CCeMiniFileSystem&); // Only declaration -> linker error on usage
};

#endif /* CCEMINIFILESYSTEM_H_ */

  • CCeMiniFileSystem.cpp
#include "CCeMiniFileSystem.h"
#include "CConfig.h"

CCeMiniFileSystem::CCeMiniFileSystem() : CConfigElement(),
                                         fileTable(NULL),
                                         config(NULL)
{
    dataSize = MAX_NUMBER_OF_FILES * sizeof(FileEntry_t);
}

void CCeMiniFileSystem::SetItsData(VU8* const d, const VU32 size)
{
    CConfigElement::SetItsData(d, size);
    fileTable = reinterpret_cast<FileEntry_t(*)[MAX_NUMBER_OF_FILES]>(data);
}

CCeMiniFileSystem::~CCeMiniFileSystem()
{
}

VBOOL CCeMiniFileSystem::FindFile(const SoftwareUpdate::SoftwareImageSelector_t fileSelector, const VU32 fileMaxSize,
                                  VBOOL& fileExists, VU32& fileEntryIndex) const
{
    fileExists     = FALSE;
    fileEntryIndex = 0U;
    while ((fileExists == FALSE) && (fileEntryIndex < MAX_NUMBER_OF_FILES))
    {
        FileEntry_t& entry = (*fileTable)[fileEntryIndex];

        if (entry.selector.StorageLocation != 2U)
        {
            // all files in flash have StorageLocation 2, so no files after this index
            // abort here --> fileEntryIndex is index for new file
            break;
        }
        if (TRUE == (entry.selector == fileSelector))
        {
            if (entry.MaxSize != fileMaxSize)
            {
                return FALSE; //lint !e904 file exists but has different size
            }
            else
            {
                fileExists = TRUE;
            }
        }
        else
        {
            fileEntryIndex++; // increment only when file not found
        }
    }

    return TRUE;
}

VBOOL CCeMiniFileSystem::GetFileInfo(const SoftwareUpdate::SoftwareImageSelector_t fileSelector, const VU32 fileMaxSize,
                                     VBOOL& isNewFile, VU32& baseAddress,
                                     VU32& currentSize)
{
    // Search file
    VBOOL fileExists;
    VU32 fileEntryIndex;
    if (FALSE == FindFile(fileSelector, fileMaxSize, fileExists, fileEntryIndex))
    {
        return FALSE; //lint !e904 file exists but has different size
    }

    FileEntry_t& entryToUse = (*fileTable)[fileEntryIndex];

    if (FALSE == fileExists)
    {
        // new file
        isNewFile                     = TRUE;
        entryToUse.selector           = fileSelector;
        entryToUse.MaxSize            = fileMaxSize;
        entryToUse.currentSizeInFlash = 0U;
        // determine baseAddress
        if (fileEntryIndex == 0U)
        {
            // first file
            entryToUse.baseAddressInFlash = 0U;
        }
        else
        {
            const VU32 indexBefore     = fileEntryIndex - 1U;
            FileEntry_t& fileBefore    = (*fileTable)[indexBefore];
            const VU32 endOfFileBefore = fileBefore.baseAddressInFlash + fileBefore.MaxSize;
            const VU32 newBaseAddress = ((endOfFileBefore / 4096U) + 1U) * 4096U; // align to next sector in flash
            entryToUse.baseAddressInFlash = newBaseAddress;
            baseAddress                   = newBaseAddress;
        }
        if (FALSE == config->Synchronize(this))
        {
            return FALSE; //lint !e904 error synchronizing fileTable to EEPROM
        }
    }
    else
    {
        // existing file
        isNewFile   = FALSE;
        baseAddress = entryToUse.baseAddressInFlash;
        currentSize = entryToUse.currentSizeInFlash;
    }
    return TRUE;
}

void CCeMiniFileSystem::UpdateCurrentSize(const SoftwareUpdate::SoftwareImageSelector_t fileSelector,
                                          const VU32 fileMaxSize,
                                          const VU32 additionalBytesWritten)
{
    // Search file
    VBOOL fileExists;
    VU32 fileEntryIndex;
    if (FALSE == FindFile(fileSelector, fileMaxSize, fileExists, fileEntryIndex))
    {
        return; //lint !e904 file exists but has different size
    }

    FileEntry_t& entryToUse = (*fileTable)[fileEntryIndex];

    entryToUse.currentSizeInFlash += additionalBytesWritten;
    config->Synchronize(this);  //lint !e534 no possibility to use return value in a helpful way
}

void CCeMiniFileSystem::SetConfig(CConfig* const configThisElementIsAttachedTo)
{
    config = configThisElementIsAttachedTo;
}

void CCeMiniFileSystem::ResetFileSystem()
{
    for (VU32 i = 0U; i < dataSize; i++)
    {
        data[i] = 0U; //lint !e1960 5-0-15 size of array behind pointer handled with dataSize
    }
    config->Synchronize(this); //lint !e534 no possibility to use return value in a helpful way
}


So now I'm not really impressed with this.

The doubts I'm having:

  • FileEntry_t (* fileTable)[MAX_NUMBER_OF_FILES]; is not going to be very well understood by everyone around me
  • the necessary dereferenciation makes it not easier to read or understand

So is code quality (performance is not an issue, it's readability, maintainability and being less error prone) really improved by adhering to the rule in this case?

\$\endgroup\$
  • 1
    \$\begingroup\$ The use of ellipsis may cause some reviewers on this website to flag a question for lack of definition. In this case you are trying to not repeat things from the first section of code but this may be mistaken. You have also not included the definition of the base class that might help reviewers review the code. It would also be better if there was code showing how this was used. \$\endgroup\$ – pacmaninbw Jun 18 at 13:51
  • \$\begingroup\$ @pacmaninbw I tried a minimalist approach to keep my zombie count low and to focus on my main problem. I've now edited my question to give a more complete view of my class in question. I cannot provide more information on the base class. I'm also not sure how I can show an example usage without going into full project review mode (which I'm not allowed to do). \$\endgroup\$ – Arsenal Jun 19 at 8:34
0
\$\begingroup\$

This was difficult to review because so many pieces were missing and had to be inferred. For that reason, you may find some of these comments off the mark because they are necessarily based on incomplete information. With that said, here are some thoughts that may help you improve your program.

Be clear about what's allowed

While your description said that no C++11 features are allowed, the use of long long types is a C++11 feature. While that may be supported by your compiler, it's probably worth explicitly noting the exceptions.

Don't write empty destructors

Even before C++11, the compiler would automatically generate empty destructors if needed, so there is no need to explicitly write ~CCeMiniFileSystem() unless that's also a rule in MISRA (the last time I looked at MISRA rules was when it only covered C).

\$\endgroup\$
  • \$\begingroup\$ long long was probably supported as an extension of the IAR compiler long (no pun intended) before C++11, wasn't actually aware that that is a new feature. You are mixing up the source file before the changes with the header file after the changes, that's why your compiler doesn't like it. What kind of context would improve the question to make it easier to review? I find it very hard to cut out enough context without making the whole thing explode. Maybe the heart of my question is better asked somewhere else... \$\endgroup\$ – Arsenal Jun 19 at 21:46
  • \$\begingroup\$ You're right -- I had mixed old and new content. Fixed now (and updated the review to match). I'll look at this more later and add to the review if I can. \$\endgroup\$ – Edward Jun 19 at 22:00
  • \$\begingroup\$ Since you asked, context that would help people review the code would include: brief descriptions of the four public members of the main class -- is there sequencing required? What are the parameters supposed to do? Just a sample stub showing the sequence of calls and the expected result would be useful. \$\endgroup\$ – Edward Jun 20 at 0:30

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.