Skip to content

esp32: Update machine_i2c.c. #17971

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

esp32: Update machine_i2c.c. #17971

wants to merge 2 commits into from

Conversation

Vincent1-python
Copy link

@Vincent1-python Vincent1-python commented Aug 22, 2025 •
edited
Loading

Summary

Replace the legacy ESP-IDF I²C driver (driver/i2c.h) with the new driver/i2c_master.h API for all ESP32-series SoCs.
This change:
Removes dependency on the deprecated peripheral driver that is no longer present in ESP-IDF ≥ 6.0.
Keeps the existing machine.I2C API unchanged, so user scripts do not require modification.

Testing

At present, it runs normally on ESP32P4, and others need help to test.

Trade-offs and Alternatives

@Vincent1-python Vincent1-python mentioned this pull request Aug 22, 2025
Open
@Vincent1-python
Copy link
Author

There is not enough space in the application area in the partition table on ESP32C6, which leads to CI failure. Do I need to modify it?

dpgeorge
dpgeorge reviewed Aug 22, 2025
View reviewed changes
@dpgeorge
Copy link
Member

There is not enough space in the application area in the partition table on ESP32C6, which leads to CI failure. Do I need to modify it?

No, we will fix that separately.

@Vincent1-python
Copy link
Author

All right, it's done

dpgeorge
dpgeorge reviewed Aug 22, 2025
View reviewed changes
@dpgeorge
Copy link
Member

Please check the code formatting CI and fix the failures that it found.

@Vincent1-python
Copy link
Author

Please check the code formatting CI and fix the failures that it found.

All right, it's done.

@Vincent1-python
esp32: Update machine_i2c.c.
2fc1eee 
Signed-off-by: Vincent1-python <pywei201209@163.com>

esp32: fix machine_i2c.c format.

Signed-off-by: Vincent1-python <pywei201209@163.com>

esp32: Update machine_i2c.c.

Signed-off-by: Vincent1-python <pywei201209@163.com>
@Vincent1-python
Copy link
Author

At present, it runs normally on ESP32P4, and others need help to test. @dpgeorge

@robert-hh
Copy link
Contributor

Tried with a Generic ESP32.

Accessing a real device (BME280) with hard I2C works.
No result with i2c.scan(). The timing for i2c.scan() look good, the device responds with ACK, but the device address is NOT returned.

Did you try I2C scan with the ESP32P4?

@Vincent1-python
Copy link
Author

Vincent1-python commented Aug 22, 2025 via email

@robert-hh
Copy link
Contributor

robert-hh commented Aug 22, 2025 •
edited
Loading

If you connect a I2C target, does i2c.scan() return the target's address?
Edit: I2C scan fails as well on a ESP32S3 device. I can try more devices, but maybe it should first be fixed for the GENERIC_ESP32 as the baseline.

@Vincent1-python
Copy link
Author

Vincent1-python commented Aug 22, 2025 via email

@robert-hh
Copy link
Contributor

It will be.

I understand your answer in that you did not test your code with a real external I2C target device. That is not good.
The difference between your code and the previous implementation of machine.I2C is, that your code does not work in all aspects as it should. Any new implementation must meet at least the API and behavior of the previous one.

@robert-hh
Copy link
Contributor

Looking into your code it does not handle the case for scan() properly. For scan only the address will be written. No data. And only the return state of i2c_master_probe() has to be returned.
The argument list for machine_hw_i2c_transfer() gets a single buffer with the length 0. This seems not to be handled properly in machine_hw_i2c_transfer().

robert-hh
robert-hh reviewed Aug 22, 2025
View reviewed changes
ports/esp32/machine_i2c.c Outdated
return -MP_ENODEV; /* No device at address, return immediately */
}
/* 1. Create a temporary device handle for this transaction */
i2c_device_config_t dev_cfg = {
Copy link
Contributor

@robert-hh robert-hh Aug 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Is it necessary to create this temporary device handle, or should't it be better created once at instantiation?
And if, it should be set up with the configured scl freq and not a default value.

Copy link
Contributor

Choose a reason for hiding this comment

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

Uüdate: It is necessary, at least with IDF V5.4.2. IDF V5.5 has an API to change the I2C address of a dev_handle. Then the handle has to be created only once.

robert-hh
robert-hh reviewed Aug 23, 2025
View reviewed changes
ports/esp32/machine_i2c.c Outdated
}

int machine_hw_i2c_transfer(mp_obj_base_t *self_in, uint16_t addr, size_t n, mp_machine_i2c_buf_t *bufs, unsigned int flags) {
machine_hw_i2c_obj_t *self = MP_OBJ_TO_PTR(self_in);

i2c_cmd_handle_t cmd = i2c_cmd_link_create();
/* 0. Probe the address to see if any device responds */
esp_err_t err = i2c_master_probe(self->bus_handle, addr, 1000);
Copy link
Contributor

Choose a reason for hiding this comment

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

i2c_master_probe() seems to use a fixed I2C freq of 100kHz. In certain situations that may not be appropriate.

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems not possible to change that. The freq is hard coded in the implementation of i2c_master_probe() .

@Vincent1-python
Copy link
Author

You can modify it directly. I'm too busy to take care of this PR. @robert-hh

@robert-hh
Copy link
Contributor

You can modify it directly. I'm too busy to take care of this PR.

@Vincent1-python Since I'm not a maintainer I cannot change the code of that PR. Attached is a interim state of the code,
which uses machine_i2c_transfer_single(). It is not fully tested, but read, write and scan work. It still has the temporary creating of the dev_handle. That can be avoided with ESP-IDF v5.5. At the moment I try to keep it compatible with ESP-IDF v5.4.2. It still uses as well the separate i2c_master_probe() call to tell, whether a device is at the bus. IMO this is sub-optimal, because a) it uses a fixed 100kHz frequency and b) it creates am additional bus transaction. Maybe I find a way to change the SCL frequency, or try, whether the return code from a write attempt could be used to tell, whether there is no device on the bus.

machine_i2c.zip

@Vincent1-python
Copy link
Author

thank you very much.

You can modify it directly. I'm too busy to take care of this PR.

@Vincent1-python Since I'm not a maintainer I cannot change the code of that PR. Attached is a interim state of the code, which uses machine_i2c_transfer_single(). It is not fully tested, but read, write and scan work. It still has the temporary creating of the dev_handle. That can be avoided with ESP-IDF v5.5. At the moment I try to keep it compatible with ESP-IDF v5.4.2. It still uses as well the separate i2c_master_probe() call to tell, whether a device is at the bus. IMO this is sub-optimal, because a) it uses a fixed 100kHz frequency and b) it creates am additional bus transaction. Maybe I find a way to change the SCL frequency, or try, whether the return code from a write attempt could be used to tell, whether there is no device on the bus.

machine_i2c.zip

@robert-hh
Copy link
Contributor

robert-hh commented Aug 23, 2025 •
edited
Loading

Tested with a few ESP32 variants: ESP32-Generic, ESP32S2, ESP32S3, ESP32C2, ESP32C3, ESP32C6 and a BME280 I2C target device. All work fine. Attached is an updated copy of the file. No functional changes to the previous one, just text edits.

machine_i2c.zip

@Vincent1-python
Copy link
Author

Vincent1-python commented Aug 24, 2025 via email

@Vincent1-python
Update machine_i2c.c
84fd0e9 
Signed-off-by: Vincent1-python <pywei201209@163.com>
@robert-hh
Copy link
Contributor

Downloaded and tested. As expected it still works. Two things cold be changed:

  • At the commit message of the last commit the full stop at the end is missing. That causes an CI error. There is anothee CI build error, but that one is not related to your PR,
  • In the file sdkconfig.base line 146 can be removed, and the related text in line 145 as well. Line 146 reads:
    CONFIG_I2C_SKIP_LEGACY_CONFLICT_CHECK=y

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dpgeorge dpgeorge Awaiting requested review from dpgeorge

1 more reviewer

@robert-hh robert-hh robert-hh approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Vincent1-python @dpgeorge @robert-hh