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

feat(sccb): new esp sccb component (IEC-92) #309

Closed

Conversation

Icarus113
Copy link
Contributor

@Icarus113 Icarus113 commented Feb 22, 2024

Checklist

  • Component contains License
  • Component contains README.md
  • Component contains idf_component.yml file with url field defined
  • Component was added to upload job
  • Component was added to build job
  • Optional: Component contains unit tests
  • CI passing

Change description

Please describe your change here

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@Icarus113
Copy link
Contributor Author

@igrr @suda-morris PTAL

@github-actions github-actions bot changed the title feat(sccb): new esp sccb component feat(sccb): new esp sccb component (IEC-92) Feb 22, 2024
@Icarus113 Icarus113 force-pushed the feat/esp_sccb_component_new branch 2 times, most recently from 901e384 to 763e82b Compare February 22, 2024 07:00
@Icarus113
Copy link
Contributor Author

@ginkgm PTAL as well

@Icarus113
Copy link
Contributor Author

@WangYuxin-esp

* - ESP_ERR_NO_MEM If out of memory
* - ESP_OK On success
*/
esp_err_t esp_sccb_new_i2c_ctlr(const esp_sccb_i2c_config_t *config, esp_sccb_handle_t *ret_handle);
Copy link
Collaborator

Choose a reason for hiding this comment

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

question, if the user wants to use the same physical I2C bus for the camera and other i2c slave sensros, how can he do? Because now inside this function, we're allocating the I2C bus, this will take the ownership, and user can't get the same bus outside the sccb driver

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will add a note in the header saying the I2C bus is owned by this API.

Copy link
Collaborator

Choose a reason for hiding this comment

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

how about let the user provide the I2C bus handle, and the sccb driver uses that bus handle?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think users won't use I2C bus together with SCCB driver

Choose a reason for hiding this comment

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

Yes, the classic usage scenario is:

  1. Call i2c_new_master_bus(&i2c_mst_config, &bus_handle) to initialize the I2C bus, obtain the handler, and then use bus_handle to control the sub-device(some sensors, just like led & motor)
  2. Then use the bus_handle to control the camera sensors on the sccb bus.
    Maintaining the reusability of this I2C port is an important issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

@Icarus113 Icarus113 force-pushed the feat/esp_sccb_component_new branch 5 times, most recently from d80a1e8 to 35405d3 Compare February 27, 2024 01:15
@Icarus113 Icarus113 marked this pull request as draft February 27, 2024 08:24
uint16_t device_address; ///< I2C device raw address. (The 7/10 bit address without read/write bit)
uint32_t scl_speed_hz; ///< I2C SCL line frequency
i2c_clock_source_t clk_source; ///< Clock source of I2C master bus, channels in the same group must use the same clock source
} esp_sccb_i2c_config_t;
Copy link
Collaborator

Choose a reason for hiding this comment

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

does this driver follow bus-device model? If so, what does the sccb represents? If it's a bus driver, I didn't expect there is a device address in the same configuration structure. BTW, device address may reside in a camera sensor's own driver package in case different camera needs special initialization commands.

@Icarus113 Icarus113 force-pushed the feat/esp_sccb_component_new branch from d6000f8 to 8f005a5 Compare February 28, 2024 03:36
@suda-morris suda-morris marked this pull request as ready for review February 28, 2024 03:50
@Icarus113 Icarus113 force-pushed the feat/esp_sccb_component_new branch 2 times, most recently from f3c265c to 0d92ad8 Compare February 29, 2024 03:27
@Icarus113 Icarus113 force-pushed the feat/esp_sccb_component_new branch from f0fc46d to 33f9ad4 Compare March 8, 2024 04:36
@Icarus113 Icarus113 force-pushed the feat/esp_sccb_component_new branch from 33f9ad4 to ee310a8 Compare March 8, 2024 04:40
@Icarus113 Icarus113 force-pushed the feat/esp_sccb_component_new branch from ee310a8 to 9f84a88 Compare March 8, 2024 04:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants