Skip to content

CO2 cleanup#25

Open
nturner01 wants to merge 2 commits intomainfrom
CO2-cleanup
Open

CO2 cleanup#25
nturner01 wants to merge 2 commits intomainfrom
CO2-cleanup

Conversation

@nturner01
Copy link
Copy Markdown

Added explicit reporting of any errors and fixed bug with missing increment for response array.

WIRE.requestFrom(address, 20, 1);
if (hasError()) return -1;
int error = checkError();
if (error != 1) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Bit of a subjective take here, but I think you should keep all the error handling logic in the hasError function and keep returning just true or false. The idea is to encapsulate as much as possible into the smallest units possible. If you want to check what type of error you got, or print the type of error, or do something else related to the error, that can still go into hasError() because it's not related to reading the actual co2. By having one function to handle and decide what to do when you get an error, you can reuse that logic.

If you want to send this error number to the dashboard though, that makes sense to have it return an int. Just be careful not to serial print things in the meantime as the serial lines are reserved for other (binary) communication and printing anything else will interfere

int error = checkError();
if (error != 1) {
Serial.print("CO2 Error: ");
Serial.println(error);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Prints for testing should be removed before you merge

Copy link
Copy Markdown

@Kenou2005 Kenou2005 left a comment

Choose a reason for hiding this comment

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

See comments

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.

4 participants