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

Zigbee: Add dimmable light endpoint class #10727

Merged
merged 10 commits into from
Dec 16, 2024

Conversation

FaBjE
Copy link
Contributor

@FaBjE FaBjE commented Dec 13, 2024

Description of Change

This PR adds a Zigbee endpoint for a normal dimmable light. (So no colors or white-temperature change)
I based it on the "color dimmable light" example.

Unfortunately this endpoint type is not in the ESP Zigbee library by default.
So I had to make my own "esp_zb_dimmable_light_clusters_create" function.
It appears to be functioning good, but please do a proper check if I figured it out correctly.

Tests scenarios

I've tested this with Home Assistant.
The Arduino applications runs on a ESP32-C6 with Arduino-esp32 core on "master" branch.

I'm looking forward to your feedback 😃

@CLAassistant
Copy link

CLAassistant commented Dec 13, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

github-actions bot commented Dec 13, 2024

Warnings
⚠️

Some issues found for the commit messages in this PR:

  • the commit message "feat(Zigbee): Add Zigbee Dimmable light endpoint class":
    • scope/component should be lowercase without whitespace, allowed special characters are _ / . , * - .
  • the commit message "feat(Zigbee): Add Zigbee Dimmable light example":
    • scope/component should be lowercase without whitespace, allowed special characters are _ / . , * - .
  • the commit message "feat(Zigbee): Add Zigbee Dimmable light to CMakeLists.txt":
    • scope/component should be lowercase without whitespace, allowed special characters are _ / . , * - .
  • the commit message "feat(Zigbee): Add additional zigbee enabled check":
    • scope/component should be lowercase without whitespace, allowed special characters are _ / . , * - .
  • the commit message "feat(Zigbee): Adjusted example author comment":
    • scope/component should be lowercase without whitespace, allowed special characters are _ / . , * - .
  • the commit message "feat(Zigbee): Applied formatter + add formatter protection":
    • scope/component should be lowercase without whitespace, allowed special characters are _ / . , * - .
  • the commit message "feat(Zigbee): Update Zigbee Dimmable light example config/define names":
    • scope/component should be lowercase without whitespace, allowed special characters are _ / . , * - .
  • the commit message "feat(Zigbee): Update Zigbee Dimmable light example to 3.1.x features":
    • 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 20 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).

⚠️ Please consider squashing your 10 commits (simplifying branch history).
⚠️

The source branch "feature/zigbeeDimmableLight" incorrect format:

  • contains uppercase letters. This can cause troubles on case-insensitive file systems (macOS).
    Please rename your branch.
⚠️
	The **target branch** for this Pull Request **must be the default branch** of the project (`master`).

	If you would like to add this feature to a different branch, please state this in the PR description and we will consider it.

👋 Hello FaBjE, we appreciate your contribution to 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 4f863d5

@me-no-dev
Copy link
Member

Please add example that is inline with the other zigbee examples

@me-no-dev
Copy link
Member

Also please target the release/v3.1.x branch

Copy link
Contributor

github-actions bot commented Dec 13, 2024

Test Results

 62 files   62 suites   5m 41s ⏱️
 21 tests  21 ✅ 0 💤 0 ❌
154 runs  154 ✅ 0 💤 0 ❌

Results for commit 4f863d5.

♻️ This comment has been updated with latest results.

@SuGlider
Copy link
Collaborator

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.

@FaBjE - Please sign the CLA.

@FaBjE FaBjE force-pushed the feature/zigbeeDimmableLight branch from 0d3656d to e473560 Compare December 13, 2024 16:14
@FaBjE FaBjE changed the base branch from master to release/v3.1.x December 13, 2024 16:14
@P-R-O-C-H-Y
Copy link
Member

P-R-O-C-H-Y commented Dec 13, 2024

Hi @FaBjE, I am happy to see a PR from community. Overall it's looking good, but I am missing an implementation of setting the light manually. Can you please check a ZigbeeColorDimmableLight you have as a template on the release/v3.1.x branch? There was more stuff added recently.

So you are missing:

  void setLightState(bool state);
  void setLightLevel(uint8_t level);
  void setLight(bool state, uint8_t level);

  bool getLightState() {
    return _current_state;
  }
  uint8_t getLightLevel() {
    return _current_level;
  }

Add a endpoint type class for a dimmable light. Based on a copy of color dimmable light.
Add example for a dimmable light. Based on a copy of color dimmable light example.
@P-R-O-C-H-Y P-R-O-C-H-Y added Status: In Progress Issue is in progress Area: Libraries Issue is related to Library support. labels Dec 13, 2024
@FaBjE FaBjE force-pushed the feature/zigbeeDimmableLight branch from e473560 to a0d57bb Compare December 13, 2024 18:16
@FaBjE
Copy link
Contributor Author

FaBjE commented Dec 13, 2024

@me-no-dev
I added an example and modified the target branch.

I modified my commit message according the bot message.
I can "rename" the branch, but we will probably loose this PR. Please advise.

@P-R-O-C-H-Y
Thanks for the review. I have:

  • Added the missing functions (Sorry copied from 3.0.7)
  • Updated the example to 3.1.x standards
  • Renamed define/config struct (did change ZIGEEE_ to ZIGBEE_ as I assumed a typo)

@P-R-O-C-H-Y
Copy link
Member

@FaBjE Please add the source file to the CMakeLists.txt file here:

libraries/Zigbee/src/ep/ZigbeeThermostat.cpp

@P-R-O-C-H-Y P-R-O-C-H-Y added this to the 3.1.0 milestone Dec 13, 2024
@FaBjE
Copy link
Contributor Author

FaBjE commented Dec 14, 2024

@FaBjE Please add the source file to the CMakeLists.txt file here:

libraries/Zigbee/src/ep/ZigbeeThermostat.cpp

Added. Somehow it just compiled fine without here...

Copy link
Contributor

github-actions bot commented Dec 14, 2024

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.

MemoryFLASH [bytes]FLASH [%]RAM [bytes]RAM [%]
TargetDECINCDECINCDECINCDECINC
ESP32C60‼️ +21K0.00‼️ +3.90💚 -476⚠️ +604💚 -1.44‼️ +2.03
ESP32H20‼️ +16K0.00‼️ +2.96💚 -596⚠️ +468💚 -1.84‼️ +1.61
Click to expand the detailed deltas report [usage change in BYTES]
TargetESP32C6ESP32H2
ExampleFLASHRAMFLASHRAM
Zigbee/examples/Zigbee_CarbonDioxide_Sensor----
Zigbee/examples/Zigbee_Color_Dimmable_Light‼️ +21K⚠️ +588‼️ +16K⚠️ +444
Zigbee/examples/Zigbee_Color_Dimmer_Switch‼️ +13K💚 -148‼️ +9K💚 -276
Zigbee/examples/Zigbee_Dimmable_Light----
Zigbee/examples/Zigbee_Occupancy_Sensor----
Zigbee/examples/Zigbee_On_Off_Light‼️ +20K⚠️ +604‼️ +16K⚠️ +468
Zigbee/examples/Zigbee_On_Off_Switch‼️ +13K💚 -172‼️ +9K💚 -276
Zigbee/examples/Zigbee_Pressure_Flow_Sensor----
Zigbee/examples/Zigbee_Scan_Networks‼️ +17K⚠️ +176‼️ +12K0
Zigbee/examples/Zigbee_Temp_Hum_Sensor_Sleepy‼️ +18K💚 -104‼️ +13K💚 -296
Zigbee/examples/Zigbee_Temperature_Sensor‼️ +15K💚 -132‼️ +11K💚 -292
Zigbee/examples/Zigbee_Thermostat‼️ +15K💚 -476‼️ +10K💚 -596

@lucasssvaz
Copy link
Collaborator

lucasssvaz commented Dec 14, 2024

@FaBjE Please add the source file to the CMakeLists.txt file here:

libraries/Zigbee/src/ep/ZigbeeThermostat.cpp

Added. Somehow it just compiled fine without here...

It is only used when compiling as an IDF component.

@P-R-O-C-H-Y P-R-O-C-H-Y added Status: Pending Merge Pull Request is ready to be merged and removed Status: In Progress Issue is in progress labels Dec 16, 2024
@P-R-O-C-H-Y
Copy link
Member

Thank you @FaBjE for adding another endpoint class :)

@P-R-O-C-H-Y P-R-O-C-H-Y removed the Status: Pending Merge Pull Request is ready to be merged label Dec 16, 2024
@P-R-O-C-H-Y P-R-O-C-H-Y added the Status: Pending Merge Pull Request is ready to be merged label Dec 16, 2024
@P-R-O-C-H-Y
Copy link
Member

P-R-O-C-H-Y commented Dec 16, 2024

PR is now ready to be merged. @FaBjE hope you don't mind that I committed small change to avoid possible conflicts for future esp-zigbee-sdk releases :)

@me-no-dev me-no-dev merged commit 926c043 into espressif:release/v3.1.x Dec 16, 2024
68 checks passed
@FaBjE
Copy link
Contributor Author

FaBjE commented Dec 16, 2024

PR is now ready to be merged. @FaBjE hope you don't mind that I committed small change to avoid possible conflicts for future esp-zigbee-sdk releases :)

Not at all. Makes complete sense, we just missed it during the rename of the others.
I kept the functions prefix the same as in the zigbee stack, but now that is outside of that scope it shouldn't be.

I'm glad it is merged, and hope it will benefit others.
I think the (only) one still missing is the white-temperature tuneable light.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Libraries Issue is related to Library support. Status: Pending Merge Pull Request is ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants