Skip to content

transport: sar: skip ACK resend when not connected #210

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

Draft
szczys wants to merge 1 commit into
base: main
Choose a base branch
from
+6 −0
Draft

transport: sar: skip ACK resend when not connected #210

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 6 additions & 0 deletions src/transport/sar/receiver.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,12 @@ static void send_ack(pouch_work_delayable_t *dwork)
int err = pouch_bearer_send(p->bearer, buf, sizeof(buf));
if (err)
{
if (err == -ENOTCONN)
{
POUCH_LOG_DBG("TX skipped (%d), not connected", err);
return;
}

@hasheddan hasheddan May 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@szczys what case are you seeing us encountering this scenario? I would expect that if the connection is lost we would be closing the receiver and canceling the work queue. However, if we do not close the receiver, then I suspect that we would want to not just ignore the failed ack send, but go ahead and terminate the transfer.

In short, if this situation is unrecoverable, then I would imagine that we should abort the transfer entirely rather than just ignoring a failure to ack. If the situation is recoverable, then we want to keep acking.

Or is this just to account for the case in which this ack is already in progress when the work queue is canceled?

@szczys szczys May 27, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'll get back to you on that. This may have been cause by other issues I'm currently troubleshooting.

@szczys szczys May 28, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I am no longer able to reproduce this after fixing other concurrency issues I was having, but let me describe the behavior:

This code is running on the device and if the broker disconnects, close() will be called in the BLE_GAP_EVENT_DISCONNECT case of pouch_gatt_gap_event. So you're right that the transfer needs to be aborted and that is indeed how the design works.

However, I was still occassionally seeing a situation where the -ENOTCONN was returned by the NimBLE stack which led to the schedule_ack() being called, starting a 1Hz loop of error messages: work_handler -> -ENOTCONN -> reschedule

One possibility is that the concurrency issues I was resolving were corrupting the memory and causing ACK to run even though a disconnect had happened.

However, the ACK is put on a delayable work queue that is running at a lower priority than some of the other threads so here's a legitimate scenarion that could happen:

  1. recv->work is scheduled to send the ACK
  2. Timer (or no wait) expires and adds the send_ack handler to the workq
  3. Broker decides to disconnect
  4. Higher priority NimBLE thread gains focus and runs the disconnect
  5. Disconnect cancels the delayable work, but it's already in the workq and the FreeRTOS port doesn't remove already-queued delayable work items
  6. When the workq gets focus, send_ack runs and we get -ENOTCONN from NimBLE
  7. 1Hz loop begins

If this is actually what's happening, it makes a case for fixing the port so that enqueued work can be removed. However, this is a significantly complex functionality to add so I'm not advocating for it.

Actually, now that I think of it, there's also a small sliver of time in the Zephyr delayable work where the same could happen. It's after the queued work has been popped from the queue but has not yet finished processing. If the work queue is preempted, then the cancel will not stop the delayable ACK work from running once focus is back on the workq thread and we'll be in the same loop.

Thank you for coming to my Ted talk.

@hasheddan hasheddan May 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@szczys thanks for writing this up! We chatted offline and the Zephyr example of work in progress was the one that I was thinking of, but it seems as though the ESP-IDF case is potentially even more likely. Regardless, my primary concern is silently not rescheduling the ack, which effectively will halt progress, without informing the transport (bearer). I'm also somewhat concerned about pouch_bearer_send() failing with an error other than -ENOTCONN, and us finding ourselves in this same ack loop.

To address the former, I think we could call end() here to signal to the bearer that the transfer is being aborted. To address the latter, I think it could make sense to only reschedule the ack in the event that the error is -EAGAIN (i.e. invert the error check logic here), and assume that if the bearer can't send and doesn't return -EAGAIN then we should abort the transfer.


// try again later:
POUCH_LOG_ERR("TX failed (%d)", err);
schedule_ack(p);
Expand Down
Loading