-
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
Add retransmission count to MRP TransmitEvent #37768
Conversation
PR #37768: Size comparison from ba98125 to 3807500 Full report (73 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
/gemini review |
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.
Code Review
The changes look good overall. The addition of the retransmissionCount
field to the TransmitEvent
struct seems well-integrated and the unit tests have been updated accordingly. I have a minor suggestion regarding naming consistency in the test file.
Summary of Findings
- Naming Consistency: Consider renaming
forthTransmitEvent
tofourthTransmitEvent
inTestReliableMessageProtocol.cpp
for better readability and consistency.
Assessment
The pull request introduces a new field retransmissionCount
to the TransmitEvent
struct, which provides valuable information for analyzing reliable message transmissions. The code changes are well-structured and include corresponding updates to the unit tests. Overall, the pull request is in good shape to be merged after addressing the minor suggestions provided below. Remember to have others review and approve this code before merging.
PR #37768: Size comparison from ba98125 to 8becdc8 Increases above 0.2%:
Full report (73 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
@@ -65,6 +67,9 @@ class ReliableMessageAnalyticsDelegate | |||
// The outgoing message counter associated with the event. If there is no outgoing message counter | |||
// this value will be 0. | |||
uint32_t messageCounter = 0; | |||
// If the eventType is kRetransmission, this value will be populated with the number of the | |||
// retransmission attempt |
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.
This needs to document what "2" means: the first retransmission (i.e. the second packet sent), or the second retransmission (third packet sent).
* Add retransmission count to MRP TransmitEvent * Self review * Change forth to fourth
Testing
New additions to unit test are passing