Go Down

Topic: New and growing well-documented, feature-complete I2C device library (Read 21670 times) previous topic - next topic

retrolefty

#15
Aug 05, 2011, 03:38 am Last Edit: Aug 05, 2011, 05:24 am by retrolefty Reason: 1
Jeff;

I found some time to try out the ADS1115 library you developed, but I seem to have a problem. I made a minimlist sketch for just the init function (which is a do nothing) and the test connection, to see if the device is seen:

Code: [Select]

#include <Wire.h>
#include "ADS1115.h"
#include "I2Cdev.h"
ADS1115 adc1115;

#define LED_PIN 13
bool blinkState = false;

void setup() {
   // join I2C bus
   Wire.begin();
   
   // initialize serial communication
   Serial.begin(38400);
   
   // initialize all devices
   Serial.println("Initializing I2C devices...");
   adc1115.initialize();
 
   Serial.println("Testing device connections...");
   Serial.println(adc1115.testConnection() ? "ADS1115 connection successful" : "ADS1115 connection failed");
   pinMode(LED_PIN, OUTPUT);
}

void loop() {
   delay(100);
   blinkState = !blinkState;
   digitalWrite(LED_PIN, blinkState);
}


It seems to fail, here with debug enabled:

Quote

Initializing I2C devices...
Testing device connections...
I2C (0x48) reading 1 words from 0x0.... Done (0 read).
ADS1115 connection failed


I think the device is avalible because I can run a simple I2C address scanner sketch and get:
Quote

I2CScanner ready!
starting scanning of I2C bus from 1 to 7F...Hex
addr: 1          addr: 2          addr: 3          addr: 4      
addr: 5          addr: 6          addr: 7          addr: 8      
addr: 9          addr: A          addr: B          addr: C      
addr: D          addr: E          addr: F          addr: 10      
addr: 11          addr: 12          addr: 13          addr: 14      
addr: 15          addr: 16          addr: 17          addr: 18      
addr: 19          addr: 1A          addr: 1B          addr: 1C      
addr: 1D          addr: 1E          addr: 1F          addr: 20      
addr: 21          addr: 22          addr: 23          addr: 24      
addr: 25          addr: 26          addr: 27          addr: 28      
addr: 29          addr: 2A          addr: 2B          addr: 2C      
addr: 2D          addr: 2E          addr: 2F          addr: 30      
addr: 31          addr: 32          addr: 33          addr: 34      
addr: 35          addr: 36          addr: 37          addr: 38      
addr: 39          addr: 3A          addr: 3B          addr: 3C      
addr: 3D          addr: 3E          addr: 3F          addr: 40      
addr: 41          addr: 42          addr: 43          addr: 44      
addr: 45          addr: 46          addr: 47          addr: 48 found!
addr: 49          addr: 4A          addr: 4B          addr: 4C      
addr: 4D          addr: 4E          addr: 4F          addr: 50      
addr: 51          addr: 52          addr: 53          addr: 54      
addr: 55          addr: 56          addr: 57          addr: 58      
addr: 59          addr: 5A          addr: 5B          addr: 5C      
addr: 5D          addr: 5E          addr: 5F          addr: 60      
addr: 61          addr: 62          addr: 63          addr: 64      
addr: 65          addr: 66          addr: 67          addr: 68      
addr: 69          addr: 6A          addr: 6B          addr: 6C      
addr: 6D          addr: 6E          addr: 6F          addr: 70      
addr: 71          addr: 72          addr: 73          addr: 74      
addr: 75          addr: 76          addr: 77          addr: 78      
addr: 79          addr: 7A          addr: 7B          addr: 7C      
addr: 7D          addr: 7E          addr: 7F          
done


Any thoughts?

Thanks, Lefty

gknight4

Activity in the Libraries is always a good thing!

I'd like to see TWI more interrupt driven - so that you could readFrom a device and do other stuff while the 100Khz bus is doing its thing, rather than having code like this (twi.c, twi_readFrom):

  while(TWI_MRX == twi_state){
    continue;
  }

Which sits there and does nothing until the truly interrupt-driven code is finished.

I'm going to be making changes to these routines for my own purposes (narrow focus), but I'd be happy to share with your much more general focus.

Thumbs Up!
Gene Knight

Jeff Rowberg

#17
Aug 05, 2011, 05:34 am Last Edit: Aug 05, 2011, 05:44 am by Jeff Rowberg Reason: 1

I found some time to try out the ADS1115 library you developed, but I seem to have a problem. I made a minimlist sketch for just the init function (which is a do nothing) and the test connection, to see if the device is seen...Any thoughts?


Hmm, I was afraid of this. A library written with no hardware to test that implements a new set of untested 16-bit register functions--not a good combination! It's possible that my testConnection() method is not written correctly, for one thing. The ADS1115 doesn't have a designated test function or "internal ID" register that some others have, so it's really just trying to read the conversion register once to see if it gets data back. I'd try three things to test further:

1. Try a short delay between the initialize() call and the testConnection() call--maybe 100ms or 200ms to be safe I don't know if this is necessary based on the info in the datasheet, but it's one less possible cause if the delay's there.

2. Try some of the other methods in the class, like reading the differential or one of the config settings to see if anything comes back.

3. Use a raw I2Cdev class call to test words vs. bytes, such as:

Code: [Select]

uint16_t wBuf[1];
int count = I2Cdev::readWord(0x48, ADS1115_RA_CONFIG, wBuf);
Serial.println(count); // display # of words read, should be 1 if working

uint8_t bBuf[2];
count = I2Cdev::readBytes(0x48, ADS1115_RA_CONFIG, 2, bBuf);
Serial.println(count); // display # of bytes read, should be 2 if working


Without knowing more, I'd be inclined to blame my 16-bit register functions because I have no way of testing them with my hardware. Those three things above should shed some light on where the problem actually is.


Activity in the Libraries is always a good thing! I'd like to see TWI more interrupt driven - so that you could readFrom a device and do other stuff while the 100Khz bus is doing its thing, rather than having code like this (twi.c, twi_readFrom)...which sits there and does nothing until the truly interrupt-driven code is finished.


I would also definitely like to go this direction. Ideally, we'd move away from the Wire implementation into something simpler that could be bundled with the I2Cdev class. This would make it really easy to use with non-Arduino projects that are only using avr-gcc without the rest of the support libraries. I've got a couple of #defineable IMPLEMENTATION constants to allow for switching between different implementations, but so far only the Wire one has any support. An interrupt-driven approach would be very useful, though all the read* and write* functions would be done very differently so they don't block and return, but instead have handler/event methods assigned.


I'd be happy to share with your much more general focus.


Anything you want to send my way would be excellent. Thanks!

   Jeff

retrolefty

Quote
3. Use a raw I2Cdev class call to test words vs. bytes, such as:


So I added the two raw calls you suggested:

Quote

Initializing I2C devices...
Testing device connections...
I2C (0x48) reading 1 words from 0x0.... Done (0 read).
ADS1115 connection failed

I2C (0x48) reading 1 words from 0x1.... Done (0 read).
0

I2C (0x48) reading 2 bytes from 0x1...85 83 . Done (2 read).
2


So it appears that reading bytes works, but 1 word fails?

Lefty


Jeff Rowberg

#19
Aug 05, 2011, 07:28 am Last Edit: Aug 05, 2011, 07:29 am by Jeff Rowberg Reason: 1

So it appears that reading bytes works, but 1 word fails?


Ah, I think I found the bug, based on your info: the readWords() method was relying on an unsigned 8-bit count variable that was being decremented once in the middle of the for() loop so the MSB and LSB could both be written to the same word index in the read buffer. The loop condition also tests to make sure the count < length, so we don't read more bytes/words than the user asked for. The problem was that count starts at 0, and since it's unsigned, count - 1 = 0xFF, which is always >= length. Oops.

I've adjusted the code so that the loop works differently so negative values are never required, and also made the count variable signed to support -1 return values, which occurs in a timeout condition. The changes are up in the repository now.

   Jeff

retrolefty

Thanks Jeff;

Unfortunately the results appear to remain the same:

Quote

Initializing I2C devices...
Testing device connections...
I2C (0x48) reading 1 words from 0x0.... Done (0 read).
ADS1115 connection failed
I2C (0x48) reading 1 words from 0x1.... Done (0 read).
0
I2C (0x48) reading 2 bytes from 0x1...85 83 . Done (2 read).
2



Lefty

Jeff Rowberg


Thanks Jeff; Unfortunately the results appear to remain the same.


Drat. But I have good news! The results of successful readBytes() and unsuccessful readWords(), now that I've thought about the info staring me in the face, changed my assumption about always requesting the number of bytes needed, even when working with 16-bit registers. The readWords() method was requesting one byte (because I thought "one" on that hardware would actually mean one word), and therefore failing because it was only ever receiving one byte. I've adjusted that call to request length * 2 bytes and pushed the changes up. Try it now.

(Incidentally, if this doesn't work, I won't be able to try another fix until tomorrow...almost 2am here, heh.)

    Jeff

retrolefty

Quote
(Incidentally, if this doesn't work, I won't be able to try another fix until tomorrow...almost 2am here, heh.)

Don't knock yourself out, I'm sure you have a day job to deal with. Don't try to keep up with the hours us old retired guys can keep. ;)

The latest changes results in compiler errors:

Quote


C:\Documents and Settings\Primary Windows User\My Documents\Arduino\libraries\ADS1115\I2Cdev.cpp: In static member function 'static int8_t I2Cdev::readWords(uint8_t, uint8_t, uint8_t, uint16_t*, uint16_t)':
C:\Documents and Settings\Primary Windows User\My Documents\Arduino\libraries\ADS1115\I2Cdev.cpp:225: error: ISO C++ says that these are ambiguous, even though the worst conversion for the first is better than the worst conversion for the second:
C:\Documents and Settings\Primary Windows User\My Documents\My Programs\Arduino\arduino-0022\libraries\Wire/Wire.h:53: note: candidate 1: uint8_t TwoWire::requestFrom(int, int)
C:\Documents and Settings\Primary Windows User\My Documents\My Programs\Arduino\arduino-0022\libraries\Wire/Wire.h:52: note: candidate 2: uint8_t TwoWire::requestFrom(uint8_t, uint8_t)



Thanks once again, I wish I was more competent with C++ classes and such.;
Lefty

retrolefty

Well progress it seems. I didn't really know what I was doing but your last change (the lenght * 2) did seem to be the source of the compiler errors, so I changed your latest change to this:

Code: [Select]

    Wire.beginTransmission(devAddr);
    //Wire.requestFrom(devAddr, length * 2); // length=words, this wants bytes, but length * 2 bombs 
                                                              // compiler with strange geek errors
    length = length * 2;                               // so put it on it's own line
    Wire.requestFrom(devAddr,length); // length=words, this wants bytes, and changed back to this, seems 
                                                     //to work!



And now when run it displays this:

Quote
Initializing I2C devices...
Testing device connections...
I2C (0x48) reading 1 words from 0x0...0 . Done (1 read).
ADS1115 connection successful
I2C (0x48) reading 1 words from 0x1...8583 . Done (1 read).
1
I2C (0x48) reading 2 bytes from 0x1...85 83 . Done (2 read).
2


So I'll try and use the higher order functions tomorrow and see if I can actually do some A/D conversions.

Thanks a million;

Lefty

Jeff Rowberg


Well progress it seems. I didn't really know what I was doing but your last change (the lenght * 2) did seem to be the source of the compiler errors...


