esphome / issues

Issue Tracker for ESPHome
https://esphome.io/
290 stars 35 forks source link

I2C: reading registers does not use "Repeated Start" #299

Closed T3m3z closed 3 years ago

T3m3z commented 5 years ago

Operating environment/Installation (Hass.io/Docker/pip/etc.): Windows 10, esphome 1.13.0.dev0

ESP (ESP32/ESP8266, Board/Sonoff): ESP8266, Wemos D1 Mini

Affected component:

I2C / MPR121

Description of problem: I use MPR121 to collect data about proximity rather than actual touch events. I tried to develop MPR121 component further to allow reading raw values from the registers. During the development, I got just nonsense data from MPR121. FInally I realised that MPR121 was actually always returning the first register (0x0, which contains touch states for the first 8 electrodes).

Reading through MPR121 datasheet I realized that ESPHome implementation of I2C does not use "Repeated Start", which is used to ensure that multiple masters can do coherent operations on I2C bus. I tested this by changing i2c.cpp a bit on line 60 by adding "false":

... this->wire_->endTransmission(false);

This fixed the problem and then I was able to read other registers too. MPR121 datasheet actually mentions this on page 25 (section "Read Format", keyword "Repeated Start"): https://www.sparkfun.com/datasheets/Components/MPR121.pdf

Problem-relevant YAML-configuration entries: None

Traceback (if applicable): Not applicable

Additional information and things you've tried: It seems that other I2C devices aren't that picky about reading those other registers. MPR121 seems to do this "right" because I2C bus might have multiple masters and master must continue right away without stopping (which could allow other master to reserve the bus and possibly leads to incoherent read action if other master changes the register).

I opened this issue because it seems that reading registers should always use "Repeated Start" after selecting the register. Nevertheless, I have limited hardware to test this and I wanted to discuss this before any actions.

OttoWinter commented 5 years ago

I opened this issue because it seems that reading registers should always use "Repeated Start" after selecting the register.

It may work for the MPR121, but I have the feeling it could be a problem for other components. At least why else would the Wire library esphome uses default to not doing it?

If it can be confirmed that this works with most components, esphome can switch to repeated start mode. Otherwise this will have to be a per-device setting in I2CDevice (like high_speed_mode in SPIDevice).

Also multiple masters on an i2c bus sounds a lot like an i2c extension - AFAIK "normal" i2c is supposed to be used with a single master.

T3m3z commented 5 years ago

I had the same concern that just blindly adding "Repeated Start" after every read could possibly lead to problems. Therefore, I wanted to spur discussion around this topic.

I believe that the problem here is that the internal state machine of MPR121 forgets (intentionally?) selected register after every STOP condition on I2C bus (in other words when master releases the bus). Therefore, after selecting the register, one should continue with "Repeated Start" as indicated in MPR121 datasheet. I2C specification mentions that a master can start next action directly without sending "STOP condition" by instead sending "Repeated Start" (section 3.1.4):

The bus stays busy if a repeated START (Sr) is generated instead of a STOP condition. In this respect, the START (S) and repeated START (Sr) conditions are functionally identical. For the remainder of this document, therefore, the S symbol is used as a generic term to represent both the START and repeated START conditions, unless Sr is particularly relevant.

There is also a page here talking about this. Therefore, I believe that every device conforming (at least to the newest 2014) I2C specification should be able to handle "Repeated Start condition" but this condition should be only sent if the master is going to continue operating with the same task.

It took me quite a long time to understand why I just got nonsense data out of my MPR121 by using ESPHome implementation of I2CDevice::read_byte_16. I doubted my code because I knew that there are multiple I2C devices already successfully integrated with ESPhome. I finally figured it out by checking an existing library: Adafruit uses "Repeated Start" in their library (check lines 188-203). I confirmed this finding by checking MPR121 datasheet (page 25, section "Read format"). It seems that most of the I2C devices remember selected register even after STOP condition (which in multi-master environment could cause problems if another master selects another register before the actual read).

About the multiple masters: I2C specification states this in the section 2:

It is a true multi-master bus including collision detection and arbitration to prevent data corruption if two or more masters simultaneously initiate data transfer.

And do not misunderstand me, I just love ESPHome and I want to help develop it further. This framework is just brilliant! :)

T3m3z commented 5 years ago

So to summarize: I2C specification allows master to use "Repeated Start" in cases where master wants to continue transferring data with the same slave. If slaves conform to I2C specification, adding "Repeated Start" before read operation shouldn't cause any problems. In real world of course this might not be true as implementations aren't exactly as in specification :)

T3m3z commented 5 years ago

I thought about this and actually the solution might be simpler if the MPR121 component just read all registers at once. This would mean that Repeated Start wouldn't be needed and existing I2C implementations would not be impacted.

Currently MPR121 component reads registers 0x00-0x01 which contain the touch information. Actual raw sensor values are in 0x04-0x1B. Based on previous experiences it is always possible to start reading from 0x00 and MPR121 returns requested amount of bytes reliably. So instead of trying to read just specific registers, I could read registers 0x00-0x1B. Does this sound reasonable?

@OttoWinter : If previous proposition makes sense to you, would it be ok if I created a new component for reading raw values from the sensor? Or is this the correct way to do it as the component I am planning is more like a sensor giving continuous values instead of the current MPR121 being like binary sensor for touch?

IvanHorban commented 5 years ago

I also have a problem with the i2с device PCF8574. Maybe it's the same problem? https://github.com/esphome/issues/issues/314

T3m3z commented 5 years ago

In your issue report you told that hot plugging PCF8574 works for you. If that is the case, I don't believe that it is related to this issue as this is all about Repeated Start condition. Also I didn't find any mentions about Repeated Start in the datasheet of PCF8574.

ti 21. toukok. 2019 klo 12.09 Ivan Horban notifications@github.com kirjoitti:

I also have a problem with the i2с device PCF8574. Maybe it's the same problem?

314 https://github.com/esphome/issues/issues/314

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/esphome/issues/issues/299?email_source=notifications&email_token=ABRGTAGFUY5RYA2LSCVTXJ3PWO34VA5CNFSM4HK5FZSKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODV3ITJI#issuecomment-494307749, or mute the thread https://github.com/notifications/unsubscribe-auth/ABRGTAGA6HXTT2HGQVDCYTTPWO34VANCNFSM4HK5FZSA .

IvanHorban commented 5 years ago

http://www.ti.com/lit/ds/symlink/pcf8574.pdf 6.6 I2C Interface Timing Requirements I found it here, maybe I did not understand that.

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

richardklingler commented 3 years ago

Just came across this closed issue....

Wanted to write two custom I2c sensors which also require the restart option....tried with every call available..but to no joy...

These sensors require the restart condition after writing the register address (working perfect with the Arduino Wire library):

richardklingler commented 3 years ago

Seems I've found my solution (o;

For reading out the Chip ID register from the VEML6075 UV sensor those sequence works for me:

uint8_t data[2];
uint8_t reg = VEML6075_ID;

this->raw_begin_transmission();
this->raw_write(&reg, 1);
this->raw_end_transmission(false);
this->raw_begin_transmission();
this->raw_write(&reg, 1);
this->read_bytes_raw(data, 2);

It reads out correctly 0x0026 now :-)

Also in the logic analyzer I see the restart sequence after writing the register address to read from..

I had to omit the last raw_end_transmission() as this would write out two times the register address to the device...

This works also with the MLX90614 :-)

i2c_restart_2

richardklingler commented 3 years ago

For me this issue can be closed as it works for me.....and no one else tried it or commented on this....