-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Error reporting cleanup #6979
Error reporting cleanup #6979
Conversation
…node id logging is a progress not an error
The benefit of this now is: i see that the message counter mismatch occurs in BLE, which may be considered odd as BLE is point-to-point so nodes do not necesarely need to validate destination node id for every incoming message. |
…nt to eventually maye this "Detail" in case logging is too verbose
@@ -141,7 +141,11 @@ CHIP_ERROR ExchangeManager::UnregisterUnsolicitedMessageHandlerForType(Protocols | |||
|
|||
void ExchangeManager::OnReceiveError(CHIP_ERROR error, const Transport::PeerAddress & source, SecureSessionMgr * msgLayer) | |||
{ | |||
ChipLogError(ExchangeManager, "Accept FAILED, err = %s", ErrorStr(error)); | |||
char srcAddressStr[Transport::PeerAddress::kMaxToStringSize]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: might be worth guarding this code with #if CHIP_ERROR_LOGGING
(not sure if it will be optimized out when logging is disabled).
src/transport/SecureSessionMgr.cpp
Outdated
ChipLogError(Inet, "Secure transport received message destined to node ID (0x" ChipLogFormatX64 ")", | ||
ChipLogValueX64(packetHeader.GetDestinationNodeId().Value())); | ||
ChipLogProgress(Inet, "Secure transport received message destined to fabric %d, node 0x" ChipLogFormatX64 ". Key ID %d", | ||
static_cast<int>(state->GetAdminId()), packetHeader.GetDestinationNodeId().Value(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You removed the ChipLogValueX64
macro (probably unintentionally).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, bad merge. Trying again. Thanks for noticing.
src/transport/SecureSessionMgr.cpp
Outdated
} | ||
else | ||
{ | ||
ChipLogError(Inet, "Secure transport received message without node ID with key ID (%d)", packetHeader.GetEncryptionKeyID()); | ||
ChipLogProgress(Inet, "Secure transport received message for fabrid %d without node ID. Key ID %d", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: fabrid
Size increase report for "esp32-example-build" from 81a4896
Full report output
|
Size increase report for "gn_qpg6100-example-build" from 81a4896
Full report output
|
Size increase report for "nrfconnect-example-build" from 81a4896
Full report output
|
* Code cleanup: better error report for on receive errors, destination node id logging is a progress not an error * Include fabric ID in progress message for secure transport. We may want to eventually maye this "Detail" in case logging is too verbose * Take into account that node ids in packet headers are optional * Restyle fixes and swap back error to progress * Review comments and merge fix * revert unwanted changes * Updated the chip error logging conditional
Problem
OnReceiveError was reporting 'accept failure' and the 'received message from node...' was logged as an error
Summary of Changes
Expand the message for OnReceive error to make it clear it is on receiving and to note from where the error came from.
Switch the progress log to 'progress' instead of error.
Testing
Manually ran ble-wifi pairing on esp32 and watced logs with
./scripts/tools/esp32_log_cat.py
Saw: