From 40553955f8661b7af3b6a33151b1a0645690c306 Mon Sep 17 00:00:00 2001 From: chrisdecenzo <61757564+chrisdecenzo@users.noreply.github.com> Date: Thu, 14 Dec 2023 03:47:40 -0800 Subject: [PATCH] Fix TV commissioning and control (#30904) * tv2 fixes * debug CI failure * address comments, add code to debug CI * add code to debug CI * add code to debug CI * Restyled by clang-format (#30960) Co-authored-by: Restyled.io * revert CI debug --------- Co-authored-by: restyled-io[bot] <32688539+restyled-io[bot]@users.noreply.github.com> Co-authored-by: Restyled.io --- examples/platform/linux/CommissionerMain.cpp | 3 +- .../UDCClientState.h | 35 ++++++---- .../UserDirectedCommissioning.h | 52 ++++++++------- .../UserDirectedCommissioningClient.cpp | 26 ++++++-- .../UserDirectedCommissioningServer.cpp | 64 ++++++++++++++++--- .../tests/TestUdcMessages.cpp | 35 ++++++---- 6 files changed, 155 insertions(+), 60 deletions(-) diff --git a/examples/platform/linux/CommissionerMain.cpp b/examples/platform/linux/CommissionerMain.cpp index 64a7a47d2dffcb..2b3f98ba5868b2 100644 --- a/examples/platform/linux/CommissionerMain.cpp +++ b/examples/platform/linux/CommissionerMain.cpp @@ -181,7 +181,8 @@ CHIP_ERROR InitCommissioner(uint16_t commissionerPort, uint16_t udcListenPort, F params.controllerICAC = icacSpan; params.controllerNOC = nocSpan; - params.defaultCommissioner = &gAutoCommissioner; + params.defaultCommissioner = &gAutoCommissioner; + params.enableServerInteractions = true; // assign prefered feature settings CommissioningParameters commissioningParams = gAutoCommissioner.GetCommissioningParameters(); diff --git a/src/protocols/user_directed_commissioning/UDCClientState.h b/src/protocols/user_directed_commissioning/UDCClientState.h index 983cfb73e21c0c..944a1e156ad9f2 100644 --- a/src/protocols/user_directed_commissioning/UDCClientState.h +++ b/src/protocols/user_directed_commissioning/UDCClientState.h @@ -43,6 +43,15 @@ enum class UDCClientProcessingState : uint8_t using PeerAddress = ::chip::Transport::PeerAddress; +/** + * Represents information in the TargetAppList of the Identification Declaration message + */ +struct TargetAppInfo +{ + uint16_t vendorId = 0; + uint16_t productId = 0; +}; + /** * Defines the handling state of a UDC Client. * @@ -99,25 +108,29 @@ class UDCClientState uint16_t GetPairingHint() const { return mPairingHint; } void SetPairingHint(uint16_t pairingHint) { mPairingHint = pairingHint; } - bool GetAppVendorId(size_t index, uint16_t & vid) const + bool GetTargetAppInfo(uint8_t index, TargetAppInfo & info) const { - if (index < mNumAppVendorIds) + if (index < mNumTargetAppInfos) { - vid = mAppVendorIds[index]; + info.vendorId = mTargetAppInfos[index].vendorId; + info.productId = mTargetAppInfos[index].productId; return true; } return false; } - size_t GetNumAppVendorIds() const { return mNumAppVendorIds; } + size_t GetNumTargetAppInfos() const { return mNumTargetAppInfos; } - void AddAppVendorId(uint16_t vid) + bool AddTargetAppInfo(TargetAppInfo vid) { - if (mNumAppVendorIds >= sizeof(mAppVendorIds)) + if (mNumTargetAppInfos >= sizeof(mTargetAppInfos)) { // already at max - return; + return false; } - mAppVendorIds[mNumAppVendorIds++] = vid; + mTargetAppInfos[mNumTargetAppInfos].vendorId = vid.vendorId; + mTargetAppInfos[mNumTargetAppInfos].productId = vid.productId; + mNumTargetAppInfos++; + return true; } UDCClientProcessingState GetUDCClientProcessingState() const { return mUDCClientProcessingState; } @@ -183,9 +196,9 @@ class UDCClientState char mPairingInst[chip::Dnssd::kMaxPairingInstructionLen + 1] = {}; uint16_t mPairingHint = 0; - constexpr static size_t kMaxAppVendorIds = 10; - size_t mNumAppVendorIds = 0; // number of vendor Ids - uint16_t mAppVendorIds[kMaxAppVendorIds]; + constexpr static size_t kMaxTargetAppInfos = 10; + uint8_t mNumTargetAppInfos = 0; // number of vendor Ids + TargetAppInfo mTargetAppInfos[kMaxTargetAppInfos]; bool mNoPasscode = false; bool mCdUponPasscodeDialog = false; diff --git a/src/protocols/user_directed_commissioning/UserDirectedCommissioning.h b/src/protocols/user_directed_commissioning/UserDirectedCommissioning.h index 12315a06b29041..d05a80a857f200 100644 --- a/src/protocols/user_directed_commissioning/UserDirectedCommissioning.h +++ b/src/protocols/user_directed_commissioning/UserDirectedCommissioning.h @@ -81,7 +81,7 @@ class DLL_EXPORT IdentificationDeclaration bool HasDiscoveryInfo() { return mVendorId != 0 || mProductId != 0 || mCdPort != 0 || strlen(mDeviceName) > 0 || GetRotatingIdLength() > 0 || - mNumAppVendorIds > 0 || mNoPasscode || mCdUponPasscodeDialog || mCommissionerPasscode || mCommissionerPasscodeReady; + mNumTargetAppInfos > 0 || mNoPasscode || mCdUponPasscodeDialog || mCommissionerPasscode || mCommissionerPasscodeReady; } const char * GetDeviceName() const { return mDeviceName; } @@ -105,25 +105,29 @@ class DLL_EXPORT IdentificationDeclaration memcpy(mRotatingId, rotatingId, mRotatingIdLen); } - bool GetAppVendorId(uint8_t index, uint16_t & vid) const + bool GetTargetAppInfo(uint8_t index, TargetAppInfo & info) const { - if (index < mNumAppVendorIds) + if (index < mNumTargetAppInfos) { - vid = mAppVendorIds[index]; + info.vendorId = mTargetAppInfos[index].vendorId; + info.productId = mTargetAppInfos[index].productId; return true; } return false; } - size_t GetNumAppVendorIds() const { return mNumAppVendorIds; } + size_t GetNumTargetAppInfos() const { return mNumTargetAppInfos; } - void AddAppVendorId(uint16_t vid) + bool AddTargetAppInfo(TargetAppInfo vid) { - if (mNumAppVendorIds >= sizeof(mAppVendorIds)) + if (mNumTargetAppInfos >= sizeof(mTargetAppInfos)) { // already at max - return; + return false; } - mAppVendorIds[mNumAppVendorIds++] = vid; + mTargetAppInfos[mNumTargetAppInfos].vendorId = vid.vendorId; + mTargetAppInfos[mNumTargetAppInfos].productId = vid.productId; + mNumTargetAppInfos++; + return true; } const char * GetPairingInst() const { return mPairingInst; } @@ -170,12 +174,12 @@ class DLL_EXPORT IdentificationDeclaration client->SetRotatingId(GetRotatingId(), GetRotatingIdLength()); client->SetPairingInst(GetPairingInst()); client->SetPairingHint(GetPairingHint()); - for (uint8_t i = 0; i < GetNumAppVendorIds(); i++) + for (uint8_t i = 0; i < GetNumTargetAppInfos(); i++) { - uint16_t vid; - if (GetAppVendorId(i, vid)) + TargetAppInfo info; + if (GetTargetAppInfo(i, info)) { - client->AddAppVendorId(vid); + client->AddTargetAppInfo(info); } } @@ -214,9 +218,10 @@ class DLL_EXPORT IdentificationDeclaration Encoding::BytesToUppercaseHexString(mRotatingId, mRotatingIdLen, rotatingIdString, sizeof(rotatingIdString)); ChipLogDetail(AppServer, "\trotating id: %s", rotatingIdString); } - for (uint8_t i = 0; i < mNumAppVendorIds; i++) + for (uint8_t i = 0; i < mNumTargetAppInfos; i++) { - ChipLogDetail(AppServer, "\tapp vendor id [%d]: %u", i, mAppVendorIds[i]); + ChipLogDetail(AppServer, "\tapp vendor id / product id [%d]: %u/%u", i, mTargetAppInfos[i].vendorId, + mTargetAppInfos[i].productId); } if (strlen(mPairingInst) != 0) { @@ -256,13 +261,16 @@ class DLL_EXPORT IdentificationDeclaration { kVendorIdTag = 1, kProductIdTag, - kNameTag, - kRotatingIdTag, - kCdPortTag, + kDeviceNameTag, + kDeviceTypeTag, kPairingInstTag, kPairingHintTag, - kAppVendorIdListTag, + kRotatingIdTag, + kCdPortTag, + kTargetAppListTag, + kTargetAppTag, kAppVendorIdTag, + kAppProductIdTag, kNoPasscodeTag, kCdUponPasscodeDialogTag, kCommissionerPasscodeTag, @@ -281,9 +289,9 @@ class DLL_EXPORT IdentificationDeclaration uint8_t mRotatingId[chip::Dnssd::kMaxRotatingIdLen]; size_t mRotatingIdLen = 0; - constexpr static size_t kMaxAppVendorIds = 10; - uint8_t mNumAppVendorIds = 0; // number of vendor Ids - uint16_t mAppVendorIds[kMaxAppVendorIds]; + constexpr static size_t kMaxTargetAppInfos = 10; + uint8_t mNumTargetAppInfos = 0; // number of vendor Ids + TargetAppInfo mTargetAppInfos[kMaxTargetAppInfos]; char mPairingInst[chip::Dnssd::kMaxPairingInstructionLen + 1] = {}; uint16_t mPairingHint = 0; diff --git a/src/protocols/user_directed_commissioning/UserDirectedCommissioningClient.cpp b/src/protocols/user_directed_commissioning/UserDirectedCommissioningClient.cpp index a3086826402e8c..1d93ae5bb1d75e 100644 --- a/src/protocols/user_directed_commissioning/UserDirectedCommissioningClient.cpp +++ b/src/protocols/user_directed_commissioning/UserDirectedCommissioningClient.cpp @@ -120,7 +120,8 @@ uint32_t IdentificationDeclaration::WritePayload(uint8_t * payloadBuffer, size_t VerifyOrExit(CHIP_NO_ERROR == (err = writer.Put(chip::TLV::ContextTag(kVendorIdTag), GetVendorId())), LogErrorOnFailure(err)); VerifyOrExit(CHIP_NO_ERROR == (err = writer.Put(chip::TLV::ContextTag(kProductIdTag), GetProductId())), LogErrorOnFailure(err)); - VerifyOrExit(CHIP_NO_ERROR == (err = writer.PutString(chip::TLV::ContextTag(kNameTag), mDeviceName)), LogErrorOnFailure(err)); + VerifyOrExit(CHIP_NO_ERROR == (err = writer.PutString(chip::TLV::ContextTag(kDeviceNameTag), mDeviceName)), + LogErrorOnFailure(err)); VerifyOrExit(CHIP_NO_ERROR == (err = writer.PutString(chip::TLV::ContextTag(kPairingInstTag), mPairingInst)), LogErrorOnFailure(err)); VerifyOrExit(CHIP_NO_ERROR == (err = writer.Put(chip::TLV::ContextTag(kPairingHintTag), mPairingHint)), LogErrorOnFailure(err)); @@ -134,12 +135,22 @@ uint32_t IdentificationDeclaration::WritePayload(uint8_t * payloadBuffer, size_t // AppVendorIdList VerifyOrExit( CHIP_NO_ERROR == - (err = writer.StartContainer(chip::TLV::ContextTag(kAppVendorIdListTag), chip::TLV::kTLVType_List, listContainerType)), + (err = writer.StartContainer(chip::TLV::ContextTag(kTargetAppListTag), chip::TLV::kTLVType_List, listContainerType)), LogErrorOnFailure(err)); - for (size_t i = 0; i < mNumAppVendorIds; i++) + for (size_t i = 0; i < mNumTargetAppInfos; i++) { - VerifyOrExit(CHIP_NO_ERROR == (err = writer.Put(chip::TLV::ContextTag(kAppVendorIdTag), mAppVendorIds[i])), + // start the TargetApp structure + VerifyOrExit(CHIP_NO_ERROR == + (err = writer.StartContainer(chip::TLV::ContextTag(kTargetAppTag), chip::TLV::kTLVType_Structure, + outerContainerType)), + LogErrorOnFailure(err)); + // add the vendor Id + VerifyOrExit(CHIP_NO_ERROR == (err = writer.Put(chip::TLV::ContextTag(kAppVendorIdTag), mTargetAppInfos[i].vendorId)), + LogErrorOnFailure(err)); + VerifyOrExit(CHIP_NO_ERROR == (err = writer.Put(chip::TLV::ContextTag(kAppProductIdTag), mTargetAppInfos[i].productId)), LogErrorOnFailure(err)); + // end the TargetApp structure + VerifyOrExit(CHIP_NO_ERROR == (err = writer.EndContainer(outerContainerType)), LogErrorOnFailure(err)); } VerifyOrExit(CHIP_NO_ERROR == (err = writer.EndContainer(listContainerType)), LogErrorOnFailure(err)); @@ -180,7 +191,12 @@ CHIP_ERROR CommissionerDeclaration::ReadPayload(uint8_t * udcPayload, size_t pay while ((err = reader.Next()) == CHIP_NO_ERROR) { chip::TLV::Tag containerTag = reader.GetTag(); - uint8_t tagNum = static_cast(chip::TLV::TagNumFromTag(containerTag)); + if (!TLV::IsContextTag(containerTag)) + { + ChipLogError(AppServer, "Unexpected non-context TLV tag."); + return CHIP_ERROR_INVALID_TLV_TAG; + } + uint8_t tagNum = static_cast(chip::TLV::TagNumFromTag(containerTag)); switch (tagNum) { diff --git a/src/protocols/user_directed_commissioning/UserDirectedCommissioningServer.cpp b/src/protocols/user_directed_commissioning/UserDirectedCommissioningServer.cpp index 266ca764c20ec9..1a2f125ae5d282 100644 --- a/src/protocols/user_directed_commissioning/UserDirectedCommissioningServer.cpp +++ b/src/protocols/user_directed_commissioning/UserDirectedCommissioningServer.cpp @@ -209,7 +209,12 @@ CHIP_ERROR IdentificationDeclaration::ReadPayload(uint8_t * udcPayload, size_t p while ((err = reader.Next()) == CHIP_NO_ERROR) { chip::TLV::Tag containerTag = reader.GetTag(); - uint8_t tagNum = static_cast(chip::TLV::TagNumFromTag(containerTag)); + if (!TLV::IsContextTag(containerTag)) + { + ChipLogError(AppServer, "Unexpected non-context TLV tag."); + return CHIP_ERROR_INVALID_TLV_TAG; + } + uint8_t tagNum = static_cast(chip::TLV::TagNumFromTag(containerTag)); switch (tagNum) { @@ -225,7 +230,7 @@ CHIP_ERROR IdentificationDeclaration::ReadPayload(uint8_t * udcPayload, size_t p // port err = reader.Get(mCdPort); break; - case kNameTag: + case kDeviceNameTag: // deviceName err = reader.GetString(mDeviceName, sizeof(mDeviceName)); break; @@ -242,25 +247,66 @@ CHIP_ERROR IdentificationDeclaration::ReadPayload(uint8_t * udcPayload, size_t p mRotatingIdLen = reader.GetLength(); err = reader.GetBytes(mRotatingId, sizeof(mRotatingId)); break; - case kAppVendorIdListTag: + case kTargetAppListTag: // app vendor list { + ChipLogProgress(AppServer, "TLV found an applist"); chip::TLV::TLVType listContainerType = chip::TLV::kTLVType_List; ReturnErrorOnFailure(reader.EnterContainer(listContainerType)); - while ((err = reader.Next()) == CHIP_NO_ERROR && mNumAppVendorIds < sizeof(mAppVendorIds)) + while ((err = reader.Next()) == CHIP_NO_ERROR && mNumTargetAppInfos < sizeof(mTargetAppInfos)) { containerTag = reader.GetTag(); - tagNum = static_cast(chip::TLV::TagNumFromTag(containerTag)); - if (tagNum == kAppVendorIdTag) + if (!TLV::IsContextTag(containerTag)) + { + ChipLogError(AppServer, "Unexpected non-context TLV tag."); + return CHIP_ERROR_INVALID_TLV_TAG; + } + tagNum = static_cast(chip::TLV::TagNumFromTag(containerTag)); + if (tagNum == kTargetAppTag) + { + ReturnErrorOnFailure(reader.EnterContainer(outerContainerType)); + uint16_t appVendorId = 0; + uint16_t appProductId = 0; + + while ((err = reader.Next()) == CHIP_NO_ERROR) + { + containerTag = reader.GetTag(); + if (!TLV::IsContextTag(containerTag)) + { + ChipLogError(AppServer, "Unexpected non-context TLV tag."); + return CHIP_ERROR_INVALID_TLV_TAG; + } + tagNum = static_cast(chip::TLV::TagNumFromTag(containerTag)); + if (tagNum == kAppVendorIdTag) + { + err = reader.Get(appVendorId); + } + else if (tagNum == kAppProductIdTag) + { + err = reader.Get(appProductId); + } + } + if (err == CHIP_END_OF_TLV) + { + ChipLogProgress(AppServer, "TLV end of struct TLV"); + ReturnErrorOnFailure(reader.ExitContainer(outerContainerType)); + } + if (appVendorId != 0) + { + mTargetAppInfos[mNumTargetAppInfos].vendorId = appVendorId; + mTargetAppInfos[mNumTargetAppInfos].productId = appProductId; + mNumTargetAppInfos++; + } + } + else { - err = reader.Get(mAppVendorIds[mNumAppVendorIds]); - mNumAppVendorIds++; + ChipLogError(AppServer, "unrecognized tag %d", tagNum); } } if (err == CHIP_END_OF_TLV) { - ChipLogError(AppServer, "TLV end of array TLV"); + ChipLogProgress(AppServer, "TLV end of array"); ReturnErrorOnFailure(reader.ExitContainer(listContainerType)); } } diff --git a/src/protocols/user_directed_commissioning/tests/TestUdcMessages.cpp b/src/protocols/user_directed_commissioning/tests/TestUdcMessages.cpp index eac29c24b4ade7..f7e7c2ab4a65d9 100644 --- a/src/protocols/user_directed_commissioning/tests/TestUdcMessages.cpp +++ b/src/protocols/user_directed_commissioning/tests/TestUdcMessages.cpp @@ -369,10 +369,11 @@ void TestUDCIdentificationDeclaration(nlTestSuite * inSuite, void * inContext) uint16_t productId = 2222; uint16_t port = 123; const char * deviceName = "device1"; - uint16_t vendorIdTemp = 0; uint16_t pairingHint = 33; const char * pairingInst = "Read 6 digit code from screen"; + TargetAppInfo appInfo; + // Rotating ID is given as up to 50 hex bytes char rotatingIdString[chip::Dnssd::kMaxRotatingIdLen * 2 + 1]; uint8_t rotatingId[chip::Dnssd::kMaxRotatingIdLen]; @@ -390,9 +391,19 @@ void TestUDCIdentificationDeclaration(nlTestSuite * inSuite, void * inContext) id.SetCdPort(port); id.SetNoPasscode(true); - id.AddAppVendorId(1); - id.AddAppVendorId(2); - id.AddAppVendorId(3); + + appInfo.vendorId = 1; + appInfo.productId = 9; + id.AddTargetAppInfo(appInfo); + + appInfo.vendorId = 2; + appInfo.productId = 8; + id.AddTargetAppInfo(appInfo); + + appInfo.vendorId = 3; + appInfo.productId = 7; + id.AddTargetAppInfo(appInfo); + id.SetCdUponPasscodeDialog(true); id.SetCommissionerPasscode(true); id.SetCommissionerPasscodeReady(true); @@ -408,10 +419,10 @@ void TestUDCIdentificationDeclaration(nlTestSuite * inSuite, void * inContext) NL_TEST_ASSERT(inSuite, pairingHint == id.GetPairingHint()); NL_TEST_ASSERT(inSuite, strcmp(id.GetPairingInst(), pairingInst) == 0); - NL_TEST_ASSERT(inSuite, id.GetNumAppVendorIds() == 3); - NL_TEST_ASSERT(inSuite, id.GetAppVendorId(0, vendorIdTemp) && vendorIdTemp == 1); - NL_TEST_ASSERT(inSuite, id.GetAppVendorId(1, vendorIdTemp) && vendorIdTemp == 2); - NL_TEST_ASSERT(inSuite, id.GetAppVendorId(2, vendorIdTemp) && vendorIdTemp == 3); + NL_TEST_ASSERT(inSuite, id.GetNumTargetAppInfos() == 3); + NL_TEST_ASSERT(inSuite, id.GetTargetAppInfo(0, appInfo) && appInfo.vendorId == 1 && appInfo.productId == 9); + NL_TEST_ASSERT(inSuite, id.GetTargetAppInfo(1, appInfo) && appInfo.vendorId == 2 && appInfo.productId == 8); + NL_TEST_ASSERT(inSuite, id.GetTargetAppInfo(2, appInfo) && appInfo.vendorId == 3 && appInfo.productId == 7); NL_TEST_ASSERT(inSuite, id.GetNoPasscode() == true); NL_TEST_ASSERT(inSuite, id.GetCdUponPasscodeDialog() == true); NL_TEST_ASSERT(inSuite, id.GetCommissionerPasscode() == true); @@ -436,10 +447,10 @@ void TestUDCIdentificationDeclaration(nlTestSuite * inSuite, void * inContext) NL_TEST_ASSERT(inSuite, strcmp(idOut.GetPairingInst(), pairingInst) == 0); NL_TEST_ASSERT(inSuite, pairingHint == idOut.GetPairingHint()); - NL_TEST_ASSERT(inSuite, id.GetNumAppVendorIds() == idOut.GetNumAppVendorIds()); - NL_TEST_ASSERT(inSuite, idOut.GetAppVendorId(0, vendorIdTemp) && vendorIdTemp == 1); - NL_TEST_ASSERT(inSuite, idOut.GetAppVendorId(1, vendorIdTemp) && vendorIdTemp == 2); - NL_TEST_ASSERT(inSuite, idOut.GetAppVendorId(2, vendorIdTemp) && vendorIdTemp == 3); + NL_TEST_ASSERT(inSuite, id.GetNumTargetAppInfos() == idOut.GetNumTargetAppInfos()); + NL_TEST_ASSERT(inSuite, idOut.GetTargetAppInfo(0, appInfo) && appInfo.vendorId == 1 && appInfo.productId == 9); + NL_TEST_ASSERT(inSuite, idOut.GetTargetAppInfo(1, appInfo) && appInfo.vendorId == 2 && appInfo.productId == 8); + NL_TEST_ASSERT(inSuite, idOut.GetTargetAppInfo(2, appInfo) && appInfo.vendorId == 3 && appInfo.productId == 7); NL_TEST_ASSERT(inSuite, id.GetNoPasscode() == idOut.GetNoPasscode()); NL_TEST_ASSERT(inSuite, id.GetCdUponPasscodeDialog() == idOut.GetCdUponPasscodeDialog());