Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Minor update for code review #1

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Minor update for code review #1

wants to merge 2 commits into from

Conversation

jdVotingWorks
Copy link
Collaborator

.ino code ready for review, I don't know if this was the best way to initiate a code review of a new repo, my bad if not.

@CLAassistant
Copy link

CLAassistant commented Nov 15, 2023

CLA assistant check
All committers have signed the CLA.

fixed datatype on BAUDRATE to uint32_t
Used the global constant for ACKTIMOUT_MS
@@ -39,11 +40,12 @@ const uint8_t BUFFERSIZE = 100; //command buffer size
const uint8_t MAXRETRIES = 500; //maximum number of state alerts. not currently implemented
const uint16_t ACKTIMEOUT_MS = 5000; //time between alert spamming
const uint16_t DEBOUNCE = 1000; //button debounce time. not currenlty implemented.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const uint16_t DEBOUNCE = 1000; //button debounce time. not currenlty implemented.
const uint16_t DEBOUNCE = 1000; //button debounce time. not currently implemented.

@@ -113,23 +115,23 @@ Command waitForCMD() {
while (!Serial.available()) {}; // Wait until there is data on the serial
getTX(); // Read the data from serial

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getTX returns char*, but the return value (a pointer to txBuffer) is never used. Instead, you simply refer to the global txBuffer. Maybe make getTX return void?

Comment on lines +131 to +133
// int length = strlen(txBuffer); // Get the length of the received data
// char ackResponse = txBuffer[5]; // Extract the acknowledgment response
char ackResponse = ackResponseBuffer;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove commented out code instead?

} else if (strncmp(message, "ACK", 3) == 0) {
DEBUG_PRINTLN("ACK command received.");
return CMD_ACK;
if (length > 5) ackResponseBuffer = txBuffer[5]; //check if the ack command has a char response
else ackResponseBuffer = '0'; // if not, add a 0 to teh ackreponse, which is an unknown ack

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
else ackResponseBuffer = '0'; // if not, add a 0 to teh ackreponse, which is an unknown ack
else ackResponseBuffer = '0'; // if not, add a 0 to the ackreponse, which is an unknown ack

@@ -178,30 +180,34 @@ char* getTX() {

Command parseTX(char* strBuffer) {
// Function to parse the transmitted data and extract command
Copy link

@eventualbuddha eventualbuddha Nov 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change to also take a uint8_t argument that is the size of the buffer and pass that along to strnlen instead of strlen? Using strlen with a buffer that contains no zero bytes results in undefined behavior. I know you're being careful in getTX and resetBuffer to avoid such a situation, but I'd argue for defense-in-depth here. Or instead of taking arguments, just refer to txBuffer and BUFFERSIZE directly in this function.

char message[length - 2 + 1]; // Buffer to store the actual message excluding '<' and '>'
Command retCMD; //return variable
uint8_t length = strlen(strBuffer); // Check if the data is enclosed in '<' and '>'
char message[length - 2 + 1]; // Buffer to store the actual message excluding '<' and '>'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider moving this into the if statement below to avoid a situation where you try to declare a char array with negative length, i.e. if length is 0. Doing so would lead to undefined behavior.

char message[length - 2 + 1]; // Buffer to store the actual message excluding '<' and '>'
Command retCMD; //return variable
uint8_t length = strlen(strBuffer); // Check if the data is enclosed in '<' and '>'
char message[length - 2 + 1]; // Buffer to store the actual message excluding '<' and '>'
if (length > 2 && strBuffer[0] == '<' && strBuffer[length - 1] == '>') {
strncpy(message, strBuffer + 1, length - 2); // Copy the message to buffer

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is kinda confusing, as you're actually copying strBuffer to message. In general I feel like there are a few too many comments that might easily get out of sync with the code they're commenting on.

char message[length - 2 + 1]; // Buffer to store the actual message excluding '<' and '>'
Command retCMD; //return variable
uint8_t length = strlen(strBuffer); // Check if the data is enclosed in '<' and '>'
char message[length - 2 + 1]; // Buffer to store the actual message excluding '<' and '>'
if (length > 2 && strBuffer[0] == '<' && strBuffer[length - 1] == '>') {
strncpy(message, strBuffer + 1, length - 2); // Copy the message to buffer
message[length - 2] = '\0'; // Null-terminate the message

// Compare the message with predefined commands and return the corresponding enum value
if (strcmp(message, "ARM") == 0) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above, I'd argue that we should use strncmp instead of strcmp.

} else if (strncmp(message, "ACK", 3) == 0) {
DEBUG_PRINTLN("ACK command received.");
return CMD_ACK;
if (length > 5) ackResponseBuffer = txBuffer[5]; //check if the ack command has a char response

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This references txBuffer rather than strBuffer. I know in practice they're always the same, but we should pick one and stick with it. Also, consider inspecting message instead of the full buffer since the difference between the two could easily lead to off-by-1 errors.

@@ -113,23 +115,23 @@ Command waitForCMD() {
while (!Serial.available()) {}; // Wait until there is data on the serial
getTX(); // Read the data from serial
Command parsed = parseTX(txBuffer); // Parse the command from the data
resetBuffer(); // Reset the buffer for the next command
return parsed; // Return the parsed command
return parsed; // Return the parsed command
}

void spamAndAck(char alert) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider replacing the recursion here with a loop. It seems like it'd be quite easy to cause a crash via stack overflow with enough unexpected responses. I know you already pointed this possibility out in Slack, but I'm inclined to say we should fix it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants