[bridge-app/esp32] Add shell commands to add/remove/rename/toggle bridged endpoints. #42300
Conversation
|
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces valuable shell commands for dynamically managing bridged device endpoints on ESP32, which is a great feature for testing purposes. The implementation is mostly sound, but I have identified a few areas for improvement to enhance code quality, robustness, and maintainability. My feedback focuses on reducing code duplication by using arrays, improving input validation by replacing atoi with strtol, and adding necessary error handling for endpoint management functions in InitServer to prevent inconsistent states. Addressing these points will make the code more robust and easier to maintain.
| // Add lights 1..3 --> will be mapped to ZCL endpoints 3, 4, 5 | ||
| AddDeviceEndpoint(&gLight1, &bridgedLightEndpoint, Span<const EmberAfDeviceType>(gBridgedOnOffDeviceTypes), | ||
| Span<DataVersion>(gLight1DataVersions), 1); | ||
| gLight1Added = true; |
There was a problem hiding this comment.
The return value of AddDeviceEndpoint on the preceding lines is not checked. If AddDeviceEndpoint fails, gLight1Added will be incorrectly set to true, leading to an inconsistent state. The return value should be checked before updating the flag. This applies to all similar calls in this function (e.g., for gLight2Added, gLight3Added, etc., and also for RemoveDeviceEndpoint).
For example:
if (AddDeviceEndpoint(...) >= 0) {
gLight1Added = true;
}References
- Always check the return value of initialization functions that can fail to ensure robust initialization and prevent unstable states.
| static bool gLight1Added = false; | ||
| static bool gLight2Added = false; | ||
| static bool gLight3Added = false; | ||
| static bool gLight4Added = false; |
There was a problem hiding this comment.
Using separate boolean variables for each light's status leads to repetitive code in the command handlers. For example, BridgeRenameHandler and BridgeToggleHandler have nearly identical blocks of code for each light. This can be improved by using arrays for both the Device objects and their 'added' status. This would allow you to use loops, which would significantly reduce code duplication and improve maintainability.
For example, with arrays gLights and gLightsAdded, BridgeToggleHandler could be simplified to a single loop:
static CHIP_ERROR BridgeToggleHandler(int argc, char ** argv)
{
// This assumes gLights and gLightsAdded are arrays of size 4.
for (size_t i = 0; i < 4; ++i)
{
if (gLightsAdded[i])
{
gLights[i].SetOnOff(!gLights[i].IsOn());
ESP_LOGI(TAG, "Toggled Light %zu: now %s", i + 1, gLights[i].IsOn() ? "ON" : "OFF");
}
}
return CHIP_NO_ERROR;
}References
- To improve maintainability and reduce code duplication, extract common logic from similar methods into private helper functions.
| return CHIP_ERROR_INVALID_ARGUMENT; | ||
| } | ||
|
|
||
| int lightNum = atoi(argv[0]); |
There was a problem hiding this comment.
atoi does not perform error checking. If the input string is not a valid number, it returns 0, which could be misinterpreted. Using strtol is safer as it allows for robust error handling. This also applies to BridgeRemoveHandler.
Example of safe parsing:
char *end;
long lightNum_long = strtol(argv[0], &end, 10);
if (end == argv[0] || *end != '\0') {
ESP_LOGE(TAG, "Invalid light number: %s", argv[0]);
return CHIP_ERROR_INVALID_ARGUMENT;
}
int lightNum = static_cast<int>(lightNum_long);There was a problem hiding this comment.
Pull request overview
This PR adds shell command infrastructure for dynamically managing bridged device endpoints in the ESP32 bridge application at runtime. The changes enable testing of dynamic endpoint addition/removal scenarios for Matter bridge certification testing (specifically TC-BR-3).
Key Changes:
- Added shell commands (
add,remove,list,rename,toggle) under thebridgecommand namespace - Introduced state tracking flags (
gLight1AddedthroughgLight4Added) to monitor which lights are currently added - Integrated with existing shell command infrastructure following the Matter SDK's shell command patterns
| return CHIP_ERROR_INVALID_ARGUMENT; | ||
| } | ||
|
|
||
| int lightNum = atoi(argv[0]); |
There was a problem hiding this comment.
The atoi function returns 0 for non-numeric strings and doesn't distinguish between "0" and invalid input. This could allow invalid values like negative numbers or non-numeric strings to pass through. Consider validating that the input is a valid number between 1-4 before calling atoi, or check if the result is within the valid range.
| return CHIP_ERROR_INVALID_ARGUMENT; | ||
| } | ||
|
|
||
| int lightNum = atoi(argv[0]); |
There was a problem hiding this comment.
The atoi function returns 0 for non-numeric strings and doesn't distinguish between "0" and invalid input. This could allow invalid values like negative numbers or non-numeric strings to pass through. Consider validating that the input is a valid number between 1-4 before calling atoi, or check if the result is within the valid range.
| if (gLight1Added) | ||
| { | ||
| gLight1.SetOnOff(!gLight1.IsOn()); | ||
| ESP_LOGI(TAG, "Toggled Light 1: now %s", gLight1.IsOn() ? "ON" : "OFF"); | ||
| } | ||
| if (gLight2Added) | ||
| { | ||
| gLight2.SetOnOff(!gLight2.IsOn()); | ||
| ESP_LOGI(TAG, "Toggled Light 2: now %s", gLight2.IsOn() ? "ON" : "OFF"); | ||
| } | ||
| if (gLight3Added) | ||
| { | ||
| gLight3.SetOnOff(!gLight3.IsOn()); | ||
| ESP_LOGI(TAG, "Toggled Light 3: now %s", gLight3.IsOn() ? "ON" : "OFF"); | ||
| } | ||
| if (gLight4Added) | ||
| { | ||
| gLight4.SetOnOff(!gLight4.IsOn()); | ||
| ESP_LOGI(TAG, "Toggled Light 4: now %s", gLight4.IsOn() ? "ON" : "OFF"); | ||
| } | ||
|
|
There was a problem hiding this comment.
[nitpick] These functions contain significant code duplication. Consider refactoring to use a loop over the lights array (similar to BridgeListHandler) instead of having separate if blocks for each light. This would make the code more maintainable and reduce the risk of inconsistencies when modifying the behavior.
| if (gLight1Added) | |
| { | |
| gLight1.SetOnOff(!gLight1.IsOn()); | |
| ESP_LOGI(TAG, "Toggled Light 1: now %s", gLight1.IsOn() ? "ON" : "OFF"); | |
| } | |
| if (gLight2Added) | |
| { | |
| gLight2.SetOnOff(!gLight2.IsOn()); | |
| ESP_LOGI(TAG, "Toggled Light 2: now %s", gLight2.IsOn() ? "ON" : "OFF"); | |
| } | |
| if (gLight3Added) | |
| { | |
| gLight3.SetOnOff(!gLight3.IsOn()); | |
| ESP_LOGI(TAG, "Toggled Light 3: now %s", gLight3.IsOn() ? "ON" : "OFF"); | |
| } | |
| if (gLight4Added) | |
| { | |
| gLight4.SetOnOff(!gLight4.IsOn()); | |
| ESP_LOGI(TAG, "Toggled Light 4: now %s", gLight4.IsOn() ? "ON" : "OFF"); | |
| } | |
| struct LightInfo { | |
| bool * added; | |
| DeviceLight * light; | |
| const char * name; | |
| }; | |
| LightInfo lights[] = { | |
| { &gLight1Added, &gLight1, "Light 1" }, | |
| { &gLight2Added, &gLight2, "Light 2" }, | |
| { &gLight3Added, &gLight3, "Light 3" }, | |
| { &gLight4Added, &gLight4, "Light 4" }, | |
| }; | |
| for (const auto & info : lights) | |
| { | |
| if (*(info.added)) | |
| { | |
| info.light->SetOnOff(!info.light->IsOn()); | |
| ESP_LOGI(TAG, "Toggled %s: now %s", info.name, info.light->IsOn() ? "ON" : "OFF"); | |
| } | |
| } |
|
PR #42300: Size comparison from 4579de1 to a53aa7c Full report (35 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
2 participants
Summary
Add shell commands to add/remove/rename/toggle bridged device endpoints at runtime for esp32.
Related issues
Dynamic addition/removal of bridged endpoints was not supported for testing.
Testing
Manually tested and verified using shell commands and chip tool that device types are correctly populate into the parts-list of the descriptor.