diff options
| -rw-r--r-- | README.mkd | 10 | ||||
| m--------- | deps/isotp-c | 10 | ||||
| -rw-r--r-- | src/obd2/obd2.c | 121 | ||||
| -rw-r--r-- | src/obd2/obd2.h | 18 | ||||
| -rw-r--r-- | tests/test_core.c | 17 |
5 files changed, 115 insertions, 61 deletions
@@ -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); |