spinel: Clarify data packing notation and fix implementation. (#1433)

The data packing format that I designed for Spinel was a bit too
clever for its own good. While elegant from a certain perspective, the
behavior of the data blob (`D`), array (`A(...)`), and structure
(`T(...)`) types was way too confusing. I thought that I was (as the
guy who came up with it) immune from such confusion, but after taking
the time to go through all the code and address all of the mishaps in
the code it seems that the subtleties befuddled even its creator on a
time or two.

The specific issue that made me realize I needed to prioritize making
this fix was #1393.

As far as packing types go, this commit makes the following changes:

*   **New type `DATATYPE_DATA_WLEN` (`d`)**: Just like `DATATYPE_DATA`
    except that it ALWAYS prepends the length, irrespective of the
    type's location in the type string. Think of WLEN as being "With
    LENgth". Unless you are adding a property that has a type which is
    just a single data blob, you should use this type instead of
    `DATATYPE_DATA` (`D`). `DATATYPE_DATA` is still useful, so it will
    be sticking around.
*   **Changed type `DATATYPE_STRUCT` (was `T`, now `t`)**: The new
    struct type, ALWAYS prepends the length of the contents of the
    struct, just like `DATATYPE_DATA_WLEN`. The old `T` style struct
    still works and behaves the same way, but its use is now
    deprecated.
*   **Changed type `DATATYPE_ARRAY` (`A`)**: This type has changed in
    documentation only, as no one is currently using it in the way
    that it was originally specified. The change is that this type
    NEVER prepends the length of the entire array, and using it
    alongside other types is now unspecified (it should only be used
    alone).

I also added some convenient macros for building up datatype signature
strings that include structs.

One of the goals of these changes was to make this commit a change in
specification only, not to change what was currently on-the-wire.
However, after updating the spec and then going through and updating
all of the code to use the new nomenclature, I was horrified to find
that, due to the confusing nature of the previous datatype signature
string format, many properties that are arrays were not implemented
correctly --- making them impossible to update or add fields to
without breaking backward compatibility. To add insult to injury, some
of these instances were written by me!

This change updates all of the array properties to reflect how they
should have been implemented to begin with.

doc/spinel-protocol-src/spinel-commands.md

@@ -87,6 +87,13 @@ packed unsigned integer format described in (#packed-unsigned-integer), followed
 the value to be inserted. The exact format of the value is defined by
 the property.
 
+If the type signature of the property specified by `PROP_ID` consists
+of a single structure enclosed by an array (`A(t(...))`), then the
+contents of `VALUE` MUST contain the contents of the structure (`...`)
+rather than the serialization of the whole item (`t(...)`).  Specifically,
+the length of the structure MUST NOT be prepended to `VALUE`. This
+helps to eliminate redundant data.
+
 If an error occurs, the value of `PROP_LAST_STATUS` will be emitted
 with the value set to the generated status code for the error.
 
@@ -110,6 +117,13 @@ packed unsigned integer format described in (#packed-unsigned-integer), followed
 the value to be removed. The exact format of the value is defined by
 the property.
 
+If the type signature of the property specified by `PROP_ID` consists
+of a single structure enclosed by an array (`A(t(...))`), then the
+contents of `VALUE` MUST contain the contents of the structure (`...`)
+rather than the serialization of the whole item (`t(...)`).  Specifically,
+the length of the structure MUST NOT be prepended to `VALUE`. This
+helps to eliminate redundant data.
+
 If an error occurs, the value of `PROP_LAST_STATUS` will be emitted
 with the value set to the generated status code for the error.
 
@@ -146,6 +160,13 @@ The payload for this command is the property identifier encoded in the
 packed unsigned integer format described in (#packed-unsigned-integer), followed by
 the value that was inserted into the given property.
 
+If the type signature of the property specified by `PROP_ID` consists
+of a single structure enclosed by an array (`A(t(...))`), then the
+contents of `VALUE` MUST contain the contents of the structure (`...`)
+rather than the serialization of the whole item (`t(...)`).  Specifically,
+the length of the structure MUST NOT be prepended to `VALUE`. This
+helps to eliminate redundant data.
+
 The resulting order of items in the list is defined by the given
 property.
 
@@ -166,6 +187,13 @@ The payload for this command is the property identifier encoded in the
 packed unsigned integer format described in (#packed-unsigned-integer), followed by
 the value that was removed from the given property.
 
+If the type signature of the property specified by `PROP_ID` consists
+of a single structure enclosed by an array (`A(t(...))`), then the
+contents of `VALUE` MUST contain the contents of the structure (`...`)
+rather than the serialization of the whole item (`t(...)`).  Specifically,
+the length of the structure MUST NOT be prepended to `VALUE`. This
+helps to eliminate redundant data.
+
 The resulting order of items in the list is defined by the given
 property.
 
@@ -240,7 +268,7 @@ individual property with `STATUS_INVALID_COMMAND_FOR_PROP`.
 
 ## CMD 22: (Host->NCP) CMD_PROP_VALUE_MULTI_SET {#prop-value-multi-set}
 
-*   Argument-Encoding: `A(T(iD))`
+*   Argument-Encoding: `A(iD)`
 *   Required Capability: `CAP_CMD_MULTI`
 
 Octets: |    1   |          1               |   *n*
@@ -273,7 +301,7 @@ individual property with `STATUS_INVALID_COMMAND_FOR_PROP`.
 
 ## CMD 23: (NCP->Host) CMD_PROP_VALUES_ARE {#prop-values-are}
 
-*   Argument-Encoding: `A(T(iD))`
+*   Argument-Encoding: `A(iD)`
 *   Required Capability: `CAP_CMD_MULTI`
 
 Octets: |    1   |          1          |   *n*

doc/spinel-protocol-src/spinel-data-packing.md

@@ -4,6 +4,12 @@ Data serialization for properties is performed using a light-weight
 data packing format which was loosely inspired by D-Bus. The format of
 a serialization is defined by a specially formatted string.
 
+This packing format is used for notational convenience. While this
+string-based datatype format has been designed so that the strings may
+be directly used by a structured data parser, such a thing is not
+required to implement Spinel. Indeed, higly constrained applications
+may find such a thing to be too heavyweight.
+
 Goals:
 
  *  Be lightweight and favor direct representation of values.
@@ -19,7 +25,7 @@ example:
  *  `C`: A single unsigned byte.
  *  `C6U`: A single unsigned byte, followed by a 128-bit IPv6
     address, followed by a zero-terminated UTF8 string.
- *  `A(6)`: An array of IPv6 addresses
+ *  `A(6)`: An array of concatenated IPv6 addresses
 
 In each case, the data is represented exactly as described. For
 example, an array of 10 IPv6 address is stored as 160 bytes.
@@ -40,10 +46,11 @@ Char | Name                | Description
  `6` | DATATYPE_IPv6ADDR    | IPv6 Address. (Big-endian)
  `E` | DATATYPE_EUI64       | EUI-64 Address. (Big-endian)
  `e` | DATATYPE_EUI48       | EUI-48 Address. (Big-endian)
- `D` | DATATYPE_DATA        | Arbitrary Data. See (#data-blobs).
+ `D` | DATATYPE_DATA        | Arbitrary data. See (#data-blobs).
+ `d` | DATATYPE_DATA_WLEN   | Arbitrary data with prepended length. See (#data-blobs).
  `U` | DATATYPE_UTF8        | Zero-terminated UTF8-encoded string.
- `T` | DATATYPE_STRUCT      | Structured datatype. Compound type. See (#structured-data).
- `A` | DATATYPE_ARRAY       | Array of datatypes. Compound type. See (#arrays).
+ `t(...)` | DATATYPE_STRUCT | Structured datatype with prepended length. See (#structured-data).
+ `A(...)` | DATATYPE_ARRAY  | Array of datatypes. Compound type. See (#arrays).
 
 All multi-byte values are little-endian unless explicitly stated
 otherwise.
@@ -53,7 +60,7 @@ otherwise.
 For certain types of integers, such command or property identifiers,
 usually have a value on the wire that is less than 127. However, in
 order to not preclude the use of values larger than 255, we would need
-to add an extra byte. Doing this would add an extra byte to the vast
+to add an extra byte. Doing this would add an extra byte to the
 majority of instances, which can add up in terms of bandwidth.
 
 The packed unsigned integer format is based on the [unsigned integer
@@ -88,15 +95,24 @@ octet with the most significant bit clear.
 
 ## Data Blobs
 
-Data blobs are special datatypes in that the data that they contain
-does not inherently define the size of the data. This means that if
-the length of the data blob isn't *implied*, then the length of the
-blob must be prepended as a packed unsigned integer.
+There are two types for data blobs: `d` and `D`.
 
-The length of a data blob is *implied* only when it is the last
-datatype in a given buffer. This works because we already know the
-size of the buffer, and the length of the data is simply the rest of
-the size of the buffer.
+*   `d` has the length of the data (in bytes) prepended to the data
+    (with the length encoded as type `S`). The size of the length
+    field is not included in the length.
+*   `D` does not have a prepended length: the length of the data is
+    implied by the bytes remaining to be parsed. It is an error for
+    `D` to not be the last type in a type in a type signature.
+
+This dichotomy allows for more efficient encoding by eliminating
+redundency. If the rest of the buffer is a data blob, encoding the
+length would be redundant because we already know how many bytes are
+in the rest of the buffer.
+
+In some cases we use `d` even if it is the last field in a type signature.
+We do this to allow for us to be able to append additional fields
+to the type signature if necessary in the future. This is usually the
+case with embedded structs, like in the scan results.
 
 For example, let's say we have a buffer that is encoded with the
 datatype signature of `CLLD`. In this case, it is pretty easy to tell
@@ -104,65 +120,48 @@ where the start and end of the data blob is: the start is 9 bytes from
 the start of the buffer, and its length is the length of the buffer
 minus 9. (9 is the number of bytes taken up by a byte and two longs)
 
-However, things are a little different with `CLDL`. Since our data
-blob is no longer the last item in the signature, the length must be
-prepended.
-
-If you are a little confused, keep reading. This theme comes up in a a
-few different ways in the following sections.
-
-When a length is prepended, the length is encoded as a little-endian
-unsigned 16-bit integer.
-
-A> Originally the length was a [PUI](#packed-unsigned-integer), but
-A> it was changed to an unsigned 16-bit integer in order to help
-A> reduce protocol requirements.
-A> <!-- RQ -- Should we consider moving back to using a PUI here? -->
+The datatype signature `CLLDU` is illegal because we can't determine
+where the last field (a zero-terminated UTF8 string) starts. But the
+datatype `CLLdU` *is* legal, because the parser can determine the
+exact length of the data blob—allowing it to know where the start
+of the next field would be.
 
 ## Structured Data
 
-The structured data type is a way of bundling together a bunch of data
-into a single data structure. This may at first seem useless. What is
-the difference between `T(Cii)` and just `Cii`? The answer is, in that
-particular case, nothing: they are stored in exactly the same way.
+The structure data type (`t(...)`) is a way of bundling together
+several fields into a single structure. It can be thought of as a
+`d` type except that instead of being opaque, the fields in the
+content are known. This is useful for things like scan results where
+you have substructures which are defined by different layers.
 
-However, one case where the structure datatype makes a difference is
-when you compare `T(Cii)L` to `CiiL`: they end up being represented
-entirely differently. This is because the structured data type follows
-the exact same semantics as the data blob type: if it isn't the last
-datatype in a signature, *it must be prepended with a length*. This is
-useful because it allows for new datatypes to be appended to the
-structure's signature while remaining *backward parsing
-compatibility*.
+For example, consider the type signature `Lt(ES)t(6C)`. In this
+hypothetical case, the first struct is defined by the MAC layer, and
+the second struct is defined by the PHY layer. Because of the use of
+structures, we know exactly what part comes from that layer.
+Additionally, we can add fields to each structure without introducing
+backward compatability problems: Data encoded as `Lt(ESU)t(6C)` (Notice
+the extra `U`) will
+decode just fine as `Lt(ES)t(6C)`. Additionally, if we don't care
+about the MAC layer and only care about the network layer, we could
+parse as `Lt()t(6C)`.
 
-More explicitly, if you take data that was encoded with `T(Cii6)L`,
-you can still decode it as `T(Cii)L`.
-
-Let's take, for example, the property `PROP_IPv6_ADDR_TABLE`.
-Conceptually it is just a list of IPv6 addresses, so we can encode it
-as `A(6c)`. However, if we ever want to associate more data with the
-type (like flags), we break our backward compatibility if we add
-another member and use `A(6cC)`. To allow for data to be added without
-breaking backward compatibility, we use the structured data type from
-the start: `A(T(6c))`. Then when we add a new member to the structure
-(`A(T(6cC))`), we don't break backward compatibility.
-
-It's also worth noting that `T(Cii)L` also parses as `DL`. You could
-then take the resultant data blob and parse it as `Cii`.
-
-When a length is prepended, the length is encoded as a little-endian
-unsigned 16-bit integer.
+Note that data encoded as `Lt(ES)t(6C)` will also parse as `Ldd`,
+with the structures from both layers now being opaque data blobs.
 
 ## Arrays
 
 An array is simply a concatenated set of *n* data encodings. For example,
 the type `A(6)` is simply a list of IPv6 addresses---one after the other.
+The type `A(6E)` likewise a concatenation of IPv6-address/EUI-64 pairs.
 
-Just like the data blob type and the structured data type, the length
-of the entire array must be prepended *unless* the array is the last
-type in a given signature. Thus, `A(C)` (An array of unsigned bytes)
-encodes identically to `D`.
+If an array contains many fields, the fields will often be surrounded
+by a structure (`t(...)`). This effectively prepends each item in the
+array with its length. This is useful for improving parsing performance
+or to allow additional fields to be added in the future in a backward
+compatible way. If there is a high certainty that additional
+fields will never be added, the struct may be omitted (saving two bytes
+per item).
 
-When a length is prepended, the length is encoded as a little-endian
-unsigned 16-bit integer.
+This specification does not define a way to embed an array as a field
+alongside other fields.
 

doc/spinel-protocol-src/spinel-feature-gpio.md

@@ -11,7 +11,7 @@ Support for this feature can be determined by the presence of `CAP_GPIO`.
 
 ### PROP 4096: PROP\_GPIO\_CONFIG ###
 
-*   Argument-Encoding: `A(CCU)`
+*   Argument-Encoding: `A(t(CCU))`
 *   Type: Read-write (Writable only using `CMD_PROP_VALUE_INSERT`,
     (#prop-value-insert))
 

doc/spinel-protocol-src/spinel-prop-core.md

@@ -262,7 +262,7 @@ commands for this property from the NCP.
 ### PROP 113: PROP_STREAM_RAW {#prop-stream-raw}
 
 * Type: Read-Write-Stream
-* Packed-Encoding: `DD`
+* Packed-Encoding: `dD`
 
 Octets: |        2       |     *n*    |       *n*
 --------|----------------|------------|----------------
@@ -303,8 +303,8 @@ The frame metadata field consists of the following fields:
 MD_POWER | (dBm) RSSI/TX-Power          | `c` int8   | 1     | -128
 MD_NOISE | (dBm) Noise floor            | `c` int8   | 1     | -128
 MD_FLAG  | Flags (defined below)        | `S` uint16 | 2     |
-MD_PHY   | PHY-specific data            | `D` data   | >=2   |
-MD_VEND  | Vendor-specific data         | `D` data   | >=2   |
+MD_PHY   | PHY-specific data            | `d` data   | >=2   |
+MD_VEND  | Vendor-specific data         | `d` data   | >=2   |
 
 The following fields are ignored by the NCP for packets sent to it from
 the host:
@@ -333,7 +333,7 @@ pertainent information.
 ### PROP 114: PROP_STREAM_NET {#prop-stream-net}
 
 * Type: Read-Write-Stream
-* Packed-Encoding: `DD`
+* Packed-Encoding: `dD`
 
 Octets: |        2       |     *n*    |       *n*
 --------|----------------|------------|----------------
@@ -356,7 +356,7 @@ format of which is described in (#frame-metadata-format).
 ### PROP 114: PROP_STREAM_NET_INSECURE {#prop-stream-net-insecure}
 
 * Type: Read-Write-Stream
-* Packed-Encoding: `DD`
+* Packed-Encoding: `dD`
 
 Octets: |        2       |     *n*    |       *n*
 --------|----------------|------------|----------------

doc/spinel-protocol-src/spinel-prop-ipv6.md

@@ -20,7 +20,7 @@ IPv6 Prefix + Prefix Length
 
 ### PROP 99: PROP_IPV6_ADDRESS_TABLE {#prop-ipv6-address-table}
 * Type: Read-Write
-* Packed-Encoding: `A(T(6CLLC))`
+* Packed-Encoding: `A(t(6CLLC))`
 
 Array of structures containing:
 

doc/spinel-protocol-src/spinel-prop-mac.md

@@ -33,7 +33,7 @@ Values switches to `SCAN_STATE_IDLE` when scan is complete.
 
 ### PROP 51: PROP_MAC_SCAN_BEACON {#prop-mac-scan-beacon}
 * Type: Read-Only-Stream
-* Packed-Encoding: `CcDD.` (or `CcT(ESSc.)T(iCUD.).`)
+* Packed-Encoding: `Ccdd` (or `Cct(ESSc)t(iCUd)`)
 
 Octets: | 1  |   1  |    2    |   *n*    |    2    |   *n*  
 --------|----|------|---------|----------|---------|----------  
@@ -46,16 +46,16 @@ The format below is for an 802.15.4 MAC with Thread:
 
 * `C`: Channel
 * `c`: RSSI of the beacon
-* `T`: MAC layer properties
+* `t`: MAC layer properties (802.15.4 layer shown below for convenience)
   * `E`: Long address
   * `S`: Short address
   * `S`: PAN-ID
   * `c`: LQI
-* `T`: NET layer properties
+* NET layer properties (Standard net layer shown below for convenience)
   * `i`: Protocol Number
   * `C`: Flags
   * `U`: Network Name
-  * `D`: XPANID
+  * `d`: XPANID
 
 Extra parameters may be added to each of the structures
 in the future, so care should be taken to read the length

doc/spinel-protocol-src/spinel-tech-thread.md

@@ -39,7 +39,7 @@ The long address and short address of the parent of this node.
 
 ### PROP 82: PROP_THREAD_CHILD_TABLE
 * Type: Read-Only
-* Packed-Encoding: `A(T(ES))`
+* Packed-Encoding: `A(t(ES))`
 
 Table containing the long and short addresses of all
 the children of this node.
@@ -84,7 +84,7 @@ The local stable network data.
 
 ### PROP 90: PROP_THREAD_ON_MESH_NETS
 * Type: Read-Write
-* Packed-Encoding: `A(T(6CbCb))`
+* Packed-Encoding: `A(t(6CbCb))`
 
 Data per item is:
 
@@ -98,7 +98,7 @@ Data per item is:
 
 ### PROP 91: PROP_THREAD_LOCAL_ROUTES
 * Type: Read-Write
-* Packed-Encoding: `A(T(6CbC))`
+* Packed-Encoding: `A(t(6CbC))`
 
 Data per item is:
 
@@ -200,7 +200,7 @@ disabled.
 
 ### PROP 5387: PROP_THREAD_NEIGHBOR_TABLE
 * Type: Read-Only
-* Packed-Encoding: `A(T(ESLCcCbLL))`
+* Packed-Encoding: `A(t(ESLCcCbLL))`
 
 Data per item is:
 

doc/spinel-protocol-src/spinel-test-vectors.md

@@ -45,7 +45,7 @@ Frame:
 * TID: 0
 * CMD: 7 (`CMD_VALUE_INSERTED`)
 * PROP: 51 (`PROP_MAC_SCAN_BEACON`)
-* VALUE: Structure, encoded as `CcT(ESSc.)T(iCUD.).`
+* VALUE: Structure, encoded as `Cct(ESSc)t(iCUd)`
     * CHAN: 15
     * RSSI: -60dBm
     * MAC_DATA: (0D 00 B6 40 D4 8C E9 38 F9 52 FF FF D2 04 00)
@@ -94,7 +94,7 @@ Frame:
 * TID: 4
 * CMD: 6 (`CMD_VALUE_IS`)
 * PROP: 90 (`PROP_THREAD_ON_MESH_NETS`)
-* VALUE: Array of structures, encoded as `A(T(6CbC))`
+* VALUE: Array of structures, encoded as `A(t(6CbC))`
 
 IPv6 Prefix  | Prefix Length | Stable Flag | Other Flags
 -------------|---------------|-------------|--------------