Add loogging of nodeID patch #33
Conversation
|
Hi @leonardmgh, thanks for your PR! Adding the node ID to the messages is definitely helpful and welcome! 🙏 We try to keep the patchset over the upstream Matter SDK as small as possible, can you try to upstream this patch as well (create a PR to the upstream project at https://github.com/project-chip/connectedhomeip). On a different note, some messages do have the node ID (as well as the fabric) logged already, see: Maybe it would be good if the SDK standardizes on a (that?) format? 🤔 |
|
I will make a PR to the Matter SDK, lets see how this goes. I have modified my implementation to just modify the macro, to keep the footprint of this change smaller and also adopted the log format for the node ID used in the SDK. Im still not sure if that's the way to go or if i should explicit add logging for the node ID everywhere where the macro currently is used. |
Nah I think this is fine or rather better. Please share here or tag me when you've created the PR, I'll follow the discussion there too. |
|
Yes, ill do that. I had a few problems getting all tests integrated into the SDK to run on my machine, so im still working on that. |
I do run some tests locally at times to reproduce, but usually rely on their GitHub action CI tests. Running the full test suite they have locally seems a bit too ambitious to me 😅 When you look at PRs and how folks add patches/iterate, I think that is how most developer handle it TBH. |
|
I have created an PR to the Matter SDK now: |
|
@agners the pr has been merged now. Is this PR still relevant or will the release branch soon be updated to use newer version of the matter repo? |
|
@leonardmgh adding Node ID to the current builds would still be helpful. Can you update this PR with the patch which got merged (and enable the new config option)? 🥺 |
|
Great, i have updated everything now. Im still verifying that everything works locally though. I also discovered that i forgot to add an option in the build system to enable the node Id logging, so i have just included that in the patch for now. I think ill create another PR to add the option to enable it using |
|
FWIW, we already patch the config in other cases (specifically with this patch). So I am fine with also changing the config through patch in this case. Just mark as ready for review in case you are happy, then I'll merge the change and make a new release. |
|
Everything works on my machine. |
This adds the logging of the node ID for log messages originating from the "ReliableMessageMgr" and "ReliableMessageContext". This hopefully adds more insight into which device communication errors originate from.
The log messages will look like this:
CHIP:EM: Rxd Ack; Removing MessageCounter:56585655 from Retrans Table on exchange 51103r with NodeID 287454020I have not tested this patch with a real world setup yet, but it passes the sdk tests.
There are more places the NodeID can be logged, but is this nessecarry?