fix(studio): Ensure putting framing byte when sending rpc#3215
fix(studio): Ensure putting framing byte when sending rpc#3215cormoran wants to merge 1 commit intozmkfirmware:mainfrom
Conversation
If tx buffer became full when sending PRC response, framing byte can be skipped and communication gets stuck. This patch adds wait for putting EOF failure and adds error handling for putting SOF failure.
| LOG_ERR("RPC TX buffer full"); | ||
| k_mutex_unlock(&rpc_transport_mutex); | ||
| return -ENOMEM; | ||
| } |
There was a problem hiding this comment.
Treating SOF put failure as error sounds fine? I've never hit error here yet.
There was a problem hiding this comment.
I'm on the fence here, whether there should be any attempt to retry a few times, and then return -ENOMEM if that fails? Would give the underlying transport a chance to empty the ring buffer to free space. I'd suggest we try that.
In the case of a failure, I usually prefer, when locking involved, to create a new local at the top of the function int ret = 0; and then in this branch, set ret = -ENOMEM; goto exit and update the exit: to return ret instead.
petejohanson
left a comment
petejohanson
left a comment
There was a problem hiding this comment.
Thanks for working on this! A few thoughts.
| LOG_ERR("RPC TX buffer full"); | ||
| k_mutex_unlock(&rpc_transport_mutex); | ||
| return -ENOMEM; | ||
| } |
There was a problem hiding this comment.
I'm on the fence here, whether there should be any attempt to retry a few times, and then return -ENOMEM if that fails? Would give the underlying transport a chance to empty the ring buffer to free space. I'd suggest we try that.
In the case of a failure, I usually prefer, when locking involved, to create a new local at the top of the function int ret = 0; and then in this branch, set ret = -ENOMEM; goto exit and update the exit: to return ret instead.
| framing_byte = FRAMING_EOF; | ||
| ring_buf_put(&rpc_tx_buf, &framing_byte, 1); | ||
| // If response is large, tx buffer can be full for async transport. | ||
| while (ring_buf_put(&rpc_tx_buf, &framing_byte, 1) == 0) { |
There was a problem hiding this comment.
This should ideally "give up" after a certain period/number of tries, and return, so we don't get stuck in an infinite loop, IMHO.
2 participants
If tx buffer became full when sending PRC response, framing byte can be skipped and communication gets stuck. This patch adds wait for putting EOF failure and adds error handling for putting SOF failure.
I hit this error when sending keylayout data on BLE gatt. Waiting in the loop practically resolved the issue.
PR check-list