You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2022/05/26 03:51:27 UTC
[GitHub] [incubator-nuttx] zhhyu7 opened a new pull request, #6330: tcp: move wd_timer from wifi driver to tcp stack
zhhyu7 opened a new pull request, #6330:
URL: https://github.com/apache/incubator-nuttx/pull/6330
Signed-off-by: zhanghongyu <zh...@xiaomi.com>
## Summary
Wifi drivers no longer need to maintain their own timers, The timer stops when there is no data transfer and keeplive.
Reduce the frequency of waking up when idle, This helps in low-power scenarios.
## Impact
## Testing
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6330: tcp: move wd_timer from wifi driver to tcp stack
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6330:
URL: https://github.com/apache/incubator-nuttx/pull/6330#discussion_r882428696
##########
net/tcp/tcp_input.c:
##########
@@ -1288,7 +1290,8 @@ static void tcp_input(FAR struct net_driver_s *dev, uint8_t domain,
if ((flags & TCP_ACKDATA) != 0)
{
conn->tcpstateflags = TCP_TIME_WAIT;
- conn->timer = TCP_TIME_WAIT_TIMEOUT * HSEC_PER_SEC;
+ tcp_update_retrantimer(conn,
+ TCP_TIME_WAIT_TIMEOUT * HSEC_PER_SEC);
Review Comment:
```suggestion
tcp_update_retrantimer(conn,
TCP_TIME_WAIT_TIMEOUT * HSEC_PER_SEC);
```
##########
net/tcp/tcp_input.c:
##########
@@ -1225,7 +1225,8 @@ static void tcp_input(FAR struct net_driver_s *dev, uint8_t domain,
if ((flags & TCP_ACKDATA) != 0 && conn->tx_unacked == 0)
{
conn->tcpstateflags = TCP_TIME_WAIT;
- conn->timer = TCP_TIME_WAIT_TIMEOUT * HSEC_PER_SEC;
+ tcp_update_retrantimer(conn,
+ TCP_TIME_WAIT_TIMEOUT * HSEC_PER_SEC);
Review Comment:
```suggestion
tcp_update_retrantimer(conn,
TCP_TIME_WAIT_TIMEOUT * HSEC_PER_SEC);
```
##########
net/devif/devif_poll.c:
##########
@@ -849,29 +798,7 @@ int devif_poll(FAR struct net_driver_s *dev, devif_poll_callback_t callback)
int devif_timer(FAR struct net_driver_s *dev, int delay,
devif_poll_callback_t callback)
{
-#if defined(NET_TCP_HAVE_STACK)
- int hsec = TICK2HSEC(delay);
-#endif
- int bstop = false;
-
-#ifdef NET_TCP_HAVE_STACK
- /* Traverse all of the active TCP connections and perform the
- * timer action.
- */
-
- bstop = devif_poll_tcp_timer(dev, callback, hsec);
-#endif
-
- /* If possible, continue with a normal poll checking for pending
- * network driver actions.
- */
-
- if (!bstop)
- {
- bstop = devif_poll(dev, callback);
- }
-
- return bstop;
+ return devif_poll(dev, callback);
Review Comment:
so `delay` parameter now is ignored?
##########
net/tcp/tcp_input.c:
##########
@@ -1263,7 +1264,8 @@ static void tcp_input(FAR struct net_driver_s *dev, uint8_t domain,
if ((tcp->flags & TCP_FIN) != 0)
{
conn->tcpstateflags = TCP_TIME_WAIT;
- conn->timer = TCP_TIME_WAIT_TIMEOUT * HSEC_PER_SEC;
+ tcp_update_retrantimer(conn,
+ TCP_TIME_WAIT_TIMEOUT * HSEC_PER_SEC);
Review Comment:
```suggestion
tcp_update_retrantimer(conn,
TCP_TIME_WAIT_TIMEOUT * HSEC_PER_SEC);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] masayuki2009 commented on pull request #6330: tcp: move wd_timer from wifi driver to tcp stack
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on PR #6330:
URL: https://github.com/apache/incubator-nuttx/pull/6330#issuecomment-1147123830
@zhhyu7
Though the commit message says that `tcp: move wd_timer from wifi driver to tcp stack`, I can not see any changes in Wi-Fi drivers.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6330: tcp: move wd_timer from wifi driver to tcp stack
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6330:
URL: https://github.com/apache/incubator-nuttx/pull/6330#discussion_r882622649
##########
net/tcp/tcp_timer.c:
##########
@@ -74,10 +76,192 @@
#define ACK_DELAY (1)
+/****************************************************************************
+ * Private Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: tcp_get_timeout
+ *
+ * Description:
+ * Gets the time of the next timeout
+ *
+ * Input Parameters:
+ * conn - The TCP "connection" to poll for TX data
+ *
+ * Returned Value:
+ * int - The time required for the next expiry (units: half-seconds)
+ *
+ * Assumptions:
+ * conn is not NULL.
+ * The connection (conn) is bound to the polling device (dev).
+ *
+ ****************************************************************************/
+
+static int tcp_get_timeout(FAR struct tcp_conn_s *conn)
+{
+ int timeout = conn->timer;
+
+#ifdef CONFIG_NET_TCP_KEEPALIVE
+ if (timeout == 0)
+ {
+ timeout = conn->keeptimer;
+ }
+ else if (conn->keeptimer > 0 && timeout > conn->keeptimer)
+ {
+ timeout = conn->keeptimer;
+ }
+#endif
+
+ return timeout;
+}
+
+/****************************************************************************
+ * Name: tcp_timer_expiry
+ *
+ * Description:
+ * Handle a TCP timer expiration for the provided TCP connection
+ * Restart a TCP timer if need to
+ *
+ * Input Parameters:
+ * arg - The TCP "connection" to poll for TX data
+ *
+ * Returned Value:
+ * None
+ *
+ * Assumptions:
+ * arg is not NULL.
+ * The connection (arg) is bound to the polling device (dev).
+ *
+ ****************************************************************************/
+
+static void tcp_timer_expiry(FAR void *arg)
+{
+ FAR struct tcp_conn_s *conn = arg;
+
+ conn->timeout = true;
+ conn->dev->d_txavail(conn->dev);
+}
+
+/****************************************************************************
+ * Name: tcp_update_timer
+ *
+ * Description:
+ * Update the TCP timer for the provided TCP connection,
+ * The timeout is accurate
+ *
+ * Input Parameters:
+ * conn - The TCP "connection" to poll for TX data
+ *
+ * Returned Value:
+ * None
+ *
+ * Assumptions:
+ * conn is not NULL.
+ * The connection (conn) is bound to the polling device (dev).
+ *
+ ****************************************************************************/
+
+static void tcp_update_timer(FAR struct tcp_conn_s *conn)
+{
+ int timeout = HSEC2TICK(tcp_get_timeout(conn));
+
+ if (timeout > 0)
+ {
+ sclock_t left = work_timeleft(&conn->work);
+ if (left <= 0 || TICK2HSEC(left) != timeout)
Review Comment:
I think with `int timeout = HSEC2TICK(tcp_get_timeout(conn));` we need to
```suggestion
if (left <= 0 || left != timeout)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #6330: tcp: move wd_timer from wifi driver to tcp stack
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6330:
URL: https://github.com/apache/incubator-nuttx/pull/6330#issuecomment-1138931720
> Hmm, do these changes depend on another PR not merged yet?
I think no. Could you try:
```
git remote add zhhyu7 git@github.com:zhhyu7/incubator-nuttx.git
git fetch zhhyu7
git cherry-pick zhhyu7/topic-3
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] zhhyu7 commented on a diff in pull request #6330: tcp: move wd_timer from wifi driver to tcp stack
Posted by GitBox <gi...@apache.org>.
zhhyu7 commented on code in PR #6330:
URL: https://github.com/apache/incubator-nuttx/pull/6330#discussion_r882471174
##########
net/devif/devif_poll.c:
##########
@@ -849,29 +798,7 @@ int devif_poll(FAR struct net_driver_s *dev, devif_poll_callback_t callback)
int devif_timer(FAR struct net_driver_s *dev, int delay,
devif_poll_callback_t callback)
{
-#if defined(NET_TCP_HAVE_STACK)
- int hsec = TICK2HSEC(delay);
-#endif
- int bstop = false;
-
-#ifdef NET_TCP_HAVE_STACK
- /* Traverse all of the active TCP connections and perform the
- * timer action.
- */
-
- bstop = devif_poll_tcp_timer(dev, callback, hsec);
-#endif
-
- /* If possible, continue with a normal poll checking for pending
- * network driver actions.
- */
-
- if (!bstop)
- {
- bstop = devif_poll(dev, callback);
- }
-
- return bstop;
+ return devif_poll(dev, callback);
Review Comment:
> so `delay` parameter now is ignored?
yes, we will remove all devif_timer in the next change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6330: tcp: move wd_timer from wifi driver to tcp stack
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6330:
URL: https://github.com/apache/incubator-nuttx/pull/6330#discussion_r882593383
##########
net/tcp/tcp_timer.c:
##########
@@ -74,10 +76,192 @@
#define ACK_DELAY (1)
+/****************************************************************************
+ * Private Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: tcp_get_timeout
+ *
+ * Description:
+ * Gets the time of the next timeout
+ *
+ * Input Parameters:
+ * conn - The TCP "connection" to poll for TX data
+ *
+ * Returned Value:
+ * int - The time required for the next expiry (units: half-seconds)
+ *
+ * Assumptions:
+ * conn is not NULL.
+ * The connection (conn) is bound to the polling device (dev).
+ *
+ ****************************************************************************/
+
+static int tcp_get_timeout(FAR struct tcp_conn_s *conn)
+{
+ int timeout = conn->timer;
+
+#ifdef CONFIG_NET_TCP_KEEPALIVE
+ if (timeout == 0)
+ {
+ timeout = conn->keeptimer;
+ }
+ else if (conn->keeptimer > 0 && timeout > conn->keeptimer)
+ {
+ timeout = conn->keeptimer;
+ }
+#endif
+
+ return timeout;
+}
+
+/****************************************************************************
+ * Name: tcp_timer_expiry
+ *
+ * Description:
+ * Handle a TCP timer expiration for the provided TCP connection
+ * Restart a TCP timer if need to
+ *
+ * Input Parameters:
+ * arg - The TCP "connection" to poll for TX data
+ *
+ * Returned Value:
+ * None
+ *
+ * Assumptions:
+ * arg is not NULL.
+ * The connection (arg) is bound to the polling device (dev).
+ *
+ ****************************************************************************/
+
+static void tcp_timer_expiry(FAR void *arg)
+{
+ FAR struct tcp_conn_s *conn = arg;
+
+ conn->timeout = true;
+ conn->dev->d_txavail(conn->dev);
+}
+
+/****************************************************************************
+ * Name: tcp_update_timer
+ *
+ * Description:
+ * Update the TCP timer for the provided TCP connection,
+ * The timeout is accurate
+ *
+ * Input Parameters:
+ * conn - The TCP "connection" to poll for TX data
+ *
+ * Returned Value:
+ * None
+ *
+ * Assumptions:
+ * conn is not NULL.
+ * The connection (conn) is bound to the polling device (dev).
+ *
+ ****************************************************************************/
+
+static void tcp_update_timer(FAR struct tcp_conn_s *conn)
+{
+ int ticks = HSEC2TICK(tcp_get_timeout(conn));
+
+ if (ticks > 0)
+ {
+ sclock_t left = work_timeleft(&conn->work);
+ if (left <= 0 || llabs(left - ticks) >= HSEC2TICK(1))
Review Comment:
Then maybe we can avoid `llabs(left - ticks)` usage, because it is not obvious.
Maybe we can
```
int timeout = tcp_get_timeout(conn);
if (timeout > 0)
{
sclock_t left = work_timeleft(&conn->work);
if (left <= 0 && TICK2HSEC(left) > timeout)
{
work_queue(LPWORK, &conn->work, tcp_timer_expiry,
conn, HSEC2TICK(timeout));
}
}
else
{
work_cancel(LPWORK, &conn->work);
}
```
or anything similar.
Maybe I understand logic in a wrong way. I do not understand `llabs(left - ticks) >= HSEC2TICK(1)`. So if `HSEC2TICK(tcp_get_timeout(conn))` is `5000` and left is `3000`, so condition will `llabs(3000 - 5000)` -> `2000` so we will start `work_queue` with `5000` timeout. The same is if `HSEC2TICK(tcp_get_timeout(conn))` is `7000`and left is `3000`.
So my question is when do we need to start `work_queue`? If it is possible to write as a bullet list?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #6330: tcp: move wd_timer from wifi driver to tcp stack
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6330:
URL: https://github.com/apache/incubator-nuttx/pull/6330#issuecomment-1147149822
It's here: https://github.com/apache/incubator-nuttx/pull/6355, @masayuki2009
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6330: tcp: move wd_timer from wifi driver to tcp stack
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6330:
URL: https://github.com/apache/incubator-nuttx/pull/6330#discussion_r882593383
##########
net/tcp/tcp_timer.c:
##########
@@ -74,10 +76,192 @@
#define ACK_DELAY (1)
+/****************************************************************************
+ * Private Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: tcp_get_timeout
+ *
+ * Description:
+ * Gets the time of the next timeout
+ *
+ * Input Parameters:
+ * conn - The TCP "connection" to poll for TX data
+ *
+ * Returned Value:
+ * int - The time required for the next expiry (units: half-seconds)
+ *
+ * Assumptions:
+ * conn is not NULL.
+ * The connection (conn) is bound to the polling device (dev).
+ *
+ ****************************************************************************/
+
+static int tcp_get_timeout(FAR struct tcp_conn_s *conn)
+{
+ int timeout = conn->timer;
+
+#ifdef CONFIG_NET_TCP_KEEPALIVE
+ if (timeout == 0)
+ {
+ timeout = conn->keeptimer;
+ }
+ else if (conn->keeptimer > 0 && timeout > conn->keeptimer)
+ {
+ timeout = conn->keeptimer;
+ }
+#endif
+
+ return timeout;
+}
+
+/****************************************************************************
+ * Name: tcp_timer_expiry
+ *
+ * Description:
+ * Handle a TCP timer expiration for the provided TCP connection
+ * Restart a TCP timer if need to
+ *
+ * Input Parameters:
+ * arg - The TCP "connection" to poll for TX data
+ *
+ * Returned Value:
+ * None
+ *
+ * Assumptions:
+ * arg is not NULL.
+ * The connection (arg) is bound to the polling device (dev).
+ *
+ ****************************************************************************/
+
+static void tcp_timer_expiry(FAR void *arg)
+{
+ FAR struct tcp_conn_s *conn = arg;
+
+ conn->timeout = true;
+ conn->dev->d_txavail(conn->dev);
+}
+
+/****************************************************************************
+ * Name: tcp_update_timer
+ *
+ * Description:
+ * Update the TCP timer for the provided TCP connection,
+ * The timeout is accurate
+ *
+ * Input Parameters:
+ * conn - The TCP "connection" to poll for TX data
+ *
+ * Returned Value:
+ * None
+ *
+ * Assumptions:
+ * conn is not NULL.
+ * The connection (conn) is bound to the polling device (dev).
+ *
+ ****************************************************************************/
+
+static void tcp_update_timer(FAR struct tcp_conn_s *conn)
+{
+ int ticks = HSEC2TICK(tcp_get_timeout(conn));
+
+ if (ticks > 0)
+ {
+ sclock_t left = work_timeleft(&conn->work);
+ if (left <= 0 || llabs(left - ticks) >= HSEC2TICK(1))
Review Comment:
Then maybe we can avoid `llabs(left - ticks)` usage, because it is not obvious.
Maybe we can
```
int timeout = tcp_get_timeout(conn);
if (timeout > 0)
{
sclock_t left = work_timeleft(&conn->work);
if (left <= 0 && TICK2HSEC(left) > timeout)
{
work_queue(LPWORK, &conn->work, tcp_timer_expiry,
conn, HSEC2TICK(timeout));
}
}
else
{
work_cancel(LPWORK, &conn->work);
}
```
or anything similar.
Maybe I understand logic in a wrong way. I do not understand `llabs(left - ticks) >= HSEC2TICK(1)`. So if `HSEC2TICK(tcp_get_timeout(conn))` is `5000` and left is `3000`, so condition will `llabs(3000 - 5000)` -> `2000` so we will start `work_queue` with `5000` timeout.
So my question is when do we need to start `work_queue`? If it is possible to write as a bullet list?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6330: tcp: move wd_timer from wifi driver to tcp stack
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6330:
URL: https://github.com/apache/incubator-nuttx/pull/6330#discussion_r882505841
##########
net/tcp/tcp_timer.c:
##########
@@ -74,10 +76,192 @@
#define ACK_DELAY (1)
+/****************************************************************************
+ * Private Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: tcp_get_timeout
+ *
+ * Description:
+ * Gets the time of the next timeout
+ *
+ * Input Parameters:
+ * conn - The TCP "connection" to poll for TX data
+ *
+ * Returned Value:
+ * int - The time required for the next expiry (units: half-seconds)
+ *
+ * Assumptions:
+ * conn is not NULL.
+ * The connection (conn) is bound to the polling device (dev).
+ *
+ ****************************************************************************/
+
+static int tcp_get_timeout(FAR struct tcp_conn_s *conn)
+{
+ int timeout = conn->timer;
+
+#ifdef CONFIG_NET_TCP_KEEPALIVE
+ if (timeout == 0)
+ {
+ timeout = conn->keeptimer;
+ }
+ else if (conn->keeptimer > 0 && timeout > conn->keeptimer)
+ {
+ timeout = conn->keeptimer;
+ }
+#endif
+
+ return timeout;
+}
+
+/****************************************************************************
+ * Name: tcp_timer_expiry
+ *
+ * Description:
+ * Handle a TCP timer expiration for the provided TCP connection
+ * Restart a TCP timer if need to
+ *
+ * Input Parameters:
+ * arg - The TCP "connection" to poll for TX data
+ *
+ * Returned Value:
+ * None
+ *
+ * Assumptions:
+ * arg is not NULL.
+ * The connection (arg) is bound to the polling device (dev).
+ *
+ ****************************************************************************/
+
+static void tcp_timer_expiry(FAR void *arg)
+{
+ FAR struct tcp_conn_s *conn = arg;
+
+ conn->timeout = true;
+ conn->dev->d_txavail(conn->dev);
+}
+
+/****************************************************************************
+ * Name: tcp_update_timer
+ *
+ * Description:
+ * Update the TCP timer for the provided TCP connection,
+ * The timeout is accurate
+ *
+ * Input Parameters:
+ * conn - The TCP "connection" to poll for TX data
+ *
+ * Returned Value:
+ * None
+ *
+ * Assumptions:
+ * conn is not NULL.
+ * The connection (conn) is bound to the polling device (dev).
+ *
+ ****************************************************************************/
+
+static void tcp_update_timer(FAR struct tcp_conn_s *conn)
+{
+ int ticks = HSEC2TICK(tcp_get_timeout(conn));
+
+ if (ticks > 0)
+ {
+ sclock_t left = work_timeleft(&conn->work);
+ if (left <= 0 || llabs(left - ticks) >= HSEC2TICK(1))
Review Comment:
Could you please describe more about `llabs(left - ticks) >= HSEC2TICK(1)`. It is not obvious for me.
Are we trying to adjust work queue timeout in case current timeout is less than `tcp_get_timeout()`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #6330: tcp: move wd_timer from wifi driver to tcp stack
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6330:
URL: https://github.com/apache/incubator-nuttx/pull/6330#discussion_r882572872
##########
net/tcp/tcp_timer.c:
##########
@@ -74,10 +76,192 @@
#define ACK_DELAY (1)
+/****************************************************************************
+ * Private Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: tcp_get_timeout
+ *
+ * Description:
+ * Gets the time of the next timeout
+ *
+ * Input Parameters:
+ * conn - The TCP "connection" to poll for TX data
+ *
+ * Returned Value:
+ * int - The time required for the next expiry (units: half-seconds)
+ *
+ * Assumptions:
+ * conn is not NULL.
+ * The connection (conn) is bound to the polling device (dev).
+ *
+ ****************************************************************************/
+
+static int tcp_get_timeout(FAR struct tcp_conn_s *conn)
+{
+ int timeout = conn->timer;
+
+#ifdef CONFIG_NET_TCP_KEEPALIVE
+ if (timeout == 0)
+ {
+ timeout = conn->keeptimer;
+ }
+ else if (conn->keeptimer > 0 && timeout > conn->keeptimer)
+ {
+ timeout = conn->keeptimer;
+ }
+#endif
+
+ return timeout;
+}
+
+/****************************************************************************
+ * Name: tcp_timer_expiry
+ *
+ * Description:
+ * Handle a TCP timer expiration for the provided TCP connection
+ * Restart a TCP timer if need to
+ *
+ * Input Parameters:
+ * arg - The TCP "connection" to poll for TX data
+ *
+ * Returned Value:
+ * None
+ *
+ * Assumptions:
+ * arg is not NULL.
+ * The connection (arg) is bound to the polling device (dev).
+ *
+ ****************************************************************************/
+
+static void tcp_timer_expiry(FAR void *arg)
+{
+ FAR struct tcp_conn_s *conn = arg;
+
+ conn->timeout = true;
+ conn->dev->d_txavail(conn->dev);
+}
+
+/****************************************************************************
+ * Name: tcp_update_timer
+ *
+ * Description:
+ * Update the TCP timer for the provided TCP connection,
+ * The timeout is accurate
+ *
+ * Input Parameters:
+ * conn - The TCP "connection" to poll for TX data
+ *
+ * Returned Value:
+ * None
+ *
+ * Assumptions:
+ * conn is not NULL.
+ * The connection (conn) is bound to the polling device (dev).
+ *
+ ****************************************************************************/
+
+static void tcp_update_timer(FAR struct tcp_conn_s *conn)
+{
+ int ticks = HSEC2TICK(tcp_get_timeout(conn));
+
+ if (ticks > 0)
+ {
+ sclock_t left = work_timeleft(&conn->work);
+ if (left <= 0 || llabs(left - ticks) >= HSEC2TICK(1))
Review Comment:
All timer in tcp use half second as the unit:
https://github.com/apache/incubator-nuttx/blob/master/net/tcp/tcp.h#L195
https://github.com/apache/incubator-nuttx/blob/master/net/tcp/tcp.h#L287-L292
so we can save the unnecessary work operation if the difference between the left time of work and the timeout value is smaller than the half second.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 merged pull request #6330: tcp: move wd_timer from wifi driver to tcp stack
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged PR #6330:
URL: https://github.com/apache/incubator-nuttx/pull/6330
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6330: tcp: move wd_timer from wifi driver to tcp stack
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6330:
URL: https://github.com/apache/incubator-nuttx/pull/6330#discussion_r882593383
##########
net/tcp/tcp_timer.c:
##########
@@ -74,10 +76,192 @@
#define ACK_DELAY (1)
+/****************************************************************************
+ * Private Functions
+ ****************************************************************************/
+
+/****************************************************************************
+ * Name: tcp_get_timeout
+ *
+ * Description:
+ * Gets the time of the next timeout
+ *
+ * Input Parameters:
+ * conn - The TCP "connection" to poll for TX data
+ *
+ * Returned Value:
+ * int - The time required for the next expiry (units: half-seconds)
+ *
+ * Assumptions:
+ * conn is not NULL.
+ * The connection (conn) is bound to the polling device (dev).
+ *
+ ****************************************************************************/
+
+static int tcp_get_timeout(FAR struct tcp_conn_s *conn)
+{
+ int timeout = conn->timer;
+
+#ifdef CONFIG_NET_TCP_KEEPALIVE
+ if (timeout == 0)
+ {
+ timeout = conn->keeptimer;
+ }
+ else if (conn->keeptimer > 0 && timeout > conn->keeptimer)
+ {
+ timeout = conn->keeptimer;
+ }
+#endif
+
+ return timeout;
+}
+
+/****************************************************************************
+ * Name: tcp_timer_expiry
+ *
+ * Description:
+ * Handle a TCP timer expiration for the provided TCP connection
+ * Restart a TCP timer if need to
+ *
+ * Input Parameters:
+ * arg - The TCP "connection" to poll for TX data
+ *
+ * Returned Value:
+ * None
+ *
+ * Assumptions:
+ * arg is not NULL.
+ * The connection (arg) is bound to the polling device (dev).
+ *
+ ****************************************************************************/
+
+static void tcp_timer_expiry(FAR void *arg)
+{
+ FAR struct tcp_conn_s *conn = arg;
+
+ conn->timeout = true;
+ conn->dev->d_txavail(conn->dev);
+}
+
+/****************************************************************************
+ * Name: tcp_update_timer
+ *
+ * Description:
+ * Update the TCP timer for the provided TCP connection,
+ * The timeout is accurate
+ *
+ * Input Parameters:
+ * conn - The TCP "connection" to poll for TX data
+ *
+ * Returned Value:
+ * None
+ *
+ * Assumptions:
+ * conn is not NULL.
+ * The connection (conn) is bound to the polling device (dev).
+ *
+ ****************************************************************************/
+
+static void tcp_update_timer(FAR struct tcp_conn_s *conn)
+{
+ int ticks = HSEC2TICK(tcp_get_timeout(conn));
+
+ if (ticks > 0)
+ {
+ sclock_t left = work_timeleft(&conn->work);
+ if (left <= 0 || llabs(left - ticks) >= HSEC2TICK(1))
Review Comment:
Then maybe we can avoid `llabs(left - ticks)` usage, because it is not obvious.
Maybe we can
```
int timeout = tcp_get_timeout(conn);
if (timeout > 0)
{
sclock_t left = work_timeleft(&conn->work);
if (left <= 0 && TICK2HSEC(left) > timeout)
{
work_queue(LPWORK, &conn->work, tcp_timer_expiry,
conn, HSEC2TICK(timeout));
}
}
else
{
work_cancel(LPWORK, &conn->work);
}
```
or anything similar.
Maybe I understand logic in a wrong way. I do not understand `llabs(left - ticks) >= HSEC2TICK(1)`. So if `HSEC2TICK(tcp_get_timeout(conn))` is `5000` and left is `3000`, so condition will `llabs(3000 - 5000)` -> `2000` so we will start `work_queue` with `5000` timeout. The same is if `HSEC2TICK(tcp_get_timeout(conn))` is `3000`and left is `5000`.
So my question is when do we need to start `work_queue`? If it is possible to write as a bullet list?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org