Add support for CH422G chip (BSP-760) #697
Conversation
|
|
There was a problem hiding this comment.
Pull request overview
This PR adds support for the CH422G I/O expander chip, which is found on the ESP32-S3-LCD-4.3 board. The implementation provides a driver component that allows ESP32 devices to communicate with the CH422G chip over I2C for expanding GPIO capabilities.
Key changes:
- New driver component for CH422G I/O expander with I2C communication support
- Test application for validating the driver functionality
- Documentation and examples for using the component
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| components/io_expander/esp_io_expander_ch422g/esp_io_expander_ch422g.c | Core driver implementation with I2C communication and register management |
| components/io_expander/esp_io_expander_ch422g/include/esp_io_expander_ch422g.h | Public API header defining the component interface |
| components/io_expander/esp_io_expander_ch422g/README.md | Documentation with usage examples and chip information |
| components/io_expander/esp_io_expander_ch422g/CMakeLists.txt | Build configuration with IDF version-dependent driver selection |
| components/io_expander/esp_io_expander_ch422g/idf_component.yml | Component metadata and dependencies |
| components/io_expander/esp_io_expander_ch422g/license.txt | Apache 2.0 license file |
| components/io_expander/esp_io_expander_ch422g/test_apps/main/test_app_ch422g.c | Test application validating driver functionality |
| components/io_expander/esp_io_expander_ch422g/test_apps/main/CMakeLists.txt | Test application build configuration |
| components/io_expander/esp_io_expander_ch422g/test_apps/main/idf_component.yml | Test application component dependencies |
| components/io_expander/esp_io_expander_ch422g/test_apps/CMakeLists.txt | Test application project configuration |
| components/io_expander/esp_io_expander_ch422g/test_apps/sdkconfig.defaults | Test application default configuration settings |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| ```c | ||
| esp_io_expander_handle_t io_expander = NULL; | ||
| esp_io_expander_new_i2c_ch422g(i2c_handle, ESP_IO_EXPANDER_I2C_CH422G, &io_expander); |
There was a problem hiding this comment.
The example code in the README doesn't match the actual function signature. The function esp_io_expander_new_i2c_ch422g takes only 2 parameters (i2c_bus and handle_ret), but this example shows 3 parameters including ESP_IO_EXPANDER_I2C_CH422G which doesn't exist in the API. The correct usage should be: esp_io_expander_new_i2c_ch422g(i2c_handle, &io_expander);
| esp_io_expander_new_i2c_ch422g(i2c_handle, ESP_IO_EXPANDER_I2C_CH422G, &io_expander); | |
| esp_io_expander_new_i2c_ch422g(i2c_handle, &io_expander); |
| } regs; | ||
| } esp_io_expander_ch422g_t; | ||
|
|
||
| static char *TAG = "ch422g"; |
There was a problem hiding this comment.
The variable should be declared as 'static const char *TAG' to make it a compile-time constant, which is the standard pattern for ESP-IDF logging tags.
| static char *TAG = "ch422g"; | |
| static const char *TAG = "ch422g"; |
| @@ -0,0 +1,8 @@ | |||
| set(req) | |||
| if("${IDF_VERSION_MAJOR}.${IDF_VERSION_MINOR}" VERSION_GREATER_EQUAL "5.2.6") | |||
There was a problem hiding this comment.
The version comparison string "5.2.6" appears unusual. The idf_component.yml specifies a minimum IDF version of ">=5.2", but this CMakeLists checks for version ">=5.2.6" which is a very specific patch version. This seems inconsistent. Typically, API changes occur in minor versions (e.g., 5.2 vs 5.3), not patch versions. Consider aligning this with the version requirement in idf_component.yml or documenting why version 5.2.6 specifically is required for the esp_driver_i2c component.
| if("${IDF_VERSION_MAJOR}.${IDF_VERSION_MINOR}" VERSION_GREATER_EQUAL "5.2.6") | |
| if("${IDF_VERSION_MAJOR}.${IDF_VERSION_MINOR}" VERSION_GREATER_EQUAL "5.2") |
There was a problem hiding this comment.
All other io_expander have it written like this, I propose keeping it like this for consistency.
| }; | ||
| ESP_GOTO_ON_ERROR(i2c_master_bus_add_device(i2c_bus, &i2c_dev_cfg2, &ch422g->i2c_set_io_handle), err, TAG, "Add new I2C device failed"); | ||
|
|
||
| const i2c_device_config_t i2c_dev_cfg3 = { |
There was a problem hiding this comment.
Inconsistent indentation detected. This line has extra leading spaces compared to the surrounding code. It should align with the declarations above and below it.
| const i2c_device_config_t i2c_dev_cfg3 = { | |
| const i2c_device_config_t i2c_dev_cfg3 = { |
| esp_io_expander_ch422g_t *ch422g = (esp_io_expander_ch422g_t *)__containerof(handle, esp_io_expander_ch422g_t, base); | ||
|
|
||
| uint8_t temp = 0; | ||
| ESP_RETURN_ON_ERROR(i2c_master_receive(ch422g->i2c_set_io_handle, &temp, sizeof(temp), I2C_TIMEOUT_MS), TAG, "Read input reg failed"); |
There was a problem hiding this comment.
The i2c_read_io_handle (address READ_IO_ADDR = 0x26) is created but never used. The read_input_reg function incorrectly uses i2c_set_io_handle (address SET_IO_ADDR = 0x38) for reading. According to the register definitions, reading input should use the READ_IO_ADDR (0x26), not SET_IO_ADDR (0x38). This appears to be a bug where the wrong I2C device handle is being used for reading inputs. Line 117 should use ch422g->i2c_read_io_handle instead of ch422g->i2c_set_io_handle.
| ESP_RETURN_ON_ERROR(i2c_master_receive(ch422g->i2c_set_io_handle, &temp, sizeof(temp), I2C_TIMEOUT_MS), TAG, "Read input reg failed"); | |
| ESP_RETURN_ON_ERROR(i2c_master_receive(ch422g->i2c_read_io_handle, &temp, sizeof(temp), I2C_TIMEOUT_MS), TAG, "Read input reg failed"); |
| uint8_t data[] = {0}; | ||
| if (value != 0) { | ||
| data[0] = 0x01; | ||
| } | ||
| ESP_RETURN_ON_ERROR(i2c_master_transmit(ch422g->i2c_gpo_handle, data, sizeof(data), I2C_TIMEOUT_MS), TAG, "Write direction reg failed"); | ||
| ch422g->regs.direction = value == 0 ? 0 : 0xff; |
There was a problem hiding this comment.
The direction register implementation doesn't match the datasheet behavior. The code treats direction as binary (0x00 or 0xff for all pins), but according to the chip limitations mentioned in README.md, all bidirectional I/O pins must be either input or output together. However, the current logic writes 0x01 for any non-zero value, which may not correctly configure individual pins. The implementation should validate that either all pins are inputs or all are outputs, or document the actual register value format expected by the CH422G chip.
| uint8_t data[] = {0}; | |
| if (value != 0) { | |
| data[0] = 0x01; | |
| } | |
| ESP_RETURN_ON_ERROR(i2c_master_transmit(ch422g->i2c_gpo_handle, data, sizeof(data), I2C_TIMEOUT_MS), TAG, "Write direction reg failed"); | |
| ch422g->regs.direction = value == 0 ? 0 : 0xff; | |
| /* CH422G limitation: all bidirectional I/Os must be inputs or outputs together. | |
| * Accept only 0x00 (all inputs) or 0xFF (all outputs) and reject mixed masks. | |
| */ | |
| if (value != 0x00 && value != 0xFF) { | |
| ESP_LOGE(TAG, "Invalid direction mask 0x%02" PRIx32 ", CH422G only supports all-input (0x00) or all-output (0xFF)", value); | |
| return ESP_ERR_INVALID_ARG; | |
| } | |
| uint8_t data[] = {0x00}; | |
| if (value == 0xFF) { | |
| /* Any non-zero config means 'all outputs' for the device; use 0x01 as per datasheet/README. */ | |
| data[0] = 0x01; | |
| } | |
| ESP_RETURN_ON_ERROR(i2c_master_transmit(ch422g->i2c_gpo_handle, data, sizeof(data), I2C_TIMEOUT_MS), TAG, "Write direction reg failed"); | |
| /* Cache the logical direction as a full mask for higher layers: 0x00 = all in, 0xFF = all out. */ | |
| ch422g->regs.direction = (value == 0x00) ? 0x00 : 0xFF; |
| esp_io_expander_print_state(io_expander); | ||
| ``` | ||
|
|
||
| Set pin 0 and pin 1 with output dircetion and low level: |
There was a problem hiding this comment.
Spelling error: 'dircetion' should be 'direction'.
| esp_io_expander_set_level(io_expander, IO_EXPANDER_PIN_NUM_0 | IO_EXPANDER_PIN_NUM_1, 0); | ||
| ``` | ||
|
|
||
| Set pin 2 and pin 3 with input dircetion: |
There was a problem hiding this comment.
Spelling error: 'dircetion' should be 'direction'.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
components/io_expander/esp_io_expander_ch422g/esp_io_expander_ch422g.c
Outdated
Show resolved
Hide resolved
components/io_expander/esp_io_expander_ch422g/esp_io_expander_ch422g.c
Outdated
Show resolved
Hide resolved
github-actions
bot
changed the title
|
Hello @fnadeau and thank you for this pull request! The CH422 is not a standard IO expander since it "abuses" the I2C standard a bit and it behaves like multiple I2C devices with each "register" being at a different device address. This doesn't really fit our current IO expander scheme and we will need to discuss it. I will let you know what is the resolution of our discussion and depending on that we can continue on this PR. |
| ESP_RETURN_ON_ERROR(i2c_master_bus_rm_device(ch422g->i2c_read_io_handle), TAG, "Remove I2C device (READ_IO_ADDR) failed"); | ||
| free(ch422g); | ||
| return ESP_OK; | ||
| } |
There was a problem hiding this comment.
Resource leak in del function on partial failure
The del function uses ESP_RETURN_ON_ERROR for each i2c_master_bus_rm_device call, which causes early return if any removal fails. This leaves the ch422g struct unfreed (memory leak) and remaining I2C devices still registered (resource leak). Since CH422G uses three I2C device handles, if the first or second removal fails, the subsequent devices remain orphaned. The initialization error path correctly handles this by attempting cleanup of all devices unconditionally before freeing memory.
It is a very odd I2C device indeed. |
3 participants
ESP-BSP Pull Request checklist
Change description
Add support for CH422G chip found on ESP32-S3-LCD-4.3 board.
Note
Adds CH422G support to the
esp_io_expanderecosystem.esp_io_expander_ch422gcomponent: implements I2C-backed driver (esp_io_expander_new_i2c_ch422g) withread_input_reg,write_output_reg,read_output_reg,write_direction_reg,read_direction_reg,reset, anddelfunctions; enforces chip constraint that all 8 I/Os are collectively input or outputCMakeLists.txtselectsesp_driver_i2cfor IDF >= 5.2.6, otherwisedriver;idf_component.ymlpublished as version1.0.1test_apps/(Unity) demonstrating init and toggling outputs on ESP32-S3-Touch-LCD-4.3; includes build files andsdkconfig.defaultsWritten by Cursor Bugbot for commit 8b9aaa3. This will update automatically on new commits. Configure here.