fix(zigbeeEP): review of names and memory allocation #11199

SuGlider · 2025-03-28T17:51:10Z

Description of Change

Just a general review of memory allocation routines and changing string variable name to avoid using reserved words.

Tests scenarios

CI only

Related links

github-actions · 2025-03-28T17:51:50Z

	Warnings
⚠️	Some issues found for the commit messages in this PR: the commit message `"fix(zigbeeEP): destructor shall free any allocated memory"`: scope/component should be lowercase without whitespace, allowed special characters are `_` `/` `.` `,` `` `-` `.` the commit message `"fix(zigbeeEP): review of names and memory allocation"`: scope/component* should be lowercase without whitespace, allowed special characters are `_` `/` `.` `,` `` `-` `.` Please fix these commit messages* - here are some basic tips: follow Conventional Commits style correct format of commit message should be: `<type/action>(<scope/component>): <summary>`, for example `fix(esp32): Fixed startup timeout issue` allowed types are: `change,ci,docs,feat,fix,refactor,remove,revert,test` sufficiently descriptive message summary should be between 10 to 72 characters and start with upper case letter avoid Jira references in commit messages (unavailable/irrelevant for our customers) `TIP:` Install pre-commit hooks and run this check when committing (uses the Conventional Precommit Linter).

👋 Hello SuGlider, we appreciate your contribution to this project!

📘 Please review the project's Contributions Guide for key guidelines on code, documentation, testing, and more.

🖊️ Please also make sure you have read and signed the Contributor License Agreement for this project.

Click to see more instructions ...

This automated output is generated by the PR linter DangerJS, which checks if your Pull Request meets the project's requirements and helps you fix potential issues.

DangerJS is triggered with each push event to a Pull Request and modify the contents of this comment.

Please consider the following:
- Danger mainly focuses on the PR structure and formatting and can't understand the meaning behind your code or changes.
- Danger is not a substitute for human code reviews; it's still important to request a code review from your colleagues.
- Resolve all warnings (⚠️ ) before requesting a review from human reviewers - they will appreciate it.
- To manually retry these Danger checks, please navigate to the Actions tab and re-run last Danger workflow.

Review and merge process you can expect ...

We do welcome contributions in the form of bug reports, feature requests and pull requests.

1. An internal issue has been created for the PR, we assign it to the relevant engineer.
2. They review the PR and either approve it or ask you for changes or clarifications.
3. Once the GitHub PR is approved we do the final review, collect approvals from core owners and make sure all the automated tests are passing.
- At this point we may do some adjustments to the proposed change, or extend it by adding tests or documentation.
4. If the change is approved and passes the tests it is merged into the default branch.

Generated by 🚫 dangerJS against 8e03dd0

github-actions · 2025-03-28T18:05:01Z

Memory usage test (comparing PR against master branch)

The table below shows the summary of memory usage change (decrease - increase) in bytes and percentage for each target.

Memory	FLASH [bytes]		FLASH [%]		RAM [bytes]		RAM [%]
Target	DEC	INC	DEC	INC	DEC	INC	DEC	INC
ESP32S3	0	⚠️ +40	0.00	⚠️ +0.01	0	0	0.00	0.00
ESP32S2	0	⚠️ +48	0.00	⚠️ +0.01	0	0	0.00	0.00
ESP32C3	0	⚠️ +16	0.00	0.00	0	0	0.00	0.00
ESP32C6	0	⚠️ +16	0.00	0.00	0	0	0.00	0.00
ESP32H2	0	⚠️ +16	0.00	0.00	0	0	0.00	0.00
ESP32	0	⚠️ +44	0.00	⚠️ +0.01	0	0	0.00	0.00

Click to expand the detailed deltas report [usage change in BYTES]

Target	ESP32S3		ESP32S2		ESP32C3		ESP32C6		ESP32H2		ESP32
Example	FLASH	RAM	FLASH	RAM	FLASH	RAM	FLASH	RAM	FLASH	RAM	FLASH	RAM
libraries/Zigbee/examples/Zigbee_Color_Dimmer_Switch	⚠️ +40	0	⚠️ +44	0	⚠️ +16	0	⚠️ +14	0	⚠️ +14	0	⚠️ +44	0
libraries/Zigbee/examples/Zigbee_Gateway	⚠️ +20	0	⚠️ +24	0	⚠️ +14	0	-	-	-	-	⚠️ +28	0
libraries/Zigbee/examples/Zigbee_On_Off_Switch	⚠️ +40	0	⚠️ +48	0	⚠️ +16	0	⚠️ +14	0	⚠️ +14	0	⚠️ +44	0
libraries/Zigbee/examples/Zigbee_Range_Extender	⚠️ +40	0	⚠️ +48	0	⚠️ +16	0	⚠️ +16	0	⚠️ +12	0	⚠️ +44	0
libraries/Zigbee/examples/Zigbee_Thermostat	⚠️ +40	0	⚠️ +44	0	⚠️ +16	0	⚠️ +14	0	⚠️ +16	0	⚠️ +40	0
libraries/Zigbee/examples/Zigbee_Analog_Input_Output	-	-	-	-	-	-	⚠️ +16	0	⚠️ +16	0	-	-
libraries/Zigbee/examples/Zigbee_CarbonDioxide_Sensor	-	-	-	-	-	-	⚠️ +14	0	⚠️ +16	0	-	-
libraries/Zigbee/examples/Zigbee_Color_Dimmable_Light	-	-	-	-	-	-	⚠️ +16	0	⚠️ +14	0	-	-
libraries/Zigbee/examples/Zigbee_Contact_Switch	-	-	-	-	-	-	⚠️ +16	0	⚠️ +12	0	-	-
libraries/Zigbee/examples/Zigbee_Dimmable_Light	-	-	-	-	-	-	⚠️ +16	0	⚠️ +16	0	-	-
libraries/Zigbee/examples/Zigbee_Illuminance_Sensor	-	-	-	-	-	-	⚠️ +14	0	⚠️ +14	0	-	-
libraries/Zigbee/examples/Zigbee_OTA_Client	-	-	-	-	-	-	⚠️ +16	0	⚠️ +14	0	-	-
libraries/Zigbee/examples/Zigbee_Occupancy_Sensor	-	-	-	-	-	-	⚠️ +14	0	⚠️ +16	0	-	-
libraries/Zigbee/examples/Zigbee_On_Off_Light	-	-	-	-	-	-	⚠️ +16	0	⚠️ +12	0	-	-
libraries/Zigbee/examples/Zigbee_Pressure_Flow_Sensor	-	-	-	-	-	-	⚠️ +16	0	⚠️ +14	0	-	-
libraries/Zigbee/examples/Zigbee_Scan_Networks	-	-	-	-	-	-	0	0	0	0	-	-
libraries/Zigbee/examples/Zigbee_Temp_Hum_Sensor_Sleepy	-	-	-	-	-	-	⚠️ +16	0	⚠️ +16	0	-	-
libraries/Zigbee/examples/Zigbee_Temperature_Sensor	-	-	-	-	-	-	⚠️ +16	0	⚠️ +14	0	-	-
libraries/Zigbee/examples/Zigbee_Vibration_Sensor	-	-	-	-	-	-	⚠️ +16	0	⚠️ +16	0	-	-
libraries/Zigbee/examples/Zigbee_Wind_Speed_Sensor	-	-	-	-	-	-	⚠️ +14	0	⚠️ +16	0	-	-
libraries/Zigbee/examples/Zigbee_Window_Covering	-	-	-	-	-	-	⚠️ +14	0	⚠️ +14	0	-	-

github-actions · 2025-03-28T18:08:59Z

Test Results

76 files 76 suites 13m 45s ⏱️
38 tests 38 ✅ 0 💤 0 ❌
241 runs 241 ✅ 0 💤 0 ❌

Results for commit 8e03dd0.

♻️ This comment has been updated with latest results.

P-R-O-C-H-Y

Looking good at first sight, will test and approve if its working fine :)

libraries/Zigbee/src/ZigbeeEP.cpp

libraries/Zigbee/src/ZigbeeEP.h

SuGlider · 2025-03-31T14:58:31Z

@me-no-dev | @P-R-O-C-H-Y :: Changes done as discussed today. PTAL. Thanks!

P-R-O-C-H-Y

LGTM! Tested with HomeAssistant + Light and switch examples. Both working

Copilot

Pull Request Overview

This PR revisits memory allocation routines and renames a variable to avoid reserved words.

Introduces a constant for maximum name length.
Updates memory allocation in setManufacturerAndModel to use stack arrays.
Revises the handling of manufacturer and model string pointers in zbReadBasicCluster.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
libraries/Zigbee/src/ZigbeeEP.h	Added a constant ZB_MAX_NAME_LENGTH and updated comments.
libraries/Zigbee/src/ZigbeeEP.cpp	Modified memory allocation for name/model handling and adjusted pointer logic.

Comments suppressed due to low confidence (1)

libraries/Zigbee/src/ZigbeeEP.cpp:39

Using stack-allocated arrays for zb_name and zb_model may lead to invalid memory references if esp_zb_basic_cluster_add_attr does not copy the data. Consider allocating memory on the heap or ensuring the data is copied safely.

char zb_name[ZB_MAX_NAME_LENGTH + 2];

libraries/Zigbee/src/ZigbeeEP.cpp

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot

Copilot wasn't able to review any files in this pull request.

Files not reviewed (2)

libraries/Zigbee/src/ZigbeeEP.cpp: Language not supported
libraries/Zigbee/src/ZigbeeEP.h: Language not supported

Copilot

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (2)

libraries/Zigbee/src/ZigbeeEP.cpp:39

Using stack-allocated arrays for zb_name and zb_model could lead to undefined behavior if esp_zb_basic_cluster_add_attr does not internally copy the data. Please verify that the attribute addition function safely duplicates the input data or consider switching to dynamic allocation.

char zb_name[ZB_MAX_NAME_LENGTH + 2];

libraries/Zigbee/src/ZigbeeEP.cpp:171

[nitpick] For consistency with modern C++ conventions, consider using nullptr instead of NULL for pointer comparisons and assignments.

