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

Fix Light Link default group name too long #129

Merged
merged 1 commit into from
Aug 27, 2024

Conversation

elupus
Copy link
Contributor

@elupus elupus commented Aug 5, 2024

This fixes the following error that occurs hidden in debug logs in tests:

    | Traceback (most recent call last):
    |   File "/Users/joakim/src/hass/zha/zha/zigbee/endpoint.py", line 200, in caller
    |     await getattr(ch, func_name)(*args)
    |   File "/Users/joakim/src/hass/zha/zha/zigbee/cluster_handlers/lightlink.py", line 48, in async_configure
    |     await coordinator.add_to_group(0x0000, name="Default Lightlink Group")
    |   File "/Users/joakim/src/hass/zigpy/zigpy/device.py", line 294, in add_to_group
    |     await ep.add_to_group(grp_id, name)
    |   File "/Users/joakim/src/hass/zigpy/zigpy/endpoint.py", line 149, in add_to_group
    |     res = await self.groups.add(grp_id, name)
    |           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |   File "/Users/joakim/src/hass/zigpy/zigpy/zcl/__init__.py", line 357, in request
    |     hdr, request = self._create_request(
    |                    ^^^^^^^^^^^^^^^^^^^^^
    |   File "/Users/joakim/src/hass/zigpy/zigpy/zcl/__init__.py", line 320, in _create_request
    |     request.serialize()  # Throw an error before generating a new TSN
    |     ^^^^^^^^^^^^^^^^^^^
    |   File "/Users/joakim/src/hass/zigpy/zigpy/types/struct.py", line 261, in serialize
    |     chunks.append(value.serialize())
    |                   ^^^^^^^^^^^^^^^^^
    |   File "/Users/joakim/src/hass/zigpy/zigpy/types/basic.py", line 983, in serialize
    |     raise ValueError(f"String is too long (>{self._max_len})")
    | ValueError: String is too long (>16)

Source of limitation: https://github.com/zigpy/zigpy/blob/925a112b8f64685164c7cbc89479a3ad76b9b5a1/zigpy/zcl/clusters/general.py#L613

Copy link

codecov bot commented Aug 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.68%. Comparing base (dfe5ae7) to head (1b55527).
Report is 8 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #129      +/-   ##
==========================================
+ Coverage   95.65%   95.68%   +0.03%     
==========================================
  Files          61       61              
  Lines        9255     9255              
==========================================
+ Hits         8853     8856       +3     
+ Misses        402      399       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dmulcahey
Copy link
Contributor

Should we just clamp the name to 16 characters when interacting w/ the ZCL layer?

image

@puddly
Copy link
Contributor

puddly commented Aug 7, 2024

We could use a UUID and map the group name in the database.

@dmulcahey
Copy link
Contributor

We could use a UUID and map the group name in the database.

I wonder... do we even need to supply the name in the cluster command? We already have the name and group id in the db...

@TheJulianJES
Copy link
Contributor

That group name is just for devices, yeah. Some don't even support keeping the group name and I don't think they're actually used anyway.
I'd just trim the name when sending it to the device or not send it at all.

Copy link
Contributor

@TheJulianJES TheJulianJES left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's include this for now. We can implement further changes (of completely capping the group names, or even removing them entirely, for ZCL) in the future.

@TheJulianJES TheJulianJES changed the title Group names are limited to 16 characters Fix Light Link default group name too long Aug 27, 2024
@TheJulianJES TheJulianJES merged commit f8d87b9 into zigpy:dev Aug 27, 2024
6 checks passed
@elupus elupus deleted the fix/length_of_group branch August 28, 2024 04:41
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.

4 participants