Allocate ISO-TP message buffer on the stack.

6 files changed, 36 insertions, 22 deletions

diff --git a/src/isotp/isotp.c b/src/isotp/isotp.c
index 1605d435..67c644f1 100644
--- a/src/isotp/isotp.c
+++ b/src/isotp/isotp.c
@@ -2,8 +2,6 @@
 #include <isotp/receive.h>
 #include <bitfield/bitfield.h>
 
-const uint16_t MAX_ISO_TP_MESSAGE_SIZE = 4096;
-const uint16_t MAX_CAN_FRAME_SIZE = 8;
 const uint8_t ISO_TP_DEFAULT_RESPONSE_TIMEOUT = 100;
 const bool ISO_TP_DEFAULT_FRAME_PADDING_STATUS = true;
 
@@ -24,10 +22,11 @@ IsoTpShims isotp_init_shims(LogShim log, SendCanMessageShim send_can_message,
 void isotp_message_to_string(const IsoTpMessage* message, char* destination,
         size_t destination_length) {
     char payload_string[message->size * 2 + 1];
+    memset(payload_string, 0, sizeof(payload_string));
     for(int i = 0; i < message->size; i++) {
         // TODO, bah this isn't working because snprintf hits the NULL char that
         // it wrote the last time and stops cold
-        snprintf(&payload_string[i * 2], 2, "%02x", message->payload[i]);
+        /* snprintf(&payload_string[i * 2], 2, "%02x", message->payload[i]); */
     }
     snprintf(destination, destination_length, "ID: 0x%02x, Payload: 0x%s",
             message->arbitration_id, payload_string);
@@ -38,7 +37,9 @@ IsoTpMessage isotp_receive_can_frame(IsoTpShims* shims, IsoTpHandle* handle,
         const uint8_t data_length) {
     IsoTpMessage message = {
         arbitration_id: arbitration_id,
-        completed: false
+        completed: false,
+        payload: {0},
+        size: 0
     };
 
     if(data_length < 1) {
@@ -47,10 +48,18 @@ IsoTpMessage isotp_receive_can_frame(IsoTpShims* shims, IsoTpHandle* handle,
 
     if(handle->type == ISOTP_HANDLE_RECEIVING) {
         if(handle->receive_handle.arbitration_id != arbitration_id) {
+            if(shims->log != NULL)  {
+                shims->log("The arb ID 0x%x doesn't match the expected rx ID 0x%x",
+                        arbitration_id, handle->receive_handle.arbitration_id);
+            }
             return message;
         }
     } else if(handle->type == ISOTP_HANDLE_SENDING) {
         if(handle->send_handle.receiving_arbitration_id != arbitration_id) {
+            if(shims->log != NULL) {
+                shims->log("The arb ID 0x%x doesn't match the expected tx continuation ID 0x%x",
+                        arbitration_id, handle->send_handle.receiving_arbitration_id);
+            }
             return message;
         }
     } else {
@@ -73,7 +82,9 @@ IsoTpMessage isotp_receive_can_frame(IsoTpShims* shims, IsoTpHandle* handle,
 
     switch(pci) {
         case PCI_SINGLE: {
-            message.payload = payload;
+            if(payload_length > 0) {
+                memcpy(message.payload, payload, payload_length);
+            }
             message.size = payload_length;
             message.completed = true;
             handle->success = true;
diff --git a/src/isotp/isotp.h b/src/isotp/isotp.h
index c0db0178..adf0f247 100644
--- a/src/isotp/isotp.h
+++ b/src/isotp/isotp.h
@@ -6,19 +6,22 @@
 #include <stdio.h>
 
 #define CAN_MESSAGE_BYTE_SIZE 8
+#define MAX_ISO_TP_MESSAGE_SIZE 4096
+// TODO we want to avoid malloc, and we can't be allocated 4K on the stack for
+// each IsoTpMessage, so for now we're setting an artificial max message size
+// here - since we only handle single frame messages, 8 bytes is plenty.
+#define OUR_MAX_ISO_TP_MESSAGE_SIZE 8
 
 #ifdef __cplusplus
 extern "C" {
 #endif
 
-const uint16_t MAX_ISO_TP_MESSAGE_SIZE;
-const uint16_t MAX_CAN_FRAME_SIZE;
 const uint8_t ISO_TP_DEFAULT_RESPONSE_TIMEOUT;
 const bool ISO_TP_DEFAULT_FRAME_PADDING_STATUS;
 
 typedef struct {
     const uint16_t arbitration_id;
-    uint8_t* payload;
+    uint8_t payload[OUR_MAX_ISO_TP_MESSAGE_SIZE];
     uint16_t size;
     bool completed;
 } IsoTpMessage;
@@ -121,7 +124,7 @@ void isotp_message_to_string(const IsoTpMessage* message, char* destination,
         size_t destination_length);
 
 IsoTpHandle isotp_send(IsoTpShims* shims, const uint16_t arbitration_id,
-        const uint8_t* payload, uint16_t size,
+        const uint8_t payload[], uint16_t size,
         IsoTpMessageSentHandler callback);
 
 IsoTpHandle isotp_receive(IsoTpShims* shims,
diff --git a/src/isotp/send.c b/src/isotp/send.c
index 85e35744..b87c5602 100644
--- a/src/isotp/send.c
+++ b/src/isotp/send.c
@@ -44,7 +44,7 @@ IsoTpHandle isotp_send_single_frame(IsoTpShims* shims, IsoTpMessage* message,
 
 IsoTpHandle isotp_send_multi_frame(IsoTpShims* shims, IsoTpMessage* message,
         IsoTpMessageSentHandler callback) {
-    // TODO make sure to copy payload into a local buffer
+    // TODO make sure to copy message into a local buffer
     shims->log("Only single frame messages are supported");
     IsoTpHandle handle = {
         success: false,
@@ -57,14 +57,14 @@ IsoTpHandle isotp_send_multi_frame(IsoTpShims* shims, IsoTpMessage* message,
 }
 
 IsoTpHandle isotp_send(IsoTpShims* shims, const uint16_t arbitration_id,
-        const uint8_t* payload, uint16_t size,
+        const uint8_t payload[], uint16_t size,
         IsoTpMessageSentHandler callback) {
     IsoTpMessage message = {
         arbitration_id: arbitration_id,
-        payload: payload,
         size: size
     };
 
+    memcpy(message.payload, payload, size);
     if(size < 8) {
         return isotp_send_single_frame(shims, &message, callback);
     } else {
diff --git a/tests/common.c b/tests/common.c
index 247d8f3d..882c266a 100644
--- a/tests/common.c
+++ b/tests/common.c
@@ -14,12 +14,12 @@ bool can_frame_was_sent;
 
 bool message_was_received;
 uint16_t last_message_received_arb_id;
-uint8_t* last_message_received_payload;
+uint8_t last_message_received_payload[OUR_MAX_ISO_TP_MESSAGE_SIZE];
 uint8_t last_message_received_payload_size;
 
 uint16_t last_message_sent_arb_id;
 bool last_message_sent_status;
-uint8_t* last_message_sent_payload;
+uint8_t last_message_sent_payload[OUR_MAX_ISO_TP_MESSAGE_SIZE];
 uint8_t last_message_sent_payload_size;
 
 void debug(const char* format, ...) {
@@ -84,8 +84,8 @@ void can_frame_sent(const uint16_t arbitration_id, const uint8_t* payload,
 void setup() {
     SHIMS = isotp_init_shims(debug, mock_send_can, mock_set_timer);
     HANDLE = isotp_receive(&SHIMS, 0x2a, message_received);
-    last_message_sent_payload = malloc(MAX_ISO_TP_MESSAGE_SIZE);
-    last_message_received_payload = malloc(MAX_ISO_TP_MESSAGE_SIZE);
+    memset(last_message_sent_payload, 0, OUR_MAX_ISO_TP_MESSAGE_SIZE);
+    memset(last_message_received_payload, 0, OUR_MAX_ISO_TP_MESSAGE_SIZE);
     memset(last_can_payload_sent, 0, sizeof(last_can_payload_sent));
     last_message_sent_status = false;
     message_was_received = false;
diff --git a/tests/test_receive.c b/tests/test_receive.c
index 5c757076..d8c2392e 100644
--- a/tests/test_receive.c
+++ b/tests/test_receive.c
@@ -15,12 +15,12 @@ extern bool can_frame_was_sent;
 
 extern bool message_was_received;
 extern uint16_t last_message_received_arb_id;
-extern uint8_t* last_message_received_payload;
+extern uint8_t last_message_received_payload[];
 extern uint8_t last_message_received_payload_size;
 
 extern uint16_t last_message_sent_arb_id;
 extern bool last_message_sent_status;
-extern uint8_t* last_message_sent_payload;
+extern uint8_t last_message_sent_payload[];
 extern uint8_t last_message_sent_payload_size;
 
 extern void setup();
diff --git a/tests/test_send.c b/tests/test_send.c
index 0320576c..ca1842a6 100644
--- a/tests/test_send.c
+++ b/tests/test_send.c
@@ -17,12 +17,12 @@ extern bool can_frame_was_sent;
 
 extern bool message_was_received;
 extern uint16_t last_message_received_arb_id;
-extern uint8_t* last_message_received_payload;
+extern uint8_t last_message_received_payload[];
 extern uint8_t last_message_received_payload_size;
 
 extern uint16_t last_message_sent_arb_id;
 extern bool last_message_sent_status;
-extern uint8_t* last_message_sent_payload;
+extern uint8_t last_message_sent_payload[];
 extern uint8_t last_message_sent_payload_size;
 
 extern void setup();
@@ -49,8 +49,8 @@ START_TEST (test_send_single_frame)
 {
     const uint8_t payload[] = {0x12, 0x34};
     uint16_t arbitration_id = 0x2a;
-    IsoTpHandle handle = isotp_send(&SHIMS, arbitration_id, payload, sizeof(payload),
-            message_sent);
+    IsoTpHandle handle = isotp_send(&SHIMS, arbitration_id, payload,
+            sizeof(payload), message_sent);
     ck_assert_int_eq(last_message_sent_arb_id, arbitration_id);
     fail_unless(last_message_sent_status);
     ck_assert_int_eq(last_message_sent_payload[0], 0x12);