Eureka! Good call on that fix, and I'm glad you have something to work with now. I did compile it in my avr-gcc-based project, which currently has an incomplete TwoWire implementation hacked together. It worked fine there, but I didn't test it in the Arduino sketch until this morning, at which point the error you mentioned showed up. The problem was that the TwoWire::requestFrom() method has at least two unique parameter lists (a.k.a. it's overloaded), which are:

Code: [Select]

TwoWire::requestFrom(uint8_t, uint8_t);
TwoWire::requestFrom(int, int);


I believe that second (int, int) one is equivalent to (int16_t, int16_t) using C99 types. Basically, one uses unsigned 8-bit integer, and one uses signed 16-bit integers. The call I have in the I2Cdev::readWords() method uses a specifically declared uint8_t length variable, which was fine up until I added that inline "length * 2" as an argument. If you take an 8-bit integer and multiply it by two, you might end up with a 9-bit value, which would need to be stored in a 16-bit integer container (since the only viable options are 8-bit and 16-bit, and it obviously won't fit in an 8-bit one). Once this potential data type conversion takes place, the function is called like this:

Code: [Select]

TwoWire::requestFrom(uint8_t, int);


...which is not one of the candidates, and requires another conversion of the first parameter to make it work. It could go in either direction, trying to fit (and potentially clip) a 9-bit unsigned integer into an 8-bit unsigned container (uint8_t, uint8_t), or it could expand the unsigned 8-bit first parameter into a larger signed 16-bit container (int, int). Apparently ISO C++ doesn't like to make such a choice arbitrarily.

So, because the length variable is really supposed to contain the number of words that you want, and your fix changes it permanently to the number of bytes, I left it the way I had it and typecast it back into an unsigned 8-bit value:

Code: [Select]

Wire.requestFrom(devAddr, (uint8_t)(length * 2)); // length=words, this wants bytes


Problem solved, and pushed to the repository. The only caveat here is that you can't request more than 127 words at a time. I have no idea whether this is a legitimate concern, but with the devices in the library so far, it isn't even remotely close to being a problem.

    Jeff

retrolefty

Thanks Jeff for the great explanation, and of course your latest 'fix' compiles and runs fine. I'm looking forward to playing with this new device and your library over the weekend (playing with the grandkids today mostly). Not sure I will be able to validate all the features, the Comparator modes are not real clear to me with it's:

Quote

COMP_MODE: Comparator mode (ADS1114 and ADS1115 only)
This bit controls the comparator mode of operation. It changes whether the comparator is implemented as a traditional comparator (COMP_MODE = '0') or as a window comparator (COMP_MODE = '1'). It serves no
function on the ADS1113.
0 : Traditional comparator with hysteresis (default)
1 : Window comparator

and it's:

0 : Non-latching comparator (default)
1 : Latching comparator


Lefty

Jeff Rowberg


I think having a one second default is a better idea. It won't block unless the device doesn't respond and in that case does it matter if the error is returned in one second rather than 250ms? (in the previous code, the error condition would cause it to block indefinitely)

The advantage of a longer default time-out is that inexperienced users won't get false errors when using devices that do take longer than 250ms. Experienced users can increase or decrease the time-out if they want, but it seems more friendly to have a default that minimizes the chance that errors could be reported even when the system if functioning correctly. The longer default time-out has no impact on performance on systems where the I2C devices are responding correctly.


Your last statement here (longer timeout values have no impact on working systems) is definitely true, and it means that I don't have a huge reason not to increase the default timeout to 1 sec. But let me elaborate on my interpretation of the situation in one last effort to stomp my proverbial foot like a spoiled three-year-old so I can still get my own way: (kidding)

The I2Cdev class is the convenience layer between actual device libraries and the TwoWire I2C implementation library (or alternative implementation in the future). This is where the default timeout of 250ms is defined currently, and it's the I2Cdev's read* methods that allow for alternate timeout values as part of their arguments. The I2Cdev object itself is not intended to be used by itself in your projects (though you could if you really wanted to). Instead, it's designed to be used by device libraries like the ADXL345 class or ITG3200 class. These classes wrap the I2C communication into more intelligible methods, like "getAcceleration(x, y, z)" or "getRotation(x, y, z)". The implementations of these functions, per device and per class method, can individually set their own timeouts.

The important bit here is that all of the I2Cdev calls and which device operations require which timeouts should be entirely transparent to the person implementing any particular device class in a project, a.k.a. the "inexperienced users" who would likely make a mistake with the timeout setting. The only time anyone has to worry about specific timeout settings is if they are actually writing a new device class, in which case it seems very likely that they would know to specify an extra long timeout value in their "observeTortoiseMarathonPath()" method.

For this reason, it seems more useful overall to have a legitimate failure come back faster rather than slower. But honestly, I'm willing to change it to 1 sec if you would still recommend it in light of the viewpoint I laid out above (and maybe you clearly understood all of that before and my explanation was redundant anyway). No hard feelings in either case; I'm just trying to be diligent.


Thanks Jeff for the great explanation, and of course your latest 'fix' compiles and runs fine. I'm looking forward to playing with this new device and your library over the weekend (playing with the grandkids today mostly). Not sure I will be able to validate all the features, the Comparator modes are not real clear to me...


Lefty, this page has some very good info on typical comparator features, including hysteresis and latching:

http://www.analog.com/library/analogDialogue/archives/34-07/comparators

Some of it is kind of hard for me to parse through since I'm still more of a software guy than a hardware guy, but if I understand correctly, the nutshell version is that using hysteresis instead of a pure window mode helps reduce the effects of noise and prevents rapid negative/positive output "bouncing" near 0V levels, and latching mode will cause the ALERT_RDY pin to remain high until the data is read even if the actual state changes before it is read. I could be wrong about either of these points, and I welcome clarification from anyone who actually knows what they are talking about.

Don't worry about testing every bit of functionality unless you actually want to; as long as the basic I/O and core functions work for you, that's the important part, and it's a good indication that the rest of the library should work as defined (unless I made a typo, which has happened on numerous previous occasions).

    Jeff

retrolefty

I put together a 'testing sketch' for my ADS1115 A/D converter using your I2C libraries and so far I'm pleased with the results of the hardware and have not yet found a problem with your libraries.

Code: [Select]
/* Basic testing and check out sketch for a TI ADS1115 4 channel,16 bit, I2C, analog to digital converter chip
   Leftyretro 08/06/11
   With thanks to Jeff Rowberg for the development of the ADS1115 and I2Cdev I2C libraries
*/
#include <Wire.h>
#include "ADS1115.h"
#include "I2Cdev.h"

ADS1115 adc;

void setup()
  {
    Wire.begin();  // join I2C bus
    Serial.begin(38400); // initialize serial communication
    Serial.println("Initializing I2C devices...");
    adc.initialize(); // initialize ADS1115 16 bit A/D chip
    Serial.println("Testing device connections...");
    Serial.println(adc.testConnection() ? "ADS1115 connection successful" : "ADS1115 connection failed");
    Serial.println("   ");
   
//  select desired conversion method
    adc.setMode(ADS1115_MODE_CONTINUOUS); // free running conversion
    // adc.setMode(ADS1115_MODE_SINGLESHOT); // single conversion

// select desired measurement range
    adc.setGain(ADS1115_PGA_6P144);  // +/- 6.144 range, .0001875 volts/step
    // adc.setGain(ADS1115_PGA_4P096);  // +/- 4.096 range, .000125 volts/step
    // adc.setGain(ADS1115_PGA_2P048);  // +/- 2.048 range, .0000625 volts/step
    // adc.setGain(ADS1115_PGA_1P024);  // +/- 1.024 range, .00003125 volts/step
    // adc.setGain(ADS1115_PGA_0P512);  // +/-  .512 range, .000015625 volts/step
    // adc.setGain(ADS1115_PGA_0P256);  // +/-  .256 range, .000007813 volts/step
   
//  Select desired sample speed
    // adc.setRate(ADS1115_RATE_8);    //   8 samples per second
    // adc.setRate(ADS1115_RATE_16); 
    // adc.setRate(ADS1115_RATE_32);
    // adc.setRate(ADS1115_RATE_64); 
    // adc.setRate(ADS1115_RATE_128); 
    // adc.setRate(ADS1115_RATE_250); 
    // adc.setRate(ADS1115_RATE_475); 
    adc.setRate(ADS1115_RATE_860);  // 860 samples per second
   
    adc.setMultiplexer(ADS1115_MUX_P0_N1);  // AN0+ Vs AN1-
    // adc.setMultiplexer(ADS1115_MUX_P0_NG);  // AN0+ Vs ground

   }
void loop() {
    int   rawValue; // holds 16 bit result read from A/D device
    float scaledValue; // used for voltage scaling
    byte incomingByte;
    Serial.print("Analog input #1 counts =  ");
    rawValue = adc.getDifferential();  // read current A/D value
    Serial.print(rawValue);
    Serial.print("  Voltage =  ");
    scaledValue = rawValue * .0001875; // scale it to a voltage, note: use proper constant per range used
    Serial.print(scaledValue,6);
    Serial.println("     Hit any key to continue");
    while(Serial.available() < 1) {}   // wait for user keystroke
    while(Serial.available() > 0) {incomingByte = Serial.read();} //read keystrokes then back to loop
}


And the output results:

Quote

Initializing I2C devices...
Testing device connections...
ADS1115 connection successful
   
Analog input #1 counts =  26719  Voltage =  5.009812     Hit any key to continue
Analog input #1 counts =  26718  Voltage =  5.009625     Hit any key to continue
Analog input #1 counts =  26718  Voltage =  5.009625     Hit any key to continue



Thanks again for you help.

Lefty

Jeff Rowberg


I put together a 'testing sketch' for my ADS1115 A/D converter using your I2C libraries and so far I'm pleased with the results of the hardware and have not yet found a problem with your libraries. Thanks again for your help.


That's awesome! And you're very welcome. Not that it makes any real difference, but you should be able to get away with including only the device class header "ADS1115.h" in your main sketch and not the "I2Cdev.h" header, because ADS1115.h automatically includes I2Cdev.h. All the headers have the typical "#ifdef ... #endif" block surrounding them though, so the compiled result will be the same either way. It's just a tiny bit cleaner in the code.

    Jeff

mem



I think having a one second default is a better idea. It won't block unless the device doesn't respond and in that case does it matter if the error is returned in one second rather than 250ms? (in the previous code, the error condition would cause it to block indefinitely)

The advantage of a longer default time-out is that inexperienced users won't get false errors when using devices that do take longer than 250ms. Experienced users can increase or decrease the time-out if they want, but it seems more friendly to have a default that minimizes the chance that errors could be reported even when the system if functioning correctly. The longer default time-out has no impact on performance on systems where the I2C devices are responding correctly.


The I2Cdev class is ...is designed to be used by device libraries like the ADXL345 class or ITG3200 class. ...The only time anyone has to worry about specific timeout settings is if they are actually writing a new device class, in which case it seems very likely that they would know to specify an extra long timeout value in their "observeTortoiseMarathonPath()" method.

For this reason, it seems more useful overall to have a legitimate failure come back faster rather than slower. But honestly, I'm willing to change it to 1 sec if you would still recommend it in light of the viewpoint I laid out above (and maybe you clearly understood all of that before and my explanation was redundant anyway). No hard feelings in either case; I'm just trying to be diligent.

    Jeff


Certainly no hard feelings, its your library so no-one should begrudge your right to choose the default values you prefer. But I still think  a longer default timeout is better.

(Hopefully) good coders will read the datasheet and determine and set the appropriate timeout. It's the less diligent people using this code on some otherwise unsupported device that can come unstuck. And if they are not careful enough to check that the default value is set correctly they may not be diligent enough to handle the false error if it does timeout.  But go with your instinct on this, I am sure you have more important  things to do than debate this point.

Michael


Go Up