Match isotp receive_can_frame style, depend less on callbacks.

5 files changed, 115 insertions, 61 deletions

diff --git a/README.mkd b/README.mkd
index 96b969ee..28aee721 100644
--- a/README.mkd
+++ b/README.mkd
@@ -53,21 +53,21 @@ what we need is another layer on top of that to handle the repeated requests.
     void my_callback(const DiagnosticResponse* response) {
     }
 
-    DiagnosticRequestHandler handler = diagnostic_init(my_callback);
+    DiagnosticRequestHandle handle = diagnostic_init(my_callback);
     DiagnosticRequest request = {
         arbitratin_id: 0x7df,
         mode: OBD2_MODE_POWERTRAIN_DIAGNOSTIC_REQUEST,
         pid: 3
     };
 
-    diagnostic_send(&handler, &request);
+    diagnostic_send(&handle, &request);
     while(true) {
-        diagnostic_handle_can_frame(&handler, 42, data, 8);
+        diagnostic_handle_can_frame(&handle, 42, data, 8);
     }
 
-    diagnostic_request_pid(&handler, DIAGNOSTIC_STANDARD_PID, 42
+    diagnostic_request_pid(&handle, DIAGNOSTIC_STANDARD_PID, 42
 
-    diagnostic_destroy(&handler);
+    diagnostic_destroy(&handle);
 
 ## Testing
 
diff --git a/deps/isotp-c b/deps/isotp-c
-Subproject 10a35b0a7c380d77cdd24ac90d6aa0abd4601f3
+Subproject fe20a273bb3979d9e806d828486633249d073ed
diff --git a/src/obd2/obd2.c b/src/obd2/obd2.c
index 33e6da15..7188af0d 100644
--- a/src/obd2/obd2.c
+++ b/src/obd2/obd2.c
@@ -20,7 +20,8 @@ DiagnosticRequestHandle diagnostic_request(DiagnosticShims* shims,
     DiagnosticRequestHandle handle = {
         type: DIAGNOSTIC_REQUEST_TYPE_PID,
         callback: callback,
-        status: true
+        success: false,
+        completed: false
     };
     uint8_t payload[MAX_DIAGNOSTIC_PAYLOAD_SIZE];
     payload[MODE_BYTE_INDEX] = request->mode;
@@ -33,38 +34,79 @@ DiagnosticRequestHandle diagnostic_request(DiagnosticShims* shims,
                 request->payload, request->payload_length);
     }
 
-    IsoTpShims isotp_shims = isotp_init_shims(shims->log,
+    handle.isotp_shims = isotp_init_shims(shims->log,
             shims->send_can_message,
             shims->set_timer);
-    handle.status = isotp_send(&isotp_shims, request->arbitration_id,
-            payload, 1 + request->payload_length + request->pid_length,
-            diagnostic_receive_isotp_message);
-
-    // TODO need to set up an isotp receive handler. in isotp, rx and tx are
-    // kind of intermingled at this point. really, there's not explicit link
-    // between send and receveice...well except for flow control. hm, damn.
-    // so there's 2 things:
-    //
-    // isotp_send needs to return a handle. if it was a single frame, we
-    // probably sent it right away so the status true and the callback was hit.
-    // the handle needs another flag to say if it was completed or not, so you
-    // know you can destroy it. you will continue to throw can frames at that
-    // handler until it returns completed (either with a  flag, or maybe
-    // receive_can_frame returns true if it's complete)
-    //
-    // the second thing is that we need to be able to arbitrarly set up to
-    // receive an iso-tp message on a particular arb id. again, you keep
-    // throwing can frames at it until it returns a handle with the status
-    // completed and calls your callback
-    //
-    // so the diagnostic request needs 2 isotp handles and they should both be
-    // hidden from the user
-    //
+    handle.isotp_send_handle = isotp_send(&handle.isotp_shims,
+            request->arbitration_id, payload,
+            1 + request->payload_length + request->pid_length,
+            // TODO is this ok to pass null here?
+            NULL);
+
+    handle.isotp_receive_handle = isotp_receive(&handle.isotp_shims,
+            // TODO need to either always add 0x8 or let the user specify
+            request->arbitration_id + 0x8,
+            // TODO this callback is mostly useful for debugging stuff as it
+            // doesn't have the internal state we need to complete the
+            // diagnositc request - can we pass NULL or will that 'splode?
+            NULL);
+
     // when a can frame is received and passes to the diagnostic handle
     // if we haven't successfuly sent the entire message yet, give it to the
     // isottp send handle
     // if we have sent it, give it to the isotp rx handle
     // if we've received properly, mark this request as completed
+    // how do you get the result? we have it set up for callbacks but that's
+    // getting to be kind of awkward
+    //
+    // say it were to call a callback...what state would it need to pass?
+    //
+    // the iso-tp message received callback needs to pass the handle and the
+    // received message
+    //
+    // so in the obd2 library, we get a callback with an isotp message. how do
+    // we know what diag request i went with, and which diag callback to use? we
+    // could store context with the isotp handle. the problem is that context is
+    // self referential and on the stack, so we really can't get a pointer to
+    // it.
+    //
+    // the diagnostic response received callback needs to pass the diagnostic
+    // handle and the diag response
+    //
+    // let's say we simplify things and drop the callbacks.
+    //
+    // isotp_receive_can_frame returns an isotp handle with a complete message
+    // in it if one was received
+    //
+    // diagnostic_receive_can_frame returns a diaghandle if one was received
+    //
+    // so in the user code you would throw the can frame at each of your active
+    // diag handles and see if any return a completed message.
+    //
+    // is there any advantage to a callback approach?  callbacks are useful when
+    // you have something that will block, bt we don't have anything that will
+    // block. it's async but we return immediately from each partial parsing
+    // attempt.
+    //
+    // argh, but will we need the callbacks when we add timers for isotp multi
+    // frame?
+    //
+    // what are the timers for exactly?
+    //
+    // when sending multi frame, send 1 frame, wait for a response
+    // if it says send all, send all right away
+    // if it says flow control, set the time for the next send
+    // instead of creating a timer with an async callback, add a process_handle
+    // function that's called repeatedly in the main loop - if it's time to
+    // send, we do it. so there's a process_handle_send and receive_can_frame
+    // that are just called continuously from the main loop. it's a waste of a
+    // few cpu cycles but it may be more  natural than callbacks.
+    //
+    // what woudl a timer callback look like...it would need to pass the handle
+    // and that's all. seems like a context void* would be able to capture all
+    // of the information but arg, memory allocation. look at how it's done in
+    // the other library again
+    //
     return handle;
 }
 
@@ -79,20 +121,27 @@ DiagnosticRequestHandle diagnostic_request_pid(DiagnosticShims* shims,
     return diagnostic_request(shims, &request, callback);
 }
 
-void diagnostic_receive_isotp_message(const IsoTpMessage* message) {
-    // TODO
-}
-
-void diagnostic_receive_can_frame(DiagnosticRequestHandle* handle,
+DiagnosticResponse diagnostic_receive_can_frame(DiagnosticShims* shims,
+        DiagnosticRequestHandle* handle,
         const uint16_t arbitration_id, const uint8_t data[],
         const uint8_t size) {
-    isotp_receive_can_frame(handle->isotp_handler, arbitration_id, data, size);
-}
-
-// TODO argh, we're now saying that user code will rx CAN messages, but who does
-// it hand them to? isotp handlers are encapsulated in diagnostic handles
+    if(!handle->isotp_send_handle.completed) {
+    } else if(!handle->isotp_receive_handle.completed) {
+    } else {
+        shims->log("Handle is already completed");
+    }
+    // TODO determine which isotp handler to pass it to based on our state
+    IsoTpMessage message = isotp_receive_can_frame(&handle->isotp_shims,
+            &handle->isotp_send_handle, arbitration_id, data, size);
 
+    DiagnosticResponse response = {
+        success: false,
+        completed: false
+    };
 
+    if(message.completed) {
+    }
+}
 
 // TODO everything below here is for future work...not critical for now.
 
diff --git a/src/obd2/obd2.h b/src/obd2/obd2.h
index 8d7d6642..727b816f 100644
--- a/src/obd2/obd2.h
+++ b/src/obd2/obd2.h
@@ -22,7 +22,7 @@ typedef struct {
 } DiagnosticRequest;
 
 // TODO I don't like this, it's hard coding isotp library stuff here
-typedef bool (*SendIsoTpMessageShim)(IsoTpHandler* handler,
+typedef bool (*SendIsoTpMessageShim)(IsoTpHandle* handle,
         const uint8_t* payload, uint16_t payload_size);
 
 // Thanks to
@@ -68,6 +68,7 @@ typedef struct {
     uint16_t arbitration_id;
     uint8_t mode;
     bool success;
+    bool completed;
     // if mode is one with a PID, read the correct numbers of PID bytes (1 or 2)
     // into this field, then store the remainder of the payload in the payload
     // field
@@ -116,7 +117,11 @@ typedef enum {
 } DiagnosticRequestType;
 
 typedef struct {
-    IsoTpHandler isotp_handler;
+    bool success;
+    bool completed;
+    IsoTpShims isotp_shims;
+    IsoTpHandle isotp_send_handle;
+    IsoTpHandle isotp_receive_handle;
     // TODO the Handle may need to keep the original request, otherwise we can't
     // compare an incoming CAN message to see if it matches the service / PID!
     // TODO i'm not sure this type/callback in here is too useful - see the
@@ -126,7 +131,6 @@ typedef struct {
     DiagnosticResponseReceived callback;
     DiagnosticMilStatusReceived mil_status_callback;
     DiagnosticVinReceived vin_callback;
-    bool status;
 } DiagnosticRequestHandle;
 
 typedef enum {
@@ -168,12 +172,8 @@ bool diagnostic_clear_dtc(DiagnosticShims* shims);
 DiagnosticRequestHandle diagnostic_enumerate_pids(DiagnosticShims* shims,
         DiagnosticRequest* request, DiagnosticPidEnumerationReceived callback);
 
-// TODO
-// void diagnostic_receive_isotp_message(DiagnosticRequestHandle* handle,
-        // const IsoTpMessage* message);
-void diagnostic_receive_isotp_message(const IsoTpMessage* message);
-
-void diagnostic_receive_can_frame(DiagnosticRequestHandle* handle,
+DiagnosticResponse diagnostic_receive_can_frame(DiagnosticShims* shims,
+        DiagnosticRequestHandle* handle,
         const uint16_t arbitration_id, const uint8_t data[],
         const uint8_t size);
 
diff --git a/tests/test_core.c b/tests/test_core.c
index 340a3268..a466e8e0 100644
--- a/tests/test_core.c
+++ b/tests/test_core.c
@@ -21,8 +21,8 @@ START_TEST (test_receive_wrong_arb_id)
 
     fail_if(last_response_was_received);
     const uint8_t can_data[] = {0x2, request.mode + 0x40, 0x23};
-    diagnostic_receive_can_frame(&handle, request.arbitration_id, can_data,
-            sizeof(can_data));
+    diagnostic_receive_can_frame(&SHIMS, &handle, request.arbitration_id,
+            can_data, sizeof(can_data));
     fail_if(last_response_was_received);
 }
 END_TEST
@@ -36,10 +36,15 @@ START_TEST (test_send_diag_request)
     DiagnosticRequestHandle handle = diagnostic_request(&SHIMS, &request,
             response_received_handler);
 
+    fail_if(handle.completed);
+
     fail_if(last_response_was_received);
     const uint8_t can_data[] = {0x2, request.mode + 0x40, 0x23};
-    diagnostic_receive_can_frame(&handle, request.arbitration_id + 0x8,
-            can_data, sizeof(can_data));
+    DiagnosticResponse response = diagnostic_receive_can_frame(&SHIMS, &handle,
+            request.arbitration_id + 0x8, can_data, sizeof(can_data));
+    fail_unless(response.success);
+    fail_unless(response.completed);
+    fail_unless(handle.completed);
     fail_unless(last_response_was_received);
     ck_assert(last_response_received.success);
     ck_assert_int_eq(last_response_received.arbitration_id,
@@ -60,7 +65,7 @@ START_TEST (test_request_pid_standard)
     fail_if(last_response_was_received);
     const uint8_t can_data[] = {0x3, 0x1 + 0x40, 0x2, 0x45};
     // TODO need a constant for the 7df broadcast functional request
-    diagnostic_receive_can_frame(&handle, 0x7df + 0x8,
+    diagnostic_receive_can_frame(&SHIMS, &handle, 0x7df + 0x8,
             can_data, sizeof(can_data));
     fail_unless(last_response_was_received);
     ck_assert(last_response_received.success);
@@ -82,7 +87,7 @@ START_TEST (test_request_pid_enhanced)
     fail_if(last_response_was_received);
     const uint8_t can_data[] = {0x4, 0x1 + 0x40, 0x0, 0x2, 0x45};
     // TODO need a constant for the 7df broadcast functional request
-    diagnostic_receive_can_frame(&handle, 0x7df + 0x8, can_data,
+    diagnostic_receive_can_frame(&SHIMS, &handle, 0x7df + 0x8, can_data,
             sizeof(can_data));
     fail_unless(last_response_was_received);
     ck_assert(last_response_received.success);