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

CommandSender::Callback API contract issue when Command has path-specific error response #30991

Closed
tehampson opened this issue Dec 13, 2023 · 0 comments · Fixed by #31324
Closed
Assignees

Comments

@tehampson
Copy link
Contributor

tehampson commented Dec 13, 2023

Background:

InvokeRequests have two kinds of errors

  • Non-path specific errors. When an invoke request message gets a corresponding invoke response with a non-path specific error, there will only be 1 error in the response message, regardless of how many Invoke Requests were in the initial request.
    • Example, client send InvokeRequestMessage that is marked with TimedRequest as TRUE, but this action is not part of a Timed Invoke transaction (i.e. there was no immediately previous Timed Invoke action), then a Status Response action with the TIMED_REQUEST_MISMATCH.
      • If there were multiple commands in the Invoke request message there would still be only 1 TIMED_REQUEST_MISMATCH error sent back in the response message
  • Path specific error. This is an error associated with the particular command within the array of InvokeRequests in the invoke request message. There can be N number of path specific errors where N is the number of requests.
    • Example, client send a command to using command path to an endpoint that doesn't exist. There is a path specific error with UNSUPPORTED_ENDPOINT
      • If there were N unique commands, but all command path, while unique, were to an endpoint that doesn't exist there would be N of errors in the response message with UNSUPPORTED_ENDPOINT.

Problem:

API contract doesn't explicitly state where path specific errors should go. In the OnResponse there is mention of cluster-specific status, but that doesn't inherently mean cluster-specific path error.

CommandSender::Callback::OnResponse* states here

* @param[in] aStatusIB       It will always have a success status. If apData is null, it can be any success status,
*                            including possibly a cluster-specific one. If apData is not null it aStatusIB will always
*                            be a generic SUCCESS status with no-cluster specific information.

CommandSender::Callback::OnError states here

* - CHIP_ERROR encapsulating a StatusIB: If we got a non-path-specific
*   status response from the server.  In that case,
*   StatusIB::InitFromChipError can be used to extract the status.

As per API contract defined today there is no where for path-specific StatusIB's to go. But as of today they are going to OnError in this section of code:

This is where we are providing an encapsulated StatusIB that may be a path-specific response incorrectly to the OnError:
            if (statusIB.IsSuccess())
            {
                mpCallback->OnResponseWithAdditionalData(this, ConcreteCommandPath(endpointId, clusterId, commandId), statusIB,
                                                         hasDataResponse ? &commandDataReader : nullptr, additionalResponseData);
            }
            else
            {
                mpCallback->OnError(this, statusIB.ToChipError());
            }

Non viable solution:

Simply calling OnResponse for path specific instead of what we are calling today OnError:
Assuming that there is code outside of SDK that implements their own version of these callback, we cannot implement a solution that would break pre-exisiting expectations. As such I don't think we can simply re-uses OnResponse, for path specific error. Unless my assumption that these callback, and/or other SDK APIs downstream of these callback (For example how chip-repl calls controller.SendCommand(...)) all live within the SDK can can all be contained within 1 PR, which I don't think is the case at all

Latest Proposed solution:

Rejected solutions:

  1. Redefine CommandSender::Callback::OnResponse API contract to take path-specific errors #31017
  2. Introduce CommandSender::Callback::OnPathSpecificError  #30998
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment