Skip to content

Conversation

htibosch
Copy link
Contributor

Description

The timer xTCPTimer is a timer that tells when the TCP engine must become active by calling xTCPTimerCheck(). In some occasions, the timer is forced to expire, for instance when recv() was called and created enough space in the reception stream: a WIN update shall be sent immediately.
However, after a certain commit to the branch IntegrationTesting1, the TCP timer was set as enabled, instead of expired.

This PR makes the following change:

-void vIPSetTCPTimerEnableState( BaseType_t xEnableState );
+void vIPSetTCPTimerExpiredState( BaseType_t xEnableState );

and

+        xTCPTimer.bActive = pdTRUE_UNSIGNED;
         if( xEnableState != pdFALSE )
         {
-            xTCPTimer.bActive = pdTRUE_UNSIGNED;
+            xTCPTimer.bExpired = pdTRUE_UNSIGNED;
         }
         else
         {
-            xTCPTimer.bActive = pdFALSE_UNSIGNED;
+            xTCPTimer.bExpired = pdFALSE_UNSIGNED;
         }

Test Steps

Send a constant stream of TCP data and look at the performance. There will be periods of inactivity.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@htibosch htibosch requested a review from a team as a code owner June 10, 2022 03:04
@angelonakos
Copy link
Contributor

CBMC_RETRY

Copy link
Member

@AniruddhaKanhere AniruddhaKanhere left a comment

Choose a reason for hiding this comment

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

@htibosch thank you for finding the bug and the raising the PR to fix it.

I just have one question. Grateful if you can answer that.

Thanks

void vIPSetTCPTimerExpiredState( BaseType_t xExpiredState )
{
if( xEnableState != pdFALSE )
xTCPTimer.bActive = pdTRUE_UNSIGNED;
Copy link
Member

Choose a reason for hiding this comment

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

If the timer is just being marked as expired, why do we need to mark it as active?
Is there any place where we will mark it as inactive thus potentially requiring the timer to be set as active again?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AniruddhaKanhere wrote:

If the timer is just being marked as expired, why do we need to mark it as active?

Because when a timer is inactive, it will be ignored by the function prvIPTimerCheck():

static BaseType_t prvIPTimerCheck( IPTimer_t * pxTimer )
{
    BaseType_t xReturn;

    if( pxTimer->bActive == pdFALSE_UNSIGNED )
    {
        /* The timer is not enabled. */
        xReturn = pdFALSE;
    }
    else

and so prvIPTimerCheck() will never see the expired flag :-(

Is there any place where we will mark it as inactive thus potentially requiring the timer to be set as active again?

The flag bActive is set during every call to vIPSetTCPTimerExpiredState(), while nobody will ever clear it. It would be enough to set it once at start-up.

At the other hand, that would require a new function vIPSetTCPTimerEnableState() and an extra function-call at start-up, and one more unit test... without a measurable performance gain.

@AniruddhaKanhere AniruddhaKanhere merged commit 2b99a0c into FreeRTOS:main Jun 13, 2022
@htibosch htibosch changed the title Let the TCP timer becomes expired in stead of active Let the TCP timer become expired in stead of active Jun 16, 2022
@htibosch htibosch deleted the repair_tcp_timer branch September 17, 2025 06:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants