Embedded IoT: local data storage (Second Update)

Question

I have made the review changes based on feedback I have got from here which was based on the feedback I got from this one. I am almost done and now looking for a final review with respect to any minor or obvious improvements/fixes that can be done quickly. In regard to supporting multiple instances, I have included that in Jira for the next release as I have to release this module on Friday for integration and testing. Before this, I want to test it properly for any corner cases.

Below is the header file.

/*
 * Bucket.h
 *
 * Created: 17/12/2020 12:57:45 PM
 *  Author: Vinay Divakar
 */


#ifndef BUCKET_H_
#define BUCKET_H_

#include <inttypes.h>
#include <stdio.h>
#include <stdbool.h>
#include <stdint.h>
#include <string.h>

#define BUCKET_MAX_FEEDS      32        
#define BUCKET_LOG2_SIZE       5
#define BUCKET_SIZE           (1u << BUCKET_LOG2_SIZE)
#define BUCKET_MASK           (BUCKET_SIZE - 1)
//#define BUCKET_TIME()  GetUnixTime(); 

typedef enum {
    UINT16,
    STRING
} cvalue_t;

// Used to register feeds to the bucket and for passing sensor data to the bucket
typedef struct {
    const char* key;        // The keys are initialized on boot 
    cvalue_t type;          // Data type     
    void* value;            // Value
    uint32_t unixTime;      // Timestamp
} cbucket_t;

bool BucketRegisterFeed(cbucket_t *feed);
int8_t BucketPut(const cbucket_t *data);
int8_t BucketGet( char *keyOut, char *dataOut, uint32_t *timestampOut);

//Debug functions
void DebugPrintRegistrationData(void);
void DebugPrintBucket(void);
void DebugPrintBucketTailToHead(void);


#endif /* BUCKET_H_ */

Below is the source code.

/*
 * Bucket.c
 *
 * Created: 17/12/2020 12:57:31 PM
 * Author: Vinay Divakar
 
 * Description: This module is used to accumulate data when 
    the network is down or unable to transmit data due to poor 
    signal quality. It currently supports uint16_t and string 
    data types.  The bucket is designed to maximize the amount 
    of data that can be stored in a given amount of memory for 
    typical use. The feeds must be registered before it can be 
    written to or read from the bucket. 
 
 The BucketPut API writes the feed to the bucket.
    E.g. 1: If the BucketPut sees that consecutive feeds written 
    have the same timestamp, then it writes the data as shown below. 
    This is done to save memory.
    
    [Slot] [Data] [Slot] [Data] [Slot] [Data]...[Timestamp]
                            
    E.g. 2: If the BucketPut sees that consecutive feeds written have 
    different timestamps, then it writes the data as shown below.
    
    [Slot] [Data] [Timestamp] [Slot] [Data] [Timestamp]-
    [Slot] [Data] [Timestamp]
                            
    E.g. 3: If the BucketPut sees a mixture of above, then it writes 
    the data as shown below.
    
    [Slot] [Data] [Slot] [Data] [Slot] [Data] [Timestamp] [Slot] [Data]- 
    [Timestamp] [Slot] [Data] [Slot] [Data] [Timestamp]
                            
 The BucketGet API reads the feed in the following format.
 
    [Key] [Value] [Timestamp]
    
 * Notes:
    1. Module only supports UINT16_T and STRING data types.
 */

/*
 *====================
 * Includes
 *====================
 */
#include "Bucket.h"

/*
*====================
* static/global vars
*====================
*/
const char *const cvaluetypes[] = {"UINT16", "STRING"};
// Stores the registered feed 
static cbucket_t *registeredFeed[BUCKET_MAX_FEEDS];
static const uint8_t unixTimeSlot = 0xFF;   //virtual time slot
// Bucket memory for edge storage and pointers to keep track or reads and writes
static uint8_t cBucketBuf[BUCKET_SIZE];
static uint8_t *cBucketBufHead = cBucketBuf;
static uint8_t *cBucketBufTail = cBucketBuf;

/*
*====================
* Static Fxns
*====================
*/
/****************************************************************
 * Function Name    : BucketGetRegisteredFeedSlot
 * Description      : Gets slot index of the registered feed
 * Returns          : slot index on OK, 0xFF on error
 * Params           @data: points to the feed struct
 ****************************************************************/
static uint8_t BucketGetRegisteredFeedSlot(const cbucket_t *data){
    uint8_t slotIdx;
    /* Check if the feed had been previously registered */
    for(slotIdx = 0 ; slotIdx < BUCKET_MAX_FEEDS ; slotIdx++) {
        //found it?
        if(data == registeredFeed[slotIdx]){
            //Get the slot index
            return slotIdx; 
        }
    }
    return 0xFF;
}

/****************************************************************
 * Function Name    : BucketCheckDataAvailable
 * Description      : Checks for data in the bucket
 * Returns          : false on empty else true.
 * Params           None.
 ****************************************************************/
static bool BucketCheckDataAvailable(void){
    return cBucketBufTail != cBucketBufHead;
}

/****************************************************************
 * Function Name    : BucketGetDataSize
 * Description      : Gets the size of the item
 * Returns          : false on error, !0 on OK
 * Params       @data :points to feed struct
 ****************************************************************/
static uint8_t BucketGetDataSize(const cbucket_t *data){
    uint8_t dataSizeOut = false;

    switch(data->type){
        case UINT16:{
            //Total data size  = 2 bytes i.e. 16 bit unsigned
            dataSizeOut = sizeof(uint16_t); 
        }
        break;
        
        case STRING:{
            //Total data size = length of the string until null terminated. Now get the length of the string by looking upto '\0'
            const uint8_t *bytePtr =  data->value;
            dataSizeOut = (uint8_t)strlen(bytePtr);
            if(dataSizeOut >= UINT8_MAX){
                printf("[BucketGetDataSize], invalid string length\r\n");
                dataSizeOut = false;
            }else{
                //Include the '\0' to be written. This will be used to indicate the end of string while reading
                dataSizeOut++;  
            }   
        }
        break;
        
        default:
        printf("[BucketGetDataSize], invalid data type\r\n");
        break;
    }
    
    return dataSizeOut;
}

/****************************************************************
 * Function Name    : RegisteredFeedFreeSlot
 * Description      : Gets first free slot(index) found, returns
 * Returns          : slot on success else 0xFF
 * Params       None.
 ****************************************************************/
static uint8_t RegisteredFeedFreeSlot(void) {
    uint8_t slot;
    //Check for slots sequentially, return index of first empty one (null pointer)
    for(slot = 0; slot < BUCKET_MAX_FEEDS; slot++) {
        if(registeredFeed[slot] == 0) 
            return slot;
    }
    //All slots full
    return 0xFF; 
}

/****************************************************************
 * Function Name    : BucketCircularBufferWriteHead
 * Description      : Writes to bucket from head
 * Returns          None.
 * Params           @offset: offset to start writing from
                    @size: data size
                    @data: ptr to data to be written
 ****************************************************************/
static void BucketCircularBufferWriteHead(int offset, size_t size, const void *data) {
    const uint8_t *ptr = data;  // convenience cast
    unsigned head = cBucketBufHead - cBucketBuf;
    for (head += offset; size; --size) {
        cBucketBuf[head & BUCKET_MASK] = *ptr++;
        ++head;
    }
    cBucketBufHead = &cBucketBuf[head & BUCKET_MASK];
}

/****************************************************************
 * Function Name    : BucketCircularBufferReadHead
 * Description      : Reads from bucket from head
 * Returns          None.
 * Params           @offset: offset to start reading from
                    @size: data size
                    @data: ptr to buffer to populate
 ****************************************************************/
static void BucketCircularBufferReadHead(int offset, size_t size, void *data) {
    uint8_t *ptr = data;  // convenience cast
    unsigned head = cBucketBufHead - cBucketBuf;
    for (head += offset; size; --size) {
        *ptr++ = cBucketBuf[head & BUCKET_MASK];
        ++head;
    }
    cBucketBufHead = &cBucketBuf[head & BUCKET_MASK];
}

/****************************************************************
 * Function Name    : BucketCircularBufferWriteTail
 * Description      : Writes to bucket from tail
 * Returns          None.
 * Params           @offset: offset to start writing from
                    @size: data size
                    @data: ptr to data to be written
 ****************************************************************/
static void BucketCircularBufferWriteTail(int offset, size_t size, const void *data) {
    const uint8_t *ptr = data;  // convenience cast
    unsigned tail = cBucketBufTail - cBucketBuf;
    for (tail += offset; size; --size) {
        cBucketBuf[tail & BUCKET_MASK] = *ptr++;
        ++tail;
    }
    cBucketBufTail = &cBucketBuf[tail & BUCKET_MASK];
}

/****************************************************************
 * Function Name    : BucketCircularBufferReadTail
 * Description      : Reads from bucket from tail.
 * Returns          None.
 * Params           @offset: offset to start reading from
                    @size: data size
                    @data: ptr to buffer to pupulate
 ****************************************************************/
static void BucketCircularBufferReadTail(int offset, size_t size, void *data) {
    uint8_t *ptr = data;  // convenience cast
    unsigned tail = cBucketBufTail - cBucketBuf;
    for (tail += offset; size; --size) {
        *ptr++ = cBucketBuf[tail & BUCKET_MASK];
        ++tail;
    }
    cBucketBufTail = &cBucketBuf[tail & BUCKET_MASK];
}

/****************************************************************
 * Function Name    : BucketGetTimeStamp
 * Description      : Gets the timestamp for the specific key
 * Returns          : true on OK, false on empty
 * Params            @timestamp: Gets the timestamp while write
 ****************************************************************/
static bool BucketPutGetTimeStamp(uint32_t *timestamp) {
    if (!BucketCheckDataAvailable()) {
        return false;
    }
    uint8_t ts;
    uint32_t timestamp_copy;
    const int sizes = sizeof(ts) + sizeof(timestamp_copy);
    BucketCircularBufferReadHead(-sizes, sizeof(ts), &ts);
    BucketCircularBufferReadHead(0, sizeof(timestamp_copy), &timestamp_copy);
    if (ts == unixTimeSlot) {
        *timestamp = timestamp_copy;
        return true;
    }
    return false;
}

/****************************************************************
 * Function Name    : BucketGetTimestampForFeed
 * Description      : Gets the timestamp for the feed
 * Returns          : false on error, true on OK.
 * Params           @timestamp: feeds timestamp while read
 ****************************************************************/
static bool BucketGetTimestampForFeed(uint32_t *timestamp){
    bool status = false;
    uint8_t ts = 0;
    *timestamp = 0;
    uint32_t timestamp_copy = 0, dummyTimestamp = 0;
    uint16_t shiftBackOffset = 0;
    
    //Read the current element
    BucketCircularBufferReadTail(0, sizeof(ts), &ts);
    //Check if tail is pointing to the virtual time slot
    if(ts == unixTimeSlot){//If so, read the timestamp
        printf("[BucketGetTimestampForFeed], tail pointing to time slot, read it.\r\n");
        uint8_t dummySlot = 0;
        int sizes  = 0;
        sizes = sizeof(dummySlot);
        BucketCircularBufferWriteTail(-sizes, sizeof(dummySlot), &dummySlot);//clear ts
        BucketCircularBufferReadTail(0, sizeof(timestamp_copy), &timestamp_copy);//read timestamp
        sizes = sizeof(timestamp_copy);
        BucketCircularBufferWriteTail(-sizes, sizeof(timestamp_copy), &dummyTimestamp);//clear timestamp
        *timestamp = timestamp_copy;
        status = true;
    }else{//timeslot not found?
        //means the next byte is the slot index for the next feed which is not what we are looking for.
        //Lets look beyond, it must be there! - if not, then there's something seriously wrong with the code
        while(ts != unixTimeSlot){
            shiftBackOffset++;
            BucketCircularBufferReadTail(0, sizeof(ts), &ts);
            //break out if buggy
            if(shiftBackOffset >= UINT16_MAX){
                printf("#####[BucketGetTimestampForFeed], Error, could not find the timeslot, seems like a bug in the code?\r\n");
                return status;//error
            }
        }//while
        const int sizes = sizeof(ts) + sizeof(timestamp_copy) + (shiftBackOffset);
        BucketCircularBufferReadTail(0, sizeof(timestamp_copy), &timestamp_copy);
        BucketCircularBufferReadTail(-sizes, 0, 0);
        *timestamp = timestamp_copy;
        status = true;
    }
    return status;
}

/****************************************************************
 * Function Name    : BucketGetReadData
 * Description      : Reads the key and value for that feed/slot.
 * Returns          : false error, true on success.
 * Params           @key: key to be populated(static).
                    @value: value read from the bucket.
 ****************************************************************/
static bool BucketGetReadData(char *key, char *value){
    uint8_t slotIdx;
    uint8_t dummyIdx = 0;
    bool status = true;
    
    BucketCircularBufferReadTail(0, sizeof(slotIdx), &slotIdx);
    const int sizes = sizeof(slotIdx);
    BucketCircularBufferWriteTail(-sizes, sizeof(slotIdx), &dummyIdx);
    if(slotIdx > BUCKET_MAX_FEEDS){ 
        printf("[BucketGetReadData], Error, Slot[%u] index is out of bounds\r\n", slotIdx); 
        return false; 
    }else{
        printf("[BucketGetReadData], Slot[%d] = %p\r\n", slotIdx, (void *)registeredFeed[slotIdx]->key);
    }
    //Copy the key for the corresponding slot
    strncpy(key, registeredFeed[slotIdx]->key, strlen(registeredFeed[slotIdx]->key));
    
    //Read data based on type
    switch(registeredFeed[slotIdx]->type){
        case UINT16:{
            uint16_t dataU16 = 0;
            BucketCircularBufferReadTail(0, sizeof(dataU16), &dataU16);
            printf("[BucketGetReadData] dataU16 = %hu [0x%X]\r\n", dataU16, dataU16);
            sprintf(value, "%hu", dataU16); //convert the u16 into string
            dataU16 = 0;
            const int sizes = sizeof(dataU16);
            BucketCircularBufferWriteTail(-sizes, sizeof(dataU16), &dataU16);
        }
        break;
        
        case STRING:{
             printf("[BucketGetReadData] dataStr = ");
             uint8_t idx = 0, eos = 99, tmp = 0;
             //Look for end of string
             while(eos != '\0'){
                 BucketCircularBufferReadTail(0, sizeof(eos), &eos);
                 const int sizes = sizeof(tmp);
                 BucketCircularBufferWriteTail(-sizes, sizeof(tmp), &tmp);
                 printf("0x%x ", eos);
                 value[idx++] = eos;
             }
             printf("\r\n");
        }
        break;
        
        default:
        printf("[BucketGetReadData], Error, invalid read type Slot[%d]\r\n", slotIdx);
        status = false;
        break;
    }
    
    return status;
}

/*
*====================
* Fxns
*====================
*/
/****************************************************************
 * Function Name    : BucketRegisterFeed
 * Description      : Registers the feed
 * Returns          : false on error, true on OK
 * Params       @feed :Feed to register
 ****************************************************************/
bool BucketRegisterFeed(cbucket_t *feed) {
    uint8_t slot = RegisteredFeedFreeSlot();
    if (slot >= BUCKET_MAX_FEEDS){
        return false;
    } else{
        registeredFeed[slot] = feed;    
    }
    return true;
}

/****************************************************************
 * Function Name    : BucketPut
 * Description      : writes to the bucket
 * Returns          : true on success else negative..
 * Params       @data :points to struct to be written
 ****************************************************************/
int8_t BucketPut(const cbucket_t *data) {
    uint8_t slot = 0;
    uint8_t dataSize = 0;
    uint8_t totalSpaceRequired = 0;
    uint16_t remaining = 0;
    uint32_t lastStoredTime = 0;
    
    //Find the slot for this feed
    slot = BucketGetRegisteredFeedSlot(data);
    if(slot > BUCKET_MAX_FEEDS){ 
        printf("[BucketPut], Error, feed not registered\r\n"); 
        return -1; 
    }
        
    //Get th size of the item and handle storing as appropriate 
    dataSize =  BucketGetDataSize(data);
    if(dataSize == false){  
        printf("[BucketPut], Error, invalid data type\r\n");  
        return -2; 
    }
    
    //Get the amount space left in the bucket 
    remaining = (cBucketBufTail + BUCKET_SIZE - cBucketBufHead-1) % BUCKET_SIZE;
    printf("[BucketPut], bucket size = %d   remaining space = %hu   dataSize = %u\r\n", (int)BUCKET_SIZE, remaining, dataSize);
    
    //Get the timestamp from the unix time slot 
    bool found = BucketPutGetTimeStamp(&lastStoredTime);
    if(found){//last stored timestamp found
        printf("[BucketPut] Last stored timestamp[%" PRIu32"] found\r\n", lastStoredTime);
    }
    
    //Check timestamps
    if(lastStoredTime == data->unixTime){
        //If timestamp matches the current feed, write only data and slot idx
        printf("[BucketPut], timestamps[%" PRIu32"] matched!\r\n",data->unixTime);
        totalSpaceRequired = (uint8_t)(dataSize + sizeof(slot));
    }else{
        //If timestamps different or unavailable, account for a write including the new timestamp
        printf("[BucketPut] Last stored timestamp[%" PRIu32"] is different from Current timestamp[%" PRIu32"]\r\n", lastStoredTime, data->unixTime);
        totalSpaceRequired = (uint8_t)(dataSize + sizeof(slot) + sizeof(data->unixTime) + sizeof(unixTimeSlot));
    }
    
    //Proceed further only if enough space is available in the bucket
    if(totalSpaceRequired > remaining) {
        printf("[BucketPut], ALERT, no space available, space = %hu,    required space = %hu\r\n", remaining, totalSpaceRequired);
        return -3;
    }else{
            printf("[BucketPut], available space = %hu  required space = %hu\r\n", remaining, totalSpaceRequired);
    }
    
    //If timestamp had matched, overwrite it with current feed data and append with timestamp.
    if(lastStoredTime == data->unixTime){
        int shiftOffset = sizeof(slot) + sizeof(data->unixTime);
        //move the head backwards by size of time slot index + size of timestamp.
        if(cBucketBuf + shiftOffset > cBucketBufHead){
            //Wrap around to start of the bucket
            shiftOffset -= BUCKET_SIZE;
        }
        cBucketBufHead -= shiftOffset;
    }//timestamp matched!
    
    //If we have reached here, means all checks have passed and its safe to write the item to the bucket 
    BucketCircularBufferWriteHead(0, sizeof(slot), &slot);
    BucketCircularBufferWriteHead(0, dataSize, data->value);
    BucketCircularBufferWriteHead(0, sizeof(unixTimeSlot), &unixTimeSlot);
    BucketCircularBufferWriteHead(0, sizeof(data->unixTime), &(data->unixTime));

    return true;
}

/****************************************************************
 * Function Name    : BucketGet
 * Description      :Gets the data
 * Returns          : true on success else negative..
 * Params       @keyOut :contains the key
                @dataOut:contains the value
                @timestampOut: contains the timestamp
 ****************************************************************/
int8_t BucketGet( char *keyOut, char *dataOut, uint32_t *timestampOut) {
    //Check if theres anything in the bucket
    printf("<==============================================================================>\r\n");
    if(!BucketCheckDataAvailable()){
        printf("[BucketGet], ALERT, Bucket is empty, no more data left to read.\r\n");
        return -1;
    }
    
    //Read the key-value for the corresponding feed/slot
    if(!BucketGetReadData(keyOut, dataOut)){ 
        printf("[BucketGet], Error, bucket read failed\r\n"); 
        return -2; 
    }
    //Read the timestamp corresponding to this feed
    if(!BucketGetTimestampForFeed(timestampOut)){
        printf("[BucketGet], Error, feed timestamp read failed\r\n"); 
        return -3; 
    }
    //All good, dump the key-value and associated timestamp
    printf("[Bucket Get] timestamp  = %" PRIu32"\r\n",*timestampOut);
    printf("[Bucket Get] key        = %s\r\n", keyOut);
    printf("[Bucket Get] value      = %s\r\n", dataOut);
    printf("<==============================================================================>\r\n");
        
    return true;
}

/*
*====================
* Debug Utils
*====================
*/
/****************************************************************
 * Function Name    : PrintFeedValue
 * Description      :Prints feed data value via void pointer
 and corrects for type FLOAT AND DOUBLE DONT PRINT ON WASPMOTE
 BUT NO REASON TO BELIEVE THEY ARE WRONG
 * Returns          : None.
 * Params       @feed: Points to the feed to be printed
 ****************************************************************/
void PrintFeedValue(cbucket_t *feed) {
    
    switch(feed->type){
        case UINT16: printf("%d",*(uint16_t*)feed->value); break;       
        case STRING: printf("%s",(char*)feed->value); break;
        default: printf("%s","UNSUPPORTED TYPE"); break;
    }
}

/****************************************************************
 * Function Name    : _DebugPrintRegistrationData
 * Description      :Prints all registered feeds and their details
 * Returns          : None.
 * Params       None.
 ****************************************************************/
void DebugPrintRegistrationData(void) {
    uint8_t slot;

    printf("********************** Current Bucket Registration Data **************************\r\n");
    printf("slot\taddress\tkey\ttype\tvalue\tunixtime\r\n");
    for(slot = 0; slot<BUCKET_MAX_FEEDS; slot++) {
        printf("%d\t", slot);                                        // Print index
        if (registeredFeed[slot] != NULL) {
            printf("%p\t",(void *)registeredFeed[slot]->key);                 // Print structure address
            printf("%s\t",registeredFeed[slot]->key);                  // Print key
            printf("%s\t",cvaluetypes[registeredFeed[slot]->type]);    // Print type
            PrintFeedValue(registeredFeed[slot]);                      // Print value
            printf("\t%" PRIu32"\r\n",registeredFeed[slot]->unixTime);        // Print time
        } else printf("--\t--\tEMPTY\t--\t--\r\n");
    }
}

/****************************************************************
 * Function Name    : _DebugPrintBucket
 * Description      :Prints all bucket memory, even if empty
 * Returns          : None.
 * Params       None.
 ****************************************************************/
void DebugPrintBucket(void) {
    uint16_t readIndex = 0;
    printf("\r\n********************* BUCKET START ********************\r\n");
    while(readIndex < BUCKET_SIZE) {
        printf("0x%04X ", readIndex);
        for (uint8_t column = 0; column < 16; column++) {
            if(readIndex < BUCKET_SIZE) 
                printf("%02X ",cBucketBuf[readIndex]);
            readIndex++;
            //delayMicroseconds(78);  // Wait for a byte to send at 115200 baud
            //delay(0.1);
        }
        printf("\r\n");
    }
    printf("********************** BUCKET END *********************\r\n");
}

/****************************************************************
 * Function Name    : _DebugPrintBucket
 * Description      : Prints bucket memory that has data
 * Returns          : None.
 * Params       None.
 ****************************************************************/
void DebugPrintBucketTailToHead(void) {
    uint8_t *cBucketBufHeadTemp = cBucketBufHead;
    uint8_t *cBucketBufTailTemp = cBucketBufTail;
    uint16_t index;
    printf("\n*************** BUCKET START FROM TAIL ****************\n");
    // Label and indent first line
    if ((cBucketBufTailTemp - cBucketBuf) % 16 != 0) {
        printf("          ");
        for (index = (uint16_t)(cBucketBufTailTemp - cBucketBuf) % 16; index > 0; index--) {
            printf("   ");
        }
    }
    // Print rest of data
    while(cBucketBufTailTemp != cBucketBufHeadTemp) { // Increment read address
        if(cBucketBufTailTemp >= &cBucketBuf[BUCKET_SIZE]) 
            cBucketBufTailTemp = cBucketBuf; // Handle wraparound
        index = (uint16_t)(cBucketBufTailTemp - cBucketBuf);  // Get current index in buffer
        if (index % 16 == 0) printf("\n0x%04X ", cBucketBufTailTemp - cBucketBuf); // New line every 0x00n0
        printf("%02X ", *cBucketBufTailTemp);
        cBucketBufTailTemp++;                                                   // Print data in buffer
    }
    printf("\n***************** BUCKET END AT HEAD ******************\n");
}

The test code is the same as shared in the previous thread.

Answer 1

Overall the code is well-written and easy to read. I found no major things to remark on, just details:

• cvaluetypes is only used by the .c file and should be declared static, to achieve private encapsulation. It's good that you use * const for read-only pointer tables, since this typically makes them flash allocated on embedded systems, as opposed to RAM where they shouldn't be.

• Minor remark: to avoid multiple returns from a function, it is best to make a habit of doing:

    static uint8_t BucketGetRegisteredFeedSlot(const cbucket_t *data){
        uint8_t slotIdx = 0xFF;
        ...
            if(data == registeredFeed[slotIdx]){
                break;
            }
        return slotIdx;
    }
  

• There's a few cases of "magic numbers" like 99. These should be made named constants with #define or const.

• unsigned head, const int etc. You suddenly slip back to the native C types here and there. Stick with stdint.h or use size_t for sizes of types and arrays.

• Strive to keep for loops as simple as possible. That is, always count upwards and avoid multiple iterators. For example you could as well do this:

    uint16_t start = cBucketBufTail - cBucketBuf + offset;
    for(size_t i=start; i<size; i++)
  

  There's many such needlessly complicated for loops in the code.

• Avoid strncpy, it's a dangerous function since it is prone to mess up the null termination. And needlessly slow when you know the exact size in advance. So you could have used memcpy:

    memcpy(key, registeredFeed[slotIdx]->key, strlen(registeredFeed[slotIdx]->key) + 1);
  

  Although in this specific case, since you call strlen anyway, you should use neither of these functions including strlen. The correct function here is strcpy:

    strcpy(key, registeredFeed[slotIdx]->key);
  

• Your error handling is inconsistent - some times you use bool, some times magic numbers -1, -2 etc. All of these function results should be replaced by a consistent error type used by the whole library, in the form of a typedef enum {...} bucket_err_t; Then document which functions that return which error codes, in what situations.

• Comments containing the function documentation should be placed in the header, in case of public functions. Comments documenting private static functions obviously need to be in the .c file like you have though.

• You forget const correctness in a few places like void PrintFeedValue(cbucket_t *feed) -> const cbucket_t *.

• It's ok to use uint8_t as the string type, particularly in embedded systems. However please note that this might upset compilers when you call standard libs with that type. Therefore make a habit of casting to char* before calling strlen etc.

---

EDIT

• In case you have functions doing return false etc upon error and those functions also return values through parameter pointers, you must document what happens to those parameters in case of errors. Are they untouched in case of errors, are they set to some error code or known value, or are they to be considered as indeterminate values?

Answer 2

Since the previous review, the standard library includes in the header have swung from one extreme to the other, so I'll focus in on our objectives here.

As a user of this library, I want to be able to #include "Bucket.h" without any preconditions, but with also without any unnecessary overhead. That means computational overhead (mainly from the number of transitive includes) and cognitive overhead (how much I need to understand).

This means that Bucket.h needs to include definitions of bool, int8_t and uint32_t, so that the header can be parsed:

#include <stdbool.h>
#include <stdint.h>

However, the other includes don't provide any value for the user of Bucket.h: they are needed for the implementation but not the interface. So these shouldn't be included from the header.

There is at least one tool to help keep your headers in line with this principle of "include what you use".

Along the lines of including only what the user needs in the public header, it seems from the test code that users don't need to know the details of the defined constants, so the header could be as simple as this:

#ifndef BUCKET_H_
#define BUCKET_H_

#include <stdbool.h>
#include <stdint.h>

// Used to register feeds to the bucket and for passing sensor data to
// the bucket
typedef struct {
    const char* key;
    enum {
        UINT16,
        STRING
    } type;
    void* value;
    uint32_t unixTime;
} cbucket_t;


bool BucketRegisterFeed(const cbucket_t *feed);
int8_t BucketPut(const cbucket_t *data);
int8_t BucketGet(char *keyOut, char *dataOut, uint32_t *timestampOut);

#ifdef BUCKET_DEBUG
void DebugPrintRegistrationData(void);
void DebugPrintBucket(void);
void DebugPrintBucketTailToHead(void);
#endif

#endif /* BUCKET_H_ */

If the user could be isolated from the implementation details of cbucket_t, we could reduce the header even further, by forward-declaring struct cbucket_t. That would enable users to very quickly understand what facilities are available to them.

Answer 3

There is a bug in the BucketPut() function. The following logic does not account for handling the case when the bucket is empty and the timestamp to be written is 0 which could be the case sometimes. In this case, the head will shift backwards by 5 steps while wrapping around and start writing from there while it should have started to write from the start of the bucket.

//If timestamp had matched, overwrite it with current feed data and append with timestamp.
if(lastStoredTime == data->unixTime){
    int shiftOffset = 0;
    //move the head backwards by size of time slot index + size of timestamp.
    if(cBucketBuf + shiftOffset > cBucketBufHead){
        //Wrap around to start of the bucket
        shiftOffset -= BUCKET_SIZE;
    }
    cBucketBufHead -= shiftOffset;
}//timestamp matched!

I fixed the above by simply checking for a bucket empty case. If the bucket is empty, no need to shift the head. Below is the fix. I hope this does not break something else as it seems a simple change.

//If timestamp had matched, overwrite it with current feed data and append with timestamp.
if(lastStoredTime == data->unixTime){
    int shiftOffset = 0;
    //Only offset if there is something in the bucket
    if(BucketCheckDataAvailable()){
        shiftOffset = sizeof(slot) + sizeof(data->unixTime);    
    }
    //move the head backwards by size of time slot index + size of timestamp.
    if(cBucketBuf + shiftOffset > cBucketBufHead){
        //Wrap around to start of the bucket
        shiftOffset -= BUCKET_SIZE;
    }
    cBucketBufHead -= shiftOffset;
}//timestamp matched!