From 325b46cd8ef955a9cce873facbce71615f46f1bc Mon Sep 17 00:00:00 2001 From: mkardous-silabs <84793247+mkardous-silabs@users.noreply.github.com> Date: Wed, 12 Jan 2022 16:00:35 -0500 Subject: [PATCH] [Group] Fix group messaging after test were disabled (#13505) * Fix Group Messaging * generated files * zap generation error * comment clean up --- .../commands/common/CommandInvoker.h | 4 +- src/app/CommandHandler.cpp | 12 ++- src/app/WriteHandler.cpp | 12 ++- src/app/server/Server.cpp | 2 +- src/app/tests/suites/TestGroupMessaging.yaml | 19 ++-- src/controller/CHIPCluster.h | 13 +-- ...ThreadStackManagerImpl_OpenThread_LwIP.cpp | 2 + src/transport/GroupSession.h | 7 ++ src/transport/SessionManager.cpp | 11 ++- src/transport/SessionManager.h | 1 + .../chip-tool/zap-generated/test/Commands.h | 90 ++++++++++++++++--- 11 files changed, 142 insertions(+), 31 deletions(-) diff --git a/examples/chip-tool/commands/common/CommandInvoker.h b/examples/chip-tool/commands/common/CommandInvoker.h index e51f141a50ea5b..fa16362d3e696a 100644 --- a/examples/chip-tool/commands/common/CommandInvoker.h +++ b/examples/chip-tool/commands/common/CommandInvoker.h @@ -115,9 +115,11 @@ class CommandInvoker final : public ResponseReceiverSendCommandRequest(session)); + ReturnErrorOnFailure(commandSender->SendCommandRequest(session.Value())); commandSender.release(); + exchangeManager->GetSessionManager()->RemoveGroupSession(session.Value()->AsGroupSession()); + return CHIP_NO_ERROR; } }; diff --git a/src/app/CommandHandler.cpp b/src/app/CommandHandler.cpp index 7e41b30b7d2c98..cabddbb1f8b57c 100644 --- a/src/app/CommandHandler.cpp +++ b/src/app/CommandHandler.cpp @@ -436,7 +436,17 @@ TLV::TLVWriter * CommandHandler::GetCommandDataIBTLVWriter() FabricIndex CommandHandler::GetAccessingFabricIndex() const { - return mpExchangeCtx->GetSessionHandle()->AsSecureSession()->GetFabricIndex(); + FabricIndex fabric = kUndefinedFabricIndex; + if (mpExchangeCtx->GetSessionHandle()->IsGroupSession()) + { + fabric = mpExchangeCtx->GetSessionHandle()->AsGroupSession()->GetFabricIndex(); + } + else + { + fabric = mpExchangeCtx->GetSessionHandle()->AsSecureSession()->GetFabricIndex(); + } + + return fabric; } CommandHandler * CommandHandler::Handle::Get() diff --git a/src/app/WriteHandler.cpp b/src/app/WriteHandler.cpp index 8ec46e57bb82cf..209246c68e9f62 100644 --- a/src/app/WriteHandler.cpp +++ b/src/app/WriteHandler.cpp @@ -279,7 +279,17 @@ CHIP_ERROR WriteHandler::AddStatus(const AttributePathParams & aAttributePathPar FabricIndex WriteHandler::GetAccessingFabricIndex() const { - return mpExchangeCtx->GetSessionHandle()->AsSecureSession()->GetFabricIndex(); + FabricIndex fabric = kUndefinedFabricIndex; + if (mpExchangeCtx->GetSessionHandle()->IsGroupSession()) + { + fabric = mpExchangeCtx->GetSessionHandle()->AsGroupSession()->GetFabricIndex(); + } + else + { + fabric = mpExchangeCtx->GetSessionHandle()->AsSecureSession()->GetFabricIndex(); + } + + return fabric; } const char * WriteHandler::GetStateStr() const diff --git a/src/app/server/Server.cpp b/src/app/server/Server.cpp index a4a39cbc246a01..c7394bc0d74d51 100644 --- a/src/app/server/Server.cpp +++ b/src/app/server/Server.cpp @@ -159,7 +159,7 @@ CHIP_ERROR Server::Init(AppDelegate * delegate, uint16_t secureServicePort, uint // for (iterate through all GroupDataProvider multicast Address) // { #ifdef CHIP_ENABLE_GROUP_MESSAGING_TESTS - err = mTransports.MulticastGroupJoinLeave(Transport::PeerAddress::Multicast(1, 1234), true); + err = mTransports.MulticastGroupJoinLeave(Transport::PeerAddress::Multicast(0, 1234), true); SuccessOrExit(err); #endif //} diff --git a/src/app/tests/suites/TestGroupMessaging.yaml b/src/app/tests/suites/TestGroupMessaging.yaml index fc9fcc2367af4c..ab7490ce0358d9 100644 --- a/src/app/tests/suites/TestGroupMessaging.yaml +++ b/src/app/tests/suites/TestGroupMessaging.yaml @@ -12,6 +12,12 @@ # See the License for the specific language governing permissions and # limitations under the License. +# !!!!!!!!!! TEST INFORMATION !!!!!!!!!!!!!!!!!! +# This test file tests Group Multicast Messaging. +# For this test to work, A Group Write/Command and a unicast read need to occur to be able to confirm that the group Communication works. Every test comes in a pair +# Only Works on Linux machines +# When building chip-tool, chip_enable_group_messaging_tests needs to be set to true in the build command for the test to pass +# ./scripts/examples/gn_build_example.sh examples/all-clusters-app/linux out/debug/standalone/ chip_config_network_layer_ble=false is_tsan=true chip_enable_group_messaging_tests=true name: Basic Group Messaging Tests config: @@ -26,41 +32,44 @@ tests: #TODO : Add Group membership command when implemented Issue #11077 # - label: "Add device to Group" # command: "TODO" - + # Test Pair 1 : Sends a Group Write Attribute - label: "Group Write Attribute" command: "writeAttribute" attribute: "location" groupId: "1234" arguments: value: "us" - #Disabled due to issue-12983 + + # Test Pair 1 : Validates previous group write attribute with a unicast to read - label: "Read back Attribute" - disabled: true command: "readAttribute" attribute: "location" response: value: "us" + # Test Pair 2 : Sends a Group Write Attribute - label: "Restore initial location value" command: "writeAttribute" attribute: "location" groupId: "1234" arguments: value: "" - #Disabled due to issue-12983 + + # Test Pair 2 : Validates previous group write attribute with a unicast to read - label: "Read back Attribute" - disabled: true command: "readAttribute" attribute: "location" response: value: "" + # Test Pair 3 : Sends a Group command - label: "Turn On the light to see attribute change" cluster: "On/Off" command: "On" groupId: "1234" disabled: true + # Test Pair 3 : Validates previous group command with a unicast to read - label: "Check on/off attribute value is true after on command" cluster: "On/Off" command: "readAttribute" diff --git a/src/controller/CHIPCluster.h b/src/controller/CHIPCluster.h index ca02eedb9f4a8e..2e1f60ae700542 100644 --- a/src/controller/CHIPCluster.h +++ b/src/controller/CHIPCluster.h @@ -115,6 +115,7 @@ class DLL_EXPORT ClusterBase WriteResponseSuccessCallback successCb, WriteResponseFailureCallback failureCb, const Optional & aTimedWriteTimeoutMs, WriteResponseDoneCallback doneCb = nullptr) { + CHIP_ERROR err = CHIP_NO_ERROR; VerifyOrReturnError(mDevice != nullptr, CHIP_ERROR_INCORRECT_STATE); auto onSuccessCb = [context, successCb](const app::ConcreteAttributePath & commandPath) { @@ -141,15 +142,17 @@ class DLL_EXPORT ClusterBase if (mGroupSession) { - return chip::Controller::WriteAttribute(mGroupSession.Get(), mEndpoint, clusterId, attributeId, requestData, - onSuccessCb, onFailureCb, aTimedWriteTimeoutMs, onDoneCb); + err = chip::Controller::WriteAttribute(mGroupSession.Get(), mEndpoint, clusterId, attributeId, requestData, + onSuccessCb, onFailureCb, aTimedWriteTimeoutMs, onDoneCb); + mDevice->GetExchangeManager()->GetSessionManager()->RemoveGroupSession(mGroupSession->AsGroupSession()); } else { - return chip::Controller::WriteAttribute(mDevice->GetSecureSession().Value(), mEndpoint, clusterId, - attributeId, requestData, onSuccessCb, onFailureCb, - aTimedWriteTimeoutMs, onDoneCb); + err = chip::Controller::WriteAttribute(mDevice->GetSecureSession().Value(), mEndpoint, clusterId, attributeId, + requestData, onSuccessCb, onFailureCb, aTimedWriteTimeoutMs, onDoneCb); } + + return err; } template diff --git a/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread_LwIP.cpp b/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread_LwIP.cpp index 171751d58947e7..603418cd252ea7 100644 --- a/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread_LwIP.cpp +++ b/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread_LwIP.cpp @@ -261,6 +261,8 @@ void GenericThreadStackManagerImpl_OpenThread_LwIP::UpdateThreadInter it->Release(); } } + + err = transportMgr->MulticastGroupJoinLeave(Transport::PeerAddress::Multicast(0, 1234), true); } #endif // CHIP_SYSTEM_CONFIG_USE_OPEN_THREAD_UDP } diff --git a/src/transport/GroupSession.h b/src/transport/GroupSession.h index f0fd85a9b66a8e..41ee154ed91361 100644 --- a/src/transport/GroupSession.h +++ b/src/transport/GroupSession.h @@ -119,6 +119,13 @@ class GroupSessionTable } } + /** + * @brief Deletes an entry from the object pool + * + * @param entry + */ + void DeleteEntry(GroupSession * entry) { mEntries.ReleaseObject(entry); } + private: BitMapObjectPool mEntries; }; diff --git a/src/transport/SessionManager.cpp b/src/transport/SessionManager.cpp index fdd4622e0e36f4..5b2e088357fc25 100644 --- a/src/transport/SessionManager.cpp +++ b/src/transport/SessionManager.cpp @@ -540,12 +540,6 @@ void SessionManager::SecureGroupMessageDispatch(const PacketHeader & packetHeade return; // malformed packet } - Optional session = FindGroupSession(packetHeader.GetDestinationGroupId().Value()); - if (!session.HasValue()) - { - return; - } - if (msg.IsNull()) { ChipLogError(Inet, "Secure transport received Groupcast NULL packet, discarding"); @@ -603,7 +597,12 @@ void SessionManager::SecureGroupMessageDispatch(const PacketHeader & packetHeade if (mCB != nullptr) { + Optional session = CreateGroupSession(packetHeader.GetDestinationGroupId().Value()); + VerifyOrReturn(session.HasValue(), ChipLogError(Inet, "Error when creating group session handle.")); + mCB->OnMessageReceived(packetHeader, payloadHeader, session.Value(), peerAddress, isDuplicate, std::move(msg)); + + RemoveGroupSession(session.Value()->AsGroupSession()); } } diff --git a/src/transport/SessionManager.h b/src/transport/SessionManager.h index c2187ed11d7919..b7005854f6e64d 100644 --- a/src/transport/SessionManager.h +++ b/src/transport/SessionManager.h @@ -212,6 +212,7 @@ class DLL_EXPORT SessionManager : public TransportMgrDelegate // TODO: implements group sessions Optional CreateGroupSession(GroupId group) { return mGroupSessions.AllocEntry(group, kUndefinedFabricIndex); } Optional FindGroupSession(GroupId group) { return mGroupSessions.FindEntry(group, kUndefinedFabricIndex); } + void RemoveGroupSession(Transport::GroupSession * session) { mGroupSessions.DeleteEntry(session); } // TODO: this is a temporary solution for legacy tests which use nodeId to send packets // and tv-casting-app that uses the TV's node ID to find the associated secure session diff --git a/zzz_generated/chip-tool/zap-generated/test/Commands.h b/zzz_generated/chip-tool/zap-generated/test/Commands.h index 50bffa05251424..42f249f4f27b9a 100644 --- a/zzz_generated/chip-tool/zap-generated/test/Commands.h +++ b/zzz_generated/chip-tool/zap-generated/test/Commands.h @@ -61975,8 +61975,16 @@ class TestGroupMessaging : public TestCommand err = TestGroupWriteAttribute_1(); break; case 2: - ChipLogProgress(chipTool, " ***** Test Step 2 : Restore initial location value\n"); - err = TestRestoreInitialLocationValue_2(); + ChipLogProgress(chipTool, " ***** Test Step 2 : Read back Attribute\n"); + err = TestReadBackAttribute_2(); + break; + case 3: + ChipLogProgress(chipTool, " ***** Test Step 3 : Restore initial location value\n"); + err = TestRestoreInitialLocationValue_3(); + break; + case 4: + ChipLogProgress(chipTool, " ***** Test Step 4 : Read back Attribute\n"); + err = TestReadBackAttribute_4(); break; } @@ -61989,7 +61997,7 @@ class TestGroupMessaging : public TestCommand private: std::atomic_uint16_t mTestIndex; - const uint16_t mTestCount = 3; + const uint16_t mTestCount = 5; chip::Optional mCluster; chip::Optional mEndpoint; @@ -62003,14 +62011,34 @@ class TestGroupMessaging : public TestCommand static void OnSuccessCallback_1(void * context) { (static_cast(context))->OnSuccessResponse_1(); } - static void OnDoneCallback_2(void * context) { (static_cast(context))->OnDoneResponse_2(); } - static void OnFailureCallback_2(void * context, EmberAfStatus status) { (static_cast(context))->OnFailureResponse_2(status); } - static void OnSuccessCallback_2(void * context) { (static_cast(context))->OnSuccessResponse_2(); } + static void OnSuccessCallback_2(void * context, chip::CharSpan location) + { + (static_cast(context))->OnSuccessResponse_2(location); + } + + static void OnDoneCallback_3(void * context) { (static_cast(context))->OnDoneResponse_3(); } + + static void OnFailureCallback_3(void * context, EmberAfStatus status) + { + (static_cast(context))->OnFailureResponse_3(status); + } + + static void OnSuccessCallback_3(void * context) { (static_cast(context))->OnSuccessResponse_3(); } + + static void OnFailureCallback_4(void * context, EmberAfStatus status) + { + (static_cast(context))->OnFailureResponse_4(status); + } + + static void OnSuccessCallback_4(void * context, chip::CharSpan location) + { + (static_cast(context))->OnSuccessResponse_4(location); + } // // Tests methods @@ -62042,7 +62070,27 @@ class TestGroupMessaging : public TestCommand void OnDoneResponse_1() { NextTest(); } - CHIP_ERROR TestRestoreInitialLocationValue_2() + CHIP_ERROR TestReadBackAttribute_2() + { + const chip::EndpointId endpoint = mEndpoint.HasValue() ? mEndpoint.Value() : 0; + chip::Controller::BasicClusterTest cluster; + cluster.Associate(mDevices[kIdentityAlpha], endpoint); + + ReturnErrorOnFailure(cluster.ReadAttribute( + this, OnSuccessCallback_2, OnFailureCallback_2)); + return CHIP_NO_ERROR; + } + + void OnFailureResponse_2(EmberAfStatus status) { ThrowFailureResponse(); } + + void OnSuccessResponse_2(chip::CharSpan location) + { + VerifyOrReturn(CheckValueAsString("location", location, chip::CharSpan("us", 2))); + + NextTest(); + } + + CHIP_ERROR TestRestoreInitialLocationValue_3() { const chip::GroupId groupId = 1234; chip::Controller::BasicClusterTest cluster; @@ -62052,15 +62100,35 @@ class TestGroupMessaging : public TestCommand locationArgument = chip::Span("garbage: not in length on purpose", 0); ReturnErrorOnFailure(cluster.WriteAttribute( - locationArgument, this, OnSuccessCallback_2, OnFailureCallback_2, OnDoneCallback_2)); + locationArgument, this, OnSuccessCallback_3, OnFailureCallback_3, OnDoneCallback_3)); return CHIP_NO_ERROR; } - void OnFailureResponse_2(EmberAfStatus status) { ThrowFailureResponse(); } + void OnFailureResponse_3(EmberAfStatus status) { ThrowFailureResponse(); } - void OnSuccessResponse_2() { NextTest(); } + void OnSuccessResponse_3() { NextTest(); } - void OnDoneResponse_2() { NextTest(); } + void OnDoneResponse_3() { NextTest(); } + + CHIP_ERROR TestReadBackAttribute_4() + { + const chip::EndpointId endpoint = mEndpoint.HasValue() ? mEndpoint.Value() : 0; + chip::Controller::BasicClusterTest cluster; + cluster.Associate(mDevices[kIdentityAlpha], endpoint); + + ReturnErrorOnFailure(cluster.ReadAttribute( + this, OnSuccessCallback_4, OnFailureCallback_4)); + return CHIP_NO_ERROR; + } + + void OnFailureResponse_4(EmberAfStatus status) { ThrowFailureResponse(); } + + void OnSuccessResponse_4(chip::CharSpan location) + { + VerifyOrReturn(CheckValueAsString("location", location, chip::CharSpan("", 0))); + + NextTest(); + } }; class Test_TC_SWDIAG_1_1 : public TestCommand