Add i2c bus recovery during initialization #2379
Conversation
|
Hey there @esphome/core, mind taking a look at this pull request as it has been labeled with an integration ( |
|
Similar code is already implemented in esp32 arduino at https://github.com/espressif/arduino-esp32/blob/2.0.0/cores/esp32/esp32-hal-i2c.c#L1426 |
chrta
force-pushed
the
add_i2c_bus_recover
branch
2 times, most recently
from
8b13e75
to
511bf5d
Compare
chrta
changed the title
This outputs 9 scl cycles on setup. This is similar the the linux kernel behaviour. The arduino libs for esp32 and esp8266 are doing something similar, esp-idf does not have this feature implemented.
chrta
force-pushed
the
add_i2c_bus_recover
branch
from
511bf5d
to
2747ece
Compare
| @@ -22,6 +22,9 @@ class ArduinoI2CBus : public I2CBus, public Component { | |||
| void set_scl_pin(uint8_t scl_pin) { scl_pin_ = scl_pin; } | |||
| void set_frequency(uint32_t frequency) { frequency_ = frequency; } | |||
|
|
|||
| private: | |||
| void recover(); | |||
There was a problem hiding this comment.
Shouldn't this have been protected and recover_? Seems the private-member check in clang-tidy doesn't work. @OttoWinter
There was a problem hiding this comment.
Hmm, right. Interesting it didn't catch that, not sure why - might be worth looking into a bit.
also: thoughts on allowing private again? it was born out of some frustrating with external libs that didn't provide access to some internals in the early esphomelib days, but now I think differently
There was a problem hiding this comment.
I'm fine with allowing private, never really understood why we disallowed it. We should do it consistently though: have tooling enforce naming (and maybe force ordering it after protected members), remove it from the contribution guide, etc.
There was a problem hiding this comment.
I looked into the clang-tidy not detecting the error and it looks like clang-tidy does not emit errors for any header files at the moment
For example, by adding these two files:
// esphome/core/asdf.h
#pragma once
class Test {
private:
int foo() { return 0; }
};
// esphome/core/asdf.cpp
#include "asdf.h"running script/clang-tidy esphome/core/asdf.cpp results in no errors generated (when the class definition is in the cpp file it works though). But when disabling the quiet mode I get this
1 warning generated.
Suppressed 1 warnings (1 in non-user code).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
Adding that header-filter option to clang-tidy makes the warning appear again :)
************* File esphome/core/asdf.cpp
1 warning generated.
/repos/esphome/esphome/core/asdf.h:5:7: error: invalid case style for private method 'foo' [readability-identifier-naming,-warnings-as-errors]
int foo() { return 0; }
^~~
foo_
But this appears to have been a problem for quite a while, as when running this I found errors that existed way before the clang-tidy changed :)
There was a problem hiding this comment.
Hmm, I seem to have dropped the header filter option in #2133. The option in .clang-tidy seems to have been wrong for way longer, but the command line option was apparently still necessary. I think I've seen clang-tidy warnings in headers since then though.
Do you want to fix this or I should I pick it up (will be after the weekend)?
There was a problem hiding this comment.
FYI: I did not find anything in clang-tidy regarding NO_PRIVATE_MEMBERS_ALWAYS_USE_PROTECTED in the readability-identifier-naming check (also nothing in llvm git history).
This outputs 9 SCL cycles on setup.
This is similar the the linux kernel behaviour. See code comments for some links.
What does this implement/fix?
Esphome did not implement i2c bus recovery, yet. The arduino libraries for esp32 and esp8266 contain recovery code. I discovered the issue when using a qmc5883l sensor. After an otau, the sensor was always unresponsive (connected via a long cable, setup failed). This may happen if the I2C master resets, when the I2C slave is within a transaction. For some (most?) I2C slaves it is necessary to let the finish their transaction before the host is able to communicate with the slave again. This is done here by configuring SCL as GPIO output and sending 9 clocks.
Types of changes
Related issue or feature (if applicable): fixes
Pull request in esphome-docs with documentation (if applicable): esphome/esphome-docs#
Test Environment
Example entry for
config.yaml:no change in yaml
Checklist:
tests/folder).If user exposed functionality or configuration variables are added/changed:
I measured this with a logic analyzer for ESP32 arduino and esp-idf build. The resulting clock on SCL is between 50kHz and 100kHz.