From b89c8f8de63a71c9d80dde39fa62bcd5b01718a6 Mon Sep 17 00:00:00 2001 From: Junior Martinez <67972863+jmartinez-silabs@users.noreply.github.com> Date: Thu, 1 Jun 2023 09:46:01 -0400 Subject: [PATCH] =?UTF-8?q?Fix=20Thread=20diagnostic=20nullable=20attribut?= =?UTF-8?q?es=20so=20they=20read=20as=20null=20when=20=E2=80=A6=20(#26960)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Fix Thread diagnostic nullable attributes so they read as null when the device is not associated to a thread network. Add a encoding method to encode directly encode null when a provide condition evaluate to false * Revert "Fix Thread diagnostic nullable attributes so they read as null when the device is not associated to a thread network. Add a encoding method to encode directly encode null when a provide condition evaluate to false" This reverts commit 0a406ac3bcf603d776935de8fca2145425d4beb5. * Simplify how the nullEncode cases are caught. The condition is based on dataset being commissioned rather than the attachment state as valuable data can be fetched with the dataset even if thread is detached or disabled * update comment * Apply suggestions from code review Co-authored-by: Boris Zbarsky --------- Co-authored-by: Boris Zbarsky --- ...nericThreadStackManagerImpl_OpenThread.hpp | 188 +++++++++--------- 1 file changed, 93 insertions(+), 95 deletions(-) diff --git a/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.hpp b/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.hpp index b502a88b2f0d0a..001fd0a883c74e 100644 --- a/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.hpp +++ b/src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.hpp @@ -1052,6 +1052,35 @@ CHIP_ERROR GenericThreadStackManagerImpl_OpenThread::_WriteThreadNetw namespace ThreadNetworkDiagnostics = app::Clusters::ThreadNetworkDiagnostics; + if (!otDatasetIsCommissioned(mOTInst)) + { + // For the following nullable attributes of the cluster, encodeNull since + // thread instance cannot provide the related data when it is not currently configured. + // + // Note that RoutingRole is nullable but not listed here as thread provides + // valid data even when disabled or detached + switch (attributeId) + { + case ThreadNetworkDiagnostics::Attributes::Channel::Id: + case ThreadNetworkDiagnostics::Attributes::NetworkName::Id: + case ThreadNetworkDiagnostics::Attributes::PanId::Id: + case ThreadNetworkDiagnostics::Attributes::ExtendedPanId::Id: + case ThreadNetworkDiagnostics::Attributes::MeshLocalPrefix::Id: + case ThreadNetworkDiagnostics::Attributes::PartitionId::Id: + case ThreadNetworkDiagnostics::Attributes::Weighting::Id: + case ThreadNetworkDiagnostics::Attributes::DataVersion::Id: + case ThreadNetworkDiagnostics::Attributes::StableDataVersion::Id: + case ThreadNetworkDiagnostics::Attributes::LeaderRouterId::Id: + case ThreadNetworkDiagnostics::Attributes::ActiveTimestamp::Id: + case ThreadNetworkDiagnostics::Attributes::PendingTimestamp::Id: + case ThreadNetworkDiagnostics::Attributes::Delay::Id: + case ThreadNetworkDiagnostics::Attributes::SecurityPolicy::Id: + case ThreadNetworkDiagnostics::Attributes::ChannelPage0Mask::Id: + case ThreadNetworkDiagnostics::Attributes::OperationalDatasetComponents::Id: + return encoder.EncodeNull(); + } + } + switch (attributeId) { case ThreadNetworkDiagnostics::Attributes::Channel::Id: { @@ -1537,125 +1566,99 @@ CHIP_ERROR GenericThreadStackManagerImpl_OpenThread::_WriteThreadNetw break; case ThreadNetworkDiagnostics::Attributes::ActiveTimestamp::Id: { - err = CHIP_ERROR_INCORRECT_STATE; - if (otDatasetIsCommissioned(mOTInst)) - { - otOperationalDataset activeDataset; - otError otErr = otDatasetGetActive(mOTInst, &activeDataset); - VerifyOrExit(otErr == OT_ERROR_NONE, err = MapOpenThreadError(otErr)); + otOperationalDataset activeDataset; + otError otErr = otDatasetGetActive(mOTInst, &activeDataset); + VerifyOrReturnError(otErr == OT_ERROR_NONE, MapOpenThreadError(otErr)); #if OPENTHREAD_API_VERSION >= 219 - uint64_t activeTimestamp = (activeDataset.mActiveTimestamp.mSeconds << 16) | - (activeDataset.mActiveTimestamp.mTicks << 1) | activeDataset.mActiveTimestamp.mAuthoritative; + uint64_t activeTimestamp = (activeDataset.mActiveTimestamp.mSeconds << 16) | (activeDataset.mActiveTimestamp.mTicks << 1) | + activeDataset.mActiveTimestamp.mAuthoritative; #else - uint64_t activeTimestamp = activeDataset.mActiveTimestamp; + uint64_t activeTimestamp = activeDataset.mActiveTimestamp; #endif - err = encoder.Encode(activeTimestamp); - } + err = encoder.Encode(activeTimestamp); } break; case ThreadNetworkDiagnostics::Attributes::PendingTimestamp::Id: { - err = CHIP_ERROR_INCORRECT_STATE; - if (otDatasetIsCommissioned(mOTInst)) - { - otOperationalDataset activeDataset; - otError otErr = otDatasetGetActive(mOTInst, &activeDataset); - VerifyOrExit(otErr == OT_ERROR_NONE, err = MapOpenThreadError(otErr)); + otOperationalDataset activeDataset; + otError otErr = otDatasetGetActive(mOTInst, &activeDataset); + VerifyOrReturnError(otErr == OT_ERROR_NONE, MapOpenThreadError(otErr)); #if OPENTHREAD_API_VERSION >= 219 - uint64_t pendingTimestamp = (activeDataset.mPendingTimestamp.mSeconds << 16) | - (activeDataset.mPendingTimestamp.mTicks << 1) | activeDataset.mPendingTimestamp.mAuthoritative; + uint64_t pendingTimestamp = (activeDataset.mPendingTimestamp.mSeconds << 16) | + (activeDataset.mPendingTimestamp.mTicks << 1) | activeDataset.mPendingTimestamp.mAuthoritative; #else - uint64_t pendingTimestamp = activeDataset.mPendingTimestamp; + uint64_t pendingTimestamp = activeDataset.mPendingTimestamp; #endif - err = encoder.Encode(pendingTimestamp); - } + err = encoder.Encode(pendingTimestamp); } break; case ThreadNetworkDiagnostics::Attributes::Delay::Id: { - err = CHIP_ERROR_INCORRECT_STATE; - if (otDatasetIsCommissioned(mOTInst)) - { - otOperationalDataset activeDataset; - otError otErr = otDatasetGetActive(mOTInst, &activeDataset); - VerifyOrExit(otErr == OT_ERROR_NONE, err = MapOpenThreadError(otErr)); - uint32_t delay = activeDataset.mDelay; - err = encoder.Encode(delay); - } + otOperationalDataset activeDataset; + otError otErr = otDatasetGetActive(mOTInst, &activeDataset); + VerifyOrReturnError(otErr == OT_ERROR_NONE, MapOpenThreadError(otErr)); + uint32_t delay = activeDataset.mDelay; + err = encoder.Encode(delay); } break; case ThreadNetworkDiagnostics::Attributes::SecurityPolicy::Id: { - err = CHIP_ERROR_INCORRECT_STATE; - - if (otDatasetIsCommissioned(mOTInst)) - { - otOperationalDataset activeDataset; - otError otErr = otDatasetGetActive(mOTInst, &activeDataset); - VerifyOrExit(otErr == OT_ERROR_NONE, err = MapOpenThreadError(otErr)); + otOperationalDataset activeDataset; + otError otErr = otDatasetGetActive(mOTInst, &activeDataset); + VerifyOrReturnError(otErr == OT_ERROR_NONE, MapOpenThreadError(otErr)); - ThreadNetworkDiagnostics::Structs::SecurityPolicy::Type securityPolicy; - static_assert(sizeof(securityPolicy) == sizeof(activeDataset.mSecurityPolicy), - "securityPolicy Struct do not match otSecurityPolicy"); - uint16_t policyAsInts[2]; - static_assert(sizeof(policyAsInts) == sizeof(activeDataset.mSecurityPolicy), - "We're missing some members of otSecurityPolicy?"); - memcpy(&policyAsInts, &activeDataset.mSecurityPolicy, sizeof(policyAsInts)); - securityPolicy.rotationTime = policyAsInts[0]; - securityPolicy.flags = policyAsInts[1]; - - err = encoder.Encode(securityPolicy); - } + ThreadNetworkDiagnostics::Structs::SecurityPolicy::Type securityPolicy; + static_assert(sizeof(securityPolicy) == sizeof(activeDataset.mSecurityPolicy), + "securityPolicy Struct do not match otSecurityPolicy"); + uint16_t policyAsInts[2]; + static_assert(sizeof(policyAsInts) == sizeof(activeDataset.mSecurityPolicy), + "We're missing some members of otSecurityPolicy?"); + memcpy(&policyAsInts, &activeDataset.mSecurityPolicy, sizeof(policyAsInts)); + securityPolicy.rotationTime = policyAsInts[0]; + securityPolicy.flags = policyAsInts[1]; + err = encoder.Encode(securityPolicy); } break; case ThreadNetworkDiagnostics::Attributes::ChannelPage0Mask::Id: { - err = CHIP_ERROR_INCORRECT_STATE; - if (otDatasetIsCommissioned(mOTInst)) + otOperationalDataset activeDataset; + otError otErr = otDatasetGetActive(mOTInst, &activeDataset); + VerifyOrReturnError(otErr == OT_ERROR_NONE, MapOpenThreadError(otErr)); + + // In the resultant Octet string, the most significant bit of the left-most byte indicates channel 0 + // We have to bitswap the entire uint32_t before converting to octet string + uint32_t bitSwappedChannelMask = 0; + for (int i = 0, j = 31; i < 32; i++, j--) { - otOperationalDataset activeDataset; - otError otErr = otDatasetGetActive(mOTInst, &activeDataset); - VerifyOrExit(otErr == OT_ERROR_NONE, err = MapOpenThreadError(otErr)); - - // In the resultant Octet string, the most significant bit of the left-most byte indicates channel 0 - // We have to bitswap the entire uint32_t before converting to octet string - uint32_t bitSwappedChannelMask = 0; - for (int i = 0, j = 31; i < 32; i++, j--) - { - bitSwappedChannelMask |= ((activeDataset.mChannelMask >> j) & 1) << i; - } - - uint8_t buffer[sizeof(uint32_t)] = { 0 }; - Encoding::BigEndian::Put32(buffer, bitSwappedChannelMask); - err = encoder.Encode(ByteSpan(buffer)); + bitSwappedChannelMask |= ((activeDataset.mChannelMask >> j) & 1) << i; } + + uint8_t buffer[sizeof(uint32_t)] = { 0 }; + Encoding::BigEndian::Put32(buffer, bitSwappedChannelMask); + err = encoder.Encode(ByteSpan(buffer)); } break; case ThreadNetworkDiagnostics::Attributes::OperationalDatasetComponents::Id: { - err = CHIP_ERROR_INCORRECT_STATE; - if (otDatasetIsCommissioned(mOTInst)) - { - otOperationalDataset activeDataset; - otError otErr = otDatasetGetActive(mOTInst, &activeDataset); - VerifyOrExit(otErr == OT_ERROR_NONE, err = MapOpenThreadError(otErr)); - ThreadNetworkDiagnostics::Structs::OperationalDatasetComponents::Type OpDatasetComponents; - - OpDatasetComponents.activeTimestampPresent = activeDataset.mComponents.mIsActiveTimestampPresent; - OpDatasetComponents.pendingTimestampPresent = activeDataset.mComponents.mIsPendingTimestampPresent; - OpDatasetComponents.masterKeyPresent = activeDataset.mComponents.mIsNetworkKeyPresent; - OpDatasetComponents.networkNamePresent = activeDataset.mComponents.mIsNetworkNamePresent; - OpDatasetComponents.extendedPanIdPresent = activeDataset.mComponents.mIsExtendedPanIdPresent; - OpDatasetComponents.meshLocalPrefixPresent = activeDataset.mComponents.mIsMeshLocalPrefixPresent; - OpDatasetComponents.delayPresent = activeDataset.mComponents.mIsDelayPresent; - OpDatasetComponents.panIdPresent = activeDataset.mComponents.mIsPanIdPresent; - OpDatasetComponents.channelPresent = activeDataset.mComponents.mIsChannelPresent; - OpDatasetComponents.pskcPresent = activeDataset.mComponents.mIsPskcPresent; - OpDatasetComponents.securityPolicyPresent = activeDataset.mComponents.mIsSecurityPolicyPresent; - OpDatasetComponents.channelMaskPresent = activeDataset.mComponents.mIsChannelMaskPresent; - - err = encoder.Encode(OpDatasetComponents); - } + otOperationalDataset activeDataset; + otError otErr = otDatasetGetActive(mOTInst, &activeDataset); + VerifyOrReturnError(otErr == OT_ERROR_NONE, MapOpenThreadError(otErr)); + ThreadNetworkDiagnostics::Structs::OperationalDatasetComponents::Type OpDatasetComponents; + + OpDatasetComponents.activeTimestampPresent = activeDataset.mComponents.mIsActiveTimestampPresent; + OpDatasetComponents.pendingTimestampPresent = activeDataset.mComponents.mIsPendingTimestampPresent; + OpDatasetComponents.masterKeyPresent = activeDataset.mComponents.mIsNetworkKeyPresent; + OpDatasetComponents.networkNamePresent = activeDataset.mComponents.mIsNetworkNamePresent; + OpDatasetComponents.extendedPanIdPresent = activeDataset.mComponents.mIsExtendedPanIdPresent; + OpDatasetComponents.meshLocalPrefixPresent = activeDataset.mComponents.mIsMeshLocalPrefixPresent; + OpDatasetComponents.delayPresent = activeDataset.mComponents.mIsDelayPresent; + OpDatasetComponents.panIdPresent = activeDataset.mComponents.mIsPanIdPresent; + OpDatasetComponents.channelPresent = activeDataset.mComponents.mIsChannelPresent; + OpDatasetComponents.pskcPresent = activeDataset.mComponents.mIsPskcPresent; + OpDatasetComponents.securityPolicyPresent = activeDataset.mComponents.mIsSecurityPolicyPresent; + OpDatasetComponents.channelMaskPresent = activeDataset.mComponents.mIsChannelMaskPresent; + + err = encoder.Encode(OpDatasetComponents); } break; @@ -1680,11 +1683,6 @@ CHIP_ERROR GenericThreadStackManagerImpl_OpenThread::_WriteThreadNetw break; } -exit: - if (err != CHIP_NO_ERROR) - { - ChipLogError(DeviceLayer, "_WriteThreadNetworkDiagnosticAttributeToTlv failed: %s", ErrorStr(err)); - } return err; }