jrowberg / i2cdevlib

I2C device library collection for AVR/Arduino or other C++-based MCUs
http://www.i2cdevlib.com
3.94k stars 7.51k forks source link

Devices with 16bit register addresses #229

Open eadf opened 8 years ago

eadf commented 8 years ago

I was looking for drivers for the Tiny RTC module (DS1307 & AT24C32) and I noticed that the EEPROM AT24C32 does not have a i2cdevlib driver (yet). After some investigation I think I understand why.

AT24C32 uses 12-bit register addresses, and that's something i2cdevlib can't use (from what I can tell)

So my questions are:

NewLunarFire commented 8 years ago

Hi,

Register addresses aren't part of the I2C protocol. The only address that exists in the I2C protocol is the 7-bit hardware address. 8-bit register addresses are just something commonly used, but is simply part of the data that comes after the hardware address.

It would be easy to reuse the existing code and make it work with 16-bit I2C addresses. I can do it on my fork, just confirm me which implementation you are using.

eadf commented 8 years ago

Yes, i know it's easy to make a one way conversion. E.g just change the readBytes() to look something like:

readBytes(uint8_t devAddr,_uint16tregAddr_, uint8_t length, uint8_t *data,...);

But I'm talking about a general purpose solution that will co-exist with the current API. Do we want a set of [read|write][Byte|Bytes|Word|Words]16() methods in the i2cdevlib API? Basically duplicating the backend codebase. And what about 32bit addresses?

jrowberg commented 8 years ago

Lots of extra implementation code shouldn't be a problem as long as the compiler is able to optimize out whatever methods aren't needed for some application. However, I'm also in favor of making the implementation as generic as possible. That might mean exposing direct I2C read/write methods similar to Wire's beginTransmission() and endTransmission() for special use cases that don't fit into the standard "8-bit register" context. I haven't thought through negative side-effects of this approach though.

NewLunarFire commented 8 years ago

Currently, yes, we do not have a choice of creating new functions for 16-bits or 32-bits and duplicating code, because we're in plain C. in C++ you would simply overload the function to accept uint16 or uint32.

The main way we can avoid duplicating the code base is having functions perform a read or a write independent of register address. An i2c_basic_read(uint8 hw_add, uint8* data) and i2c_basic_write(uint8 hw_add, uint8* data) function that get called from all the readByte16 readBytes16 writeByte16 and etc., then do the bit-banging depending on if the register is 8, 16 or 32-bit and if the data is 8, 16 or 32 bit.

A simple flow would be: User calls readByte16(uint8_t addr, uint16_t reg, uint8_t data) readByte16 creates a write data buffer and puts the 2 reg bytes in it readByte16 calls basicWrite with addr and write buffer readByte16 creates a 1-byte read data buffer readByte16 calls basicRead with addr and read buffer readByte16 returns read buffer index 0

We could also avoid duplicating writeBytes for every register and data format by always using void pointers and casting within the function to the appropriate type, but I don't generally like this approach. This way, you'd have a prototype that would look like writeBytes(uint8_t addr, void* register_addr, void* data, uint8_t register_addr_size, uint8_t data_size, uint8_t data_bytes). But that's something I'd avoid.

The main issue is really how to name those functions. Something like I2Cdevlib_Read[Byte|Short|Long]Reg[8|16|24|32]

eadf commented 8 years ago

An alternative to the void* register_address could be to use C union (if plain old C really is required)

typedef union AddressV {
  uint32_t a32;
  uint16_t a16;
  uint8_t a8;
  uint8_t asBytes[sizeof(uint32_t)]; // or whatever size the largest address is
} AddressV;

typedef enum { A32=4, A24=3, A16=2, A8=1} AddressT;

typedef struct {
  AddressV v; // value 
  AddressT t; // type
} Address;

readBytesX(uint8_t devAddr, Address regAddr, uint8_t length, uint8_t *data,...);

Pros:

Cons:

For the bread'n-butter 8-bit register addresses we would still use the old (memory efficient) API.

NewLunarFire commented 8 years ago

Using different function names is a much cleaner solution and doesn't conflict with the existing code base. Also, unless the compiler optimization is turned off (like with -o0 with gcc), the unused functions won't be included in the generated binaries.

Also, does this library support 10-bit slave addresses?

NewLunarFire commented 8 years ago

It's been a while that I went through the code, but I realized: This isn't pure C by any means. This is C++. So we could simply overload the functions with uint16_t and uint32_t for register addresses.

eadf commented 3 years ago

Think I should mention this while it is on top of my stack: - I made an experimental, templated, version of i2cdevlib that supports any kind of address lengths https://github.com/lacklustrlabs/i2cdevlib back in 2017.

Maybe some ideas from that can be salvaged?

P.S. I don't have access to any i2c devices at the moment, so I'm unable to run any tests.