Skip to content

Let the TCP timer become expired in stead of active #481

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

Merged
merged 7 commits into from
Jun 13, 2022
Merged

Let the TCP timer become expired in stead of active #481

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a194165
Let the TCP timer becomes expired in stead of active
Jun 10, 2022
da209a1
Merge branch 'main' into repair_tcp_timer
htibosch Jun 10, 2022
23b7268
Renamed parameter of function vIPSetTCPTimerExpiredState
Jun 10, 2022
407d22d
Adapt unit tests to use the new name and field
Jun 10, 2022
8bab49e
Change bActive to bExpired
Jun 10, 2022
5497db5
Empty commit
AniruddhaKanhere Jun 10, 2022
b1b0d2b
Fix spell check
AniruddhaKanhere Jun 10, 2022
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
1 change: 1 addition & 0 deletions .github/lexicon.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1478,6 +1478,7 @@ xeventgroup
xeventgroupwaitbits
xexpected
xexpectedmessagetype
xexpiredstate
xflags
xfound
xfreebufferslist
Expand Down
4 changes: 2 additions & 2 deletions source/FreeRTOS_IP.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -429,7 +429,7 @@ static void prvProcessIPEventsAndTimers( void )

/* Simply mark the TCP timer as expired so it gets processed
* the next time prvCheckNetworkTimers() is called. */
vIPSetTCPTimerEnableState( pdTRUE );
vIPSetTCPTimerExpiredState( pdTRUE );
#endif /* ipconfigUSE_TCP */
break;

Expand Down Expand Up @@ -1100,7 +1100,7 @@ BaseType_t xSendEventStructToIPTask( const IPStackEvent_t * pxEvent,
/* TCP timer events are sent to wake the timer task when
* xTCPTimer has expired, but there is no point sending them if the
* IP task is already awake processing other message. */
vIPSetTCPTimerEnableState( pdTRUE );
vIPSetTCPTimerExpiredState( pdTRUE );

if( uxQueueMessagesWaiting( xNetworkEventQueue ) != 0U )
{
Expand Down
12 changes: 7 additions & 5 deletions source/FreeRTOS_IP_Timers.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -382,17 +382,19 @@ static BaseType_t prvIPTimerCheck( IPTimer_t * pxTimer )
/**
* @brief Enable/disable the TCP timer.
*
* @param[in] xEnableState: pdTRUE - enable timer; pdFALSE - disable timer.
* @param[in] xExpiredState: pdTRUE - set as expired; pdFALSE - set as non-expired.
*/
void vIPSetTCPTimerEnableState( BaseType_t xEnableState )
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.


if( xExpiredState != pdFALSE )
{
xTCPTimer.bActive = pdTRUE_UNSIGNED;
xTCPTimer.bExpired = pdTRUE_UNSIGNED;
}
else
{
xTCPTimer.bActive = pdFALSE_UNSIGNED;
xTCPTimer.bExpired = pdFALSE_UNSIGNED;
}
}
/*-----------------------------------------------------------*/
Expand Down
2 changes: 1 addition & 1 deletion source/include/FreeRTOS_IP_Timers.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@

void vIPTimerStartARPResolution( TickType_t xTime );

void vIPSetTCPTimerEnableState( BaseType_t xEnableState );
void vIPSetTCPTimerExpiredState( BaseType_t xExpiredState );

void vIPSetARPTimerEnableState( BaseType_t xEnableState );

Expand Down
8 changes: 4 additions & 4 deletions test/unit-test/FreeRTOS_IP/FreeRTOS_IP_utest.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -945,7 +945,7 @@ void test_prvProcessIPEventsAndTimers_eTCPTimerEvent( void )
xQueueReceive_ExpectAnyArgsAndReturn( pdTRUE );
xQueueReceive_ReturnMemThruPtr_pvBuffer( &xReceivedEvent, sizeof( xReceivedEvent ) );

vIPSetTCPTimerEnableState_Expect( pdTRUE );
vIPSetTCPTimerExpiredState_Expect( pdTRUE );

prvProcessIPEventsAndTimers();
}
Expand Down Expand Up @@ -1371,7 +1371,7 @@ void test_xSendEventStructToIPTask_IPTaskInit_eTCPTimerEvent( void )
xIPTaskInitialised = pdTRUE;
xEvent.eEventType = eTCPTimerEvent;

vIPSetTCPTimerEnableState_Expect( pdTRUE );
vIPSetTCPTimerExpiredState_Expect( pdTRUE );

uxQueueMessagesWaiting_ExpectAndReturn( xNetworkEventQueue, 0 );

Expand All @@ -1393,7 +1393,7 @@ void test_xSendEventStructToIPTask_IPTaskInit_eTCPTimerEvent1( void )
xIPTaskInitialised = pdTRUE;
xEvent.eEventType = eTCPTimerEvent;

vIPSetTCPTimerEnableState_Expect( pdTRUE );
vIPSetTCPTimerExpiredState_Expect( pdTRUE );

uxQueueMessagesWaiting_ExpectAndReturn( xNetworkEventQueue, 0 );

Expand All @@ -1415,7 +1415,7 @@ void test_xSendEventStructToIPTask_IPTaskInit_eTCPTimerEvent2( void )
xIPTaskInitialised = pdTRUE;
xEvent.eEventType = eTCPTimerEvent;

vIPSetTCPTimerEnableState_Expect( pdTRUE );
vIPSetTCPTimerExpiredState_Expect( pdTRUE );

uxQueueMessagesWaiting_ExpectAndReturn( xNetworkEventQueue, 1 );

Expand Down
16 changes: 8 additions & 8 deletions test/unit-test/FreeRTOS_IP_Timers/FreeRTOS_IP_Timers_utest.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -531,22 +531,22 @@ void test_prvIPTimerCheck_TimerNotExpired1( void )
TEST_ASSERT_EQUAL( pdFALSE, xTimer.bExpired );
}

void test_vIPSetTCPTimerEnableState_False( void )
void test_vIPSetTCPTimerExpiredState_False( void )
{
BaseType_t xEnableState = pdFALSE;
BaseType_t xExpiredState = pdFALSE;

vIPSetTCPTimerEnableState( xEnableState );
vIPSetTCPTimerExpiredState( xExpiredState );

TEST_ASSERT_EQUAL( xEnableState, xTCPTimer.bActive );
TEST_ASSERT_EQUAL( xExpiredState, xTCPTimer.bExpired );
}

void test_vIPSetTCPTimerEnableState_True( void )
void test_vIPSetTCPTimerExpiredState_True( void )
{
BaseType_t xEnableState = pdTRUE;
BaseType_t xExpiredState = pdTRUE;

vIPSetTCPTimerEnableState( xEnableState );
vIPSetTCPTimerExpiredState( xExpiredState );

TEST_ASSERT_EQUAL( xEnableState, xTCPTimer.bActive );
TEST_ASSERT_EQUAL( xExpiredState, xTCPTimer.bExpired );
}

void test_vIPSetARPTimerEnableState_False( void )
Expand Down