Draft: TLV-JSON-TLV converter #27279
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…oid tv-app platform and content apps)
pullapprove
bot
requested review from
amitnj,
andy31415,
anush-apple,
arkq,
bzbarsky-apple,
carol-apple,
chshu,
chulspro,
cliffamzn,
CodeChronos928,
Damian-Nordic,
dhrishi,
electrocucaracha,
franck-apple,
gjc13,
harimau-qirex,
harsha-rajendran,
hawk248,
jelderton,
jepenven-silabs,
jmartinez-silabs,
jmeg-sfy,
joonhaengHeo,
jtung-apple,
kcoppock,
kkasperczyk-no,
ksperling-apple and
lazarkov
pullapprove
bot
requested review from
selissia,
sharadb-amazon,
tecimovic,
tehampson,
turon,
vijs,
vivien-apple,
woody-apple,
xylophone21,
younghak-hwang and
yufengwangca
chrisdecenzo
commented
Jun 15, 2023
| @@ -139,6 +139,7 @@ CHIP_ERROR CastingServer::OpenBasicCommissioningWindow(std::function<void(CHIP_E | |||
| mOnConnectionSuccessClientCallback = onConnectionSuccess; | |||
| mOnConnectionFailureClientCallback = onConnectionFailure; | |||
| mOnNewOrUpdatedEndpoint = onNewOrUpdatedEndpoint; | |||
| ChipLogError(AppServer, "-----------------------CastingServer PrepareForCommissioning"); | |||
|
PR #27279: Size comparison from eef9529 to 09dc61b Increases (10 builds for bl602, cc32xx, cyw30739, esp32, nrfconnect, telink)
Decreases (8 builds for bl702, psoc6, telink)
Full report (46 builds for bl602, bl702, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
andy31415
requested changes
Jun 15, 2023
| return CHIP_NO_ERROR; | ||
| } | ||
|
|
||
| class JsonParser2 |
There was a problem hiding this comment.
Should this be in some namespace of chip?
| * @namespace chip::TLV::Json | ||
| * | ||
| * @brief | ||
| * This namespace includes types and interfaces for json conversion of CHIP TLV. |
There was a problem hiding this comment.
these comments do not seem to help too much ... namespace re-states the namespace name and it seems obvious that chip::TLV::Json describes a json interface for TLV
Can we add more useful commets (e.g. explaining what the json format is with examples and explaining what a context is and such)
| void * mContext; | ||
| }; | ||
|
|
||
| extern CHIP_ERROR WriteJSON(const TLVReader & aReader, Encoding::BufferWriter & bWriter); |
There was a problem hiding this comment.
we probably do not need "extern"
| * buffer. On failure, the value of *octetCount is undefined. | ||
| */ | ||
| template <typename F> | ||
| CHIP_ERROR ConvertHexToBytes(chip::CharSpan hex, F bufferAllocator, size_t * octetCount) |
There was a problem hiding this comment.
this does not seem TLV-specific so should probably not be in JSONTlv.h
We have some lib/support/BytesToHex ... maybe we want HexToBytes
There was a problem hiding this comment.
We have HexToBytes too, inside the BytesToHex header.
There was a problem hiding this comment.
Well.... that is confusing ... maybe we can find a better name or split headers.
| class JsonTLVParser | ||
| { | ||
| public: | ||
| static CHIP_ERROR Put(chip::TLV::TLVWriter * writer, chip::TLV::Tag tag, Json::Value & value) |
There was a problem hiding this comment.
This looks like complex code in a header. Should probably be in a C++ file.
Also please add unit tests.
andy31415
reviewed
Jun 15, 2023
| chip::TLV::TLVWriter writer; | ||
| chip::TLV::TLVReader reader; | ||
|
|
||
| uint8_t buf[74]; |
There was a problem hiding this comment.
Need to explain the sizes here ...74 seems a very particular constant.
andy31415
reviewed
Jun 15, 2023
| return CHIP_NO_ERROR; | ||
| } | ||
|
|
||
| CHIP_ERROR RunJsonTests() |
There was a problem hiding this comment.
These tests should be unit tests run during test execution, not a shell command (we can include it in the casting app as well, but a variant of unit tests that are stand alone should exist).
andy31415
reviewed
Jun 15, 2023
| CHIP_ERROR RunJsonTests() | ||
| { | ||
| char json[] = "{" | ||
| "\"0x0\" : { \"tlvType\":\"0x00\", \"value\": 7 }," |
There was a problem hiding this comment.
may want some NULL tests as well.
|
@chrisdecenzo I moved your PR to draft since the title said so. |
|
|
||
| if (TLVTypeIsContainer(type)) | ||
| { | ||
| if (tagControl == TLVTagControl::ContextSpecific) |
There was a problem hiding this comment.
What about profile tags in TLV lists?
| } | ||
| else | ||
| { | ||
| if (tagControl == TLVTagControl::ContextSpecific) |
There was a problem hiding this comment.
Again, this does not seem to handle profile tags right. But also, shouldn't that "tag" : prefix just be written out independently of whether the tag is a container or not?
| case kTLVType_ByteString: | ||
| err = temp.GetDataPtr(strbuf); | ||
| VerifyOrExit(err == CHIP_NO_ERROR, FormattedWrite(aWriter, "Error in kTLVType_ByteString")); | ||
| FormattedWrite(aWriter, "hex:"); |
There was a problem hiding this comment.
I don't think this produces valid JSON, unless I am missing something.....
| uint64_t uVal; | ||
| err = temp.Get(uVal); | ||
| VerifyOrExit(err == CHIP_NO_ERROR, FormattedWrite(aWriter, "Error in kTLVType_UnsignedInteger")); | ||
| FormattedWrite(aWriter, "%" PRIu64, uVal); |
There was a problem hiding this comment.
We're OK with the data corruptions for values > 2^53?
| * buffer. On failure, the value of *octetCount is undefined. | ||
| */ | ||
| template <typename F> | ||
| CHIP_ERROR ConvertHexToBytes(chip::CharSpan hex, F bufferAllocator, size_t * octetCount) |
There was a problem hiding this comment.
We have HexToBytes too, inside the BytesToHex header.
| { | ||
| public: | ||
| // Returns whether the parse succeeded. | ||
| static bool ParseComplexArgument(const char * label, const char * json, Json::Value & value) |
There was a problem hiding this comment.
This seems to be duplicating the chip-tool complex argument parser, right? Is it different from that code? If so, maybe it should not be called ParseComplexArgument... If it's not different, why is it separate?
| constexpr const char kHexNumPrefix[] = "0x"; | ||
| constexpr size_t kHexNumPrefixLen = ArraySize(kHexNumPrefix) - 1; | ||
|
|
||
| Json::Value objTlvType = value.get("tlvType", "0x0c"); |
There was a problem hiding this comment.
Please document why the defaulting to 0x0c.
|
@chrisdecenzo we are preparing one pull request that provide the generic TLV-JSON-TLV converter using C++ along with documentation, similar to the kotlin one we provided in #26527 |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
|
This stale pull request has been automatically closed. Thank you for your contributions. |
|
This seems in draft for over 1 year. @chrisdecenzo can we close it? |
|
Closing very old PR in draft form. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
(for message serialization between android tv-app platform and content apps)
TODO: