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

T6713 CO2 Sensor Driver #355

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

T6713 CO2 Sensor Driver #355

wants to merge 5 commits into from

Conversation

odeysiusstuon
Copy link
Contributor

This branch contains just the files for the T6713 class, which acts as a driver for reading PPM values from the T6713 CO2 sensor.

Everything that needs to be documented in the code should have already been documented using the Doxygen format, which means documentation may be automatically generated using the Doxygen software.

I have also tested to make sure the class works properly with the sensor using Bradley's setup as seen in Payload's channel on Slack.

@PieFilling
Copy link

I approve this pull request.

Copy link

@PieFilling PieFilling left a comment

Choose a reason for hiding this comment

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

The sensor functions and returns adequate values with this code.

Copy link
Member

@WattsUp WattsUp left a comment

Choose a reason for hiding this comment

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

See feedback. Only have one location of the function headers. In the source file. Member variables don't need doxygen comments

* DEFAULT_T6713_SLAVE_ADDRESS
*/
T6713(I2C & i2c, uint8_t addr = DEFAULT_T6713_SLAVE_ADDRESS) :
i2c(i2c), addr(addr) {};
Copy link
Member

Choose a reason for hiding this comment

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

Why is the constructor defined here and not the source?

* @brief The slave address of the sensor to which the I2C should read/write
*
*/
uint8_t addr;
Copy link
Member

Choose a reason for hiding this comment

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

Is the address configurable? If so, make it an enum class of the allowable values, if not make it a static const member.

// T6713 CO2 Sensor I2C default slave address
const uint8_t DEFAULT_T6713_SLAVE_ADDRESS = 0x15 << 1;
// Same command found in the T67XX CO2 Sensor Module documentation, see pg. 29
static const char READ_CO2_COMMAND[5] = {0x04, 0x13, 0x8B, 0x00, 0x01};
Copy link
Member

Choose a reason for hiding this comment

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

Have these as part of the class instead of the default namespace

* @return mbed_error_status_t MBED_SUCCESS if the reading was successful,
* otherwise the error status of the reading
*/
mbed_error_status_t readPPM(int & outValue);
Copy link
Member

Choose a reason for hiding this comment

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

Use uint32_t for unsigned 32b integers. Aka specify the number of bits

@odeysiusstuon
Copy link
Contributor Author

Commit 9e5f480 should resolve the feedback.

By the way, this pull request should resolve both #212 and #213.

@odeysiusstuon
Copy link
Contributor Author

@WattsUp I'm not sure what you mean when you say

Only have one location of the function headers. In the source file.

Does this mean the function headers (in this case, just readPPM()) should only be in T6713.cpp? I've tried just removing the function header in T6713.h, but that gives an error: class "T6713" has no member "readPPM"

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

Successfully merging this pull request may close these issues.

3 participants