Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix i2c scanning eror for Arduino #2364

Merged
merged 4 commits into from Sep 22, 2021
Merged

fix i2c scanning eror for Arduino #2364

merged 4 commits into from Sep 22, 2021

Conversation

martgras
Copy link
Contributor

revert to the old scan implementation.

What does this implement/fix?

getting correct i2c scan results

Quick description and explanation of changes

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Other

Related issue or feature (if applicable): fixes esphome/issues#2453

Pull request in esphome-docs with documentation (if applicable): esphome/esphome-docs#

Test Environment

  • ESP32
  • ESP8266

Example entry for config.yaml:

substitutions:
  updates: 60s
  unique_id: bme280

esphome:
  name: ${unique_id}

esp32:
  board: TinyPICO
  framework:
    type: arduino

logger:
  level: VERBOSE
i2c:
  sda: 21
  scl: 22
  scan: True
  id: bus_a

sensor:
  - platform: bme280
    temperature:
      name: "Temperatur (Huette)"
      id: hut_temperature
    humidity:
      name: "Luftfeuchtigkeit (Huette)"
      id: hut_humidity
    update_interval: ${updates}
    address: 0x76

Checklist:

  • The code change is tested and works locally.
  • Tests have been added to verify that the new code works (under tests/ folder).

revert to the old scan implementation.
@probot-esphome
Copy link

Hey there @esphome/core, mind taking a look at this pull request as it has been labeled with an integration (i2c) you are listed as a code owner for? Thanks!
(message by CodeOwnersMention)

Copy link
Member

@mmakaay mmakaay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why revert the code? I think the roll forward option might be to use:

auto err = writev(address, nullptr, 0);

That should work too IMO, and would be closer to the intended target code.

@@ -34,7 +34,8 @@ void ArduinoI2CBus::dump_config() {
ESP_LOGI(TAG, "Scanning i2c bus for active devices...");
uint8_t found = 0;
for (uint8_t address = 1; address < 120; address++) {
auto err = readv(address, nullptr, 0);
wire_->beginTransmission(address);
auto err = wire_->endTransmission(true);

if (err == ERROR_OK) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ERROR_OK is of type i2c::ErrorCode; but endTransmission returns type int - it may just happen that the i2c error codes match up with what endTransmission returns, but we can't assume that in general.

Copy link
Contributor Author

@martgras martgras Sep 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated the PR to use writev.
Is there a reason why the scan ends at address 119 and not 127?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

iirc those addresses are reserved for special uses and so can't have i2d devices connected

looks like this explains it: https://electronics.stackexchange.com/a/7553

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok changed the range to 8 - 119 to exclude the 2 reserved ranges

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually for esp-idf readv works as well but switching to writev for consistency.

@martgras
Copy link
Contributor Author

@mmakaay is right auto err = writev(address, nullptr, 0); works as well for me and we don't have to use the underlying wire_ object here.

update the scan range to 0..127
@OttoWinter
Copy link
Member

@mmakaay is right auto err = writev(address, nullptr, 0); works as well for me and we don't have to use the underlying wire_ object here.

The same applies to the esp_idf implementation, could you also update the _esp_idf.cpp file? Thanks!

excluding the two sets of  reserved addresses 1111XXX and 0000XXX
@martgras
Copy link
Contributor Author

BTW: The CI check doesn't seem to pull the correct files https://github.com/esphome/esphome/actions/runs/1261652210 shows files that are not part of this commit

@oxan
Copy link
Member

oxan commented Sep 22, 2021

BTW: The CI check doesn't seem to pull the correct files https://github.com/esphome/esphome/actions/runs/1261652210 shows files that are not part of this commit

The CI runs on a simulated merge between dev and your PR, but you unfortunately just hit the moment where I broke CI on dev. I've scheduled a rerun, it should be fine now.

Copy link
Member

@OttoWinter OttoWinter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@OttoWinter OttoWinter merged commit 262d693 into esphome:dev Sep 22, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Sep 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

i2c scanning not working after after #2303
4 participants