Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

CAN: Request/response fixes #53

Merged
merged 4 commits into from
Aug 12, 2024
Merged

CAN: Request/response fixes #53

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7f0692a
fix request response timer
garethpotter Jul 23, 2024
9a27543
CAN ID on callback
garethpotter Jul 23, 2024
61640b1
always call sent callback
garethpotter Jul 24, 2024
61de8ff
code review fixes: callback expects a node address, not a full CAN ID
garethpotter Aug 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions src/can.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -449,7 +449,7 @@ static void thingset_can_reqresp_timeout_handler(struct k_timer *timer)
{
struct thingset_can_request_response *rr =
CONTAINER_OF(timer, struct thingset_can_request_response, timer);
rr->callback(NULL, 0, 0, -ETIMEDOUT, 0, rr->cb_arg);
rr->callback(NULL, 0, 0, -ETIMEDOUT, rr->can_id, rr->cb_arg);
garethpotter marked this conversation as resolved.
Show resolved Hide resolved
thingset_can_reset_request_response(rr);
}

Expand Down Expand Up @@ -481,7 +481,7 @@ int thingset_can_send_inst(struct thingset_can *ts_can, uint8_t *tx_buf, size_t
ts_can->request_response.callback = callback;
ts_can->request_response.cb_arg = callback_arg;
k_timer_init(&ts_can->request_response.timer, thingset_can_reqresp_timeout_handler, NULL);
k_timer_start(&ts_can->request_response.timer, timeout, timeout);
k_timer_start(&ts_can->request_response.timer, timeout, K_NO_WAIT);
ts_can->request_response.can_id = thingset_can_get_tx_addr(&tx_addr).ext_id;
}

Expand Down Expand Up @@ -548,9 +548,15 @@ static void thingset_can_reqresp_recv_error_callback(int8_t error, struct isotp_
static void thingset_can_reqresp_sent_callback(int result, void *arg)
{
struct thingset_can *ts_can = arg;
if (ts_can->request_response.callback != NULL && result != 0) {
ts_can->request_response.callback(NULL, 0, 0, result, 0, ts_can->request_response.cb_arg);
if (ts_can->request_response.callback != NULL) {
ts_can->request_response.callback(NULL, 0, 0, result, ts_can->request_response.can_id,
garethpotter marked this conversation as resolved.
Show resolved Hide resolved
ts_can->request_response.cb_arg);
thingset_can_reset_request_response(&ts_can->request_response);
if (result == 0) {
/* maintain unlocking semantics of previous iteration of this code */
struct shared_buffer *sbuf = thingset_sdk_shared_buffer();
k_sem_give(&sbuf->lock);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering why we are unlocking the buffer here at all, as the shared buffer doesn't seem to be used by the thingset_can_send_inst function. Only at my phone at the moment, so it's difficult to check the entire code flow...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's somewhat lost in the mists of time now, but I think the rationale was that we only want to unlock the buffer once the message has been successfully sent. I agree that it's a bit odd. In truth, the whole thing probably wants a bit of a rethink, but I figured this approach at least wouldn't make things any worse.

}
}
else {
struct shared_buffer *sbuf = thingset_sdk_shared_buffer();
Expand Down
Loading