-
Notifications
You must be signed in to change notification settings - Fork 5k
Add support for AGS02MA TVOC Sensor #24109
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
base: development
Are you sure you want to change the base?
Conversation
| #define USE_AGS02MA // [I2cDriver 93] Enable AGS02MA Air Quality Sensor (I2C address 0x1A) | ||
|
|
||
| #endif // USE_I2C | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say that the new sensor driver should not by default be included in builds, hence the #define should be as a comment line, allowing the user to pick it in a custom build. Looks like you should have "I2CDriver95" instead of "I2CDriver 93" - same number as elsewhere, and no blank.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have made the changes mentioned:
Updated the number to I2CDriver 95
Commented the #define line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks better, but for consistency, I also mentioned the detail that it should be "I2cDriver95" with no blank. This is related to it being a command (where no space is allowed before the driver number), and also to make keyword searches easy. All the other drivers in the file use this syntax without inserting a space.
BTW. It would also be better to insert the line right after all the other [I2cDriverxx] lines in the file, instead of at the end of the whole USE_I2C block.
To align here, the line should start with "// " only two blanks instead of 4. This number of blanks is to enhance readability of the structure, where 4 blanks is used to nest one symbol with others in the same block, and 2 blanks with no nesting.
In both cases, the // can be overwritten with blanks when a feature is selected, like when default features already have 4 blanks before the #define, or 6 blanks if nested under a dependency symbol.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have updated the the code as mentioned
Device disabled by default
|
Thanks, this looks good to me after the last changes |
Description:
This adds support for the AGS02MA TVOC Sensor using the driver written by RobTillaart
Related issue (if applicable): N/A
Checklist:
NOTE: The code change must pass CI tests. Your PR cannot be merged unless tests pass