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

Invoke Batch Commands Feature for 1.3 #30453

Open
45 of 53 tasks
tehampson opened this issue Nov 13, 2023 · 0 comments
Open
45 of 53 tasks

Invoke Batch Commands Feature for 1.3 #30453

tehampson opened this issue Nov 13, 2023 · 0 comments
Assignees

Comments

@tehampson
Copy link
Contributor

tehampson commented Nov 13, 2023

Nice to complete TODO, but shouldn't affect feature launch

TODO issues discovered while working on this feature that would be good to follow up on

  • When CommandHandler fails to send the first message (when calling CommandHandler::StartSendingCommandResponses), we should be sending back a failure if possible. Existing unit tests start failing when this was added in
  • Spec mentions:
    If this action 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 Status Code SHALL be submitted to the message layer and this interaction SHALL terminate.
    But code send UnsupportedAccess. Note TestPlan expected behavior is looking for UnsupportedAccess
    https://github.com/CHIP-Specifications/chip-test-plans/pull/3841
    Fix IM error when TimeRequest flag doesn't state of isTimedInvoke #31485
  • It is pretty dangerous for CommandHandler to provide access to GetExchangeContext(). Unfortunately there is already a precedent of using in, but it seems like everything wants info that could be alternatively provided instead of giving access to the ExchangeContext directly. More info here
  • CommandHandler::GetCommandDataIBTLVWriter should not be public. This is a footgun in causing issue with CommandHandler state that is a larger issue for batch commands
  • Rolling back InvokeResponseMessag and InvokeRequestMessage's Builders requires manually resetting internal errors directly instead of it being handled for us. This would be a good quality of life improvement.

Archived section (as work is completed)

TODO before FC (Dec 2023):

CSG Cert work (Before TE2):

Critical bug fixes required (Before TE2):

Between TE2 and SVE

These items focus on improvements not with the feature itself, but with QoL improvements for those to more easily send or receive batch commands:

Important TODOs:

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

No branches or pull requests

2 participants