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

Update ClusterBase AssociateWithGroup method #11850

Closed
jepenven-silabs opened this issue Nov 16, 2021 · 4 comments · Fixed by #16583
Closed

Update ClusterBase AssociateWithGroup method #11850

jepenven-silabs opened this issue Nov 16, 2021 · 4 comments · Fixed by #16583

Comments

@jepenven-silabs
Copy link
Contributor

jepenven-silabs commented Nov 16, 2021

Problem

Current implementation is tailor made for testing group communication.

Proposed Solution

Refactor the Test part so that we don't need a DeviceProxy object in order to handle group methods (invoke commands/ write attributes)

@jepenven-silabs jepenven-silabs changed the title Update ClusterBase Associate method for group Update ClusterBase AssociateWithGroup method Nov 16, 2021
@turon
Copy link
Contributor

turon commented Feb 1, 2022

Reference to code in question:

cluster.AssociateWithGroup({{>device}}, groupId);

It looks like both group and non-group cases are handled, so perhaps this can be closed:

 {{#if isGroupCommand}}
        cluster.AssociateWithGroup({{>device}}, groupId);
 {{else}}
        cluster.Associate({{>device}}, endpoint);
{{/if}}

Removing tag v1_triage_split_4

@jepenven-silabs
Copy link
Contributor Author

@turon Wrong code quoted.

Problem is still present in master. The code assumes that the DeviceProxy object will have a valid SecureSession which is always true in the context of a yaml test but won't be in other context (e.g. inside Chip-tool ? ).

So this issue is still valid and still need to be fixed

CHIP_ERROR ClusterBase::AssociateWithGroup(DeviceProxy * device, GroupId groupId)
{
    // TODO Update this function to work in all possible conditions Issue #11850

    CHIP_ERROR err = CHIP_NO_ERROR;

    mDevice = device;
    if (mDevice->GetSecureSession().HasValue())
    {
        // Local copy to preserve original SessionHandle for future Unicast communication.
        Optional<SessionHandle> session = mDevice->GetExchangeManager()->GetSessionManager()->CreateGroupSession(
            groupId, mDevice->GetSecureSession().Value()->AsSecureSession()->GetFabricIndex());
        // Sanity check
        if (!session.HasValue() || !session.Value()->IsGroupSession())
        {
            err = CHIP_ERROR_INCORRECT_STATE;
        }

        mGroupSession.Grab(session.Value());
    }
    else
    {
        // something fishy is going on
        err = CHIP_ERROR_INCORRECT_STATE;
    }

    // Set to 0 for now.
    mEndpoint = 0;

    return err;
}

@jepenven-silabs
Copy link
Contributor Author

This might need to be fixed before #1818

@jepenven-silabs jepenven-silabs self-assigned this Feb 23, 2022
@jepenven-silabs
Copy link
Contributor Author

Not needed after all. #1818 was done without this changes.

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