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

Fix handling of ZCL Default Response #5009

Merged
merged 3 commits into from
Jun 13, 2021

Conversation

manup
Copy link
Member

@manup manup commented Jun 13, 2021

This PR adds automatic handling of ZCL Default Response to also catch cases where we currently miss sending the response.

The magic happens by observing outgoing APS requests if they contain a specific ZCL response and automatically send the ZCL Default Response at the end of the APS indication processing. For this C++ RAII is used with an extra ZclDefaultResponder object which sends the response in its destructor.

  • All outgoing APS requests use ApsControllerWrapper instead of plain ApsController.
  • All manual sending of ZCL Default Responses was removed.
  • Moved isSameAddress()into utils.h.
  • Removed sendZclDefaultResponse() from plugin.

This PR adds automatic handling of ZCL Default Response to also catch cases where we currently miss sending the response.

The magic happens by observing outgoing APS requests if they contain a specific ZCL response and automatically send the ZCL Default Response at the end of the APS indication processing. For this C++ RAII is used with an extra ZclDefaultResponder object which sends the response in its destructor.

All manual sending of ZCL Default Responses was removed.
@manup manup added this to the v2.12.1-beta milestone Jun 13, 2021
@manup manup requested review from Smanar, SwoopX and ebaauw June 13, 2021 16:00
@Smanar
Copy link
Collaborator

Smanar commented Jun 13, 2021

Hudge code.

So we already can remove all

        if ((zclFrame.commandId() == XXXX )&& !(zclFrame.frameControl() & deCONZ::ZclFCDisableDefaultResponse))
        {
            sendZclDefaultResponse(ind, zclFrame, deCONZ::ZclSuccessStatus);
        }

?

Or need to check case by case ?

@manup
Copy link
Member Author

manup commented Jun 13, 2021

So we already can remove all

        if ((zclFrame.commandId() == XXXX )&& !(zclFrame.frameControl() & deCONZ::ZclFCDisableDefaultResponse))
        {
            sendZclDefaultResponse(ind, zclFrame, deCONZ::ZclSuccessStatus);
        }

Yes, in fact the PR removed all of these :)

Or need to check case by case ?

No, basically the PR is made to not worry about ZCL Default Response at all anymore.

@Smanar
Copy link
Collaborator

Smanar commented Jun 13, 2021

Yes, have see it after ^^.

Real improvement for me, was realy a weakness for me.

@manup
Copy link
Member Author

manup commented Jun 13, 2021

I hope so. We have seen increasingly problems for missing ZCL Default Reponse and I guess lots of issues are related although the reason wasn't seen to be this problem, with effects like devices dropping off or rejoining etc.

The PR touches lots of files but the actual code is rather minimal.

@SwoopX
Copy link
Collaborator

SwoopX commented Jun 13, 2021

Great stuff. From my perspective, this addresses one of the current major weaknesses and will hopefully automagically solve some issues users are experiencing.

I've let the code run a bit in my test environment and made some observations so far:

  • A default response is sent prior to a response (now observed with 2 devices)

grafik
grafik

  • A default response might have taken too long to prevent a resend of the initial command

grafik

Especially the latter is a bit hard to judge upon, since the device is not yet supported. Guess APS data could give some clarity.

@manup manup merged commit eb09170 into dresden-elektronik:master Jun 13, 2021
@manup
Copy link
Member Author

manup commented Jul 15, 2021

I've let the code run a bit in my test environment and made some observations so far:

* A default response is sent prior to a response (now observed with 2 devices)

Oh I need to double check this, I think the default responder shouldn't handle OTA related commands since the REST plugin is independent of the OTA plugin.

@manup manup deleted the zcl_default_resp branch January 3, 2023 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants