Я внес большинство изменений в обзор на основе отзывов, которые я получил отсюда. Теперь я вернулся, чтобы сделать второй обзор, чтобы улучшить свой код. Перед публикацией изменений я не изменил следующее.
Я оставил операторы печати как есть. Причина этого в том, что иногда, если что-то идет не так на клиентском сайте, мы обычно просим их взять журналы и поделиться ими с нами. Я до сих пор недостаточно продумал механизм управления тем, что нужно печатать, а что нет.
Я все еще ищу способы еще больше упростить весь код с точки зрения логики и структуры. Особенно, когда дело доходит до обработки циклического перехода при чтении меток времени для записи и чтения данных.
Мне все еще нужно выяснить, как заставить тесты работать на людях. На создание тестовых примеров и кодов уйдет некоторое время. Но я пока отложу это, учитывая количество времени, которое у меня осталось на выполнение этой задачи.
Мне удалось избавиться от большинства предупреждений. Однако у меня все еще есть несколько предупреждений, как показано ниже.
Я немного неохотно исправляю эти предупреждения, поскольку они приводят к нарушению кода. Например, выполнение следующих действий избавит от предупреждения.
*timestamp |= *(uint32_t*)cBucketBufHeadTmp << (8 * I);
Но это нарушает код, так как теперь указатель имеет тип для чтения uint32_t, а не uint8_t, что я не собирался делать. То же самое и с остальными.
Теперь, перейдя к коду, ниже находится файл заголовка.
/*
* 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_ */
Ниже находится исходный файл.
/*
* 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 ' '
const uint8_t *bytePtr = (uint8_t*)data->value;
dataSizeOut = (uint8_t)strlen((const char*)bytePtr);
//Include the ' ' to be written. This will be used to indicate the end of string while reading
dataSizeOut++;
}
break;
default:
printf("[BucketGetDataSize], invalid data typern");
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.rn");
*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.rn");
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 boundsrn", slotIdx);
return(false);
}else{
printf("[BucketGetReadData], Slot[%d] = %prn", 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]rn", dataU16, dataU16);
sprintf(value, "%hu", dataU16); //convert the u16 into string
}
break;
case STRING:{
uint8_t i = 0; printf("[BucketGetReadData] dataStr = ");
while(*cBucketBufTail != ' '){
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("rn");
}
break;
default:
printf("[BucketGetReadData], Error, invalid read type Slot[%d]rn", 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 registeredrn");
return(-1);
}
//Get th size of the item and handle storing as appropriate
dataSize = BucketGetDataSize(data);
if(dataSize == false){
printf("[BucketPut], Error, invalid data typern");
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 = %urn", (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] foundrn", lastStoredTime);
}
//Check timestamps
if(lastStoredTime == data->unixTime){
//If timestamp matches the current feed, write only data and slot idx
printf("[BucketPut], [%lu] timestamps matched!rn",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]rn", 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 = %hurn", remaining, totalSpaceRequired);
return(-3);
}else{
printf("[BucketPut], available space = %hu required space = %hurn", 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 largern");
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("<==============================================================================>rn");
if(!BucketCheckDataAvailable()){
printf("[BucketGet], AlLERT, Bucket is empty, no more data left to read.rn");
return(-1);
}
//Read the key-value for the corresponding feed/slot
if(!BucketGetReadData(keyOut, dataOut)){
printf("[BucketGet], Error, bucket read failedrn");
return(-2);
}
//Read the timestamp corresponding to this feed
if(!BucketGetTimestampForFeed(timestampOut)){
printf("[BucketGet], Error, feed timestamp read failedrn");
return(-3);
}
//All good, dump the key-value and associated timestamp
printf("[Bucket Get] timestamp = %lu rn",*timestampOut);
printf("[Bucket Get] key = %srn", keyOut);
printf("[Bucket Get] value = %srn", dataOut);
printf("<==============================================================================>rn");
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 **************************rn");
printf("slottaddresstkeyttypetvaluetunixtimern");
for(slot = 0; slot<BUCKET_MAX_FEEDS; slot++) {
printf("%dt", slot); // Print index
if (registeredFeed[slot] != NULL) {
printf("%pt",(void *)registeredFeed[slot]->key); // Print structure address
printf("%st",registeredFeed[slot]->key); // Print key
printf("%st",cvaluetypes[registeredFeed[slot]->type]); // Print type
PrintFeedValue(registeredFeed[slot]); // Print value
printf("t%lurn",registeredFeed[slot]->unixTime); // Print time
} else printf("--t--tEMPTYt--t--rn");
}
}
/****************************************************************
* Function Name : _DebugPrintBucket
* Description :Prints all bucket memory, even if empty
* Returns : None.
* Params None.
****************************************************************/
void DebugPrintBucket(void) {
uint16_t readIndex = 0;
printf("rn********************* BUCKET START ********************rn");
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("rn");
}
printf("********************** BUCKET END *********************rn");
}
/****************************************************************
* 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");
}
Ниже приведен тестовый код
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("=========================================================================================================rn");
printf("[BucketPut], Test case-1: Checking the bucket does not overflow when filled with data same timestamps...rn");
printf("=========================================================================================================rn");
status = false;
for(int i = 0 ; i < 10; i++){
status = BucketPut(&sensor2);
if(status == -3)
break;
}
if(status == -3){
printf("Test case-1: PASSEDrn");
}else{
printf("Test case-1: FAILEDrn");
}
DebugPrintBucket();
//Empty the bucket
while(true){
int8_t status = BucketGet(keyOutT, valOutT, &timeStmp);
if(status < 0){
break;
}else{
DebugPrintBucket();
}
}
printf("=========================================================================================================rn");
printf("[BucketPut], Test case-2: Checking the bucket does not overflow when filled with data different timestamps...rn");
printf("=========================================================================================================rn");
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: PASSEDrn");
}else{
printf("Test case-2: FAILEDrn");
}
DebugPrintBucket();
//Empty the bucket
while(true){
int8_t status = BucketGet(keyOutT, valOutT, &timeStmp);
if(status < 0){
break;
}else{
DebugPrintBucket();
}
}
printf("=========================================================================================================rn");
printf("[BucketPut], Test case-3: Checking the bucket write mixture of data same and different timestamps...rn");
printf("=========================================================================================================rn");
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("=========================================================================================================rn");
printf("[BucketPut], Test case-3: Checking the bucket write with timestamp wrapped around end-->startrn");
printf("=========================================================================================================rn");
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("=========================================================================================================rn");
printf("[BucketPut], Test case-4: Checking the bucket write with mix of string and uint16_trn");
printf("=========================================================================================================rn");
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
}
}
2 ответа
Поскольку Тоби Спейт и мой собственный обзор предыдущего кода уже охватили большинство проблем, я просто хотел описать несколько дополнительных важных аспектов, которые могут помочь вам упростить и улучшить ваш код.
Сделайте размер буфера степенью двойки
Вот как я бы написал несколько важных констант:
#define BUCKET_LOG2_SIZE 5
#define BUCKET_SIZE (1u << BUCKET_LOG2_SIZE)
#define BUCKET_MASK (BUCKET_SIZE - 1)
Вот почему: вместо того, чтобы каждый раз проводить дорогостоящее сравнение с указателем на хвост или голову, мы можем сделать очень дешевый & операция вместо этого. Если мы храним голову как индекс вместо абсолютного указателя мы можем сделать это:
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];
}
Обратите внимание, как переход вперед или назад стал тривиальным и автоматическим. Я написал их так, чтобы они были совместимы с вашим существующим кодом, но в комплексном дизайне я бы удалил cBucketBufHead полностью и просто хранить head значение индекса.
Вот одна из ваших функций, гораздо проще переписанная с использованием этого:
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), ×tamp_copy);
if (ts == unixTimeSlot) {
*timestamp = timestamp_copy;
return true;
}
return false;
}
Аналогичное упрощение можно сделать для чтения и записи кода в хвосте.
Рассмотрите возможность использования более одного экземпляра
На данный момент разрешен только один экземпляр этого буфера. Я бы посоветовал очень аккуратным способом улучшить это — собрать все детали в struct а затем передайте указатель на экземпляр каждой из функций, которые в нем нуждаются. Здесь слишком много внимания уделяется глобальным переменным, что способствует хрупкости и негибкости дизайна.
Устранение двойной буферизации
В коде теперь используется uint8_t frame[64] в качестве вторичного буфера. Он записывает в этот буфер, а затем копирует данные в реальный буфер. Это тратит циклы процессора и память (и мощность, если он работает от батареи), и мы можем добиться большего. Вместо этого:
memset(frame, 0, sizeof(frame));
BucketPrepareFrame(dataSize, frame, slot, data);
CircularBufferWrite(totalItemSize, frame);
Вы можете просто написать это и исключить BucketPrepareFrame функционировать полностью:
CircularBufferWriteHead(0, sizeof(slot), &slot);
CircularBufferWriteHead(0, dataSize, data->value);
CircularBufferWriteHead(0, sizeof(unixTimeSlot), &unixTimeSlot);
CircularBufferWriteHead(0, sizeof(data->unixTime), &(data->unixTime));
Я очень рекомендую включить "Bucket.h" первый в файле реализации. Рассмотрим включения в том виде, в котором они есть сейчас:
#include <stdio.h> #include <string.h> #include <stdint.h> #include <stdbool.h> #include "atmel_start.h" #include "Bucket.h"
Это маскирует проблему Bucket.h не включает в себя все, что ему нужно. В настоящее время он не будет работать, если пользователи не включили <stdint.h> и <stdbool.h> первый. Это разочаровывает ваших пользователей, которые рассчитывают, что смогут #include "Bucket.h" и просто вперед.
Включение "atmel_start.h" кажется, здесь нет необходимости — держите его подальше от библиотеки, чтобы сохранить переносимость. Вы должны включать только это в исходный код, обязательно платформенно-зависимый.
Полезный совет — всегда включать заголовки стандартных библиотек в последовательном порядке (большинство из нас использует алфавитный порядок), чтобы легко проверить, присутствует ли уже необходимое включение.
Здесь у нас есть комментарий, который не совсем соответствует коду:
const char* key; // MUST BE CONST, since the feeds reference key will never change.
Я думаю, что это действительно должно быть const, т.е.
const char *const key; // the feeds reference key will never change.
По аналогии,
/* For debug purposes */
const char *const cvaluetypes[] = {"UINT16", "STRING"};
Осталось еще несколько ошибок строки формата. Например:
printf("[BucketPut] Last stored timestamp[%lu] foundrn", lastStoredTime);
должно быть
printf("[BucketPut] Last stored timestamp[%" PRIu32"] foundrn", lastStoredTime);
static uint8_t cbucketBuf[BUCKET_SIZE]; static uint8_t *cBucketBufHead = &cbucketBuf[0]; static uint8_t *cBucketBufTail = &cbucketBuf[0];
&cbucketBuf[0] это просто длинный способ написать cbucketBuf — предпочитаю более простое выражение.
Есть ли причина для двух разных написаний cbucket и cBucket?
Это выглядит громоздко:
const uint8_t *bytePtr = (uint8_t*)data->value; dataSizeOut = (uint8_t)strlen((const char*)bytePtr);
На самом деле нет смысла отбрасывать от void* к uint8_t* если все, что мы делаем с этим, сводится к char*. Просто перейдите прямо к char*:
const char *bytePtr = data->value;
dataSizeOut = (uint8_t)strlen(bytePtr);
Здесь есть ошибка, если strlen >= UINT8_MAX, мы должны сообщить об этом как об ошибке.
Другие приведения избыточны:
*timestamp |= *(uint8_t*)cBucketBufHeadTmp << (8 * i);
cBucketBufHeadTmp уже есть uint8_t*, поэтому приведение не имеет никакого эффекта, кроме затруднения чтения кода (поскольку все приведения требуют ручной проверки, поскольку мы сказали компилятору игнорировать несоответствие).
Мне не нравится цикл, в котором достаточно простого вычитания:
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
Это требует тщательной проверки, чтобы определить, что все, что он делает, — это вычитание размера из cBucketBufHead. Гораздо проще:
int shiftOffset = sizeof slot + sizeof data->unixTime;
if (cbucketBuf + shiftOffset > cBucketBufHead) {
/* wrap around */
shiftOffset -= BUCKET_SIZE;
}
cBucketBufHead -= shiftOffset;
Я не сторонник чрезмерного использования скобок там, где они не нужны. Примеры:
memset(cbucketBuf, 0, sizeof(cbucketBuf)); return(0xFF);
Их было бы легче читать и идиоматичнее, если бы
memset(cbucketBuf, 0, sizeof cbucketBuf);
return 0xFF;
Я внес изменения и разместил обновления здесь. Быстрый заключительный обзор, прежде чем я закрою? 🙂
— Винай Дивакар