-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright! I'll leave it to you :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just pushed the changes to fix this issue with string_length_size_4.cpp
.
Please rebase your branch onto 7.x
.
Note that I created string_length_size_{1,2,4}.cpp
to check that the fix actually works, so you'll have a conflict on string_length_size_4.cpp
.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really good.
I'll do an in-depth review tomorrow.
Thank you for this outstanding PR.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.