openthread / ot-commissioner

OpenThread Commissioner, a Thread commissioner for joining new Thread devices and managing Thread networks.
https://openthread.io/
BSD 3-Clause "New" or "Revised" License
49 stars 36 forks source link

[active-dataset] returns raw Active Operational Dataset TLVs #142

Closed wgtdkp closed 3 years ago

wgtdkp commented 4 years ago

Parsing the Active Operational Dataset into struct ActiveOperationalDataset is for manipulating each field of the dataset and the commissioner app doesn't need to write a decoder by their own. But the commissioner app may store or transfer Active Operational Dataset as a byte array. It would be convenient to have the raw TLV format of the dataset so that the commissioner app doesn't need to write an encoder by their own.

This PR adds a new APIs (GetRawActiveDataset(...)) to return raw Active Operational Dataset.

codecov-commenter commented 4 years ago

Codecov Report

Merging #142 into master will decrease coverage by 0.12%. The diff coverage is 56.66%.

@@            Coverage Diff             @@
##           master     #142      +/-   ##
==========================================
- Coverage   70.23%   70.11%   -0.13%     
==========================================
  Files          52       52              
  Lines        4731     4752      +21     
==========================================
+ Hits         3323     3332       +9     
- Misses       1408     1420      +12     
Impacted Files Coverage Δ
include/commissioner/commissioner.hpp 46.66% <ø> (+2.91%) :arrow_up:
src/library/commissioner_impl.hpp 96.29% <0.00%> (-3.71%) :arrow_down:
src/library/commissioner_safe.cpp 79.92% <0.00%> (-3.55%) :arrow_down:
src/library/commissioner_safe.hpp 100.00% <ø> (ø)
src/library/commissioner_impl.cpp 75.82% <94.44%> (+0.16%) :arrow_up:
wgtdkp commented 3 years ago

@simonlingoogle @jwhui Thanks for your comments, I will close this PR since CHIP is not going to use the Thread TLV format directly.

See handling of Active Operational Dataset in CHIP: https://github.com/project-chip/connectedhomeip/blob/9754e3d0231d56aeb5495a580d6059cc1d2f1872/examples/common/chip-app-server/Server.cpp#L75-L140

jwhui commented 3 years ago

@simonlingoogle @jwhui Thanks for your comments, I will close this PR since CHIP is not going to use the Thread TLV format directly.

See handling of Active Operational Dataset in CHIP: https://github.com/project-chip/connectedhomeip/blob/9754e3d0231d56aeb5495a580d6059cc1d2f1872/examples/common/chip-app-server/Server.cpp#L75-L140

I think CHIP must treat the Active Operational Dataset as a binary blob. The Active Operational Dataset include fields defined by the Thread Specification and newer versions of the Thread Specification may introduce new fields. CHIP should be able to support newer versions of Thread without requiring software update of CHIP itself.

bukepo commented 3 years ago

@simonlingoogle @jwhui Thanks for your comments, I will close this PR since CHIP is not going to use the Thread TLV format directly. See handling of Active Operational Dataset in CHIP: https://github.com/project-chip/connectedhomeip/blob/9754e3d0231d56aeb5495a580d6059cc1d2f1872/examples/common/chip-app-server/Server.cpp#L75-L140

I think CHIP must treat the Active Operational Dataset as a binary blob. The Active Operational Dataset include fields defined by the Thread Specification and newer versions of the Thread Specification may introduce new fields. CHIP should be able to support newer versions of Thread without requiring software update of CHIP itself.

Should CHIP be aware of some of the fields? For example, when forming a new Thread network, CHIP may want to set the network name and channel mask. Thoughts?

jwhui commented 3 years ago

CHIP can be aware of the fields, but CHIP must not exclude fields simply because it does not understand them.

wgtdkp commented 3 years ago

I think CHIP must treat the Active Operational Dataset as a binary blob. The Active Operational Dataset include fields defined by the Thread Specification and newer versions of the Thread Specification may introduce new fields. CHIP should be able to support newer versions of Thread without requiring software update of CHIP itself.

This makes sense! Thanks! @jwhui