Embedded IoT: local data storage (Updated)

Question

I have made most of the review changes based on feedback I have got from here. Now I am back here for a second review to further better my code. Before posting the changes, somethings I haven't changed are the following.

1. I have left the print statements as-is for now. The reason for this is, at times if something goes wrong at the client site, we usually ask them to grab logs and share them with us. I still haven't given enough thought to the mechanism to control what needs to be printed and what not.

2. I am still in the midst of figuring out ways to further simplify the whole code from logic and structure. Especially when it comes to handling the wrap-around while reading the timestamps for data writes and reads.

3. I still need to figure out how to make the tests work for humans. It will take some time to come up with test cases and codes. But I am going to keep that aside for now considering the amount of time I have left to complete this task.

4. I have managed to get rid of most of the warnings. However, I still have a few warnings as shown below. I am a bit reluctant to fix these warnings as they end up breaking the code. For instance, doing the below will get rid of the warning.

          *timestamp |= *(uint32_t*)cBucketBufHeadTmp << (8 * I);
   

But it breaks the code since now the pointer typecasted to read uint32_t and not uint8_t which is not what I intend to do. The same goes for the others.

Now coming to the code, below is the header file.

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


#ifndef BUCKET_H_
#define BUCKET_H_

#define BUCKET_SIZE           32        // Range 10-32000 Memory in bytes allocated to bucket for edge storage
#define BUCKET_MAX_FEEDS      32        // Range 2-64 Number of unique feeds the bucket can hold
//#define BUCKET_TIME()  GetUnixTime(); // Need to define link to function that will provide unix time to this macro
#define BUCKET_MAX_VALUE_SIZE   64      // Maximum value string length

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;    // MUST BE CONST, since the feeds reference key will never change.
    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);
uint8_t BucketNumbOfRegisteredFeeds(void);
void BucketZeroize(void);

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


#endif /* BUCKET_H_ */

Below is the source file.

/*
 * 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 hasn't been tested for STRING data type. 
 */

/*
 *====================
 * Includes
 *====================
 */
#include <stdio.h>
#include <string.h>
#include <stdint.h>
#include <stdbool.h>
#include "atmel_start.h"
#include "Bucket.h"
//#include "INIconfig.h"

/* For debug purposes */
const char* cvaluetypes[] = {"UINT16", "STRING"};

/*
*====================
* static/global vars
*====================
*/
// 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[0];
static uint8_t *cBucketBufTail = &cbucketBuf[0];

/*
*====================
* Fxns
*====================
*/
static uint8_t RegisteredFeedFreeSlot(void);

/****************************************************************
 * Function Name    : BucketZeroize
 * Description      : Zeroize the bucket
 * Returns          : None.
 * Params           :None.
 ****************************************************************/
void BucketZeroize(void){
    memset(cbucketBuf, 0, sizeof(cbucketBuf));
}

/****************************************************************
 * 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    : BucketHeadWrapAroundToStart
 * Description      : Wraps the head to start of the bucket
 * Returns          None.
 * Params           None.
 ****************************************************************/
static void BucketHeadWrapAroundToStart(void){
    if(cBucketBufHead >= &cbucketBuf[BUCKET_SIZE]){
        cBucketBufHead = &cbucketBuf[0];    
    }
}

/****************************************************************
 * Function Name    : BucketTailWrapAroundToStart
 * Description      : Wraps the tail to start of the bucket
 * Returns          None.
 * Params           None.
 ****************************************************************/
static void BucketTailWrapAroundToStart(void){
    if(cBucketBufTail >= &cbucketBuf[BUCKET_SIZE]){
        cBucketBufTail = &cbucketBuf[0];
    }
}

/****************************************************************
 * 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){
    bool status = false;
    *timestamp = 0;
    
    //There's no data left to be read from the bucket i.e. tail = head
    if(!BucketCheckDataAvailable()){
        return(false);
    }
    
    //Attempt looking for the time slot, 
    uint8_t *cBucketBufHeadTmp = cBucketBufHead;
    //Iterate through the cells while handling wraparounds, this is to go backwards looking for the timeslot
    for(int i = 0 ; i < 5 ; i++, cBucketBufHeadTmp--){
         //if head points to start of bucket, wrap to end of the bucket
        if(cBucketBufHeadTmp <= &cbucketBuf[0]){
            cBucketBufHeadTmp = &cbucketBuf[BUCKET_SIZE];
        }
    }
    
    //Now, we should be pointing to the virtual time slot i.e. 0xff
    if(*cBucketBufHeadTmp == unixTimeSlot){
       //Inc address to skip the virtual time slot i.e. 0xFF
       cBucketBufHeadTmp++; 
        
        //load the timestamp
        for(uint8_t i = 0 ; i < sizeof(uint32_t) ; i++, cBucketBufHeadTmp++){
            //Handle if head needs to be wrapped around to read the timestamp
            if(cBucketBufHeadTmp >= &cbucketBuf[BUCKET_SIZE]){
                cBucketBufHeadTmp = &cbucketBuf[0];
            }
            *timestamp |= *(uint8_t*)cBucketBufHeadTmp << (8 * i);
        }//for
        //we found it!
        status = true;
    }//points to timeslot
    
    return(status);
}

/****************************************************************
 * 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 = 0;

    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 =  (uint8_t*)data->value;
            dataSizeOut = (uint8_t)strlen((const char*)bytePtr);
            //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    : 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    : 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){
    int8_t status = false;
    *timestamp = 0;
    
    BucketTailWrapAroundToStart();
    //Check if tail is pointing to the virtual time slot
    if(*cBucketBufTail == unixTimeSlot){//If so, read the timestamp
        printf("[BucketGetTimestampForFeed], tail pointing to time slot, read it.\r\n");
        *cBucketBufTail++ = 0;  //Skip the virtual timeslot and point to the timestamp
        for(uint8_t i = 0 ; i < sizeof(uint32_t) ; i++){
            BucketTailWrapAroundToStart();
            *timestamp |= *(uint8_t*)cBucketBufTail << (8 * i);
            *cBucketBufTail++ = 0;
        }//for
        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
        uint8_t *cBucketBufTailTmp = cBucketBufTail;
        //Iterate and look for the time slot
        while(cBucketBufTailTmp++ <= &cbucketBuf[BUCKET_SIZE]){
                if(cBucketBufTailTmp >= &cbucketBuf[BUCKET_SIZE]){
                    cBucketBufTailTmp = &cbucketBuf[0];
                }
            //Check if we are pointing to the virtual time slot
            if(*cBucketBufTailTmp == unixTimeSlot){
                printf("[BucketGetTimestampForFeed], yes!, time slot has been found.\r\n");             
                cBucketBufTailTmp++;//Skip the virtual timeslot and point to the start of the timestamp
                for(uint8_t  i = 0 ; i < sizeof(uint32_t) ; i++, cBucketBufTailTmp++){
                    if(cBucketBufTailTmp >= &cbucketBuf[BUCKET_SIZE]){
                        cBucketBufTailTmp = &cbucketBuf[0];
                    }
                    *timestamp |= *(uint8_t*)cBucketBufTailTmp << (8 * i);
                }//read the timestamp
                status = true;
                break;
            }//timeslot
        }//while
    }//else
    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;
    bool status = true;
    
    //Check if the tail is pointing to the end of the bucket or beyond, wrap around to start of bucket to continue reading
    BucketTailWrapAroundToStart();
        
    //Read the slot index for the feed
    slotIdx = *(uint8_t*)cBucketBufTail;    //this is an int8_t type, if greater, will lead to undefined behavior.
    *cBucketBufTail++ = 0;
    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;
            for(uint8_t i = 0 ; i < sizeof(uint16_t) ; i++){//Read 2 bytes
                BucketTailWrapAroundToStart();
                dataU16 |= *cBucketBufTail << (8 * i);
                *cBucketBufTail++ = 0;
            }//for
            printf("[BucketGetReadData] dataU16 = %hu [0x%X]\r\n", dataU16, dataU16);
            sprintf(value, "%hu", dataU16); //convert the u16 into string
        }
        break;
        
        case STRING:{
            uint8_t i = 0; printf("[BucketGetReadData] dataStr = ");
            while(*cBucketBufTail != '\0'){
                BucketTailWrapAroundToStart();
                printf("0x%x ", *cBucketBufTail);
                value[i++] = *cBucketBufTail;
                *cBucketBufTail++ = 0;
            }//copy data until end of string is reached
            //copy the null terminated character and point to start of the next address
            value[i] = *cBucketBufTail;
            *cBucketBufTail++ = 0;
            printf("\r\n");
        }
        break;
        
        default:
        printf("[BucketGetReadData], Error, invalid read type Slot[%d]\r\n", slotIdx);
        status = false;
        break;
    }
    
    return(status);
}

/****************************************************************
 * 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    : CircularBufferWrite
 * Description      : Writes data to the bucket
 * Returns          None.
 * Params           @itemSize: Size of the data
                    @data: ptr to data to be written
 ****************************************************************/
static void CircularBufferWrite(uint8_t itemSize, uint8_t *data){
    for(uint8_t i = 0 ; i < itemSize ; i++, cBucketBufHead++){
        BucketHeadWrapAroundToStart();
        *cBucketBufHead = data[i];
    }
}

/****************************************************************
 * Function Name    : BucketPrepareFrame
 * Description      : Prepares frame to be written to the bucket
 * Returns          None.
 * Params           @dataSizeIn: Size of actual feed data
                    @frameOut: ptr to frame buffer
                    @slotIn: slot index for the feed
                    @dataIn: ptr to the feed struct.
 ****************************************************************/
static void BucketPrepareFrame(uint8_t dataSizeIn, uint8_t *frameOut, uint8_t slotIn, const cbucket_t *dataIn){
    uint8_t i = 0;
    uint8_t *bytePtr =  (uint8_t*)dataIn->value;
    for(i = 0 ; i <= dataSizeIn ; i++){
        if(i == 0){
            frameOut[i] = slotIn;//copy slot index
        }else{
            frameOut[i] = *bytePtr++; //copy data
        }
    }

    frameOut[i++] = unixTimeSlot; //copy timeslot index
    for(uint8_t j = 0 ; j < sizeof(dataIn->unixTime); j++){
        frameOut[i++] = (dataIn->unixTime  >> 8*j) & 0xFF; //copy the timestamp
    }
}

/****************************************************************
 * 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;
    uint8_t frame[64];
    
    //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[%lu] found\r\n", lastStoredTime);
    }
    
    //Check timestamps
    if(lastStoredTime == data->unixTime){
        //If timestamp matches the current feed, write only data and slot idx
        printf("[BucketPut], [%lu] timestamps 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[%lu] is different from Current feed timestamp[%lu]\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], Error, 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){
        uint8_t shiftOffset = sizeof(slot) + sizeof(data->unixTime);
        //move the head backwards by size of time slot index + size of timestamp.
        for(int i = 0 ; i < shiftOffset ; i++, cBucketBufHead--){
            if(cBucketBufHead <= &cbucketBuf[0]){
                cBucketBufHead = &cbucketBuf[BUCKET_SIZE];
            }
        }//for
    }//timestamp matched!
    
    //Get the size of the item based on the data type
    uint8_t totalItemSize = (uint8_t)(dataSize + sizeof(slot) + sizeof(data->unixTime) + sizeof(unixTimeSlot));
    
    //Prepare data to be written to the bucket
    if(totalItemSize > sizeof(frame)){
        printf("[BucketPut], Error, item size is very large\r\n");
        return(-4);
    }
    
    //If we have reached here, means all checks have passed and its safe to write the item to the bucket 
    memset(frame, 0, sizeof(frame));
    BucketPrepareFrame(dataSize, frame, slot, data);
    CircularBufferWrite(totalItemSize, frame);

    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], AlLERT, 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  = %lu \r\n",*timestampOut);
    printf("[Bucket Get] key        = %s\r\n", keyOut);
    printf("[Bucket Get] value      = %s\r\n", dataOut);
    printf("<==============================================================================>\r\n");
        
    return(true);
}

/****************************************************************
 * Function Name    : BucketNumbOfRegisteredFeeds
 * Description      : Gets the num of registered feeds
 * Returns          : No. of registered feeds
 * Params       None.
 ****************************************************************/
uint8_t BucketNumbOfRegisteredFeeds(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)
        break;
    }
    return (slot);
}

/*
*====================
* 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%lu\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[0]) % 16 != 0) {
        printf("          ");
        for (index = (uint16_t)(cBucketBufTailTemp - &cbucketBuf[0]) % 16; index > 0; index--) {
            printf("   ");
        }
    }
    // Print rest of data
    while(cBucketBufTailTemp != cBucketBufHeadTemp) {                                                                  // Increment read address
        if(cBucketBufTailTemp >= &cbucketBuf[BUCKET_SIZE]) cBucketBufTailTemp = &cbucketBuf[0]; // Handle wraparound
        index = (uint16_t)(cBucketBufTailTemp - &cbucketBuf[0]);                                            // Get current index in buffer
        if (index % 16 == 0) printf("\n0x%04X ", cBucketBufTailTemp - &cbucketBuf[0]);          // New line every 0x00n0
        printf("%02X ", *cBucketBufTailTemp);
        cBucketBufTailTemp++;                                                   // Print data in buffer
    }
    printf("\n***************** BUCKET END AT HEAD ******************\n");
}

Below is the test code

char sensorValStr1[]    = "aaaaaaa";
char sensorKey1[]       ="sensorRef1";
cbucket_t sensor1       = {sensorKey1, STRING, sensorValStr1,               1613343690};

uint16_t sensorValU16_2 = 0xAAAA;
char sensorKey2[]       ="sensorRef2";
cbucket_t sensor2       = {sensorKey2, UINT16, (uint8_t*)&sensorValU16_2,   1613343689};

uint16_t sensorValU16_3 = 0xBBBB;
char sensorKey3[]       ="sensorRef3";
cbucket_t sensor3       ={sensorKey3, UINT16, (uint8_t*)&sensorValU16_3,    1613343689};

uint16_t sensorValU16_4 = 0xCCCC;
char sensorKey4[]       ="sensorRef4";
cbucket_t sensor4       ={sensorKey4, UINT16, (uint8_t*)&sensorValU16_4,    1613343691};
    
int main(void) {
    char keyOutT[32] = {0};
    char valOutT[32] = {0};
    int8_t status = 0;
    uint32_t timeStmp = 0;
    
    //Initialize drivers
    //atmel_start_init();                   

    BucketRegisterFeed(&sensor1);
    BucketRegisterFeed(&sensor2);
    BucketRegisterFeed(&sensor3);
    BucketRegisterFeed(&sensor4);
    DebugPrintRegistrationData();
    
    printf("=========================================================================================================\r\n");
    printf("[BucketPut], Test case-1: Checking the bucket does not overflow when filled with data same timestamps...\r\n");
    printf("=========================================================================================================\r\n");
    status = false;
    for(int i = 0 ; i < 10; i++){
        status = BucketPut(&sensor2);
        if(status == -3)
            break;
    }
    
    if(status == -3){
        printf("Test case-1: PASSED\r\n");
    }else{
        printf("Test case-1: FAILED\r\n");
    }
    DebugPrintBucket();
    
    //Empty the bucket
    while(true){
        int8_t status = BucketGet(keyOutT, valOutT, &timeStmp);
        if(status < 0){
            break;
        }else{
            DebugPrintBucket();
        }
    }

    printf("=========================================================================================================\r\n");
    printf("[BucketPut], Test case-2: Checking the bucket does not overflow when filled with data different timestamps...\r\n");
    printf("=========================================================================================================\r\n");
    status = false;
    for(int i = 0 ; i < 5 ; i++){
        status = BucketPut(&sensor2);
        if(status == -3){//bucket is full
            //do nothing
        }else{
            sensor2.unixTime += 1;
        }
    }
        
    if(status == -3){
        printf("Test case-2: PASSED\r\n");
    }else{
        printf("Test case-2: FAILED\r\n");
    }
    DebugPrintBucket();
    
    //Empty the bucket
    while(true){
        int8_t status = BucketGet(keyOutT, valOutT, &timeStmp);
        if(status < 0){
            break;
        }else{
            DebugPrintBucket();
        }
    }
    
    printf("=========================================================================================================\r\n");
    printf("[BucketPut], Test case-3: Checking the bucket write mixture of data same and different timestamps...\r\n");
    printf("=========================================================================================================\r\n");
    
    status = false;
    for(int i = 0 ; i < 5 ; i++){
        status = BucketPut(&sensor3);
        if(status == -3){//bucket is full
            //do nothing
        }else{
            if(i == 2){
                
            }else if(i == 3){
                
            }else{
                sensor2.unixTime += 1;  
            }
        }//else
    }
    DebugPrintBucket();
    
    //Empty the bucket
    while(true){
        int8_t status = BucketGet(keyOutT, valOutT, &timeStmp);
        if(status < 0){
            break;
        }else{
            DebugPrintBucket();
        }
    }
    
    printf("=========================================================================================================\r\n");
    printf("[BucketPut], Test case-3: Checking the bucket write with timestamp wrapped around end-->start\r\n");
    printf("=========================================================================================================\r\n");
    
    status = false;
    for(int i = 0 ; i < 2; i++){
        if(i == 0){
            BucketPut(&sensor1);
        }
        status = BucketPut(&sensor2);
        if(status == -3)
        break;
    }
    DebugPrintBucket();
    
    //Empty the bucket
    while(true){
        int8_t status = BucketGet(keyOutT, valOutT, &timeStmp);
        if(status < 0){
            break;
        }else{
            DebugPrintBucket();
        }
    }
    
    printf("=========================================================================================================\r\n");
    printf("[BucketPut], Test case-4: Checking the bucket write with mix of string and uint16_t\r\n");
    printf("=========================================================================================================\r\n");
    status = false;
    for(int i = 0 ; i < 4; i++){
        if(i == 0){
            BucketPut(&sensor1);
        }
        status = BucketPut(&sensor3);
        if(status == -3)
        break;
    }
    DebugPrintBucket();
        
    //Empty the bucket
    while(true){
        int8_t status = BucketGet(keyOutT, valOutT, &timeStmp);
        if(status < 0){
            break;
        }else{
            DebugPrintBucket();
        }
    }
    
    while (true) {
        //empty loop
    }
}

Answer 1

Since Toby Speight and my own review of the previous code already covered most of the issues, I just wanted to describe a few additional important aspects that may help you simplify and improve your code.

Make the buffer size a power of two

Here's how I would write some important constants:

#define BUCKET_LOG2_SIZE       5  
#define BUCKET_SIZE           (1u << BUCKET_LOG2_SIZE)      
#define BUCKET_MASK           (BUCKET_SIZE - 1)

Here's why: rather than having to do a costly comparison with the tail or head pointer every time, we can do a very cheap & operation instead. If we store the head as an index instead of an absolute pointer, we can do this:

static void CircularBufferWriteHead(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];
}

static void CircularBufferReadHead(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];
}

Notice how wrapround and moving forwards or backwards is now trivial and automatic. I have written these to be compatible with your existing code, but in a comprehensive design, I'd remove cBucketBufHead entirely and simply store a head index value.

Here's one of your functions, rewritten much more simply, using this:

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);
    CircularBufferReadHead(-sizes, sizeof(ts), &ts);
    CircularBufferReadHead(0, sizeof(timestamp_copy), &timestamp_copy);
    if (ts == unixTimeSlot) {
        *timestamp = timestamp_copy;
        return true;
    }
    return false;
}

One can make a similar simplification for code reading and writing at the tail.

Consider allowing more than a single instance

At the moment, there is only allowed to be a single instance of this buffer. I would suggest that a very neat way to improve this would be to gather all of the details into a struct and then pass a pointer to an instance to each of the functions needing it. There is an overreliance on global variables here than contributes to a brittle and inflexible design.

Eliminate double buffering

The code now uses a uint8_t frame[64] as a secondary buffer. It writes to that buffer and then copies the data into the real buffer. This wastes CPU cycles and memory (and power if this is battery operated) and we can do better. Instead of this:

memset(frame, 0, sizeof(frame));
BucketPrepareFrame(dataSize, frame, slot, data);
CircularBufferWrite(totalItemSize, frame);

You could simply write this and eliminate the BucketPrepareFrame function entirely:

CircularBufferWriteHead(0, sizeof(slot), &slot);
CircularBufferWriteHead(0, dataSize, data->value);
CircularBufferWriteHead(0, sizeof(unixTimeSlot), &unixTimeSlot);
CircularBufferWriteHead(0, sizeof(data->unixTime), &(data->unixTime));

Answer 2

I really recommend including "Bucket.h" first in the implementation file. Consider the includes as we currently have them:

#include <stdio.h>
#include <string.h>
#include <stdint.h>
#include <stdbool.h>
#include "atmel_start.h"
#include "Bucket.h"

This masks the problem that Bucket.h doesn't include everything it needs. At present, it will fail if users haven't included <stdint.h> and <stdbool.h> first. That's frustrating for your users, who expect to be able to #include "Bucket.h" and just get going.

The include of "atmel_start.h" seems not to be needed here - keep it out of the library to maintain portability. You should only be including that in source code that's necessarily platform-dependent.

A helpful tip is to always include standard library headers in a consistent order (most of us use alphabetical order), so it's easy to confirm whether a needed include is already present.

---

Here we have a comment that doesn't quite agree with the code:

const char* key;    // MUST BE CONST, since the feeds reference key will never change.

I think that really should be const, i.e.

const char *const key;    // the feeds reference key will never change.

Similarly,

/* For debug purposes */
const char *const cvaluetypes[] = {"UINT16", "STRING"};

---

There's still a few format string errors remaining. For example:

printf("[BucketPut] Last stored timestamp[%lu] found\r\n", lastStoredTime);

should be

printf("[BucketPut] Last stored timestamp[%" PRIu32"] found\r\n", lastStoredTime);

---

static uint8_t cbucketBuf[BUCKET_SIZE];
static uint8_t *cBucketBufHead = &cbucketBuf[0];
static uint8_t *cBucketBufTail = &cbucketBuf[0];

&cbucketBuf[0] is just a long-winded way to write cbucketBuf - prefer the simpler expression.

Is there a reason for the two different spellings cbucket and cBucket?

---

This looks unwieldy:

        const uint8_t *bytePtr =  (uint8_t*)data->value;
        dataSizeOut = (uint8_t)strlen((const char*)bytePtr);

There's really no point casting from void* to uint8_t* if all we do with that is cast to char*. Just go directly to char*:

        const char *bytePtr =  data->value;
        dataSizeOut = (uint8_t)strlen(bytePtr);

There's a bug here, in that if strlen >= UINT8_MAX, we should be reporting that as an error.

Other casts are redundant:

        *timestamp |= *(uint8_t*)cBucketBufHeadTmp << (8 * i);

cBucketBufHeadTmp already is a uint8_t*, so casting has no effect other than to make the code harder to read (because all casts require manual checking, since we've told the compiler to ignore the mismatch).

---

I don't like the loop that's used where a simple subtraction would suffice:

    uint8_t shiftOffset = sizeof(slot) + sizeof(data->unixTime);
    //move the head backwards by size of time slot index + size of timestamp.
    for(int i = 0 ; i < shiftOffset ; i++, cBucketBufHead--){
        if(cBucketBufHead <= &cbucketBuf[0]){
            cBucketBufHead = &cbucketBuf[BUCKET_SIZE];
        }
    }//for

That requires a lot of inspection to determine that all it's doing as subtracting the size from cBucketBufHead. Much simpler as:

    int shiftOffset = sizeof slot + sizeof data->unixTime;
    if (cbucketBuf + shiftOffset > cBucketBufHead) {
        /* wrap around */
        shiftOffset -= BUCKET_SIZE;
    }
    cBucketBufHead -= shiftOffset;

---

I'm not a fan of the overuse of parentheses where they are not needed. Examples:

memset(cbucketBuf, 0, sizeof(cbucketBuf));

return(0xFF);

Those would be easier to read and more idiomatic as

memset(cbucketBuf, 0, sizeof cbucketBuf);

return 0xFF;

Answer 3

"Store and forward". Minimize effort in the remote device.

• Always store, even if you are online.
• Serialize the data into a text string. JSON is a good candidate. (XML is too fat and ambiguous.)
• A number is a string of digit characters (plus "." if you have decimal places.) Do not worry in the device about how big the numbers can be.
• Use a "circular" buffer of bytes.
• The buffer is a fixed size
• The sensors are pushing serialized strings into it.
• If it is multi-threaded, you will need some simple interlock to avoid multiple sensors stepping on each other.
• The network transmission will pull as much data out of the buffer as is practical based on data available, packetsize, etc.
• The central computer will worry about parsing the numbers, etc.
• If the circular buffer overflows, you may need some simple checkpoint of where to stop: If less than 100 bytes available, stop pushing data into the buffer. (This means it is really a FIFO.)