search

eclipse / upm

UPM is a high level repository that provides software drivers for a wide variety of commonly used sensors and actuators. These software drivers interact with the underlying hardware platform through calls to MRAA APIs.
MIT License
661 stars 410 forks source link

Repeated I2C_SLAVE ioctls before every RD/WR fail on Android Things #547

Open spitfire88 opened 7 years ago

spitfire88 commented 7 years ago

According to the I2C spec, if you are using the SMBus method to RD/WR you need do the I2C_SLAVE ioctl only once and then proceed with multiple read/write calls.

See here: https://www.kernel.org/doc/Documentation/i2c/dev-interface "_When you have opened the device, you must specify with what device address you want to communicate:

int addr = 0x40; / The I2C address /

if (ioctl(file, I2C_SLAVE, addr) < 0) { / ERROR HANDLING; you can check errno to see what went wrong / exit(1); }

Well, you are all set up now. You can now use SMBus commands or plain I2C to communicate with your device._"

Android Things and MRAA both use the SMBus method to RD/WR. But some drivers like LSM303 do not follow this spec and repeatedly set the address before every RD/WR. In fact there are 6 of them: ds1307, itg3200, lsm303dlh, pn532 and adxl345.

Why is the address set repeatedly in these drivers and not the other I2C based ones? Is it due to the “repeated start condition” in I2C? It would explain why the address is set before every RD/WR. This condition doesn't apply to SMBus RD/WR method according to the spec. It is needed in case ioctl I2C_RDWR is used for reading and writing the slave device. None of the said drivers use this method (although it is supported in MRAA).

Can we get rid of the repeated calls? Here is an example:

  1. https://github.com/intel-iot-devkit/upm/blob/master/src/lsm303dlh/lsm303dlh.cxx#L119
  2. https://github.com/intel-iot-devkit/upm/blob/master/src/lsm303dlh/lsm303dlh.cxx#L121
bjbeare commented 7 years ago

Please post the patch so that the maintainer can accept/reject/comment.

jontrulson commented 7 years ago

Hi,

These were early drivers, where the authors (myself included) ASSUMED that since each I2C transaction (at the hardware bus level) starts with the device address, then the mraa_i2c_address() call must be called before every transaction. This was due to a misunderstanding of exactly what the mraa_i2c_address() call actually did under the hood.

As you notice, this is not required except in very specific circumstances. The mraa_i2c_address() function (or C++ .address() method) caches the address specified in the I2c context, which is then used when issuing "real" I2C transactions. Calling it repeatedly as you notice is not necessary, but also does not affect the I2C transaction on the hardware level either. As you point out, newer drivers do not do this.

The only place where this is actually needed is when you are using a single mraa I2C context to talk to multiple devices on the same bus (which of course will have different addresses). This is in fact what is happening on the LSM303DLH, as it is talking to two separate devices (magnetometer and accelerometer).

However, the .address() call on line 121 is superfluous and could be removed. The same goes for line 162. The others should probably stay. This is old code, so when it was recently renamed to lsm303dlh (from lsm303) no functionality changes or "fixes" like this were made. If it wasn't an obsolete chip, we would probably want to rewrite it from scratch along the lines of the LSM303AGR driver.

But, in the case of the DS1307, the extra calls (all of the ones NOT in the ctor) are superfluous and can be removed. We are only talking to a single device there, and only one .address() call is necessary.

Hopefully that helps explain some things.