if (_read_manufacturer != NULL) {

fix(zigbeeEP): review of names and memory allocation

d3a44fc

SuGlider added Status: In Progress ⚠️ Issue is in progress Area: Zigbee Issues and Feature Request about Zigbee labels Mar 28, 2025

SuGlider added this to the 3.2.0 milestone Mar 28, 2025

SuGlider self-assigned this Mar 28, 2025

SuGlider added 2 commits March 28, 2025 14:53

fix(zigbeeEP): destructor shall free any allocated memory

7ad636f

fix(zigbee_ep): forgotten var name change

2c44063

SuGlider requested review from P-R-O-C-H-Y, me-no-dev and lucasssvaz March 28, 2025 18:27

SuGlider added Status: Review needed Issue or PR is awaiting review and removed Status: In Progress ⚠️ Issue is in progress labels Mar 28, 2025

P-R-O-C-H-Y reviewed Mar 28, 2025

View reviewed changes

me-no-dev reviewed Mar 28, 2025

View reviewed changes

libraries/Zigbee/src/ZigbeeEP.cpp Outdated Show resolved Hide resolved

SuGlider added 4 commits March 29, 2025 09:15

Merge branch 'master' into zigbee_memory_review

83d0f42

feat(zigbee_ep): use static heap memory for manufacturer and model names

49d983a

feat(zigbee_ep): changed model and manufacturer to heap

bf3e5c1

feat(zigbee_ep): use static heap memory allocation

6c6b980

SuGlider added Status: In Progress ⚠️ Issue is in progress and removed Status: Review needed Issue or PR is awaiting review labels Mar 30, 2025

SuGlider commented Mar 30, 2025

View reviewed changes

libraries/Zigbee/src/ZigbeeEP.cpp Outdated Show resolved Hide resolved

me-no-dev reviewed Mar 31, 2025

View reviewed changes

libraries/Zigbee/src/ZigbeeEP.h Outdated Show resolved Hide resolved

SuGlider added 2 commits March 31, 2025 08:48

fix(zigbee_ep): using stack only for adding attribute

c15fbf3

Merge branch 'master' into zigbee_memory_review

b6cb584

SuGlider modified the milestones: 3.2.0, 3.2.1 Mar 31, 2025

feat(zigbee_ep): reverting back read data type

0763c2a

SuGlider added 3 commits March 31, 2025 11:48

fix(zigbee_ep): rooling back to use malloc for remote attr reading

cfe4457

feat(zigbee_ep): check malloc return for null

a7c7713

fix(zigbee_ep): replace nullptr by NULL after C malloc()

48e5f17

SuGlider requested a review from me-no-dev March 31, 2025 14:57

SuGlider removed the Status: In Progress ⚠️ Issue is in progress label Mar 31, 2025

SuGlider requested a review from P-R-O-C-H-Y March 31, 2025 14:57

SuGlider added the Status: Review needed Issue or PR is awaiting review label Mar 31, 2025

P-R-O-C-H-Y approved these changes Apr 1, 2025

View reviewed changes

Merge branch 'master' into zigbee_memory_review

828d911

lucasssvaz approved these changes Apr 1, 2025

View reviewed changes

me-no-dev approved these changes Apr 2, 2025

View reviewed changes

me-no-dev added Status: Pending Merge Pull Request is ready to be merged and removed Status: Review needed Issue or PR is awaiting review labels Apr 2, 2025

ci(pre-commit): Apply automatic fixes

4681be4

me-no-dev requested a review from Copilot April 3, 2025 12:02

Copilot AI reviewed Apr 3, 2025

View reviewed changes

libraries/Zigbee/src/ZigbeeEP.cpp Outdated Show resolved Hide resolved

libraries/Zigbee/src/ZigbeeEP.cpp Outdated Show resolved Hide resolved

SuGlider and others added 2 commits April 3, 2025 09:09

fix(zigbee_ep): fix variable scope

d113ba2

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

fix(zigbee_ep): fix variable scope

8e03dd0

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

SuGlider requested a review from Copilot April 4, 2025 17:27

Copilot AI reviewed Apr 4, 2025

View reviewed changes

me-no-dev requested a review from Copilot April 5, 2025 15:46

Copilot AI reviewed Apr 5, 2025

View reviewed changes

me-no-dev merged commit b333bf2 into master Apr 5, 2025
56 checks passed

me-no-dev deleted the zigbee_memory_review branch April 5, 2025 15:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(zigbeeEP): review of names and memory allocation #11199

fix(zigbeeEP): review of names and memory allocation #11199

SuGlider commented Mar 28, 2025

github-actions bot commented Mar 28, 2025 •

edited

Loading

github-actions bot commented Mar 28, 2025 •

edited

Loading

github-actions bot commented Mar 28, 2025 •

edited

Loading

P-R-O-C-H-Y left a comment

SuGlider commented Mar 31, 2025

P-R-O-C-H-Y left a comment

Copilot AI left a comment

Copilot AI left a comment

Copilot AI left a comment

fix(zigbeeEP): review of names and memory allocation #11199

fix(zigbeeEP): review of names and memory allocation #11199

Conversation

SuGlider commented Mar 28, 2025

Description of Change

Tests scenarios

Related links

github-actions bot commented Mar 28, 2025 • edited Loading

github-actions bot commented Mar 28, 2025 • edited Loading

Memory usage test (comparing PR against master branch)

github-actions bot commented Mar 28, 2025 • edited Loading

Test Results

P-R-O-C-H-Y left a comment

Choose a reason for hiding this comment

SuGlider commented Mar 31, 2025

P-R-O-C-H-Y left a comment

Choose a reason for hiding this comment

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot AI left a comment

Choose a reason for hiding this comment

github-actions bot commented Mar 28, 2025 •

edited

Loading

github-actions bot commented Mar 28, 2025 •

edited

Loading

github-actions bot commented Mar 28, 2025 •

edited

Loading