FabricSync example changed to use ScopedNodeId in some locations #35805
Conversation
|
Review changes with SemanticDiff. |
| @@ -80,6 +80,8 @@ static_library("fabric-admin-utils") { | |||
| "commands/pairing/OpenCommissioningWindowCommand.h", | |||
| "commands/pairing/PairingCommand.cpp", | |||
| "commands/pairing/ToTLVCert.cpp", | |||
| "device_manager/BridgeAdminDeviceMapper.cpp", | |||
There was a problem hiding this comment.
I am open to alternative naming suggestion
There was a problem hiding this comment.
ScopedNodeIdHandleMapper? Reflects the role of mapping between ScopedNodeId and handles. But I am not clear why can't we just use ScopedNodeId directly.
|
PR #35805: Size comparison from 7eb96cd to cbb6c6e Full report (88 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
pullapprove
bot
requested review from
andy31415,
andyg-apple,
anush-apple,
arkq,
axelnxp,
bauerschwan,
bzbarsky-apple,
carol-apple,
cecille,
chapongatien,
chrisdecenzo,
chshu,
chulspro,
cliffamzn,
Damian-Nordic,
dhrishi,
doru91 and
fessehaeve
pullapprove
bot
requested review from
mkardous-silabs,
mspang,
nivi-apple,
p0fi,
pidarped,
plauric,
rcasallas-silabs,
ReneJosefsen,
saurabhst,
selissia,
sharadb-amazon,
tcarmelveilleux,
tecimovic,
tima-q,
tobiasgraf,
turon,
vivien-apple,
wiba-nordic,
woody-apple,
younghak-hwang,
yufengwangca and
yunhanw-google
| // If admin side ever needs more data other than ScopedNodeId we can change | ||
| // mHandleToScopedNodeId value type from ScopedNodeId to AdminDeviceInfo (or something | ||
| // of that nature). | ||
| std::unordered_map<uint64_t, chip::ScopedNodeId> mHandleToScopedNodeId; |
There was a problem hiding this comment.
The handle in this context seems act as an additional layer of abstraction or indirection, the NodeId is unique, and its corresponding ScopedNodeId is also unique. So why do we need an additional translation layer that requires dynamic allocation and maintenance?
There was a problem hiding this comment.
The handle in this context seems act as an additional layer of abstraction or indirection
This is exactly what it is, aggregator code shouldn't care and need to marshal data that is relevant to admin. While today it one thing, in the future it can be more data. I can move to marshaling the ScopedNodeId data instead, but as I mentioned in the PR description on why I didn't not go with that alternative approach. I am okay with changing to the alternative approach if we discuss it based on those merits.
NodeId is unique
NodeId is not. It is unique on a per fabric basis, but it is not unique.
So why do we need an additional translation layer that requires dynamic allocation and maintenance?
We either need to pass the ScopedNodeId back and forth, or we need this translation layer. I am more in favor of forward thinking solutions as I don't think marshaling data to systems where that data is meaningless is not scalable only for it to pass that data back, in all other systems that I have worked with this is where you introduce a handle of some sort which is what I was trying to do here.
| // mHandleToScopedNodeId value type from ScopedNodeId to AdminDeviceInfo (or something | ||
| // of that nature). | ||
| std::unordered_map<uint64_t, chip::ScopedNodeId> mHandleToScopedNodeId; | ||
| std::unordered_map<chip::ScopedNodeId, uint64_t, ScopedNodeIdHasher> mScopedNodeIdToHandle; |
There was a problem hiding this comment.
is ScopedNodeIdHasher same as handle?
There was a problem hiding this comment.
https://en.cppreference.com/w/cpp/container/unordered_map
It is the Hash class. Because std::hash<ScopedNodeId> is non-existant we need to define it. I didn't want to define it in the struct of ScopedNodeId. But I can look into do that as a follow up PR where just that is being evaluated
| { | ||
| std::size_t operator()(const chip::ScopedNodeId & scopedNodeId) const | ||
| { | ||
| std::size_t h1 = std::hash<uint64_t>{}(scopedNodeId.GetFabricIndex()); |
There was a problem hiding this comment.
The NodeId within ScopedNodeId is 64-bit, so is it possible for different ScopedNodeId instances to map to the same hash value, even if the chances are minimized?
There was a problem hiding this comment.
Collisions are fine. Overall a hash table is best case O(1), when there isn't a collision and worst case of O(n) when there are collisions all the time
https://en.wikipedia.org/wiki/Hash_table#Collision_resolution
unordered_map's default hashing funtion already maps to size_t right so already there is always a risk of a collision with anything you have ever used in the past
| @@ -6,7 +6,7 @@ package chip.rpc; | |||
|
|
|||
| // Define the message for a synchronized end device with necessary fields | |||
| message DeviceCommissioningWindowInfo { | |||
| uint64 node_id = 1; | |||
| uint64 handle = 1; | |||
There was a problem hiding this comment.
Why not just use ScopedNodeId instead of node_id?
There was a problem hiding this comment.
This was discussed in the PR's description. I am open to discussing them but please let me know which point you disagree with
|
Went with alternative approach: #35864 |
Problem:
Solution:
Alternative Considered:
Note: Normally changing a proto should not be done. It is done in this PR as we know that the proto is for examples compiled from source and running on the same machine and not accross networks where it is more difficult to ensure everything is updated all at once.
Fixes: #33221
Test: