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.

Sign up for GitHub

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

Jump to bottom

[BUG] The command handler state tracking does not work properly for the groupcast messages. #30472

Closed
kkasperczyk-no opened this issue Nov 14, 2023 · 2 comments · Fixed by #30484, #31146 or meulemanelectronics/connectedhomeip#24
Closed

[BUG] The command handler state tracking does not work properly for the groupcast messages. #30472

kkasperczyk-no opened this issue Nov 14, 2023 · 2 comments · Fixed by #30484, #31146 or meulemanelectronics/connectedhomeip#24
Assignees
@tehampson @kkasperczyk-no
Labels
bug Something isn't working

Comments

@kkasperczyk-no
Copy link
Contributor

kkasperczyk-no commented Nov 14, 2023 •
edited
Loading

Reproduction steps

The DispatchCommand is called the same way for both groupcast and unicast scenarios: mpCallback->DispatchCommand(*this, concretePath, dataReader); and it doesn't pass the information about the originator of this call. It results in attempts of updating the status (with AddStatus method) for the invoking response purposes.

For the groupcast scenario updating status does not make sense, as response will not be sent anyway and additionally it leads to the application being killed if more than one endpoint existing on a single Matter node was added to the same group.
In such case receiving the groupcast command invokes the status update, which works well for the first endpoint from a list, but it fails for the following ones on a following check: VerifyOrDie(FallibleAddStatus(aCommandPath, aStatus, context) == CHIP_NO_ERROR);

This issue affects all devices that would like to configure single group on more than one endpoint within the single Matter node and also bridge devices that may represent several different bridged devices belonging to a single group.

Bug prevalence

Always

GitHub hash of the SDK that was being used

Tested with 181b0cb, but it applies to master as well.

Platform

core

Platform Version(s)

All

Anything else?

The potential solution could be adding isGroupcastInvoked-like boolean flag to the DispatchCommand method, forward it up to the AddStatus call and then conditionally return without updating the status in case of groupcast scenario.
@kkasperczyk-no kkasperczyk-no added bug Something isn't working needs triage labels Nov 14, 2023
@tehampson
Copy link
Contributor

I think the suggestion that was proposed in slack is instead of plumbing a flag through CommandHandler::DispatchCommand all the way back to CommandHandler::AddStatus. That you just indicate through some state variable in CommandHandler that we are handling a group command and that way it gets dropped once CommandHandler::AddStatus is called

kkasperczyk-no added a commit to kkasperczyk-no/connectedhomeip that referenced this issue Nov 15, 2023
@kkasperczyk-no
Fix handling command handler status for group messages
cd20a53 
The command handler status is added for the response purposes
even in case of invoking commands targeted to the group.
In case of using more than one endpoint on a single
Matter node that was added to the same group, it leads to the
application crash. The direct reason is a state check,
that succeeds only for the first endpoint and fails for the
subsequent calls.

Added a new state flag, which informs if the command that is
currently handled origins from groupcast communication or not.
In case of targeting the group, application returns prematurely
and do not try to add status.

Fixes: project-chip#30472
kkasperczyk-no added a commit to kkasperczyk-no/connectedhomeip that referenced this issue Nov 15, 2023
@kkasperczyk-no
Fix handling command handler status for group messages
124cea2 
The command handler status is added for the response purposes
even in case of invoking commands targeted to the group.
In case of using more than one endpoint on a single
Matter node that was added to the same group, it leads to the
application crash. The direct reason is a state check,
that succeeds only for the first endpoint and fails for the
subsequent calls.

Added a new state flag, which informs if the command that is
currently handled origins from groupcast communication or not.
In case of targeting the group, application returns prematurely
and do not try to add status.

Fixes: project-chip#30472
@kkasperczyk-no kkasperczyk-no mentioned this issue Nov 15, 2023
Merged
@kkasperczyk-no kkasperczyk-no self-assigned this Nov 15, 2023
@mergify mergify bot closed this as completed in #30484 Nov 15, 2023
mergify bot pushed a commit that referenced this issue Nov 15, 2023
@kkasperczyk-no
Fix handling command handler status for group messages (#30484)
1ada682 
The command handler status is added for the response purposes
even in case of invoking commands targeted to the group.
In case of using more than one endpoint on a single
Matter node that was added to the same group, it leads to the
application crash. The direct reason is a state check,
that succeeds only for the first endpoint and fails for the
subsequent calls.

Added a new state flag, which informs if the command that is
currently handled origins from groupcast communication or not.
In case of targeting the group, application returns prematurely
and do not try to add status.

Fixes: #30472
@kkasperczyk-no
Copy link
Contributor Author

TODO: Add yaml tests covering the scenario of using multiple endpoints belonging to the same group and existing on a single Matter node.

@tehampson tehampson mentioned this issue Dec 20, 2023
Merged
@mergify mergify bot closed this as completed in #31146 Jan 4, 2024
maciejbaczmanski pushed a commit to maciejbaczmanski/connectedhomeip that referenced this issue Jul 15, 2024
@kkasperczyk-no
[nrf fromlist] Fix handling command handler status for group messages
99f80de 
The command handler status is added for the response purposes
even in case of invoking commands targeted to the group.
In case of using more than one endpoint on a single
Matter node that was added to the same group, it leads to the
application crash. The direct reason is a state check,
that succeeds only for the first endpoint and fails for the
subsequent calls.

Added a new state flag, which informs if the command that is
currently handled origins from groupcast communication or not.
In case of targeting the group, application returns prematurely
and do not try to add status.

Fixes: project-chip#30472
MauritsFassaert pushed a commit to meulemanelectronics/connectedhomeip that referenced this issue Aug 23, 2024
@kkasperczyk-no @MauritsFassaert
Fix handling command handler status for group messages (project-chip#…
17c08e4 
…30484)

The command handler status is added for the response purposes
even in case of invoking commands targeted to the group.
In case of using more than one endpoint on a single
Matter node that was added to the same group, it leads to the
application crash. The direct reason is a state check,
that succeeds only for the first endpoint and fails for the
subsequent calls.

Added a new state flag, which informs if the command that is
currently handled origins from groupcast communication or not.
In case of targeting the group, application returns prematurely
and do not try to add status.

Fixes: project-chip#30472
@MauritsFassaert MauritsFassaert mentioned this issue Aug 23, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@tehampson tehampson

@kkasperczyk-no kkasperczyk-no

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
2 participants
@tehampson @kkasperczyk-no