uint8_t fifo_buffer[8];
int16_t acc_temp[3];
int16_t gyro_x;
void loop(){
if(state_fifo_int){ // interrupt has triggered
blink(0,1); // really quick blink // really quick blink
//Serial.println(x);
state_fifo_int = false;
accelGyroMag.resetFIFO();
accelGyroMag.getFIFOBytes(fifo_buffer,8); // fill in fifo bytes
accelGyroMag.getIntFIFOBufferOverflowStatus(); // read the INT_STATUS reg in order to clear the Latch.
acc_temp[0] = (((int16_t)fifo_buffer[0]) << 8) | fifo_buffer[1];
acc_temp[1] = (((int16_t)fifo_buffer[2]) << 8) | fifo_buffer[3];
acc_temp[2] = (((int16_t)fifo_buffer[4]) << 8) | fifo_buffer[5];
gyro_x = (((int16_t)fifo_buffer[7]) << 8) | fifo_buffer[8];
timeout = millis();
}
}
This code attemmpts to access memory in the variable fifo_buffer at a location that is larger than it's size. One solution to this could be to simply increase fifo_buffer from fifo_buffer[8] to fifo_buffer[9].
uint8_t fifo_buffer[9];
int16_t acc_temp[3];
int16_t gyro_x;
void loop(){
if(state_fifo_int){ // interrupt has triggered
blink(0,1); // really quick blink // really quick blink
//Serial.println(x);
state_fifo_int = false;
accelGyroMag.resetFIFO();
accelGyroMag.getFIFOBytes(fifo_buffer,8); // fill in fifo bytes
accelGyroMag.getIntFIFOBufferOverflowStatus(); // read the INT_STATUS reg in order to clear the Latch.
acc_temp[0] = (((int16_t)fifo_buffer[0]) << 8) | fifo_buffer[1];
acc_temp[1] = (((int16_t)fifo_buffer[2]) << 8) | fifo_buffer[3];
acc_temp[2] = (((int16_t)fifo_buffer[4]) << 8) | fifo_buffer[5];
gyro_x = (((int16_t)fifo_buffer[7]) << 8) | fifo_buffer[8];
timeout = millis();
}
}
void abs()
{
char* charData = new char;
*charData = 'h';
BYTE* byteData = new BYTE;
*byteData = *(BYTE*)charData;
}
Here, charData and byteData are allocated memory but are then not deallocated before the function ends, which is improper memory management.
void abs()
{
char* charData = new char;
*charData = 'h';
BYTE* byteData = new BYTE;
*byteData = *(BYTE*)charData;
delete charData;
delete byteData;
}
int *readPot() ///read potentiometer value
{
int tempValue = analogRead(A0);
int *potValue = &tempValue;
return potValue;
}
The issue with the snippet is that the function returns a pointer to a local variable, which is automatically deallocated at the end of the function. This means the pointer that is returned is pointing to nothing, which will lead to unpredictable results if used. To fix this, memory will need to be allocated if the intention is the use the returned pointer. This pointer will then have to deallocated later when it is not longer being used.
int *readPot() ///read potentiometer value
{
int* potValue = new int;
*potValue = analogRead(A0);
return potValue;
}
#include <stdio.h>
#include <time.h>
#include <string.h>
#include <wiringPi.h>
#include <wiringSerial.h>
#define MAXLINE 120
extern int serialOpenB (const char *device, const int baud, const int n);
extern char * serialGets (char *buf, const int n, const int fd);
char * rstrip(char *s) {
char *p = s + strlen(s)-1;
while((*p == '\n')||(*p == '\n')) {
p--;
}
*p = '\0';
return s;
}
....
The while loop in this condition contains duplicate conditions, which makes no logical sense. So assuming the intent was to check for another char such as '\r', the code can be fixed like shown below.
#include <stdio.h>
#include <time.h>
#include <string.h>
#include <wiringPi.h>
#include <wiringSerial.h>
#define MAXLINE 120
extern int serialOpenB (const char *device, const int baud, const int n);
extern char * serialGets (char *buf, const int n, const int fd);
char * rstrip(char *s) {
char *p = s + strlen(s)-1;
while((*p == '\n')||(*p == '\r')) {
p--;
}
*p = '\0';
return s;
}
....
int getSize(char* ch){
int tmp=0;
while (*ch) {
*ch++;
tmp++;
}return tmp;}
Here, the line *ch++; increments the pointer itself and not the value of the pointer. To fix this, ch++; should be used instead.
int getSize(char* ch){
int tmp=0;
while (*ch) {
ch++;
tmp++;
}return tmp;}
#include <iostream>
#include <bitset>
#define NUM_BITS 10
int main(int argc, char *argv[])
{
const int numBits = NUM_BITS;
bool binary[numBits];
const int myValue = 1;
std::bitset myBitset(myValue);
//for (int i = 0; i < NUM_BITS; i++)
//binary[i] = myBitset[i];
for(int i = 0, l = NUM_BITS; i < l; ++i) {
std::cout<< (binary[i]?'1':'0')<<" ";
}
std::cout<<"\n";
int mask = 1; // binary 10000000 00000000 ...
for (int i = 0, l = NUM_BITS; i < l; ++i) {
// binary & operation does
// AND logic operation for all corresponging bit
// so 0010&0011=0010
binary[i] = myValue & mask;
// move the bits in mask one to the right
mask = mask>>1;
}
for (int i = 0, l = NUM_BITS; i < l; ++i) {
std::cout<< (binary[i]?'1':'0')<<" ";
}
}
In the for loop highlighted in this code, the array "binary" is being accessed without being initialized first, which means you could potentially be reading garbage values. Since it is an array of boolean values, one option to initialize the variable to set all elements to false, which is shown below.
#include <iostream>
#include <bitset>
#define NUM_BITS 10
int main(int argc, char *argv[])
{
const int numBits = NUM_BITS;
bool binary[numBits] = {false};
const int myValue = 1;
std::bitset myBitset(myValue);
//for (int i = 0; i < NUM_BITS; i++)
//binary[i] = myBitset[i];
for(int i = 0, l = NUM_BITS; i < l; ++i) {
std::cout<< (binary[i]?'1':'0')<<" ";
}
std::cout<<"\n";
int mask = 1; // binary 10000000 00000000 ...
for (int i = 0, l = NUM_BITS; i < l; ++i) {
// binary & operation does
// AND logic operation for all corresponging bit
// so 0010&0011=0010
binary[i] = myValue & mask;
// move the bits in mask one to the right
mask = mask>>1;
}
for (int i = 0, l = NUM_BITS; i < l; ++i) {
std::cout<< (binary[i]?'1':'0')<<" ";
}
}
int convertTo10(char *num_26, int len) {
int tmp = 0; //hold the output
i; //indexing the input
while (len--) tmp = tmp * 26 + (num_26[i++]-'A');
return tmp;
}
In this case, because the variable "i" is not initialized, the expression num_26[i++] will lead to unpredictable behaviour.
int convertTo10(char *num_26, int len) {
int tmp = 0; //hold the output
i=0; //indexing the input
while (len--) tmp = tmp * 26 + (num_26[i++]-'A');
return tmp;
}
unsigned long hexToDec(String hexString) {
unsigned long ret;
for(int i=0, n=hexString.length(); i!=n; ++i) {
char ch = hexString[i];
int val = 0;
if('0'<=ch && ch<='9') val = ch - '0';
else if('a'<=ch && ch<='f') val = ch - 'a' + 10;
else if('A'<=ch && ch<='F') val = ch - 'A' + 10;
else continue; // skip non-hex characters
ret = ret*16 + val;
}
return ret;
}
Here the variable "ret" is uninitialized which means that the expression ret = ret*16 + val will lead to unpredictable results.
unsigned long hexToDec(String hexString) {
unsigned long ret=0;
for(int i=0, n=hexString.length(); i!=n; ++i) {
char ch = hexString[i];
int val = 0;
if('0'<=ch && ch<='9') val = ch - '0';
else if('a'<=ch && ch<='f') val = ch - 'a' + 10;
else if('A'<=ch && ch<='F') val = ch - 'A' + 10;
else continue; // skip non-hex characters
ret = ret*16 + val;
}
return ret;
}
#include <stdio.h>
#include <limits.h>
int main(void) {
printf("int has %d bytes\n",sizeof(int));
printf("long has %d bytes\n",sizeof(long));
int a = INT_MAX;
int b = 2;
long var1 = a;
long var2 = b;
long var3 = a * b;
long var4 = var1 * var2;
printf ("var3=%ld\n", var3);
printf ("var4=%ld\n", var4);
return 0;
}
The issue in this snippet is the return type of the sizeof method is size_t, which is an unsigned int and not used with %d. So in the fixed code, %zu is used instead.
#include <stdio.h>
#include <limits.h>
int main(void) {
printf("int has %zu bytes\n",sizeof(int));
printf("long has %zu bytes\n",sizeof(long));
int a = INT_MAX;
int b = 2;
long var1 = a;
long var2 = b;
long var3 = a * b;
long var4 = var1 * var2;
printf ("var3=%ld\n", var3);
printf ("var4=%ld\n", var4);
return 0;
}
#include <iostream>
#include <string>
int main()
{
int8_t matt = 0;
matt += 1;
char string[1];
sprintf(string, "%ld", matt);
std::cout << string << std::endl;
return 0;
}
Here the issue is that %ld is being used for an int8_t variable, which is not standard practice and could lead to issues later in terms of maintenance. In the fixed code, %d is used instead.
#include <iostream>
#include <string>
int main()
{
int8_t matt = 0;
matt += 1;
char string[1];
sprintf(string, "%d", matt);
std::cout << string << std::endl;
return 0;
}
...
if (ok.wasReleased()) {
switch (menu_current) {
case 0:
screen = 2;
break;
case 1:
screen = 3;
break;
case 2:
screen = 0;
break;
break;
}
drawScreen();
}
...
Here the last break statement essentially has no purpose and does not effect the behaviour of the switch statement, so it is safe to remove it.
if (ok.wasReleased()) {
switch (menu_current) {
case 0:
screen = 2;
break;
case 1:
screen = 3;
break;
case 2:
screen = 0;
break;
}
drawScreen();
}
...
#include <stdio.h>
int a = 1, b = 40;
int main() {
printf("1 << 40 = %d\n", 1 << 40);
printf("1 << 40 = %d\n", 1 << 40);
printf("1 << 40 = %d\n", 1 << 40);
printf("%d << %d = %d\n", a, b, a << b);
}
Here the value 1, which is an int and 32 bits, is being shifted by 40 bits, which will lead to undefined behaviour. To fix this, convert 1 to a larger data type before shifting.
#include <stdio.h>
int a = 1, b = 40;
int main() {
printf("1 << 40 = %llu\n", (unsigned long long)1 << 40);
printf("1 << 40 = %llu\n", (unsigned long long)1 << 40);
printf("1 << 40 = %llu\n", (unsigned long long)1 << 40);
printf("%d << %d = %d\n", a, b, a << b);
}
void setup() {
Serial.begin(9600);
}
void loop(){
int x = 0;
int p = 50;
int result;
result = p/x;
Serial.println(result);
delay(5000);
}
Here p is being divided by x which is initialized to zero, thus resulting in a divide by zero error. To fix this either change the value of x, or handle the divide by zero case as shown below.
void setup() {
Serial.begin(9600);
}
void loop(){
int x = 0;
int p = 50;
int result;
if (x != 0) {
result = p / x;
} else {
Serial.println("Error: Division by zero.");
}
Serial.println(result);
delay(5000);
}
#define CHANNELS 11
void setup()
{
Serial.begin(9600);
while(!Serial);
}
void loop()
{
char packet[CHANNELS + 1];
int values[CHANNELS];
// Read 12 bytes (11 bytes of data + 1 byte delimiter)
int nBytes = Serial.readBytes(packet, sizeof(CHANNELS + 1));
if(nBytes == CHANNELS + 1) {
// Convert bytes to ints;
for(int i=0; i<CHANNELS; i++) {
values[i] = packet[i];
}
} else {
// Error.
}
// Here you display your graph.
}
The issue in this code is that a calculation is being done inside of the sizeof function, which does not perform operations at runtime. So, calculations should be done outside of it. In the fixed code, packet is passed into sizeof instead since the packet char array has CHANNELS+1 elements, so the sizeof method should correctly return the size of that.
#define CHANNELS 11
void setup()
{
Serial.begin(9600);
while(!Serial);
}
void loop()
{
char packet[CHANNELS + 1];
int values[CHANNELS];
// Read 12 bytes (11 bytes of data + 1 byte delimiter)
int nBytes = Serial.readBytes(packet, sizeof(packet));
if(nBytes == CHANNELS + 1) {
// Convert bytes to ints;
for(int i=0; i<CHANNELS; i++) {
values[i] = packet[i];
}
} else {
// Error.
}
// Here you display your graph.
}
int ardprintf(char *str, ...)
{
int i, count=0, j=0, flag=0;
char temp[ARDBUFFER+1];
for(i=0; str[i]!='\0';i++) if(str[i]=='%') count++;
va_list argv;
va_start(argv, count);
for(i=0,j=0; str[i]!='\0';i++)
{
if(str[i]=='%')
{
temp[j] = '\0';
Serial.print(temp);
j=0;
temp[0] = '\0';
switch(str[++i])
{
case 'd': Serial.print(va_arg(argv, int));
break;
case 'l': Serial.print(va_arg(argv, long));
break;
case 'f': Serial.print(va_arg(argv, double));
break;
case 'c': Serial.print((char)va_arg(argv, int));
break;
case 's': Serial.print(va_arg(argv, char *));
break;
default: ;
};
}
else
{
temp[j] = str[i];
j = (j+1)%ARDBUFFER;
if(j==0)
{
temp[ARDBUFFER] = '\0';
Serial.print(temp);
temp[0]='\0';
}
}
};
Serial.println();
return count + 1;
}
#undef ARDBUFFER
#endif
Here, to ensure proper memory management and resource use, va_end should be called after using the variable arguments.
int ardprintf(char *str, ...)
{
int i, count=0, j=0, flag=0;
char temp[ARDBUFFER+1];
for(i=0; str[i]!='\0';i++) if(str[i]=='%') count++;
va_list argv;
va_start(argv, count);
for(i=0,j=0; str[i]!='\0';i++)
{
if(str[i]=='%')
{
temp[j] = '\0';
Serial.print(temp);
j=0;
temp[0] = '\0';
switch(str[++i])
{
case 'd': Serial.print(va_arg(argv, int));
break;
case 'l': Serial.print(va_arg(argv, long));
break;
case 'f': Serial.print(va_arg(argv, double));
break;
case 'c': Serial.print((char)va_arg(argv, int));
break;
case 's': Serial.print(va_arg(argv, char *));
break;
default: ;
};
}
else
{
temp[j] = str[i];
j = (j+1)%ARDBUFFER;
if(j==0)
{
temp[ARDBUFFER] = '\0';
Serial.print(temp);
temp[0]='\0';
}
}
};
Serial.println();
return count + 1;
}
va_end(argv)
#undef ARDBUFFER
#endif
int freeRam()
{
extern int __heap_start, *__brkval;
int v;
return (int)&v - (__brkval == 0 ? (int)&__heap_start : (int)__brkval);
}
uint8_t * stack()
{
uint8_t * ptr;
ptr = (uint8_t *)malloc(4);
free(ptr);
return ((uint8_t *)(SP));
}
uint8_t * heap()
{
uint8_t * ptr;
ptr = (uint8_t *)malloc(4);
free(ptr);
return(ptr);
}
This code attempts to return a pointer after it has been deallocated, and accessing that pointer would result in unexpected behaviour. To address this, the free(ptr) statement should be removed altogether from the function and the returned pointer should be deallocated outside after it has been used.
int freeRam()
{
extern int __heap_start, *__brkval;
int v;
return (int)&v - (__brkval == 0 ? (int)&__heap_start : (int)__brkval);
}
uint8_t * stack()
{
uint8_t * ptr;
ptr = (uint8_t *)malloc(4);
free(ptr);
return ((uint8_t *)(SP));
}
uint8_t * heap()
{
uint8_t * ptr;
ptr = (uint8_t *)malloc(4);
return(ptr);
}
byte* floatToByteArray(float f) {
byte* ret = malloc(4 * sizeof(byte));
unsigned int asInt = *((int*)&f);
int i;
for (i = 0; i < 4; i++) {
ret[i] = (asInt >> 8 * i) & 0xFF;
}
return ret;
}
The highlighted line assumes that representation of int in memory is compatible with float, which is not guaranteed and lead to portability issues. The fix below uses memcpy which ensures proper memory access and is more portable as it is a standard C library.
byte* floatToByteArray(float f) {
byte* ret = malloc(4 * sizeof(byte));
unsigned int asInt;
memcpy(&asInt, &f, sizeof(float));
int i;
for (i = 0; i < 4; i++) {
ret[i] = (asInt >> 8 * i) & 0xFF;
}
return ret;
}
const uint8_t PIN_A = 8;
const uint8_t PIN_B = 9;
const uint32_t HALF_PERIOD = 5000; // 5000 us
const float PHASE_SHIFT = 0.25; // 1/4 period
uint8_t state_A = LOW;
uint8_t state_B = LOW;
uint32_t last_A_toggle = 0;
uint32_t last_B_toggle = - 2 * HALF_PERIOD * PHASE_SHIFT;
void setup() {
pinMode(PIN_A, OUTPUT);
pinMode(PIN_B, OUTPUT);
}
void loop() {
uint32_t now = micros();
if (now - last_A_toggle >= HALF_PERIOD) {
last_A_toggle += HALF_PERIOD;
state_A = !state_A;
digitalWrite(PIN_A, state_A);
}
if (now - last_B_toggle >= HALF_PERIOD) {
last_B_toggle += HALF_PERIOD;
state_B = !state_B;
digitalWrite(PIN_B, state_B);
}
}
In this code, a potentially negative value is being converted to an unsigned integer. If the result of the expression is negative, it will be implicitly converted to an unsigned integer, which could lead to unexpected behavior. To address this, either change the type to signed (int32_t) or handle the negative result case separately like shown below.
const uint8_t PIN_A = 8;
const uint8_t PIN_B = 9;
const uint32_t HALF_PERIOD = 5000; // 5000 us
const float PHASE_SHIFT = 0.25; // 1/4 period
uint8_t state_A = LOW;
uint8_t state_B = LOW;
uint32_t last_A_toggle = 0;
int result = -2 * HALF_PERIOD * PHASE_SHIFT;
if (result < 0) {
// Handle the case where the result is negative
last_B_toggle = DEFAULT_VALUE;
} else {
last_B_toggle = (uint32_t)result;
}
void setup() {
pinMode(PIN_A, OUTPUT);
pinMode(PIN_B, OUTPUT);
}
void loop() {
uint32_t now = micros();
if (now - last_A_toggle >= HALF_PERIOD) {
last_A_toggle += HALF_PERIOD;
state_A = !state_A;
digitalWrite(PIN_A, state_A);
}
if (now - last_B_toggle >= HALF_PERIOD) {
last_B_toggle += HALF_PERIOD;
state_B = !state_B;
digitalWrite(PIN_B, state_B);
}
}
int result = -2 * HALF_PERIOD * PHASE_SHIFT;
if (result < 0) {
// Handle the case where the result is negative
// For example, set last_B_toggle to a specific value
last_B_toggle = DEFAULT_VALUE;
} else {
last_B_toggle = (uint32_t)result; // Safe to assign as result is non-negative
}