nadavmatalon / MCP3221

MCP3221 Driver (12-BIT Single Channel ADC with I2C Interface)
10 stars 9 forks source link

multiple MCP3221s in one sketch #2

Closed AlesSt closed 6 years ago

AlesSt commented 6 years ago

HI!

First of all GREAT work. You really made all features available. But (there's always a but :) ) I have a problem with running multiple MCPs on one Pro Mini 3.3V 8MHz.

I followed instructions - sorry if asking a stupid question - but as long as I run only one MCP like:

#include "MCP3221.h"

const byte SENS1_ADDR = 0x4D;                            // I2C address of the MCP3221 (Change as needed)
const byte SENS2_ADDR = 0x4C;                            // I2C address of the MCP3221 (Change as needed)
MCP3221 S1(SENS1_ADDR);
//MCP3221 S2(SENS2_ADDR);

unsigned long timeNow;

void setup() {
  Serial.begin(9600);
  Wire.begin();
  Serial.print(F("\n\nserial is open\n\n"));
  S1.setVref(3300);
  S1.setVinput(VOLTAGE_INPUT_5V);
  S1.setRes1(10000);
  S1.setRes2(4700);
  S1.setAlpha(178);
/*
  S2.setVref(3300);
  S2.setVinput(VOLTAGE_INPUT_5V);
  S2.setRes1(10000);
  S2.setRes2(4700);
  S2.setAlpha(178);
*/

timeNow = millis();

void loop () {
  if (millis() - timeNow >= 10000) {
    timeNow = millis();
    Serial.print(S1.getVoltage());
    //Serial.print(S2.getVoltage());
  }
}

I get results that are consistent with DMM measurements on AIN. Soon as I uncomment ONLY //MCP3221 S2(SENS2_ADDR);

readings on S1 are 0.

If I uncomment everything, then second sensor works fine. Also tried making a measurement on S1 and then ~MCP3221 S1(); and afterward start S2 and also destroying it and loop it like that - no go :)

Any thoughts or help would be REALLY apreciated :)

BR Ales

nadavmatalon commented 6 years ago

Hi Ales,

Thanks for the kind words :-)

I'd be happy to try and help.

As best I can remember (I wrote this library a long time ago...), I tested the code with 2 sensors before publishing it and it worked as intended. But perhaps I missed something somewhere along the way.

I also had a look at the code you quoted and it looks fine too, so let's start by trying to rule out a few things:

This comment makes me a bit worried (or perhaps confused would be a better word) : Soon as I uncomment ONLY //MCP3221 S2(SENS2_ADDR); Merely constructing another sensor object shouldn't interfere with the operation of anything else so that's weird. Also, constructing and deconstructing objects in a loop like you describe doesn't sound to me like a good way to go about this, even if it somehow worked (you seem to share my opinion on this, I think).

Another solution I would try is adding some delays between operations, particularly between reading from the first sensor and the second one. Perhaps the sensors don't have enough time to generate a reading and that's what's causing the problem. You'll need to play with this one a bit (i.e. different delay periods) and possibly consult the sensor's datasheet to find out how much time is minimally required for a good reading.

Apart from that, when debugging something like this in general I find it best to try and break things down into small operations and test things individually so as to get a better picture of exactly what goes wrong and where and through that pin out the specific problem/s.

Let me know how goes and we can take it from there.

All the best, Nadav

nadavmatalon commented 6 years ago

Forgot to mention, you note that you're running the sensors on a Pro Mini 3.3V 8MHz. I don't have any experience with this particular MCU and only tested the library with a regular arduino so the problem might lay in the fact that you're using a different device with a different operating voltage (3.3V vs 5V) and different cpu frequency (8MHz vs 16MHz). If you have an arduino on hand, it might be a good idea to try and run the 2 sensors with it and see how it works. If nothing else, this would help in ruling out the possibility that the problem stems from the difference in devices.

AlesSt commented 6 years ago

Hi, thanks for swift answers....i hoped you forgot to mention something that is important for operation with 2 mcps :) tried everything else (what you suggested - separatly running them - scanned addresses did it all......no problem when only one but as soon as i just start the second instance....it stops :) must be that they changed something in IDE or some libs that you use (like Wire or something) so that it stopped working for two :)

But good point the second msg....will try on some 5V arduinohave some of them :) will let you know how will everything evolve.

Again REALLY appreciate the work you did with it and also the swiftness :)

BR Ales

AlesSt commented 6 years ago

Hi there :)

Sorry for long delay.......not using millis :) Have tried everything with 5V nano but as soon as I declare second MCP unit, first one stops working....well it kindda "moves" as the second one does (talking about mV :) - so i guess there is a problem with something in lib....my guess is they changed something in IDE as I am using 1.8.1 (outdated........but don't have the balls to upgrade..... :)

BR Ales PS I am still not done with this :) will keep you - and community updated with findings :)

AlesSt commented 6 years ago

This is how it is working :)

#include "MCP3221.h"

const byte NH3_ADDR = 0x4D;                            // I2C address of the MCP3221 (Change as needed)
const byte O2_ADDR = 0x4C;                            // I2C address of the MCP3221 (Change as needed)

unsigned long timeNow;

unsigned int O2mV = 0;
unsigned int O2 = 0;
unsigned int NH3mV = 0;
unsigned int NH3 = 0;

void setup() {
  Serial.begin(9600);
  Wire.begin();
  Serial.print(F("\n\nserial is open\n\n"));
  timeNow = millis();
}

void loop() {
  if (millis() - timeNow >= 1000) {
    NH3meritev();
    O2meritev();
    Serial.print(F("O2: "));
    Serial.print(O2mV);
    Serial.print(F("mV  O2: "));
    Serial.print(O2);
    Serial.print(F("%  "));
    Serial.print(F("NH3: "));
    Serial.print(NH3mV);
    Serial.print(F("mV  NH3: "));
    Serial.print(NH3);
    Serial.print(F("ppm  \n"));
    timeNow = millis();
  }
}

void NH3meritev() {
  MCP3221 NH3_SENSOR(NH3_ADDR);
  NH3_SENSOR.setVref(5000);                            // sets voltage reference for the ADC in mV (change as needed)
  NH3_SENSOR.setVinput(VOLTAGE_INPUT_5V);              // sets voltage input type to be measured (change as needed)
  NH3_SENSOR.setSmoothing(NO_SMOOTHING);
  NH3mV = NH3_SENSOR.getVoltage();
  NH3 = map (NH3mV, 20, 5000, 0, 166);
}

void O2meritev() {
  MCP3221 O2_SENSOR(O2_ADDR);
  O2_SENSOR.setVref(5000);                            // sets voltage reference for the ADC in mV (change as needed)
  O2_SENSOR.setVinput(VOLTAGE_INPUT_5V);              // sets voltage input type to be measured (change as needed)
  O2_SENSOR.setSmoothing(NO_SMOOTHING);
  O2mV = O2_SENSOR.getVoltage();
  O2 = map (O2mV, 20, 5000, 0, 50);
}
nadavmatalon commented 6 years ago

Hi Ales, Very cool! well-done for getting it sorted out :-) But I'm curious, what did you change to make it work now? What was missing/wrong before? All the best, Nadav

AlesSt commented 6 years ago

Hi,

if you check the last code i've sent.....i am declaring MCP3221 O2_SENSOR(O2_ADDR); each time before getVoltage for oxygen and declaring MCP3221 NH3_SENSOR(NH3_ADDR); before ammonia getVoltage

if i declare both at setup {} then only last declared is working......i guess something in library is locking to the last declared.

nadavmatalon commented 6 years ago

Hi Ales,

Ahhh, now I see. I guess I was too tired & distracted when I looked at the code before because I missed it completely.

To my mind this is actually more of a workaround than a fix given that the loop is constantly constructing new sensor objects. Note that this approach could potentially lead to serious memory problems down the line, so it might be worth adding an object destructor after each reading, though I think it would be better to find the actual problem and fix it.

I've had another look at the library itself and I still can't get why it doesn't work for you as expected. There shouldn't be any need to constantly construct new objects for each reading.

I'm now baffled and curious enough to go and test this myself. I'll hook up a quick circuit to try to figure out what the problem is and let you know what I find (though it might take me a bit of time).

All the best, Nadav

AlesSt commented 6 years ago

Hi Nadav,

what is the proper syntax for destructing an object?

I tried almost everything but nothing passes........ BR Ales

nadavmatalon commented 6 years ago

Hi Ales,

This evening I finally sat down to try and figure out what's going on with this library, and then discovered that all 3 of my MCP3221 ICs have the exact same I2C address... so ordered another one with a different address and will continue with this when it arrives in a few day's time.

To your questions, the general structure of a destructor function looks like this (it's located in the *.h file under the constructor function):

~MCP3221()

Hence, if you have the constructor at the top of your sketch like this:

const byte DEV_ADDR = 0x4D; MCP3221 mcp3221(DEV_ADDR);

Then in 'setup' or 'loop', this following should destruct that specific object:

mcp3221.~MCP3221();

NB you can't put the destructor line at the top of the sketch (i.e. before 'setup') as the compiler will throw an error (I seem to remember this has to do with how all functions, apart from constructors, are processed, but not sure).

Hope this helps :-) Nadav

nadavmatalon commented 6 years ago

Hi Ales,

Took some time, but problem found!

Tried finding the bug myself and couldn't so after several days went on the Arduino forum with this. Had to take a bit of crap from various 'experts', but in the end a helpful soul came to the rescue and pointed out the problematic line which is this (in the *.cpp file):

_samples[MAX_NUM_SAMPLES] = { 0 };

If you change it to this:

_samples[MAX_NUM_SAMPLES];

It should work as intended with 2 devices.

I tested it on my setup just now and it does what it should, but would be great if you could verify this on your side too.

As to why this particular line caused so much trouble, I don't know yet. It looks fine to me and I certainly didn't see the possibility that a private array inside one object would be able to interfere with the properties of another object... I've asked the guy who spotted the problem to explain and hopefully he'll be able to enlighten me on the subject.

Lastly, I haven't made the correction in the library yet, but will do so after I understand the problem better.

All the best, Nadav

nadavmatalon commented 6 years ago

Actual fix applied to the constructor:

memset(_samples, 0, sizeof(_samples));

AlesSt commented 5 years ago

Just going through some waaaaay overdue forum posts :) and noticed i havent thanked you :) GREAT work will test again since i will need this in my next home automation upgrade :)

Oh and yeah i know them "experts". but usually they are just frustrated because others crap on them. I always write i am noob or sorry for silly question or something.......usually you can quickly see to where the knowledge of the person asking goes......and them "experts" they answer you with just you should just do "jadhfkjdacnasdkjFDJSKGKJ" well kindda gibberish :) so the answer of no help :)

Ok back to the subject: _samples line and memset line should be corrected? or i guess I can just download new version :)

THANK YOU for great job and enjoy :)