Add Matter HRAP Support #2283
Add Matter HRAP Support #2283
Conversation
|
Duplicate profile check: Passed - no duplicate profiles detected. |
|
Invitation URL: |
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against 304cbf1 |
Test Results 69 files 448 suites 0s ⏱️ Results for commit 304cbf1. ♻️ This comment has been updated with latest results. |
hcarter-775
requested review from
tpmanley,
nand-nor,
ctowns,
nickolas-deboom,
z-michel and
HunsupJung
|
I looked over the cluster-capability mappings again and passed over the driver again and things look good from my perspective. One more question I had is have you tested using the embedded cluster definitions? |
Good callout! Yes, this has been tested now. There actually was a small typo bug that got fixed in the latest commit |
|
@hcarter-775 |
hcarter-775
force-pushed
the
add-matter-hrap
branch
2 times, most recently
from
75fe862 to
85bd6ca
Compare
| device:subscribe() | ||
| local tbrm_eps = embedded_cluster_utils.get_endpoints(device, clusters.ThreadBorderRouterManagement.ID) | ||
| if tbrm_eps and #tbrm_eps > 0 then | ||
| device:send(clusters.ThreadBorderRouterManagement.server.commands.GetActiveDatasetRequest(device, tbrm_eps[1])) |
There was a problem hiding this comment.
Just confirming - is the active dataset expected to change at any point? Why is it read each driver startup, but not cached? And if it is expected to change, why not subscribe to this attribute?
There was a problem hiding this comment.
yes, this dataset can change, and we can't subscribe to it because this is a command, not an attribute. Perhaps we could add some better logic to not re-read the data on each and every restart? Can look into this a bit more.
There was a problem hiding this comment.
It is strange that this could be expected to change, but the spec doesn't provide an event that we can subscribe to. I not super familiar with HRAP stuff, so sorry if this is a dumb question - what is a scenario in which this value would change?
Based on a cursory read of the spec, it seems like the ActiveDatasetTimestamp attribute contains the most recent timestamp for the active dataset, and shall be updated when a new active dataset is configured. I think then we are supposed to monitor this attribute and then send the command to get the latest dataset when that attribute changes, or when we notice that the timestamp does not match the last timestamp that we cached in the case of a driver restart.
There was a problem hiding this comment.
to answer your first question, there are a few cases. One would be going from no thread network configured to one being configured, and to move between networks would occur in the case that a user wanted to merge their thread networks across multiples thread border routers, which is a major part of the thread unification initiative.
To the second point, I think that's a good idea and a cleaner solution than arbitrarily retrying on a restart.
There was a problem hiding this comment.
added in latest commit
There was a problem hiding this comment.
Agree with @ctowns, we can subscribe to the active timestamp attribute, which is a reliable indicator of dataset change. This is also how we track changes within hub-core
| if not ib.data.value then | ||
| device.log.info("No Thread operational dataset configured") | ||
| elseif ib.data.value ~= device:get_field(CURRENT_ACTIVE_DATASET_TIMESTAMP) then | ||
| local tbrm_ep = embedded_cluster_utils.get_endpoints(device, clusters.ThreadBorderRouterManagement.ID)[1] |
There was a problem hiding this comment.
I think we could use use the ib.endpoint_id instead of getting the endpoint that supports border router management, since that should be the same endpoint that provided this interacton block.
There was a problem hiding this comment.
for sure, moved it thoughtlessly out of device_init where this handling was required.
hcarter-775
force-pushed
the
add-matter-hrap
branch
from
b5176f1 to
7c97195
Compare
hcarter-775
force-pushed
the
add-matter-hrap
branch
from
7c97195 to
304cbf1
Compare
Description of Change
Implements handling for the TBR and NIM Device Types. Includes a basic thread operational dataset parser.
Summary of Completed Tests
Tested using CHIP Example App, VDA, and is verified with unit tests.