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

Implement new idf-v5 I2C driver #677

Merged
merged 5 commits into from
Sep 13, 2024
Merged

Conversation

playduck
Copy link
Contributor

This PR removes the legacy I2C driver from the SCCB and implements the newer version (see issues #649 )
All interfaces outside the SCCB are entirely unchanged and identical!
I've tested the project with an OV2640 on an ESP32-Cam board - it functions identically.

To keep these identical I had to translate the unique i2c slave address to their distinctive i2c_device_handle_t.
This requires saving both of these in an array of a locally defined struct.
I'm not sure if this is an ideal solution moving forward but as it doesn't break anything outside of the SCCB I think this is reasonable (This whole translation thing is further explained in a comment from line 51 onwards)

Currently this requires the esp-idf head version (not the latest release), as the code makes heavy use of the i2c_master_get_bus_handle function which is rather new (at the time of this PR).
The use of this function isn't required and could be changed.

Also of note:
I've translated the functions SCCB_Read16, SCCB_Write16, SCCB_Read_Addr16_Val16 and SCCB_Write_Addr16_Val16 as well. However it seems like these are entirely unused.
Perhaps these could be removed?

@me-no-dev
Copy link
Member

Hi @playduck ! Thanks for the submission. I have one request, please have the new driver as new .c file. Maybe sccb-ng.c. That way we can make it use the proper driver depending on IDF version or configuration value. Now it replaces the current driver, which will break for older versions of IDF

@playduck
Copy link
Contributor Author

playduck commented Aug 30, 2024

I agree; I didn't think about breaking the older versions.
I now moved the driver to sccb-ng.c in the same directory.
I restored the old sccb.c and added a #warning deprecation message at the top, as the legacy I2C driver may be removed in future versions.
The CMake now conditionally includes either the new or the old version with a small comment explaining the reasoning behind this.

@playduck
Copy link
Contributor Author

playduck commented Sep 5, 2024

@me-no-dev any update on this?

driver/sccb.c Outdated
@@ -6,6 +6,9 @@
* SCCB (I2C like) driver.
*
*/

#warning "legacy I2C API is deprecated, please use sccb-ng.c instead"
Copy link
Member

Choose a reason for hiding this comment

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

please remove this line. It will issue a warning on old IDF releases

@me-no-dev
Copy link
Member

@playduck sorry for the delay. Looks good with just one remark :) please fix it and I will merge it

@playduck
Copy link
Contributor Author

@me-no-dev No worries about the delay. I removed the warning.

@me-no-dev me-no-dev merged commit 8e1ec63 into espressif:master Sep 13, 2024
22 of 24 checks passed
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