Skip to content

Fix Async polling interval to 1 second #77

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
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Fix Async polling interval to 1 second #77

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
2 changes: 1 addition & 1 deletion src/AsyncTCP.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ struct tcp_core_guard {
TCP poll interval is specified in terms of the TCP coarse timer interval, which is called twice a second
https://github.com/espressif/esp-lwip/blob/2acf959a2bb559313cd2bf9306c24612ba3d0e19/src/core/tcp.c#L1895
*/
#define CONFIG_ASYNC_TCP_POLL_TIMER 1
#define CONFIG_ASYNC_TCP_POLL_TIMER 2 // Called every 2 * 500ms
Copy link
Preview

Copilot AI Jul 14, 2025

Choose a reason for hiding this comment

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

[nitpick] Consider defining a named constant (e.g., COARSE_TIMER_TICKS_PER_SECOND) instead of using the magic number 2 to improve readability.

Copilot uses AI. Check for mistakes.

Copy link
Member

Choose a reason for hiding this comment

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

why this change ?

if you need to make such change, should this me a config, like core, size size, etc ?

Copy link
Author

Choose a reason for hiding this comment

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

I reviewed the function err_t AsyncClient::_poll(tcp_pcb *pcb) L1063 and my understanding of its design purpose, which is to handle ACK timeouts, RX timeouts, and the polling interval. The intention is for the polling interval to be specified in seconds.

The function static void _bind_tcp_callbacks(tcp_pcb *pcb, AsyncClient *client) L382 registers a tcp_poll callback with an interval argument set to CONFIG_ASYNC_TCP_POLL_TIMER = 1 L95 which corresponds to 500 ms.

To align with the original design intention of using a polling interval in seconds, I suggest updating the configuration to CONFIG_ASYNC_TCP_POLL_TIMER = 2.

I also referred to the TCP poll interval specification at:
esp-lwip tcp.c#L1895

Let me know if I’ve misunderstood something. Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

@willmmiles @me-no-dev WDYT ?

Copy link

Choose a reason for hiding this comment

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

I think the current default of 500ms (the smallest possible poll interval) is probably the best default value for most use cases. Given that it's a compile time selection, it's much easier for client applications to ignore extra polls than add another task to poll more often.

The intention is for the polling interval to be specified in seconds.

I don't think this is correct, do you have a source for this? The comment on line 95 is quite clear that the polling interval is specified in LwIP TCP ticks, and it doesn't appear in any other documentation. The use cases of the LwIP poll callback aren't just - or even primarily - about timeouts; it's most useful as an application feature for scheduling connection-related work on the async thread. Frequent polling is an asset IMO; personally I've wished it could be set to a smaller value!

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for your comments!
I understand what you’re saying.

I’m confident that this configuration does not change the default polling interval of the LwIP core.

What I meant is that this change only affects the Async thread, where the intended polling interval is 1 second. I reviewed the _poll function and the API settings for ACK timeout and RX timeout, and their intention appears to be in seconds, not 500ms.

However, the current configuration using a 500ms polling interval works fine because the logic inside the _poll function correctly checks for a timeout after 1000ms.

I believe this update is just an improvement to make the behavior more consistent and easier to understand.

I may have misunderstood the original design concept, but it works fine for me.

Please let me know if this update is invalid, and I will cancel it.

Thank you!

Copy link

Choose a reason for hiding this comment

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

I do not think we should change the default polling interval. I think you may have missed that the the _poll function has three tasks:

  • Checking for ACK timeout, where the configured interval is specified in seconds
  • Checking for RX timeout, where the configured interval is specified in seconds
  • Calling the user poll function, which is configured only by CONFIG_ASYNC_TCP_POLL_TIMER, and is NOT specified in seconds

Although the first two are configured in seconds, the third is not, and so the whole _poll wrapper should not be assumed or expected to operate in whole seconds. As you've observed, the existing code does not make this assumption. Further, existing client programs may be expecting the current 500ms calls of their poll functions. It is easy for client applications to ignore polls they don't want; and more difficult to change the configuration to increase the polling interval if the default is too long.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for your feedback!

You are right about the third task.

However, I don’t fully understand the point you mentioned regarding the user application.

The current default configuration sets the ACK timeout to 3 seconds. The user application, such as AsyncWebServer, also uses a default RX timeout of 3 seconds.
However an expected polling interval from the user application is 500ms.

Could you share some use cases for the motivation behind this configuration?

Copy link

Choose a reason for hiding this comment

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

Some use cases that come to mind:

  • A data-generating process, such as a temperature monitor sending samples via MQTT, could use the poll callback to check if new data is ready to be sent. Polling in the async context makes that kind of code very simple, as it doesn't have to worry about the connection state changing or deal with mutexing the AsyncClient object across tasks.
  • Implementing a liveness check that's more granular than "any traffic". For example, an application might expect a periodic "heartbeat" message from its partner; and it could want to respond to missing heartbeats, even if other messages are still arriving over the same connection.
  • I've used it as a "retry" timer when processing web requests that have to interlock with some shared resource that was contended. For example, serializing a large JSON object might consume a significant fraction of the microcontroller's RAM, so we permit only one web request to do so at a time; I used the poll callback to re-check if the shared JSON buffer is available so this request can proceed. (Honestly I wished for a faster poll!)

Copy link
Author

Choose a reason for hiding this comment

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

Thanks a lot for sharing!
I agree that this change could impact other client applications that currently implement interval polling.

On the other hand, do you think we should support a function to configure the polling interval for other types of client applications, such as the one I intend to use?

Copy link

Choose a reason for hiding this comment

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

Sorry for taking so long to follow up on this one.

With respect to the default behaviour of an AsyncClient, my personal opinion is that the best approach for AsyncTCP is to set the polling interval to the minimum value, Although it's clear from the name that someone considered it in the past, I don't think we should support a compile-time override for the default poll timer. I think it's better to minimize the number of configuration options; it makes it easier to avoid cases where someone else "tunes" AsyncTCP in a way that other users of the library don't expect. A common standard configuration provides a stable base for other code to build upon.

Another key point is that the poll timer is also the maximum latency before an application will be notified of a timeout. For example, if you set the poll timer to 2 seconds, with an Rx timeout of 2 seconds, you could see something like:

  • t=0ms: Rx packet
  • t=100ms: Poll timer fires. No timeout detected. (Last_rx - t = 100ms)
  • t=200ms: Rx packet
  • t=2100ms: Poll timer fires. No timeout detected. (Last_rx - t = 1900ms, less than 2 seconds)
  • t=4100ms: Poll timer fires. Only now is application notified of Rx timeout (1900ms later than "real" timeout!!)

That said: I don't have any objections to adding a new AsyncClient::setPollInterval() call that maps down to tcp_poll(), beyond the above note that raising the value can delay the processing of the Rx and Ack timeouts. This would permit individual library users to make an informed choice about how much latency they're prepared to accept for their application.


/*
* TCP/IP Event Task
Expand Down