-
Notifications
You must be signed in to change notification settings - Fork 319
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
Plugin for CM1107 CO2 sensor #126
base: master
Are you sure you want to change the base?
Conversation
Plugin for CM1107 CO2 sensor
Create _P177_CM1107.ino
_P177_CM1107.ino
Outdated
|
||
boolean plugin_177_begin() | ||
{ | ||
Wire.begin(); |
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.
Do not let a plugin call Wire.begin()
It is already being dealt with in ESPeasy and this may disrupt other I2C communications of other plugins
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.
Removed
// Returns true (1) if successful, false (0) if there was an I2C error | ||
{ | ||
// Write auto calibration configuration | ||
Wire.beginTransmission(CM1107_ADDR); |
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.
Why not use the functions in the existing I2C.ino file?
This makes it much harder to fix bugs or change I2C related code later.
Also lots of plugins tend to do just the same, so better use the existing functions then it is also clear what's being done.
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 didn't find any function that would cover sensor's requirements, number of bytes to read and write at once.
_P177_CM1107.ino
Outdated
checksum += status; | ||
checksum += Wire.read(); //CS | ||
|
||
String log = F("CM1107: CO2 ppm:"); |
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.
Please wrap this log creation code in a check for logLevelActiveFor(LOG_LEVEL_INFO)
That will prevent unneeded memory allocation.
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.
Could you please expand how it's done?
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.
if(logLevelActiveFor(LOG_LEVEL_INFO)) {
String log = F("....");
log += bla;
addLog(LOG_LEVEL_INFO, log);
}
It will make less memory allocations if not needed.
String handling may cause memory fragmentation and memory allocation is a rather expensive operation.
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.
Thanks. It's now added to the code but compilers does not like it much. I checked other plugins using it and nothing special to include such function shuldn't be needed.
error: 'logLevelActiveFor' was not declared in this scope
_P177_CM1107.ino
Outdated
case PLUGIN_READ: | ||
{ | ||
plugin_177_begin(); | ||
co2 = plugin_177_getCO2(); |
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.
Where is this variable co2
declared?
If this code is working, then I'm really curious to see where it is being declared and what value is being messed with by this code.
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.
Fixed, missed declaration. Code worked because of same name global variable from my different plugin.
Plugin code for Cubic CM1107. Should be compatible also with CM1108. It's not easy to get the sensor and there are other options to get CO2 sensing. I needed to test the sensor for a coworker. I'm putting it here for anyone playing with the sensor.