espressif / esp-matter

Espressif's SDK for Matter
Apache License 2.0
658 stars 154 forks source link

descriptor::create should be belong to Base Device Type. (CON-1222) #983

Closed ronny-antoon closed 1 month ago

ronny-antoon commented 3 months ago

Describe the question/query that you have I think the descriptor::create should be belong to the function where the create endpoint and not where the add. the spec says it blong to Base Device Type

Additional context this line should be removed https://github.com/espressif/esp-matter/blob/4ca1207fad42075d136284ec7ac90e7a5095fdd2/components/esp_matter/esp_matter_endpoint.cpp#L162C5-L162C15

and add it to here before the add func

https://github.com/espressif/esp-matter/blob/4ca1207fad42075d136284ec7ac90e7a5095fdd2/components/esp_matter/esp_matter_endpoint.cpp#L146

shubhamdp commented 3 months ago

the spec says it blong to Base Device Type

Can you point me to the spec text, section Id would be a good pointer.

In esp_matter_endpoint.cpp, we have APIs to create device types. The create() and add() methods are designed to create the endpoint and the requested device type.

If you want to add more device types to the same endpoint, you should use the add() API.

All cluster-name::create() APIs, such as descriptor::create(), are safe to call multiple times and won't create the same cluster again.

ronny-antoon commented 3 months ago

Specification Page 18 - 1.1.7. Base Device Type Requirements for Matter

the descriptor cluster should belong to the Base Device Type. it means that it should be in the create function and not the add function.

like here:- https://github.com/espressif/esp-matter/blob/91b8cb1596cdcf09bf379fdefa2cd11e46a83009/components/esp_matter/esp_matter_endpoint.cpp#L143-L148

and not here :- https://github.com/espressif/esp-matter/blob/91b8cb1596cdcf09bf379fdefa2cd11e46a83009/components/esp_matter/esp_matter_endpoint.cpp#L150-L170

because it belong to the Base Device Type and not to the on_off_light type. so when you create any device type it will create the descriptor cluster and if you add a device type it should not create the descriptor cluster. "I know its not an error, but its a warning and handled well (safe to call multiple times)"

jonsmirl commented 3 months ago

Don't descriptor and identify have to be on every endpoint? If so, you could combine them with create endpoint.

ronny-antoon commented 3 months ago

Don't descriptor and identify have to be on every endpoint? If so, you could combine them with create endpoint.

No, the identify not for all endpoint, like root_node dont have identify but it have descriptor

shubhamdp commented 1 month ago

Fixed with https://github.com/espressif/esp-matter/commit/71991319ce7dbb49376b33e21a0c087a317c5110 and this issue can be closed

ronny-antoon commented 1 month ago

thank you, I wish I could get a Contributor here XD Good Job Team

shubhamdp commented 1 month ago

I wish I could get a Contributor here XD

@ronny-antoon we accept pull request as well :-)