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

Plugin for CM1107 CO2 sensor #126

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

Conversation

V0JT48
Copy link
Contributor

@V0JT48 V0JT48 commented Feb 4, 2019

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.

Plugin for CM1107 CO2 sensor
_P177_CM1107.ino Outdated

boolean plugin_177_begin()
{
Wire.begin();
Copy link
Member

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

Copy link
Contributor Author

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);
Copy link
Member

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.

Copy link
Contributor Author

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:");
Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Member

@TD-er TD-er Jul 22, 2019

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.

Copy link
Contributor Author

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();
Copy link
Member

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.

Copy link
Contributor Author

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.

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.

2 participants