Improve error checking in the command encoder.

There are actually two changes here:

1) Improve the error checking so that we correctly check that the
   buffer actually has enough space when writing a multibyte value.

2) Introduce a way to ask the encoder how much space it needs to
   encode a given APS frame.

Fixes #1644

src/app/chip-zcl-zpro/command-encoder/chip-zcl-zpro-codec.h

@@ -76,6 +76,27 @@ uint16_t extractMessage(uint8_t * buffer, uint16_t buffer_length, uint8_t ** msg
  */
 void printApsFrame(EmberApsFrame * frame);
 
+/**
+ * @brief Encode an APS frame into the given buffer.  Returns the number of
+ * bytes of buffer used by the encoding or 0 if the given buffer is not big
+ * enough.  If buffer is null, no encoding will happen; the function will
+ * instead return the number of bytes that would be needed to encode the APS
+ * frame.
+ *
+ * @param[in] buffer The buffer to write to.  If null, the call is in "count the
+ *                   bytes" mode, and no writing will happen.
+ * @parem[in] buf_length The size of the buffer.  Ignored if buffer is null.
+ * @param[in] apsFrame The frame to encode.
+ *
+ * @return
+ *   - If buffer is null, the number of bytes needed to encode.
+ *   - If buffer is non-null but buf_length is not enough to hold the
+ *     EmberApsFrame, 0.
+ *   - If buffer us non-null and buf_length is large enough, the number of bytes
+ *     placed in buffer.
+ */
+uint16_t encodeApsFrame(uint8_t * buffer, uint16_t buf_length, EmberApsFrame * apsFrame);
+
 #ifdef __cplusplus
 }
 #endif

src/app/chip-zcl-zpro/command-encoder/encoder.c

@@ -16,98 +16,67 @@
  */
 
 #include "chip-zcl-zpro-codec.h"
+#include <assert.h>
 #include <stdio.h>
 #include <string.h>
 
-uint32_t encodeApsFrame(uint8_t * buffer, uint32_t buf_length, uint16_t profileID, uint16_t clusterId, uint8_t sourceEndpoint,
-                        uint8_t destinationEndpoint, EmberApsOption options, uint16_t groupId, uint8_t sequence, uint8_t radius)
+static uint16_t doEncodeApsFrame(uint8_t * buffer, uint32_t buf_length, uint16_t profileID, uint16_t clusterId,
+                                 uint8_t sourceEndpoint, uint8_t destinationEndpoint, EmberApsOption options, uint16_t groupId,
+                                 uint8_t sequence, uint8_t radius)
 {
-    uint32_t size = 0;
-    if (buffer == NULL || buf_length == 0)
-    {
-        return size;
-    }
     size_t nextOutByte = 0;
 
-    if (nextOutByte >= buf_length)
-    {
-        return size;
-    }
+#define TRY_WRITE(dataItem)                                                                                                        \
+    do                                                                                                                             \
+    {                                                                                                                              \
+        size_t neededSize = nextOutByte + sizeof(dataItem);                                                                        \
+        if (buffer)                                                                                                                \
+        {                                                                                                                          \
+            if (neededSize > buf_length)                                                                                           \
+            {                                                                                                                      \
+                return 0;                                                                                                          \
+            }                                                                                                                      \
+            memcpy(buffer + nextOutByte, &dataItem, sizeof(dataItem));                                                             \
+        }                                                                                                                          \
+        nextOutByte = neededSize;                                                                                                  \
+    } while (0) /* No semicolon so callers have to provide it. */
 
     // Simulated APS "frame control" byte.
-    buffer[nextOutByte] = 0;
-    ++nextOutByte;
-
-    if (nextOutByte >= buf_length)
-    {
-        return size;
-    }
-
-    memcpy(buffer + nextOutByte, &profileID, sizeof(uint16_t));
-    nextOutByte += sizeof(uint16_t);
-
-    if (nextOutByte >= buf_length)
-    {
-        return size;
-    }
-    memcpy(buffer + nextOutByte, &clusterId, sizeof(uint16_t));
-    nextOutByte += sizeof(uint16_t);
-
-    if (nextOutByte >= buf_length)
-    {
-        return size;
-    }
-    memcpy(buffer + nextOutByte, &sourceEndpoint, sizeof(uint8_t));
-    nextOutByte += sizeof(uint8_t);
-
-    if (nextOutByte >= buf_length)
-    {
-        return size;
-    }
-    memcpy(buffer + nextOutByte, &destinationEndpoint, sizeof(uint8_t));
-    nextOutByte += sizeof(uint8_t);
+    uint8_t controlByte = 0;
+    TRY_WRITE(controlByte);
+    TRY_WRITE(profileID);
+    TRY_WRITE(clusterId);
+    TRY_WRITE(sourceEndpoint);
+    TRY_WRITE(destinationEndpoint);
+    TRY_WRITE(options);
+    TRY_WRITE(groupId);
+    TRY_WRITE(sequence);
+    TRY_WRITE(radius);
 
-    if (nextOutByte >= buf_length)
-    {
-        return size;
-    }
-    memcpy(buffer + nextOutByte, &options, sizeof(EmberApsOption));
-    nextOutByte += sizeof(EmberApsOption);
+#undef TRY_WRITE
 
-    if (nextOutByte >= buf_length)
-    {
-        return size;
-    }
-    memcpy(buffer + nextOutByte, &groupId, sizeof(uint16_t));
-    nextOutByte += sizeof(uint16_t);
-
-    if (nextOutByte >= buf_length)
-    {
-        return size;
-    }
-    memcpy(buffer + nextOutByte, &sequence, sizeof(uint8_t));
-    nextOutByte += sizeof(uint8_t);
-
-    if (nextOutByte >= buf_length)
-    {
-        return size;
-    }
-    memcpy(buffer + nextOutByte, &radius, sizeof(uint8_t));
-    nextOutByte += sizeof(uint8_t);
+    assert(nextOutByte < UINT16_MAX);
 
     buf_length = nextOutByte;
     printf("Encoded %d bytes of aps frame\n", buf_length);
     return buf_length;
 }
 
+uint16_t encodeApsFrame(uint8_t * buffer, uint16_t buf_length, EmberApsFrame * apsFrame)
+{
+    return doEncodeApsFrame(buffer, buf_length, apsFrame->profileId, apsFrame->clusterId, apsFrame->sourceEndpoint,
+                            apsFrame->destinationEndpoint, apsFrame->options, apsFrame->groupId, apsFrame->sequence,
+                            apsFrame->radius);
+}
+
 uint32_t _encodeOnOffCommand(uint8_t * buffer, uint32_t buf_length, int command, uint8_t destination_endpoint)
 {
     uint32_t result = 0;
     // pick cluster id as 6 for now.
     // pick source and destination end points as 1 for now.
     // Profile is 65535 because that matches our simple generated code, but we
     // should sort out the profile situation.
-    result = encodeApsFrame(buffer, buf_length, 65535, 6, 1, destination_endpoint, 0, 0, 0, 0);
+    result = doEncodeApsFrame(buffer, buf_length, 65535, 6, 1, destination_endpoint, 0, 0, 0, 0);
     if (result == 0 || result > buf_length)
     {
         printf("Error encoding aps frame result %d", result);