[diag] add network diagnostic data definition and parsing #303

ZhangLe2016 · 2024-10-16T09:41:54Z

supported TLVs:

Ipv6addressList
ChildIpv6AddressList
Mode
ChildTable
LeaderData
Route64
MacCounters
Eui64
ExtMacAddress
MacAddress

supported TLVs: - Ipv6addressList - ChildIpv6AddressList - Mode - ChildTable - LeaderData - Route64 - MacCounters - Eui64 - ExtMacAddress - MacAddress

include/commissioner/network_diag_data.hpp

supported TLVs: - Ipv6addressList - ChildIpv6AddressList - Mode - ChildTable - LeaderData - Route64 - MacCounters - Eui64 - ExtMacAddress - MacAddress

include/commissioner/network_diag_data.hpp

Co-authored-by: Kangping <wgtdkp@google.com>

include/commissioner/network_diag_data.hpp

src/library/commissioner_impl.cpp

include/commissioner/network_diag_data.hpp

wgtdkp

Is it possible to add unit tests in commissioner_impl_test.cpp?

Co-authored-by: Kangping <wgtdkp@google.com>

wgtdkp

Thanks for resolving the comments!
In internal code review, we asked to mark comments resolved when you followed the suggestion in it and leave the other open. This is useful for reviewers to re-review the code and skip comments which has been resolved. It makes the review much easier.

Can you follow that practice in GitHub as well? For example, you can and should only close comments which you replied "done" :)

wgtdkp · 2024-10-17T03:21:38Z

include/commissioner/network_diag_data.hpp

+    ByteArray                         mExtMacAddress;
+    uint16_t                          mMacAddress = 0;
+    Route64                           mRoute64;
+    LeaderData                        mLeaderData;
+    MacCounters                       mMacCounters;
+    std::vector<std::string>          mAddrs;


Some place we use "Addr" while some place we use "Address", it doesn't matter which one is used, but it would be better to make it consistent. Looking at existing code, "Addr" is always used for addresses. Can we update the code in PR to follow the existing naming? Thanks

I see, I changed addrs to address

No, if you look at include/commissioner/commissioner.hpp, "Addr" is used but not "Address"

src/library/commissioner_impl.cpp

include/commissioner/network_diag_data.hpp

Co-authored-by: Kangping <wgtdkp@google.com>

ZhangLe2016 · 2024-10-17T05:56:41Z

Is it possible to add unit tests in commissioner_impl_test.cpp?

Looks like the unit test cases need to call the API functions defined into commissioner.hpp. For this PR, the new functions are defined in private, I am not sure if we can add corresponding test cases for them, do you have any suggestions?

Co-authored-by: Kangping <wgtdkp@google.com>

ZhangLe2016 · 2024-10-17T09:15:01Z

Is it possible to add unit tests in commissioner_impl_test.cpp?

Looks like the unit test cases need to call the API functions defined into commissioner.hpp. For this PR, the new functions are defined in private, I am not sure if we can add corresponding test cases for them, do you have any suggestions?

Added a new unit test case into commissioner_impl_test.cpp

Co-authored-by: Kangping <wgtdkp@google.com>

wgtdkp

Looks good to me overall, just minor comments!

src/library/tlv.hpp

src/library/commissioner_impl_test.cpp

Co-authored-by: Kangping <wgtdkp@google.com>

codecov · 2024-10-23T06:52:09Z

Codecov Report

Attention: Patch coverage is 76.71958% with 44 lines in your changes missing coverage. Please review.

Project coverage is 72.54%. Comparing base (6a5cddb) to head (76985db).
Report is 10 commits behind head on main.

Files with missing lines	Patch %	Lines
src/library/commissioner_impl.cpp	74.85%	44 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #303      +/-   ##
==========================================
- Coverage   72.75%   72.54%   -0.22%     
==========================================
  Files          72       72              
  Lines        7444     7740     +296     
==========================================
+ Hits         5416     5615     +199     
- Misses       2028     2125      +97

Files with missing lines	Coverage Δ
src/library/commissioner_impl.hpp	`96.66% <ø> (+0.23%)`	⬆️
src/library/tlv.cpp	`62.13% <100.00%> (-5.82%)`	⬇️
src/library/commissioner_impl.cpp	`74.87% <74.85%> (+0.14%)`	⬆️

... and 1 file with indirect coverage changes

---- 🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests

ZhangLe2016 · 2024-10-23T07:09:30Z

@jwhui Could you please review this PR? Thanks~~

ZhangLe2016 · 2024-11-20T03:22:04Z

@jwhui, Could you please review this Pull Request for me? and really appreciate your help!

[diag] Add network diagnostic data: definition and analysis

57e2b7d

supported TLVs: - Ipv6addressList - ChildIpv6AddressList - Mode - ChildTable - LeaderData - Route64 - MacCounters - Eui64 - ExtMacAddress - MacAddress

ZhangLe2016 requested review from lanyuwen and wgtdkp October 16, 2024 09:41

ZhangLe2016 mentioned this pull request Oct 16, 2024

[api] enable DIAG_GET query function into commissioner #298

Closed

wgtdkp reviewed Oct 16, 2024

View reviewed changes

include/commissioner/network_diag_data.hpp Outdated Show resolved Hide resolved

