msgpack-arduino icon indicating copy to clipboard operation
msgpack-arduino copied to clipboard

Implicit functions question

Open Sod-Almighty opened this issue 7 years ago • 2 comments

Hi, and thanks for making this library available.

I notice that writeMapSize(Stream&, const uint8_t) checks if the value exceeds 1 << 4, and if not, encodes it as a map4 value.

However, I note that the other writeMapSize overloads (uint16_t and uint32_t) don't perform this check. Why is this? Surely if the value can fit in a map4, then it should be encoded as such? Wouldn't that be more space-efficient?

Sod-Almighty avatar Nov 13 '18 10:11 Sod-Almighty

Interesting point. Thanks for continuing to use this code following your previous frustrations.

In my application - application size became the limiting factor since I was targeting ATMega328 (arduino nano/uno). I had to remove/add lines of code on each compile to get it to successfully upload to the device within the available program size. Therefore, I opted for compile-time checks of size where possible, and the pattern that you describe would need to be a runtime check (i.e. the code would need to be run on the microcontroller whenever the function was called to perform the size comparison).

An alternative might be to add a general compile time flag like MSGPACK_ENABLE_DYNAMIC_SIZE_CHECKS which added the extra lines of code in for people who want them, or alternatively MSGPACK_DISABLE_DYNAMIC_SIZE_CHECKS or MSGPACK_MINIMUM_PROGRAM_SIZE for applications where application size is more important.

elliotwoods avatar Nov 13 '18 13:11 elliotwoods

Yes, I figured out how to use it with the ESP6266 WifiUDP class, instead of trying to assemble a string beforehand. As you said, a string buffer would probably eat too much memory in any case.

You make a valid point regarding program size. I think compile-time flags make sense. In some cases, the size of the data payload may be important. For example, there is a hard limit on the size of a UDP packet, and it's pretty damn small.

stugol avatar Nov 13 '18 13:11 stugol