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

i2c_master.h for ESP8266 RTOS SDK (IDFGH-13827) #14678

Closed
trombik opened this issue Oct 6, 2024 · 12 comments
Closed

i2c_master.h for ESP8266 RTOS SDK (IDFGH-13827) #14678

trombik opened this issue Oct 6, 2024 · 12 comments
Labels
Resolution: Done Issue is done internally Status: Done Issue is done internally Type: Feature Request Feature request for IDF

Comments

@trombik
Copy link
Contributor

trombik commented Oct 6, 2024

Is your feature request related to a problem?

We are maintaining device drivers for esp-idf and ESP8266 RTOS SDK at https://github.com/UncleRus/esp-idf-lib/. Recently, users asked for updating our libraries to newer I2C driver introduced in esp-idf 5.2. We are willing to do so but the issue is ESP8266 support. As i2c_master.h is completely different from i2c.h, it is not practically possible to maintain two versions of each i2c drivers. Our options are:

  1. Wrapping our i2c device driver so that the device drivers supports both i2c.h and i2c_master.h
  2. Supporting both i2c.h and i2c_master.h by creating two versions of the drivers
  3. Dropping ESP8266 support

It seems that 1st and 2nd are not practically possible. We do want to avoid 3rd.

Do you have any plan to add i2c_master.h and its friends to ESP8266 RTOS SDK? If you do, we can continue to support ESP8266.

I do know that this repository is for esp-idf, but the maintainers of ESP8266 RTOS SDK repository rarely reply to issues or PRs, I am asking here.

See also: UncleRus/esp-idf-lib#655

Describe the solution you'd like.

Update ESP8266 RTOS SDK with i2c_master.h and its friends.

Describe alternatives you've considered.

See above.

Additional context.

Thanks to the common I2C component and the interface, we have been able to support ESP8266 and ESP32. We do know Espressif has few reasons to update ESP8266 RTOS SDK, but I saw some ESP8266 support is still in some tools, like the latest toolchains. We don't have numbers, but there are users who still want to use ESP8266 RTOS SDK and our drivers.

@trombik trombik added the Type: Feature Request Feature request for IDF label Oct 6, 2024
@espressif-bot espressif-bot added the Status: Opened Issue is new label Oct 6, 2024
@github-actions github-actions bot changed the title i2c_master.h for ESP8266 RTOS SDK i2c_master.h for ESP8266 RTOS SDK (IDFGH-13827) Oct 6, 2024
@wujiangang
Copy link
Contributor

Hi @trombik, thank you for your great work on the esp-idf-lib project. As you know, the ESP8266 RTOS SDK is under very limited maintenance and is based on a much older version of IDF, which has undergone significant changes since then. We will assess the effort required to support the new I2C driver and get back to you later.

@trombik
Copy link
Contributor Author

trombik commented Oct 10, 2024

@wujiangang Thank you for the feedback. Yes, I can guess how difficult to support i2c_master.h in ESP8266 RTOS SDK. The product is "not recommended for new designs" and in a fade-out phase. You probably want to spend the resources on the newer products. Eventually, we will drop ESP8266 support, too. We will wait for your assessment to make our own decision.

Thanks.

@donghengqaz
Copy link
Collaborator

Hi @trombik Could you give me a list about your used APIs in IDF's i2c_master.h? I checked all APIs in i2c_master.h, some parts can be wrapped directly by current functions, but to implement others, some refactoring for 8266's I2C is required.

@trombik
Copy link
Contributor Author

trombik commented Oct 10, 2024

@donghengqaz Technically, it is possible to wrap all the functions of i2c_master.h. UncleRus/esp-idf-lib#655 is my attempt to do so. However, the motivation behind the newer headers is to re-design how users interact with I2C bus and devices from applications (that's my understanding). But the implementation I wrote does not achieve the objective, e.g. splitting I2C bus and devices. Yes, the drivers will work interchangeably but users expect better experiences from new i2c_master.h. My attempt to hack around the backward compatibilities would bind user's application to the old architecture and get in the way to adopt all the goodies in newer esp-idf releases.

If you have a better idea to support ESP8266, I am all ears.

@donghengqaz
Copy link
Collaborator

@donghengqaz Technically, it is possible to wrap all the functions of i2c_master.h. UncleRus/esp-idf-lib#655 is my attempt to do so. However, the motivation behind the newer headers is to re-design how users interact with I2C bus and devices from applications (that's my understanding). But the implementation I wrote does not achieve the objective, e.g. splitting I2C bus and devices. Yes, the drivers will work interchangeably but users expect better experiences from new i2c_master.h. My attempt to hack around the backward compatibilities would bind user's application to the old architecture and get in the way to adopt all the goodies in newer esp-idf releases.

If you have a better idea to support ESP8266, I am all ears.

I check your PR, it is very valuable for reference. If your requirement is to implement the esp-idf v5.2 I2C APIs for 8266, I will try to do this. Maybe 2 weeks later or even next week, I may upload a beta version.

@trombik
Copy link
Contributor Author

trombik commented Oct 10, 2024

@donghengqaz That's incredible. Some notes:

  • I don't fully understand the implementation. You might need to ask the original author.
  • The PR is premature. The code was not tested with the consumers, our I2C drivers.
  • We have a document for contributors, but you may ignore it. We will deal with it, i.e. code style or conventions

@K0I05
Copy link

K0I05 commented Oct 11, 2024

I started porting a few drivers utilizing the latest design pattern with i2c_master.h. However, ever since I upgrade to ESP-IDF v5.3.1 I have been getting unexpected NACK errors on a few sensors, even with pull-up resistors, and trouble shooting with a logic analyzer. It is probably something else causing the problem.

Examples are here but they haven't been updated to work with ESP-IDF v.5.3.1: https://github.com/K0I05/esp32-s3

I'll post updated libraries for ESP-IDF v.5.3.1 soon.

@trombik
Copy link
Contributor Author

trombik commented Oct 11, 2024

@K0I05 please don't hijack the issue with an irrelevant issue.

@K0I05
Copy link

K0I05 commented Oct 11, 2024

at least I contributed to the relevant issue, the existing libraries are helpful but the design pattern is dated.

@donghengqaz
Copy link
Collaborator

donghengqaz commented Oct 25, 2024

Hi @trombik I develop the I2C wrapper APIs, the source code is in attachment
i2c_master.zip, but I don't have a I2C slave device which can cover all test case in my hand, so I have to develop a I2C slave example in ESP32 platform to test my ESP8266 I2C master APIs, and this is in progress, and most test cases are done and passed.

This suit also lacks of async APIs, because the original ESP8266 I2C driver doesn't support this, if you really need this, I will try to implement this, but this is much complex than just adding wrapper APIs. From the next week, I am to support customers, so more I2C APIs testing have to be stopped, and when I am free from this work, the testing will continue. I hope my code is helpful to you.

@trombik
Copy link
Contributor Author

trombik commented Oct 25, 2024

@donghengqaz That's great, thanks. I will review it and make a branch for it in esp-idf-llib repository so that our developers and users can discuss.

As for async APIs, no, it's not mandatory (the existing APIs and drivers are not async either). We will internally talk and decide what we should do.

I am grateful that you contributed so much when you have other tasks.

@trombik
Copy link
Contributor Author

trombik commented Oct 27, 2024

A branch in our repository was made and I will close this. Thank you for all the help. We will also continue to support Espressif and its products.

@trombik trombik closed this as completed Oct 27, 2024
@espressif-bot espressif-bot added Status: Done Issue is done internally Resolution: Done Issue is done internally and removed Status: Opened Issue is new labels Oct 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: Done Issue is done internally Status: Done Issue is done internally Type: Feature Request Feature request for IDF
Projects
None yet
Development

No branches or pull requests

5 participants