Skip to content
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

[tlv] add new tlvs for network diagnostic #266

Closed
wants to merge 13 commits into from

Conversation

ZhangLe2016
Copy link
Contributor

libotcommissioner.jar, a versatile Java library, provides support for wide range of TMF message transactions, including MeshCop protocols and diagnostic TLVs. This PR focuses on integrating all the diagnostic TLVs, as defined in the spec 10.11.3 into the library's TLV module

libotcommissioner.jar, a versatile Java library, provides support for
wide range of TMF message transactions, including MeshCop protocols and
diagnostic TLVs. This PR focuses on integrating all the diagnostic TLVs,
as defined in the spec `10.11.3` into the library's TLV module
@ZhangLe2016 ZhangLe2016 requested review from lanyuwen and wgtdkp May 7, 2024 03:19
{
// Network disgnostic layer TLVs
case Type::kNetworkDiagExtMacAddress:
return length == 8;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We generally consider a TLV valid if its length is at least as long as the currently defined value length. This accommodates potential future extensions of the TLV format.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@abtink Thank you for your review. I have clarified the length of TLVs as per the definition in the spec. For instance, the length of the extended MAC address TLV in spec 5.19.2 is 8. Could you please kindly indicate any other inappropriate definitions in this PR?

Copy link
Member

@wgtdkp wgtdkp May 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think @abtink wants you to change the check to return length >= 8; so that the extended MAC address can be extended to maybe 16 bytes in Thread 2.0 or whatever later versions

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it, Thanks @wgtdkp clarification. Let me update check condition.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @wgtdkp :). Yes. Exactly.
Thank @ZhangLe2016 for changing.

libotcommissioner.jar, a versatile Java library, provides support for
wide range of TMF message transactions, including MeshCop protocols and
diagnostic TLVs. This PR focuses on integrating all the diagnostic TLVs,
as defined in the spec `10.11.3` into the library's TLV module
libotcommissioner.jar, a versatile Java library, provides support for
wide range of TMF message transactions, including MeshCop protocols and
diagnostic TLVs. This PR focuses on integrating all the diagnostic TLVs,
as defined in the spec `10.11.3` into the library's TLV module
Copy link
Member

@lanyuwen lanyuwen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jwhui jwhui requested a review from abtink May 9, 2024 15:05
case Type::kNetworkDiagVendorSWVersion:
return length <= 2;
case Type::kNetworkDiagChild:
return length <= 43;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these also use >=?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I understand it, the TLV will only have content when the queried device has children. If the queried device has no children, the value of TLV is empty, so the length should be 0. If we check the length to be >= 43, then when the value of TLV is empty, the TLV will be considered invalid.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TLV kNetworkDiagVendorSWVersion is similar. If the device itself does not have a sw version content, then the value of TLV is empty. When the commissioner gets an empty TLV, it should not be judged as invalid tlv. Please correct me~~ Thanks!

wgtdkp and others added 10 commits July 16, 2024 04:19
Current unit tests are writing intermediate testing files to current
dir. This is polluting the source dir when executing the tests in the
source root dir.

This commit updates the unit tests to output the testing data files to a
dedicated ./tmp dir and ignore dir tmp/ from git.
It's an overkill to define a dedicated class/struct for a simple
`uint16_t` just for pretty print to json in HEX string.
This commits cleans up / updates the existing MeshCoP functionality in
the Android sample app:
1. Removes the local BorderAgentDatabase and data types by leveraging
   the Thread GMS SDK
2. Replaces the "discriminator" with the "id" mDNS TXT entry
3. Removes dead code such as `ThreadCommissionerServiceFactory`
In order to enable libotcommissioner.jar to support the diagnostic
commands, it is necessary to integrate all diagnostic URIs into the uri
module.
…y transaction (openthread#279)

Currently, while the commissioner is transmitting udp_tx messages, the
UDP source port is provided by the `DtlsSession` interface. This value
is randomly generated. However, the CoAP layer on the border router
side performs checks on the received udp_tx port to ensure that the
destination UDP port used to outgoing messages matches the source port
of the received messages. This leads to the commissioner's ACK
messages not being processed, causing the BR side to repeatedly
retransmit the same udp_rx message.

To resolve this issue, we use the default MM port as the only
supported destination port for UDP_RX messages.
There could be potential out of bound error if the token length in CoAP
header is zero (`&aBuf[offset]` could be referring to the next element
after the end of the buffer).

Note that the added test can pass without the fix in coap.cpp but
it's probably because the toolchain has some optimization to avoid such
issues...
The commissioner.i file is behind the APIs in include/commissioner/.

Also ignores `PrintTo` given it's only for googletest.
libotcommissioner.jar, a versatile Java library, provides support for
wide range of TMF message transactions, including MeshCop protocols and
diagnostic TLVs. This PR focuses on integrating all the diagnostic TLVs,
as defined in the spec `10.11.3` into the library's TLV module
@ZhangLe2016 ZhangLe2016 deleted the diag-tlv-1 branch July 16, 2024 04:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants