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

Compatibility extended: New initializer with Wire interface pointer a… #7

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

k-nowicki
Copy link

…dded to extend platform and library compatibility. Tested on ESP32, ESP-IDF, arduino-esp32

…dded to extend platform and library compatibility. Tested on ESP32, ESP-IDF, arduino-esp32
@Erriez
Copy link
Owner

Erriez commented Dec 4, 2022

@k-nowicki Thanks for your pull request. I like the idea to add an additional interface to select a different I2C (TwoWire) bus.

I started with a code review and integration test and have some findings:

  1. AVR integration tests failed with build errors on include file wire.h instead of Wire.h. Maybe there is a difference between architectures.
  2. For which architecture are macro's ENERGIA and __arc__ set?
  3. When the build passes for AVR, ESP8266, ESP32, DUE and RP2040, the (README.md)[https://github.com/Erriez/ErriezDS3231/blob/master/README.md] and library.*should be updated.

FYI: Maintenance added to my Todo list (spare time):

  1. This library was still using the old deprecated Travis which should be upgraded to use Github Actions.
  2. I deprecated CMakeLists.txt for CLion a while ago as it is broken (already removed in my other libraries).
  3. I'll create a footprint difference as RAM availability is limited for AVR.

@k-nowicki
Copy link
Author

...build errors on include file wire.h instead of Wire.h. Maybe there is a difference between architectures.

Are you sure it is included in the lib? I can only see the capitalized version.

For which architecture are macro's ENERGIA and arc set?

Aren't they project specific? This lib do not need them and I have no such macros defined.

(README.md)... and library.* should be updated.

You are absolutely right. I have updated README and maked new pull request. In case of library.* files I am unsure how to update them correctly.

I am using ESP-IDF framework and toolchain on Esp32 platform.
I have not tested it on Arduino IDE nor other MCU, but changes made with this pull request should not affect platform compatibility.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants