MsgPack bin8/bin16/bin32 support #2078
Conversation
|
Hi @Sanae6, Thank you for this excellent PR! Before starting the review, I'd like you to remove Best regards, |
|
I removed LinkedBinaryValue and renamed OwnedBinaryValue to BinaryValue. Not sure what to name Binary, but I left it as something sensible enough. |
| REQUIRE(reinterpret_cast<const char*>(binary.data())[0] == 5); | ||
| } | ||
|
|
||
| #if ARDUINOJSON_STRING_LENGTH_SIZE >= 4 |
There was a problem hiding this comment.
Please move this test to a dedicated file in MixedConfiguration, so you can control the value of ARDUINOJSON_STRING_LENGTH_SIZE
There was a problem hiding this comment.
This test currently fails. It seems that even when I apply the string length size, it doesn't actually apply to StringNode::length_type. Printing StringNode::maxLength in the StringNode::create function returned 0xffff instead of 0xffffffff. Any advice on how to solve that?
There was a problem hiding this comment.
This is surely due to the fact that the namespace doesn't include the value of ARDUINOJSON_STRING_LENGTH_SIZE.
Let me fix that.
Each file in this folder is named according to the setting name, so this file should be named string_length_size_4.cpp.
Anyway, thank you again for this excellent work 👍
There was a problem hiding this comment.
Alright! I'll leave it to you :)
Signed-off-by: Aubrey (Sanae) <aubrey@hall.ly>
Signed-off-by: Aubrey (Sanae) <aubrey@hall.ly>
Signed-off-by: Aubrey (Sanae) <aubrey@hall.ly>
|
Alright! Force pushed, seems nothing is broken 👍 |
|
It seems that you lost your version of |
Signed-off-by: Aubrey (Sanae) <aubrey@hall.ly>
|
Made the changes you wanted, testing MsgPackBinary comparison actually revealed my initial implementation didn't work, so I fixed that as well. |
Signed-off-by: Aubrey (Sanae) <aubrey@hall.ly>
| writeByte(0xC5); | ||
| writeInteger(uint16_t(value.size())); | ||
| } else { | ||
| writeByte(0xC6); |
There was a problem hiding this comment.
This line is not covered by the tests.
Please add a test in string_length_size_4.cpp
| return MsgPackBinary(content_.asOwnedString->data, | ||
| content_.asOwnedString->length); | ||
| default: | ||
| return MsgPackBinary(nullptr, 0); |
There was a problem hiding this comment.
This line is not covered by the tests.
| static void setBinary(VariantData* var, MsgPackBinary value, | ||
| ResourceManager* resources) { | ||
| if (!var) | ||
| return; |
There was a problem hiding this comment.
This line is not covered by the tests.
|
I added the three missing tests, squashed, and merged. Thank you again for this excellent contribution ❤️ |
|
This feature is available in ArduinoJson 7.1.0 |
I've added binary support to the msgpack deserializer and serializer, along with a fallback for the json serializer. I've also added some tests to the relevant files. Let me know if there's anything I can do to make this implementation better. Thank you for the amazing library :)
This would fix and close #922.