Log node ID in exchanges #32550
Merged
Log node ID in exchanges #32550
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
|
PR #32550: Size comparison from ad14dbc to 5cffd1b Increases above 0.2%:
Increases (68 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, stm32, telink)
Decreases (1 build for linux)
Full report (73 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, stm32, telink)
|
|
This has significant impact on code size while usually the mapping between the exchange ID and the node ID can be found based on other messages. Could you make this feature optional so that we can disable it on platforms that prefer more concise logs for code size or performance reasons (printing logs to UART takes time and may affect the system performance)? |
|
PR #32550: Size comparison from ad14dbc to 01c3974 Increases above 0.2%:
Increases (68 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, stm32, telink)
Decreases (1 build for linux)
Full report (73 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, stm32, telink)
|
I agree that the increase in code size is significant, which was unexpected. I could make this feature optional. However, perhaps I should entirely revise my approach to explicitly log the node ID every time |
|
PR #32550: Size comparison from ad14dbc to 47cc338 Increases (3 builds for linux)
Decreases (4 builds for efr32, linux)
Full report (73 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, stm32, telink)
|
|
It's a little surprising that the codesize impact is that large, and concerning: How much codesize are we paying for the existing exchange logging? The other thing that we could do is to only add the ScopedNodeId logging to specific logging locations, instead of making it part of the exchange logging in all cases.... |
|
@leonardmgh this requires restyle changes - please run |
For now i have made the changes optional, which is probably the easiest solution for now. Otherwise adding the ScopedNodeId logging in specific locations could be a good alternative. Maybe the logging of the node id could replace the logging of the exchange context in some error messages, since i guess that it might be more useful to know node ID. |
pullapprove
bot
requested review from
andy31415,
andyg-apple,
anush-apple,
arkq,
carol-apple,
cecille,
chrisdecenzo,
chshu,
chulspro and
cliffamzn
pullapprove
bot
requested review from
mkardous-silabs,
mspang,
nivi-apple,
p0fi,
pidarped,
plauric,
rcasallas-silabs,
robszewczyk,
saurabhst,
selissia,
sharadb-amazon,
tcarmelveilleux,
tecimovic,
tehampson,
tima-q,
tobiasgraf,
turon,
vivien-apple,
wiba-nordic,
woody-apple,
younghak-hwang,
yufengwangca and
yunhanw-google
bzbarsky-apple
approved these changes
Mar 19, 2024
Damian-Nordic
approved these changes
Mar 19, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously, the SDK only recorded the exchange ID when logging information about exchanges, as illustrated by the following example:
Failed to Send CHIP MessageCounter:68460922 on exchange 30872i sendCount: 4 max retries: 4To enable more precise troubleshooting, this PR proposes to also log the peer node ID. The proposed log format change is as follows:
Dropping unexpected message of type 0x1 with protocolId (0, 1) and MessageCounter:43669734 on exchange 11672r to: [1:0000000012344321]I have used the same format as here:
connectedhomeip/src/app/OperationalSessionSetup.cpp
Line 53 in ad14dbc
Further Context:
The HomeAssistant Matter server frequently logs the mentioned message when adding Nanoleaf Matter devices into the thread network. This situation has sparked debates regarding the source of the error – whether it is exclusive to Nanoleaf devices or all devices relaying traffic through them, leading to timeout issues. This enhancement aims to provide clearer insights into such occurrences.