[E2E][JF] Implement Joint Fabric nodesync feature #41535
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements the Joint Fabric nodesync feature, which allows a Joint Fabric Administrator to synchronize datastore entries (like Group IDs, Bindings, and ACLs) with other nodes on the fabric. This is achieved by introducing a JFADatastoreSync class that acts as a delegate for the JointFabricDatastore. When entries are added or removed from the datastore, the delegate is called to perform the synchronization with the target node over a CASE session. The implementation uses an asynchronous pattern with callbacks to handle device communication. The changes are well-structured, but I have a few suggestions to improve code clarity and consistency.
| class DevicePairedCommand : public Controller::DevicePairingDelegate | ||
| { |
There was a problem hiding this comment.
The DevicePairedCommand class inherits from Controller::DevicePairingDelegate, but it doesn't seem to function as a pairing delegate. Its purpose is to execute actions on an already connected device, not to handle pairing events. This inheritance is unnecessary and could be misleading. Consider removing the inheritance from Controller::DevicePairingDelegate.
class DevicePairedCommand
{| { | ||
| ChipLogProgress(DeviceLayer, "OnDeviceConnectionFailureFn - Not syncing device with node id: " ChipLogFormatX64, | ||
| ChipLogValueX64(cbContext->nodeId)); | ||
| // TODO: Remove Node Id |
There was a problem hiding this comment.
This TODO comment suggests that some cleanup logic is missing for when a device connection fails. Please clarify what "Remove Node Id" means in this context and implement the intended failure handling. For example, should the node be removed from a list of active nodes, or should there be a retry mechanism?
| it->statusEntry.state = Clusters::JointFabricDatastore::DatastoreStateEnum::kDeletePending; | ||
|
|
||
| // TODO: Update device | ||
| Clusters::JointFabricDatastore::Structs::DatastoreEndpointGroupIDEntryStruct::Type endpointGroupIdNullEntry{ 0 }; |
There was a problem hiding this comment.
Using a zero-initialized struct ({ 0 }) to signal a deletion to the delegate is an implicit convention. While it works, it can be hard to understand for future maintainers. Consider adding a comment to explain that a zero-initialized struct is used to signify a deletion for the SyncNode delegate method.
| virtual CHIP_ERROR | ||
| SyncNode(NodeId nodeId, | ||
| Clusters::JointFabricDatastore::Structs::DatastoreEndpointGroupIDEntryStruct::Type & endpointGroupIDEntry, | ||
| std::function<void()> onComplete) | ||
| { | ||
| return CHIP_ERROR_NOT_IMPLEMENTED; | ||
| } |
There was a problem hiding this comment.
The SyncNode overload for DatastoreEndpointGroupIDEntryStruct takes its struct parameter by a non-const reference, while the other SyncNode overloads take const references. This is inconsistent. Since the endpointGroupIDEntry object is not modified by the caller, it should be passed by const reference to maintain consistency across the delegate interface.
This change will also need to be applied to the implementing classes:
examples/jf-admin-app/linux/include/JFADatastoreSync.hexamples/jf-admin-app/linux/JFADatastoreSync.cpp
virtual CHIP_ERROR
SyncNode(NodeId nodeId,
const Clusters::JointFabricDatastore::Structs::DatastoreEndpointGroupIDEntryStruct::Type & endpointGroupIDEntry,
std::function<void()> onComplete)
{
return CHIP_ERROR_NOT_IMPLEMENTED;
}
|
PR #41535: Size comparison from c1f377d to 351f096 Full report (37 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #41535 +/- ##
==========================================
+ Coverage 51.04% 51.06% +0.02%
==========================================
Files 1386 1385 -1
Lines 100958 100901 -57
Branches 13061 13056 -5
==========================================
- Hits 51534 51530 -4
+ Misses 49424 49371 -53 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
PR #41535: Size comparison from cde4477 to 4fda0bc Full report (4 builds for cc32xx, realtek)
|
| if constexpr (std::is_same_v<T, | ||
| app::Clusters::JointFabricDatastore::Structs::DatastoreEndpointGroupIDEntryStruct::Type>) | ||
| { | ||
| attributeId = Attributes::EndpointGroupIDList::Id; | ||
| } |
There was a problem hiding this comment.
The sync is being done with a regular Matter device (not another Datastore) and so in the case of a DatastoreEndpointGroupIDEntryStruct, the operation to send is:
IM Type: Command
EndpointId=newGroupEntry.endpointID
ClusterId=0x0004 (groups)
CommandId=0x00 (add group)
Arg0=newGroupEntry.groupID
Arg1=GroupName
| else if constexpr (std::is_same_v<T, | ||
| app::Clusters::JointFabricDatastore::Structs::DatastoreNodeKeySetEntryStruct::Type>) | ||
| { | ||
| attributeId = Attributes::NodeKeySetList::Id; | ||
| } |
There was a problem hiding this comment.
In the case of DatastoreNodeKeySetEntryStruct, the operation to send is:
IM Type: Command
EndpointId=0 (root endpoint)
ClusterId=0x003F (group key management)
CommandId=0x00 (key set write)
Arg0=groupKeySet
| { | ||
| attributeId = Attributes::EndpointBindingList::Id; | ||
| } |
There was a problem hiding this comment.
In the case of DatastoreEndpointBindingEntryStruct, the operation to send is:
IM Type: AttributeWrite
EndpointId=0 (root endpoint)
ClusterId=0x001E (bindings cluster)
AttributeId=0x00 (binding)
Arg0=list of all bindings for this node from datastore
| { | ||
| attributeId = Attributes::NodeACLList::Id; | ||
| } |
There was a problem hiding this comment.
In the case of DatastoreACLEntryStruct, the operation to send is:
IM Type: AttributeWrite
EndpointId=0 (root endpoint)
ClusterId=0x001F (ACL cluster)
AttributeId=0x00 (ACL)
Arg0=list of all ACLs for this node from datastore
| CHIP_ERROR err = cluster.WriteAttribute(cbContext->objectToWrite, pairingCommand, Clusters::JointFabricDatastore::Id, | ||
| attributeId, OnWriteSuccessResponse, OnWriteFailureResponse, NullOptional); |
There was a problem hiding this comment.
this should be moved above since some are using WriteAttribute while others will use SendCommand
2 participants
Summary
Implement Joint Fabric nodesync feature in jfa-admin-app
Related issues
Addresses Issue: #40592.
Testing
Verified using the below instructions.