[diag] Add network diagnostic data: definition and analysis

b1bc0e8

supported TLVs: - Ipv6addressList - ChildIpv6AddressList - Mode - ChildTable - LeaderData - Route64 - MacCounters - Eui64 - ExtMacAddress - MacAddress

wgtdkp reviewed Oct 16, 2024

View reviewed changes

ZhangLe2016 and others added 3 commits October 16, 2024 18:45

Update include/commissioner/network_diag_data.hpp

d0ce06f

Co-authored-by: Kangping <wgtdkp@google.com>

Update include/commissioner/network_diag_data.hpp

d343417

Co-authored-by: Kangping <wgtdkp@google.com>

Update include/commissioner/network_diag_data.hpp

9c19157

Co-authored-by: Kangping <wgtdkp@google.com>

wgtdkp reviewed Oct 16, 2024

View reviewed changes

wgtdkp changed the title ~~[diag] Add network diagnostic data: definition and analysis~~ [diag] Add network diagnostic data: definition and parsing Oct 16, 2024

wgtdkp changed the title ~~[diag] Add network diagnostic data: definition and parsing~~ [diag] Add network diagnostic data definition and parsing Oct 16, 2024

wgtdkp changed the title ~~[diag] Add network diagnostic data definition and parsing~~ [diag] add network diagnostic data definition and parsing Oct 16, 2024

wgtdkp reviewed Oct 16, 2024

View reviewed changes

Update include/commissioner/network_diag_data.hpp

64379b8

Co-authored-by: Kangping <wgtdkp@google.com>

wgtdkp reviewed Oct 17, 2024

View reviewed changes

ZhangLe2016 and others added 2 commits October 17, 2024 05:21

Update include/commissioner/network_diag_data.hpp

33f9723

Co-authored-by: Kangping <wgtdkp@google.com>

Update include/commissioner/network_diag_data.hpp

f8c0a53

Co-authored-by: Kangping <wgtdkp@google.com>

ZhangLe2016 and others added 2 commits October 17, 2024 06:12

Update include/commissioner/network_diag_data.hpp

8b98d71

Co-authored-by: Kangping <wgtdkp@google.com>

Update include/commissioner/network_diag_data.hpp

1a996e8

Co-authored-by: Kangping <wgtdkp@google.com>

Update include/commissioner/network_diag_data.hpp

174f15c

Co-authored-by: Kangping <wgtdkp@google.com>

wgtdkp approved these changes Oct 17, 2024

View reviewed changes

src/library/tlv.hpp Outdated Show resolved Hide resolved

src/library/commissioner_impl_test.cpp Outdated Show resolved Hide resolved

ZhangLe2016 and others added 5 commits October 17, 2024 18:10

Update src/library/tlv.hpp

773fc8b

Co-authored-by: Kangping <wgtdkp@google.com>

Update src/library/tlv.hpp

e4560cb

Co-authored-by: Kangping <wgtdkp@google.com>

Update src/library/tlv.hpp

3b94b30

Co-authored-by: Kangping <wgtdkp@google.com>

Update src/library/tlv.hpp

1d82f9d

Co-authored-by: Kangping <wgtdkp@google.com>

Update src/library/tlv.hpp

fd5e986

Co-authored-by: Kangping <wgtdkp@google.com>

ZhangLe2016 requested a review from jwhui October 18, 2024 02:25

Update src/library/tlv.hpp

76985db

Co-authored-by: Kangping <wgtdkp@google.com>

ZhangLe2016 removed the request for review from lanyuwen November 20, 2024 03:19

jwhui approved these changes Nov 20, 2024

View reviewed changes

jwhui merged commit 67c828a into openthread:main Nov 20, 2024
33 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[diag] add network diagnostic data definition and parsing #303

[diag] add network diagnostic data definition and parsing #303

ZhangLe2016 commented Oct 16, 2024

wgtdkp left a comment

wgtdkp left a comment

wgtdkp Oct 17, 2024

ZhangLe2016 Oct 17, 2024

wgtdkp Oct 17, 2024

ZhangLe2016 commented Oct 17, 2024

ZhangLe2016 commented Oct 17, 2024

wgtdkp left a comment

codecov bot commented Oct 23, 2024 •

edited

Loading

ZhangLe2016 commented Oct 23, 2024

ZhangLe2016 commented Nov 20, 2024

[diag] add network diagnostic data definition and parsing #303

[diag] add network diagnostic data definition and parsing #303

Conversation

ZhangLe2016 commented Oct 16, 2024

wgtdkp left a comment

Choose a reason for hiding this comment

wgtdkp left a comment

Choose a reason for hiding this comment

wgtdkp Oct 17, 2024

Choose a reason for hiding this comment

ZhangLe2016 Oct 17, 2024

Choose a reason for hiding this comment

wgtdkp Oct 17, 2024

Choose a reason for hiding this comment

ZhangLe2016 commented Oct 17, 2024

ZhangLe2016 commented Oct 17, 2024

wgtdkp left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 23, 2024 • edited Loading

Codecov Report

ZhangLe2016 commented Oct 23, 2024

ZhangLe2016 commented Nov 20, 2024

codecov bot commented Oct 23, 2024 •

edited

Loading