From ac1536dc580e55d277d940ddb87b27d1f16f3ad5 Mon Sep 17 00:00:00 2001 From: C Freeman Date: Fri, 24 Nov 2023 10:09:22 -0500 Subject: [PATCH 1/3] Data model XML: Add ICD, regen from ToT (#30638) --- data_model/clusters/ContentControl.xml | 88 ++++----- data_model/clusters/ICDManagement.xml | 180 +++++++++++++++++++ data_model/clusters/KeypadInput.xml | 4 +- data_model/clusters/MediaPlayback.xml | 14 +- data_model/clusters/MicrowaveOvenControl.xml | 2 +- data_model/clusters/PowerSourceCluster.xml | 16 +- data_model/clusters/TimeSync.xml | 2 +- data_model/spec_sha | 2 +- scripts/spec_xml/generate_spec_xml.py | 3 +- 9 files changed, 254 insertions(+), 57 deletions(-) create mode 100644 data_model/clusters/ICDManagement.xml diff --git a/data_model/clusters/ContentControl.xml b/data_model/clusters/ContentControl.xml index 3642777b46a76d..ff38faf1f10909 100644 --- a/data_model/clusters/ContentControl.xml +++ b/data_model/clusters/ContentControl.xml @@ -79,12 +79,12 @@ Davis, CA 95616, USA - + - + @@ -92,60 +92,66 @@ Davis, CA 95616, USA - + + - + + - - + + - + + - - + + - + + - + + - + + - + - + @@ -159,13 +165,13 @@ Davis, CA 95616, USA - + - + @@ -173,20 +179,12 @@ Davis, CA 95616, USA - + - - - - - + - - - - @@ -194,7 +192,7 @@ Davis, CA 95616, USA - + @@ -202,36 +200,44 @@ Davis, CA 95616, USA - + - - - - - + - + - - - - - + - + + + + + + + - + + + + + + + + + + + diff --git a/data_model/clusters/ICDManagement.xml b/data_model/clusters/ICDManagement.xml new file mode 100644 index 00000000000000..711b98b49b104c --- /dev/null +++ b/data_model/clusters/ICDManagement.xml @@ -0,0 +1,180 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + \ No newline at end of file diff --git a/data_model/clusters/KeypadInput.xml b/data_model/clusters/KeypadInput.xml index 30936891c6c71d..7179ae676b65fe 100644 --- a/data_model/clusters/KeypadInput.xml +++ b/data_model/clusters/KeypadInput.xml @@ -72,7 +72,7 @@ Davis, CA 95616, USA - + @@ -348,7 +348,7 @@ Davis, CA 95616, USA - + diff --git a/data_model/clusters/MediaPlayback.xml b/data_model/clusters/MediaPlayback.xml index 21001d287f6120..c53dc862808c12 100644 --- a/data_model/clusters/MediaPlayback.xml +++ b/data_model/clusters/MediaPlayback.xml @@ -61,10 +61,10 @@ Davis, CA 95616, USA - + - + @@ -203,12 +203,22 @@ Davis, CA 95616, USA + + + + + + + + + + diff --git a/data_model/clusters/MicrowaveOvenControl.xml b/data_model/clusters/MicrowaveOvenControl.xml index e97821db771a77..3f127cebd90367 100644 --- a/data_model/clusters/MicrowaveOvenControl.xml +++ b/data_model/clusters/MicrowaveOvenControl.xml @@ -55,7 +55,7 @@ Connectivity Standards Alliance 508 Second Street, Suite 206 Davis, CA 95616, USA --> - + diff --git a/data_model/clusters/PowerSourceCluster.xml b/data_model/clusters/PowerSourceCluster.xml index f7b5da82ce20fc..27caade3c634a3 100644 --- a/data_model/clusters/PowerSourceCluster.xml +++ b/data_model/clusters/PowerSourceCluster.xml @@ -69,24 +69,24 @@ Davis, CA 95616, USA - + - + - + - + - + - + - + - + diff --git a/data_model/clusters/TimeSync.xml b/data_model/clusters/TimeSync.xml index c7ad7b0032aaf7..4515d44012085e 100644 --- a/data_model/clusters/TimeSync.xml +++ b/data_model/clusters/TimeSync.xml @@ -57,7 +57,7 @@ Connectivity Standards Alliance 508 Second Street, Suite 206 Davis, CA 95616, USA --> - + diff --git a/data_model/spec_sha b/data_model/spec_sha index 03c3b85baa2427..d0a25923c7c7b5 100644 --- a/data_model/spec_sha +++ b/data_model/spec_sha @@ -1 +1 @@ -49003c1b2337aa51dad227977981b763667d1f75 +8e4f91da4aacda4e799b9979605342a315ac7e43 diff --git a/scripts/spec_xml/generate_spec_xml.py b/scripts/spec_xml/generate_spec_xml.py index d0b2635bcf3398..ab954f17a14fb7 100755 --- a/scripts/spec_xml/generate_spec_xml.py +++ b/scripts/spec_xml/generate_spec_xml.py @@ -66,7 +66,8 @@ def scrape_clusters(scraper, spec_root, output_dir, dry_run): media_clusters_dir = os.path.abspath(os.path.join(app_clusters_dir, 'media')) clusters_output_dir = os.path.abspath(os.path.join(output_dir, 'clusters')) dm_clusters_list = ['ACL-Cluster.adoc', 'Binding-Cluster.adoc', 'bridge-clusters.adoc', - 'Descriptor-Cluster.adoc', 'Group-Key-Management-Cluster.adoc', 'Label-Cluster.adoc'] + 'Descriptor-Cluster.adoc', 'Group-Key-Management-Cluster.adoc', 'ICDManagement.adoc', + 'Label-Cluster.adoc'] sdm_exclude_list = ['AdminAssistedCommissioningFlows.adoc', 'BulkDataExchange.adoc', 'CommissioningFlows.adoc', 'DeviceCommissioningFlows.adoc', 'DistributedComplianceLedger.adoc', 'OTAFileFormat.adoc'] app_exclude_list = ['appliances.adoc', 'closures.adoc', 'general.adoc', From b5dfe72508a27018af00520ac895f8e24c002385 Mon Sep 17 00:00:00 2001 From: C Freeman Date: Fri, 24 Nov 2023 10:50:23 -0500 Subject: [PATCH 2/3] RVC: Fix conformance (#30600) - admin commissioning doesn't have the BCW feature, but had the command on - turned off command - Identify cluster revision is now 4. It already included the v4 attribute, it was just the revision that was incorrect. --- examples/rvc-app/rvc-common/rvc-app.matter | 8 +------- examples/rvc-app/rvc-common/rvc-app.zap | 10 +--------- 2 files changed, 2 insertions(+), 16 deletions(-) diff --git a/examples/rvc-app/rvc-common/rvc-app.matter b/examples/rvc-app/rvc-common/rvc-app.matter index 03773df659e16b..2c565deef1c480 100644 --- a/examples/rvc-app/rvc-common/rvc-app.matter +++ b/examples/rvc-app/rvc-common/rvc-app.matter @@ -664,12 +664,7 @@ server cluster AdministratorCommissioning = 60 { octet_string<32> salt = 4; } - request struct OpenBasicCommissioningWindowRequest { - int16u commissioningTimeout = 0; - } - timed command access(invoke: administer) OpenCommissioningWindow(OpenCommissioningWindowRequest): DefaultSuccess = 0; - timed command access(invoke: administer) OpenBasicCommissioningWindow(OpenBasicCommissioningWindowRequest): DefaultSuccess = 1; timed command access(invoke: administer) RevokeCommissioning(): DefaultSuccess = 2; } @@ -1162,7 +1157,6 @@ endpoint 0 { ram attribute clusterRevision default = 0x0001; handle command OpenCommissioningWindow; - handle command OpenBasicCommissioningWindow; handle command RevokeCommissioning; } @@ -1218,7 +1212,7 @@ endpoint 1 { callback attribute eventList; callback attribute attributeList default = 0; ram attribute featureMap default = 0; - ram attribute clusterRevision default = 2; + ram attribute clusterRevision default = 4; handle command Identify; handle command TriggerEffect; diff --git a/examples/rvc-app/rvc-common/rvc-app.zap b/examples/rvc-app/rvc-common/rvc-app.zap index a8453c00f44c68..e81e19248c72bb 100644 --- a/examples/rvc-app/rvc-common/rvc-app.zap +++ b/examples/rvc-app/rvc-common/rvc-app.zap @@ -1426,14 +1426,6 @@ "isIncoming": 1, "isEnabled": 1 }, - { - "name": "OpenBasicCommissioningWindow", - "code": 1, - "mfgCode": null, - "source": "client", - "isIncoming": 1, - "isEnabled": 1 - }, { "name": "RevokeCommissioning", "code": 2, @@ -2095,7 +2087,7 @@ "storageOption": "RAM", "singleton": 0, "bounded": 0, - "defaultValue": "2", + "defaultValue": "4", "reportable": 1, "minInterval": 1, "maxInterval": 65534, From d82ce5744c7b24af0718736ff3d6313823121bfe Mon Sep 17 00:00:00 2001 From: Pradip De Date: Fri, 24 Nov 2023 12:22:18 -0800 Subject: [PATCH 3/3] Ensure that MRP logic is not executed for messages over other Transports (#30124) * Ensure that MRP logic is not executed for messages over other Transports MRP based Ack handling and retransmissions should not be performed when messages are not sent using MRP, e.g., over TCP. Install guards on Send/Receive paths to steer traffic to bypass MRP logic when going over TCP. * Rename IsTransportTCP() to AllowsLargePayload() --- src/messaging/ExchangeContext.cpp | 84 +++++++++++---------- src/messaging/ExchangeMessageDispatch.cpp | 78 +++++++++++-------- src/messaging/ExchangeMessageDispatch.h | 4 + src/messaging/ExchangeMgr.cpp | 6 +- src/transport/GroupSession.h | 6 +- src/transport/SecureSession.h | 4 +- src/transport/Session.h | 3 +- src/transport/UnauthenticatedSessionTable.h | 4 +- 8 files changed, 112 insertions(+), 77 deletions(-) diff --git a/src/messaging/ExchangeContext.cpp b/src/messaging/ExchangeContext.cpp index c9d912daf88091..31d579e660502c 100644 --- a/src/messaging/ExchangeContext.cpp +++ b/src/messaging/ExchangeContext.cpp @@ -113,7 +113,7 @@ CHIP_ERROR ExchangeContext::SendMessage(Protocols::Id protocolId, uint8_t msgTyp // If session requires MRP, NoAutoRequestAck send flag is not specified and is not a group exchange context, request reliable // transmission. bool reliableTransmissionRequested = - GetSessionHandle()->RequireMRP() && !sendFlags.Has(SendMessageFlags::kNoAutoRequestAck) && !IsGroupExchangeContext(); + GetSessionHandle()->AllowsMRP() && !sendFlags.Has(SendMessageFlags::kNoAutoRequestAck) && !IsGroupExchangeContext(); bool currentMessageExpectResponse = false; // If a response message is expected... @@ -322,8 +322,8 @@ ExchangeContext::ExchangeContext(ExchangeManager * em, uint16_t ExchangeId, cons SetAckPending(false); - // Do not request Ack for multicast - SetAutoRequestAck(!session->IsGroupSession()); + // Try to use MRP by default, if it is allowed. + SetAutoRequestAck(session->AllowsMRP()); #if CHIP_CONFIG_ENABLE_ICD_SERVER app::ICDNotifier::GetInstance().BroadcastActiveRequestNotification(app::ICDListener::KeepActiveFlag::kExchangeContextOpen); @@ -531,34 +531,37 @@ CHIP_ERROR ExchangeContext::HandleMessage(uint32_t messageCounter, const Payload MessageHandled(); }); - if (mDispatch.IsReliableTransmissionAllowed() && !IsGroupExchangeContext()) + if (mSession->AllowsMRP()) { - if (!msgFlags.Has(MessageFlagValues::kDuplicateMessage) && payloadHeader.IsAckMsg() && - payloadHeader.GetAckMessageCounter().HasValue()) + if (mDispatch.IsReliableTransmissionAllowed()) { - HandleRcvdAck(payloadHeader.GetAckMessageCounter().Value()); + if (!msgFlags.Has(MessageFlagValues::kDuplicateMessage) && payloadHeader.IsAckMsg() && + payloadHeader.GetAckMessageCounter().HasValue()) + { + HandleRcvdAck(payloadHeader.GetAckMessageCounter().Value()); + } + + if (payloadHeader.NeedsAck()) + { + // An acknowledgment needs to be sent back to the peer for this message on this exchange, + HandleNeedsAck(messageCounter, msgFlags); + } } - if (payloadHeader.NeedsAck()) + if (IsAckPending() && !mDelegate) { - // An acknowledgment needs to be sent back to the peer for this message on this exchange, - HandleNeedsAck(messageCounter, msgFlags); + // The incoming message wants an ack, but we have no delegate, so + // there's not going to be a response to piggyback on. Just flush the + // ack out right now. + ReturnErrorOnFailure(FlushAcks()); } - } - if (IsAckPending() && !mDelegate) - { - // The incoming message wants an ack, but we have no delegate, so - // there's not going to be a response to piggyback on. Just flush the - // ack out right now. - ReturnErrorOnFailure(FlushAcks()); - } - - // The SecureChannel::StandaloneAck message type is only used for MRP; do not pass such messages to the application layer. - if (isStandaloneAck) - { - return CHIP_NO_ERROR; - } + // The SecureChannel::StandaloneAck message type is only used for MRP; do not pass such messages to the application layer. + if (isStandaloneAck) + { + return CHIP_NO_ERROR; + } + } // AllowsMRP // Since the message is duplicate, let's not forward it up the stack if (isDuplicate) @@ -566,23 +569,26 @@ CHIP_ERROR ExchangeContext::HandleMessage(uint32_t messageCounter, const Payload return CHIP_NO_ERROR; } - if (IsEphemeralExchange()) + if (mSession->AllowsMRP()) { - // The EphemeralExchange has done its job, since StandaloneAck is sent in previous FlushAcks() call. - return CHIP_NO_ERROR; - } + if (IsEphemeralExchange()) + { + // The EphemeralExchange has done its job, since StandaloneAck is sent in previous FlushAcks() call. + return CHIP_NO_ERROR; + } - if (IsWaitingForAck()) - { - // The only way we can get here is a spec violation on the other side: - // we sent a message that needs an ack, and the other side responded - // with a message that does not contain an ack for the message we sent. - // Just drop this message; if we delivered it to our delegate it might - // try to send another message-needing-an-ack in response, which would - // violate our internal invariants. - ChipLogError(ExchangeManager, "Dropping message without piggyback ack when we are waiting for an ack."); - return CHIP_ERROR_INCORRECT_STATE; - } + if (IsWaitingForAck()) + { + // The only way we can get here is a spec violation on the other side: + // we sent a message that needs an ack, and the other side responded + // with a message that does not contain an ack for the message we sent. + // Just drop this message; if we delivered it to our delegate it might + // try to send another message-needing-an-ack in response, which would + // violate our internal invariants. + ChipLogError(ExchangeManager, "Dropping message without piggyback ack when we are waiting for an ack."); + return CHIP_ERROR_INCORRECT_STATE; + } + } // AllowsMRP #if CHIP_CONFIG_ENABLE_ICD_SERVER // message received diff --git a/src/messaging/ExchangeMessageDispatch.cpp b/src/messaging/ExchangeMessageDispatch.cpp index 1ffbfe3657c6cd..950be48ae5802a 100644 --- a/src/messaging/ExchangeMessageDispatch.cpp +++ b/src/messaging/ExchangeMessageDispatch.cpp @@ -51,45 +51,61 @@ CHIP_ERROR ExchangeMessageDispatch::SendMessage(SessionManager * sessionManager, PayloadHeader payloadHeader; payloadHeader.SetExchangeID(exchangeId).SetMessageType(protocol, type).SetInitiator(isInitiator); - // If there is a pending acknowledgment piggyback it on this message. - if (reliableMessageContext->HasPiggybackAckPending()) + if (session->AllowsMRP()) { - payloadHeader.SetAckMessageCounter(reliableMessageContext->TakePendingPeerAckMessageCounter()); - } - - if (IsReliableTransmissionAllowed() && reliableMessageContext->AutoRequestAck() && - reliableMessageContext->GetReliableMessageMgr() != nullptr && isReliableTransmission) - { - auto * reliableMessageMgr = reliableMessageContext->GetReliableMessageMgr(); - - payloadHeader.SetNeedsAck(true); - - ReliableMessageMgr::RetransTableEntry * entry = nullptr; - - // Add to Table for subsequent sending - ReturnErrorOnFailure(reliableMessageMgr->AddToRetransTable(reliableMessageContext, &entry)); - auto deleter = [reliableMessageMgr](ReliableMessageMgr::RetransTableEntry * e) { - reliableMessageMgr->ClearRetransTable(*e); - }; - std::unique_ptr entryOwner(entry, deleter); - - ReturnErrorOnFailure(sessionManager->PrepareMessage(session, payloadHeader, std::move(message), entryOwner->retainedBuf)); - CHIP_ERROR err = sessionManager->SendPreparedMessage(session, entryOwner->retainedBuf); - err = ReliableMessageMgr::MapSendError(err, exchangeId, isInitiator); - ReturnErrorOnFailure(err); - reliableMessageMgr->StartRetransmision(entryOwner.release()); + // If there is a pending acknowledgment piggyback it on this message. + if (reliableMessageContext->HasPiggybackAckPending()) + { + payloadHeader.SetAckMessageCounter(reliableMessageContext->TakePendingPeerAckMessageCounter()); + } + + if (IsReliableTransmissionAllowed() && reliableMessageContext->AutoRequestAck() && + reliableMessageContext->GetReliableMessageMgr() != nullptr && isReliableTransmission) + { + auto * reliableMessageMgr = reliableMessageContext->GetReliableMessageMgr(); + + payloadHeader.SetNeedsAck(true); + + ReliableMessageMgr::RetransTableEntry * entry = nullptr; + + // Add to Table for subsequent sending + ReturnErrorOnFailure(reliableMessageMgr->AddToRetransTable(reliableMessageContext, &entry)); + auto deleter = [reliableMessageMgr](ReliableMessageMgr::RetransTableEntry * e) { + reliableMessageMgr->ClearRetransTable(*e); + }; + std::unique_ptr entryOwner(entry, deleter); + + ReturnErrorOnFailure( + sessionManager->PrepareMessage(session, payloadHeader, std::move(message), entryOwner->retainedBuf)); + CHIP_ERROR err = sessionManager->SendPreparedMessage(session, entryOwner->retainedBuf); + err = ReliableMessageMgr::MapSendError(err, exchangeId, isInitiator); + ReturnErrorOnFailure(err); + reliableMessageMgr->StartRetransmision(entryOwner.release()); + } + else + { + ReturnErrorOnFailure(PrepareAndSendNonMRPMessage(sessionManager, session, payloadHeader, std::move(message))); + } } else { - // If the channel itself is providing reliability, let's not request MRP acks - payloadHeader.SetNeedsAck(false); - EncryptedPacketBufferHandle preparedMessage; - ReturnErrorOnFailure(sessionManager->PrepareMessage(session, payloadHeader, std::move(message), preparedMessage)); - ReturnErrorOnFailure(sessionManager->SendPreparedMessage(session, preparedMessage)); + ReturnErrorOnFailure(PrepareAndSendNonMRPMessage(sessionManager, session, payloadHeader, std::move(message))); } return CHIP_NO_ERROR; } +CHIP_ERROR ExchangeMessageDispatch::PrepareAndSendNonMRPMessage(SessionManager * sessionManager, const SessionHandle & session, + PayloadHeader & payloadHeader, + System::PacketBufferHandle && message) +{ + payloadHeader.SetNeedsAck(false); + EncryptedPacketBufferHandle preparedMessage; + ReturnErrorOnFailure(sessionManager->PrepareMessage(session, payloadHeader, std::move(message), preparedMessage)); + ReturnErrorOnFailure(sessionManager->SendPreparedMessage(session, preparedMessage)); + + return CHIP_NO_ERROR; +} + } // namespace Messaging } // namespace chip diff --git a/src/messaging/ExchangeMessageDispatch.h b/src/messaging/ExchangeMessageDispatch.h index 35f12e06e80d36..8585ebd422fc5f 100644 --- a/src/messaging/ExchangeMessageDispatch.h +++ b/src/messaging/ExchangeMessageDispatch.h @@ -48,6 +48,10 @@ class ExchangeMessageDispatch // TODO: remove IsReliableTransmissionAllowed, this function should be provided over session. virtual bool IsReliableTransmissionAllowed() const { return true; } + +private: + CHIP_ERROR PrepareAndSendNonMRPMessage(SessionManager * sessionManager, const SessionHandle & session, + PayloadHeader & payloadHeader, System::PacketBufferHandle && message); }; } // namespace Messaging diff --git a/src/messaging/ExchangeMgr.cpp b/src/messaging/ExchangeMgr.cpp index dddc9c6fd4ce8f..2ca22031094bb7 100644 --- a/src/messaging/ExchangeMgr.cpp +++ b/src/messaging/ExchangeMgr.cpp @@ -372,8 +372,10 @@ void ExchangeManager::SendStandaloneAckIfNeeded(const PacketHeader & packetHeade const SessionHandle & session, MessageFlags msgFlags, System::PacketBufferHandle && msgBuf) { - // If we need to send a StandaloneAck, create a EphemeralExchange for the purpose to send the StandaloneAck - if (!payloadHeader.NeedsAck()) + + // If using the MRP protocol and we need to send a StandaloneAck, create an EphemeralExchange to send + // the StandaloneAck. + if (!session->AllowsMRP() || !payloadHeader.NeedsAck()) return; // If rcvd msg is from initiator then this exchange is created as not Initiator. diff --git a/src/transport/GroupSession.h b/src/transport/GroupSession.h index 608bc78891b0e0..f20ff069ab029f 100644 --- a/src/transport/GroupSession.h +++ b/src/transport/GroupSession.h @@ -57,7 +57,8 @@ class IncomingGroupSession : public Session, public ReferenceCounted