Skip to content

Update ZigbeeColorDimmableLight to clamp color hue and saturation to 0-254 (Fixes #11527) #11528

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

thorrak
Copy link

@thorrak thorrak commented Jun 29, 2025

Description of Change

The Zigbee standard requires color saturation in the range of 0-254, but espRgbColorToHsvColor returns saturations in the range of 0-255. As referenced in #11527 when a full saturation color is set, the following error is received:

[  6303][E][ZigbeeColorDimmableLight.cpp:180] setLight(): Failed to set light saturation: 0x87: Invalid value

This PR clamps the range of the saturation used in setLight() to 0-254 (reducing it to 254 from 255 when full saturation is set), eliminating the "Invalid value" error.

Tests scenarios

I have tested this change on my XIAO ESP32-C6

Related links

Fixes #11527

@thorrak thorrak requested a review from P-R-O-C-H-Y as a code owner June 29, 2025 14:14
@CLAassistant
Copy link

CLAassistant commented Jun 29, 2025

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

github-actions bot commented Jun 29, 2025

Warnings
⚠️

Some issues found for the commit messages in this PR:

  • the commit message "Clamp Zigbee color saturation to 0-254":
    • probably contains Jira ticket reference (0-254). Please remove Jira tickets from commit messages.
    • summary looks empty
    • type/action looks empty
  • the commit message "Clamp hue to 0-254 for Zigbee color lights":
    • probably contains Jira ticket reference (0-254). Please remove Jira tickets from commit messages.
    • summary looks empty
    • type/action looks empty
  • the commit message "Use std::min instead of ternary operator":
    • summary looks empty
    • type/action looks empty

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 thorrak, 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 020aedd

@thorrak
Copy link
Author

thorrak commented Jun 29, 2025

One downside to this fix (and with the Zigbee standard in general) is that there is necessarily a slight loss of information when mapping from a 0-255 saturation range to a 0-254 saturation range. This means that the color value known to Zigbee will always differ from any cached value set in code for colors with 255 saturation.

For example - if I set a color of #FF0000 in code (0H, 255S, 255V), it will get converted to #FF0101 (0H, 254S, 255V).

In practice, this may not be as much of an issue, as any color received via Zigbee will already have a maximum saturation value of 254 per the standard, but any color set from code will not be similarly limited.

I have no idea how to handle this other than potentially to log a warning to the console, but in my PR this error will just silently occur.

@thorrak thorrak changed the title Update ZigbeeColorDimmableLight to clamp color saturation to 0-254 (Fixes #11527) Update ZigbeeColorDimmableLight to clamp color hue and saturation to 0-254 (Fixes #11527) Jun 29, 2025
@thorrak
Copy link
Author

thorrak commented Jun 29, 2025

On a hunch I tested what happens when the hue was set to something that mapped to 255, and - sure enough - saw similar behavior:

[ 57201][E][ZBCDL.cpp:178] setLight(): Failed to set light hue: 0x87: Invalid value

I went ahead and applied the same fix for hue as I did for saturation.

I expect the same issue here as for saturation, unfortunately - any colors that would map to a hue of 255 (if set directly within code) will end up being clamped to a hue of 254. As this is a limitation of the Zigbee standard, I don't know if there is any way to correct for this, however.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Zigbee Color Dimmable Light - Invalid Saturation
3 participants