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

[controller] Move callbacks to CommandResponseStatus since timing related issue is resolved. #7060

Merged
merged 1 commit into from
May 31, 2021

Conversation

erjiaqing
Copy link
Contributor

@erjiaqing erjiaqing commented May 24, 2021

Problem

  • When [im] Always use interaction model to send commands #5945 is landed, the DeviceControllerInteractionModelDelegate is introduced with some workaround to fix timing issues for callback manager.
    • This workaround is introduced with following considerations:
      • We cannot have multiple command transactions at the same time
      • We have apps that will send another response in the callback of one command.
  • With the landing of [IM] Decouple CommandSender with CHIPDevice #6802, we can have multiple command transactions at the same time, so we can replace the workaround by the correct implementation.

Change overview

  • Replace the workaround by the correct implementation.
  • This PR also eliminates the wrong message that the success callback is called after failure callback messages.

Testing

The test is covered by Darwin tests that the callbacks can be called correctly.

This PR requires #7041 and #7114

@woody-apple
Copy link
Contributor

Can confirm this regressed the tests in Darwin here:
#7270

Looks like a real failure

@woody-apple
Copy link
Contributor

Confirmed this PR had a failed build due a regression here:
https://github.com/project-chip/connectedhomeip/actions/runs/893496528

woody-apple added a commit that referenced this pull request Jun 1, 2021
…ming related issue is resolved. (#7060)"

This reverts commit 46599f3.
andy31415 added a commit to andy31415/connectedhomeip that referenced this pull request Jun 1, 2021
@bzbarsky-apple
Copy link
Contributor

It looks like there's a merge issue with 7a61a83. Specifically, this PR somehow causes message counter verification (which 7a61a83 re-enables) to fail. I am debugging this now.

andy31415 pushed a commit that referenced this pull request Jun 1, 2021
…ming related issue is resolved. (#7060)" (#7276)

This reverts commit 46599f3.
erjiaqing added a commit to erjiaqing/connectedhomeip that referenced this pull request Jun 11, 2021
andy31415 pushed a commit that referenced this pull request Jun 11, 2021
…since timing related issue is resolved. (#7060)" (#7276)" (#7542)

This reverts commit 872051e.
nikita-s-wrk pushed a commit to nikita-s-wrk/connectedhomeip that referenced this pull request Sep 23, 2021
nikita-s-wrk pushed a commit to nikita-s-wrk/connectedhomeip that referenced this pull request Sep 23, 2021
nikita-s-wrk pushed a commit to nikita-s-wrk/connectedhomeip that referenced this pull request Sep 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants