Skip to content
This repository has been archived by the owner on Nov 11, 2024. It is now read-only.

esp idf i2c legacy driver issue #245

Closed
jimakos96 opened this issue May 30, 2024 · 14 comments
Closed

esp idf i2c legacy driver issue #245

jimakos96 opened this issue May 30, 2024 · 14 comments
Assignees

Comments

@jimakos96
Copy link

Hello

I can see on the port/platform/esp-idf/src/u_port_i2c.c that the legacy i2c driver is used for esp-idf , so for now only the legacy i2c driver is used , is there any plan on updating to the new version of driver (i2c_master.h) , because the legacy driver will be deprecated in the future .

@RobMeades
Copy link
Contributor

Hi there: it is indeed on the TODO list; we will probably combine it with moving forward to ESP-IDF 5.2. We can't drop support for the old I2C driver of course, so it will make the ESP-IDF port-layer code a bit messy, and it will need to be done under a compilation switch so as not to break existing users.

Suggest we leave this issue open and then we can updated it when done.

@RobMeades
Copy link
Contributor

I've just begun implementing against the new ESP-IDF I2C API, however it seems to be missing one vital feature: it does not permit I2C stop bits to be suppressed. In the old API one had full control over this; we need it in order to determine the number of bytes available in the GNSS device (write 0xFD followed by no stop bit to the GNSS device, read two bytes and that gives you the amount of data it has buffered). Unless I've missed something...?

I have raised a feature request on ESP-IDF: espressif/esp-idf#13952

@RobMeades
Copy link
Contributor

FYI, the ESP-IDF I2C API does have that feature, in fact i2c_master_transmit_receive() does exactly that, it just doesn't say that it does that in the API description.

I'm on to the next issue, which is that it doesn't seem possible to implement our uPortI2cAdopt() API on top of the new ESP-IDF I2C API. I have asked (espressif/esp-idf#13968) if Espressif can see a way to do it.

@RobMeades
Copy link
Contributor

Hmmm, I'm now hitting a couple more issues which seem to be known and fixed on ESP-IDF main but not yet ported to a release branch:

espressif/esp-idf#13962
espressif/esp-idf#13922

I think we might wait a little while and let it settle down...

@RobMeades
Copy link
Contributor

RobMeades commented Jul 4, 2024

FYI, Espressif have now provided an API that allows uPortI2cAdopt() to be implemented, see espressif/esp-idf@992d8bc.

Once this, and hopefully the two fixed issues mentioned above, have made it into a tagged ESP-IDF release we will try again.

@RobMeades
Copy link
Contributor

FYI, it looks like ESP-IDF are gearing up for a 5.3 release; it is holiday season here now so likely we can try this in September when 5.3 is released.

@synologic
Copy link

5.3 is released :)

Beer is guaranteed for this port to be available soon ;-)

@RobMeades RobMeades self-assigned this Aug 28, 2024
@RobMeades
Copy link
Contributor

Beer is guaranteed for this port to be available soon ;-)

Then I'm your man :-).

@RobMeades
Copy link
Contributor

RobMeades commented Aug 28, 2024

B*gger: looks like the feature we needed, even though it was committed to ESP-IDF master two months ago, isn't in the ESP-IDF 5.3 release, it stops on the revision of i2c_master.c just before. I have asked when it will make it into an official release.

In the mean time I will try testing with ESP-IDF master; we might then choose to test with a specific commit hash ahead of v5.3.

@RobMeades
Copy link
Contributor

Update: I have taken ESP-IDF master as of yesterday and am testing with that but things are terribly flaky, I2C transactions generally fail with a HW timeout (and work perfectly if I switch back to the old I2C API). I have raised an issue with ESP-IDF asking what I might be doing wrong.

@synologic
Copy link

So i'm guessing we should stick to the old API and disable the warning at the boot :)))

@RobMeades
Copy link
Contributor

I guess it depends how quickly you need to get working: you will see from the issue I raised on ESP-IDF that someone else had raised what looks like the same issue (talking to a u-blox GNSS chip) back at the start of July, updated just last week, to which mythbuster5 responded that they would take a look, but then a few hours ago the espressif-bot marked that issue as "done" with resolution "NA", which seems a bit odd.

What is the warning you get at boot? Some form of run-time deprecation thingy?

@RobMeades
Copy link
Contributor

Update: looks like the issue that was raised earlier on ESP-IDF concerning talking to a u-blox GNSS chip using the new I2C API has, in fact, been fixed on ESP-IDF master in commit espressif/esp-idf@8aee667. I've just done a very quick test with that included and things are looking good, so don't lose hope!

I will need to do some more testing but, if things continue to look good, I will post a preview branch here with the changes to use the new ESP-IDF I2C API in it so that you can try it if you wish, while I get it put through regression test/review here.

@RobMeades
Copy link
Contributor

RobMeades commented Sep 2, 2024

I have pushed preview_esp_idf_i2c_new_api_rmea here: this uses the new ESP-IDF I2C API by default if you compile it against ESP-IDF master (and of course you need commit espressif/esp-idf@8aee667 or later) and passes our regression testing.

Once Espressif have included the fixes that are in commits espressif/esp-idf@992d8bc and espressif/esp-idf@8aee667 in a release branch we will update ubxlib master to use the new ESP-IDF I2C API for that ESP-IDF release and later; we will let you know when that is done. We will delete the preview branch sometime after that.

@RobMeades RobMeades assigned m-abubakar and unassigned RobMeades Oct 2, 2024
@manemannen manemannen closed this as not planned Won't fix, can't repro, duplicate, stale Nov 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants