CM-386 / Reduce payload size

[aentinger]

The stated aim of CM-386 is to reduce the payload size by exchanging the string keys used to identify names, data types, ... by integer numbers by using the table found at RFC8428.

Example: [{"n": "test", "vs": "test"}] becomes [{0: "test", 3: "test"}].

This has been successfully implemented when for the encode method (data is sent from device to server) and verified via unit tests.

Difficulties arise when trying to implement the same functionality for the method decode (data is sent from server to the device). This is due to the fact that the function used for retrieve the value for a given key cbor_value_map_find_value only except string keys and unfortunately no integer keys as we would need.

The consequence of this fact would be to perform a major rewrite of method ArduinoCloudThing::decode in order to use less of the higher-level CBOR functions like cbor_value_map_find_value and implement the functionality using lower level CBOR functions.

I'd like to have input wether to live with the fact that the compression currently only works for encoding or if a rewrite of the decode function should be undertaken.

Last but nut least: There has been a lingering bug when half float values were sent from the server due to the nature of the implementation of the cbor_value_get_half_float function and the ArduinoCloudThing's library usage:

CborError cbor_value_get_half_float(const CborValue *value, void *result)
{
    uint16_t v;
    cbor_assert(cbor_value_is_half_float(value));

    /* size has been computed already */
    v = get16(value->ptr + 1);
    memcpy(result, &v, sizeof(v));
    return CborNoError;
}

and

float val;
cbor_value_get_half_float(&propValue, &val);

As one can see memcpy-ing a 16-Bit unsigned int to a float can simply not work.

Code for properly converting from the CBOR half float contained in the uint16_t can be found on page #50 of RFC7049 and is fairly esoteric.

#include <math.h>

double decode_half(unsigned char *halfp) {
  int half = (halfp[0] << 8) + halfp[1];
  int exp = (half >> 10) & 0x1f;
  int mant = half & 0x3ff;
  double val;
  if (exp == 0) val = ldexp(mant, -24);
  else if (exp != 31) val = ldexp(mant + 1024, exp - 25);
  else val = mant == 0 ? INFINITY : NAN;
  return half & 0x8000 ? -val : val;
}

The bugfix has been implemented and tested.

[facchinm]

The PR looks convincing, of course it can't be merged until both decode and encode won't use the same format; one of my earliest proposals was to make the tag usage optional (via a runtime switch) so the backend can be adapted progressively so we could test tag encoding only for boards with low bitrate like Lora.
@mattiabertorello @mastrolinux

[aentinger]

Making the tag usage configurable at runtime (or maybe dependent on a compile time switch, aka #ifdef LORA_BOARD) would of course smooth out the transition. I very much agree that the decode method should work the same way as the encode method so I shall proceed to work on the former.

[aentinger]

Rewrite complete - according to the unit tests decode/encode should work for both old and new protocol version.

[mattiabertorello]

I test this PR with the onudra cli and It almost working:
If I use this CBOR 81a200647465737402fb40aae60000000000 [{0: "test", 2: 3443.0}] It works
But if I use this one with base name and base time It NOT works 81a421782d75726e3a757569643a32383432616461362d633264352d346530342d613132342d66376330336232353834656522fb41d714742200000000647465737402fb4026000000000000 [{-2: "urn:uuid:2842ada6-c2d5-4e04-a124-f7c03b2584ee", -3: 1548865672.0, 0: "test", 2: 11.0}]

81a322fb41d714757280000000647465737402fb4041800000000000 Only with base time

The base name could be removed but the base time and also the tag time is mandatory

[aentinger]

The old parser "ate" such CBOR data sets but it did not extract neither BaseName nor BaseTime (and also did not use those parameters in the firmware code). Therefore I'm wondering: Are those entries necessary? If yes, than this was a hidden requirement because the code showed no such thing. If I should include parsing capability for more CBOR tags than Name, Value, StringValue, BooleanValue please let me know which CBOR tags should be parsed by ArduinoCloudThing::decode.

[umbynos]

Yes, they are necessary. The CBOR tags that should be added are BaseName (which can be optional) (-2), BaseTime (-3) and Time (6).

[mattiabertorello]

The BaseTime and the Time tag will be use in the feature device-shadow, in any case the decode must be follow the Robustness principle
Be conservative in what you send, be liberal in what you accept

[aentinger]

Okay - that is certainly possible. I've got one more question though - do we need the support for BaseName, BaseTime, Time right now or can we merge this pull request now and implement the additional features in another branch? Basically it depends if those tags are sent from the cloud to the device.

[umbynos]

Yes, we can merge, but before it's mandatory to ignore the tags not yet supported.

[aentinger]

In this case I think the better approach is to do the full implementation. Will update when done.

[aentinger]

Ready to merge ;)