Hi, I just wrote a basic driver for the MMA8452 accelerometer (see http://mbed.org/users/ashleymills/code/MMA8452/file/7620a11149b8/MMA8452.cpp, and

While I was using the mbed I2C interface a couple of things occurred to me that I would like to share:

Inconsistent return values from functions

The long versions of read and write return 0 on success and 1 on failure. But the single byte write returns 1 on success and 0 on failure. So the functions are inconsistent. This is probably difficult to change if people already built libraries based on this, but I thought I'd mention it anyway.

Inconvenient way to write multiple bytes to a register or vwrite style chains

If you look at the function definition for write:

int write(int address, const char * data, int length, bool repeated = false)	

The way this is designed assumes you will prepend the register address to data.

Normally however, if you are writing a library for an I2C device, the *device* address above will be fixed, whereas the register address will change often. So it would be good if a convenience function existed for this. For example, here is the one I used in my library:

int MMA8452::writeRegister(char addr, char *data, int nbytes) {
    // writing multiple bytes is a little bit annoying because
    // the I2C library doesn't support sending the address separately
    // so we just do it manually
    
    // 1. tell I2C bus to start transaction
    _i2c.start();
    // 2. tell slave we want to write (slave address & write flag)
    if(_i2c.write(_writeAddress)!=1) {
       return 1;
    }
    // 3. send the write address
    if(_i2c.write(addr)!=1) {
       return 1;
    }
    // 4. send the data to write
    for(int i=0; i<nbytes; i++) {
       if(_i2c.write(data[i])!=1) {
          return 1;
       }
    }
    // 5. tell I2C bus to end transaction
    _i2c.stop();
    return 0;
}

You will notice that I can't use two calls to the multi-byte write function in my convenience function, because the multi-byte write pre-pends it's own START condition on the I2C bus.

If you added a flag that allowed supression of the START condition (like you do for the END condition), then it would be much cleaner to push multiple data buffers, since the calls could be chained. For example, the function could look like this:

int write(int address, const char * data, int length, bool noStart = false, bool noStop=false)

An alternative would be to have an additional function like this:

int write(int deviceAddress, int registerAddress, const char * data, int length, bool repeated = false)	

And abstract like I have above. But the supression of START would still be handy for writev style usage which is pretty common in programming. So I think it would make sense to have the following two functions:

int write(int deviceAddress, const char * data, int length, bool noStart = false, bool noStop=false)
int write(int deviceAddress, int registerAddress, const char * data, int length, bool noStart = false, bool noStop=false)

Then this would cover most use cases for I2C.

Slightly odd typing

address is always an int in your functions, but an I2C address is a maximum of 10 bits in extended addressing mode, but it is never signed.

I don't understand why you would use a signed integer for an unsigned value. It would make more sense IMO for it to be a uint16_t since that's what is actually being used. Note that you'd have to be careful in any register writing function to get the endianess correct for the 10 bit addresses.

Also, why is the data buffer a char buffer?

char in c is actually an integer type, and it can be signed in some implementations. Again this makes no sense for raw data for which a sign is meaningless. I think a more sensible type would be uint8_t. But perhaps you did it like this for compatibility with string manipulation functions, IDK.

BR

Ashley

Return value: True, but for reasons you mention that will never change.

Register address: I can't say it seems to be easier with all the possible options you propose. For that you can also just use single byte writes, and manually set start/stop. I do agree it might in practise be easier if you can seperately set a register address and data, since that is what most I2C devices do, although not necesarily, there is nothing in the protocol which forces you to do it that way. If you would do it, it would imo be a good idea to introduce for example a read register function, which first writes register address, and then restarts bus and reads. Problem again is compatibility, and how important it really is.

Typing: In c there is afaik no reason why char would be an integer type, it is platform dependent. It would indeed be nicer if it was uint8_t, on the other hand on the mbed platform it is defined as unsigned 8-bit, and it might be a bit more accessible. From programming perspective uint8_t would be best though.

Address as unsigned int would make sense, but I don't know if making it 16-bit really has advantages. 16-bit operations will often be slower than 32-bit operations on ARMs.

Thanks for the response.

I would agree that the read register function would also be handy.

Whilst it's true that you can do it all manually, which is what I've done, I think the abstractions would help a lot of people to write basic things without having to understand the underlying protocol in detail.

Perhaps I'll make an I2CDevice base class that provides the abstractions I mentioned for the common case of register reading and writing being an "axiom".

In anycase, thanks. And I just want to say that the library works well despite my criticisms :D

Ashley

A simple library which does that for sure cannot hurt: it is true that everytime you make an I2C sensor lib you need to implement a few standard functions. Okay it is just a few lines, but still.

Just single byte and array read/write functions to/from registers and an option to set a device address would be nice.