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 I2CBus::write() bug and add i2c documentation #5947

Merged
merged 4 commits into from
Dec 18, 2023

Conversation

DrCoolzic
Copy link
Contributor

@DrCoolzic DrCoolzic commented Dec 16, 2023

I fixed a bug where the buffer length was incorrectly specified as a uin8_t instead of a size_t.
I have also added a lot of documentation to the i2c library, as well as aliased ERROR_OK to NO_ERROR

File affected:

  • esphome/components/i2c/i2c.h
  • esphome/components/i2c/i2c_bus.h

What does this implement/fix?

  • This PR fixes the problem of not being able to write buffer of 256 or more bytes when using the i2c bus (this affect the ESP-IDF framework as well as the Arduino framework.
  • at the same time, I took the opportunity to document the I²C library usage
  • and I have added (and not replaced so it does not break the I/F) an alias to ERROR_OK

The only code changes are:

ErrorCode write(const uint8_t *data, uint8_t len, bool stop = true) { return bus_->write(address_, data, len, stop); }

As been replaced by

ErrorCode write(const uint8_t *data, size_t len, bool stop = true) { return bus_->write(address_, data, len, stop); }

Plus the addition of the alias NO_ERROR in i2c_bus.h.

As far as the documentation is concerned, I've added information on classes, methods and attributes to help users of the I2C library. Please note that my native language is not English, so it's likely that the text contains errors. Please feel free to correct them.
Note also that I have used a syntax that allow detailed documentation in Doxygen output when looking the API, but that is hidden (only the details are hidden) when hovering in intellisense.

What does this PR do not fix?

Unfortunately, this PR does not fix the problem of not being able to read messages with 256 or bytes when using the Arduino framework is used. Fixing this problem requires to modify the Wire.h file in the Arduino framework and I do not know how to do that (or even if it is possible). However, as a courtesy I provide the code to do so in case you know how to modify Arduino framework.

In Wire.h the following code:

uint8_t TwoWire::requestFrom(int address, int len) {
    return requestFrom(static_cast<uint16_t>(address), static_cast<size_t>(len), true);
}
uint8_t TwoWire::requestFrom(int address, int len, int sendStop) {
    return static_cast<uint8_t>(requestFrom(static_cast<uint16_t>(address), static_cast<size_t>(len), static_cast<bool>(sendStop)));
}

Needs to be replaced by

size_t TwoWire::requestFrom(int address, int len) {
    return requestFrom(static_cast<uint16_t>(address), static_cast<size_t>(len), true);
}
size_t TwoWire::requestFrom(int address, int len, int sendStop) {
    return requestFrom(static_cast<uint16_t>(address), static_cast<size_t>(len), static_cast<bool>(sendStop));
}

PR Summary

When integrated this fix will allow to use buffers of 256 or more bytes in i2c library when using the ESP-IDF framework.
Unfortunately for the Arduino framework the buffer size will still be limited to 255 until the framework is also fixed.

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

Test Environment

  • ESP32
  • ESP32 IDF
  • ESP8266
  • RP2040
  • BK72xx
  • RTL87xx

Example entry for config.yaml:

esphome:
  name: test-wk2132-i2c-arduino
  platformio_options:
    build_flags:
      - -DI2C_BUFFER_LENGTH=257
esp32:
  board: esp32dev
  framework:
    type: arduino

external_components:
  - source: github://pr#5932
    components: [uart]
    refresh: 0s
  - source: github://pr#5218
    components: [wk2132_i2c]
    refresh: 0s

i2c:
  sda: 21
  scl: 22
  scan: true
  id: bus_i2c
  frequency: 600kHz

more info in this document

Bus i2c problems and solutions en-us.pdf

Checklist:

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

I fixed the length of the buffer incorrectly specified as an uin8_t
to of a size_t.
Added a lot of documentation to the i2c library
Added alias NO_ERROR to ERROR_OK

On branch drcoolzic_fix_i2c
Changes to be committed:
	modified:   esphome/components/i2c/i2c.h
	modified:   esphome/components/i2c/i2c_bus.h
@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)

lint does not like slave and bit(s) in comments ?
@DrCoolzic DrCoolzic changed the title Fix I2CBus::write() bug add documentation Fix I2CBus::write() bug and add i2c documentation Dec 16, 2023
esphome/components/i2c/i2c_bus.h Outdated Show resolved Hide resolved
@jesserockz jesserockz added this to the 2023.12.0b3 milestone Dec 17, 2023
@DrCoolzic
Copy link
Contributor Author

Is there a way to fix the bug in Wire.h of the Arduino library?

@jesserockz jesserockz modified the milestones: 2023.12.0b3, 2023.12.0b4 Dec 18, 2023
Copy link
Member

@jesserockz jesserockz left a comment

Choose a reason for hiding this comment

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

Thanks @DrCoolzic

@jesserockz
Copy link
Member

Is there a way to fix the bug in Wire.h of the Arduino library?

The Wire.h code is part of the Arduino Framework and would need to be changed there (If it is not already in a newer Arduino version).

@jesserockz jesserockz merged commit 0a117eb into esphome:dev Dec 18, 2023
48 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Dec 20, 2023
@DrCoolzic DrCoolzic deleted the drcoolzic_fix_i2c branch January 30, 2024 17:42
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.

None yet

2 participants