Add NetworkDown notification to NetworkInterface.c

[zipperowiec]

Add NetworkDown notification support to STM32FXX network interface.

Description

NetworkInterface for STM32FXX lacks support for NetworkDown notification.
Current driven only disables MAC interface without notification to higher layers.
Proposed solution add this notification after MAC layer operations.

Test Steps

1. Run FreeRTOS TCP stack on STM32FXX board with ethernet cable attached.
2. Disconnect ethernet cable.
3. NetworkDown notification will be submitted, higher stack layers will be notified about situation and user callback will be called ( vApplicationIPNetworkEventHook(eNetworkDown) )

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

[zipperowiec]

/bot run uncrustify

[shubnil]

Hi,
Thanks for sharing the change.
However, I have a query. Are there some observations which make it specific to STM32FXX or is it applicable to all HW. Please clarify.

[zipperowiec]

Hi shubnil,

I would say this solution is pretty generic.

I used FreeRTOS_NetworkDown() to notify IP stack that network is down and this function has no hardware dependencies.
However, Ethernet driver must support some mechanism that give us an information about network state (xGetPhyLinkStatus() it STM32FXXX case) and place where we get information from networkPhy that link is down

prvEMACHandlerTask():
if( xPhyCheckLinkStatus( &xPhyObject, xResult ) != 0 ) // <-- link is down here
{
    /* Something has changed to a Link Status, need re-check. */
    prvEthernetUpdateConfig( pdFALSE ); // disable mac layer, calls  HAL_ETH_Stop( &xETH ); inside

    if( xGetPhyLinkStatus() == pdFALSE )
    {
        FreeRTOS_NetworkDown();
    }
}

Keep in mind that if driver refresh state in IRQ instead of task (as it's done in STM32Fxx case) then the is a need to use FreeRTOS_NetworkDownFromISR() instead of FreeRTOS_NetworkDown();

[AniruddhaKanhere]

Thank you for this PR @zipperowiec!

Seems like an appropriate change to me.
Can you also take a look @htibosch?

[htibosch]

Hello @zipperowiec,

You wrote:

give us an information about network state (xGetPhyLinkStatus() it STM32FXXX case)

Every network driver must implement one of these functions:

    /* Single interface ( see main branch ) */
    BaseType_t xGetPhyLinkStatus( void );

    /* In case multiple interfaces are used ( see labs/ipv6_multi branch ) */
    BaseType_t xGetPhyLinkStatus( struct xNetworkInterface *pxInterface );

The mechanism of going "down" and "up" is very important. While going up, an IP-address will be acquired, either static or dynamically.
Indeed the application will be informed, so it can (forcefully) close all sockets.

When the connection to a LAN is interrupted for a short period of time, any open TCP connection can continue immediately after the connection is restored. That is why I would not have any hurry to respond.
The function FreeRTOS_NetworkDownFromISR() and other FromISR in the +TCP library are more or less deprecated. Interrupts should just notify the application, and leave the work to a normal task.

There are 2 macro's that determine the Link Status polling behaviour:

    /* Check if the LinkStatus in the PHY is still high after
     * 15 seconds of not receiving packets. */
    #define ipconfigPHY_LS_HIGH_CHECK_TIME_MS    15000U

    /* Check if the LinkStatus in the PHY is still low every second. */
    #define ipconfigPHY_LS_LOW_CHECK_TIME_MS     1000U

( you can define these macro's in your copy of FreeRTOSIPConfig.h )

So when the disconnection is shorter than 15 seconds, it is assumed that the same LAN is still in use, and there is no need to go down and up.

Note that polling the PHY very frequently can be disturbing for the data transport. In a Xilinx Zynq I once saw tens of packets dropped because the driver was polling the PHY every 100 ms or so.

There is nothing wrong with your change, except that it is not downward compatible. It will break existing applications and the developers will be surprised about it.

Here is solution that uses an "iptrace" macro:

 if( xPhyCheckLinkStatus( &xPhyObject, xResult ) != 0 )
 {
     /* Something has changed to a Link Status, need re-check. */
     prvEthernetUpdateConfig( pdFALSE );
 
+    iptraceNETWORK_INTERFACE_STATUS_CHANGE()
 }

The macro or macros should be added to IPTraceMacroDefaults.h along with a clear comment:

    /* This macro will be called when the Link Status of the PHY changes.
     * xGetPhyLinkStatus() can be called to determine the current status. */
    #define iptraceNETWORK_INTERFACE_STATUS_CHANGE()

And in your FreeRTOSIPConfig.h :

#define iptraceNETWORK_INTERFACE_STATUS_CHANGE() \
    if( xGetPhyLinkStatus() == pdFALSE ) \
    { \
        FreeRTOS_NetworkDown(); \
    }

[oleszekf]

Hi @htibosch,

Thank you very much for your comment, there are many important points in it.

However, I would like to address your concerns:
Context
The main point for this PR is that STM32FXX network layer didn't inform IP stack about "Network Down" event and by extend User also hadn't information about this event. Changes in this PR address this issue.

polling the PHY
I agree that polling the PHY to often can leads to problems in the system. However non of my changes touch the PHY polling frequency or mechanism how and when this polling is realized.
The function xGetPhyLinkStatus() doesn't read any information from PHY, it just returns internal state of the network driver:

BaseType_t xGetPhyLinkStatus( void )
{
    BaseType_t xReturn;
    if( xPhyObject.ulLinkStatusMask != 0 )
    {
        xReturn = pdPASS;
    }
    else
    {
        xReturn = pdFAIL;
    }
    return xReturn;
}

not downward compatible
Could you please describe with more details how this PR breaks the downward combability?

FreeRTOS+TCP main mechanism to inform user about network state is vApplicationIPNetworkEventHook callback.
It can be called with eNetworkUp or eNetworkDown flag. This PR doesn't change this, It only fills the gap in network driver implementation for STM32FXX that was lack of support for eNetworkDown notification.

Even the example implementation of this callback, which can be found in documentation, states that user shall check if vApplicationIPNetworkEventHook has been called with eNetworkUp or eNetworkDown event before perform any actions.

/* Defined by the application code, but called by FreeRTOS-Plus-TCP when the network
connects/disconnects (if ipconfigUSE_NETWORK_EVENT_HOOK is set to 1 in
FreeRTOSIPConfig.h). */
void vApplicationIPNetworkEventHook( eIPCallbackEvent_t eNetworkEvent )
{
uint32_t ulIPAddress, ulNetMask, ulGatewayAddress, ulDNSServerAddress;
static BaseType_t xTasksAlreadyCreated = pdFALSE;
int8_t cBuffer[ 16 ];

    /* Check this was a network up event, as opposed to a network down event. */
    if( eNetworkEvent == eNetworkUp )
    {
    .... rest of the code ....

vApplicationIPNetworkEventHook

[htibosch]

Could you please describe with more details how this PR breaks the downward combability?

Very simple: when I disconnect and connect my STM32Fx board, the IP-address will remain the same.
When the driver calls FreeRTOS_NetworkDown(), the DHCP procedure will start again.

I propose in stead that a macro is placed:

    iptraceNETWORK_INTERFACE_STATUS_CHANGE();

And you can define the macro as you wish in your FreeRTOSIPConfig.h:

#define iptraceNETWORK_INTERFACE_STATUS_CHANGE() \
    if( xGetPhyLinkStatus() == pdFALSE ) \
    { \
        FreeRTOS_NetworkDown(); \
    }

I'm sorry, but I refuse to approve your PR as long as it will call FreeRTOS_NetworkDown() unconditionally.

[zipperowiec]

@htibosch
I'm not sure if FreeRTOS TCP stack supports it but in general DHCP standard allows to request previously assigned IP address.
But I see your point here, however I have a different proposition for that, because I have some code related concerns regarding your solution.

FreeRTOS_NetworkDown() network function is define in FreeRTOS_Ip_Private.h header and using it in user code breaks the stack interface and ther is no garantee that function will not be changed/deleted in the future (like it is an stack internals).

So, my proposal is:
to introduce config flag ipconfigSUPPORT_NETWORK_DOWN_EVENT
By default ipconfigSUPPORT_NETWORK_DOWN_EVENT will not be defined -> there won't be any changes in driver behavior.

And change code to:

....Rest of the code....
        if( xPhyCheckLinkStatus( &xPhyObject, xResult ) != 0 )
        {
            /* Something has changed to a Link Status, need re-check. */
            prvEthernetUpdateConfig( pdFALSE );

#ifdef ipconfigSUPPORT_NETWORK_DOWN_EVENT 
            if( xGetPhyLinkStatus() == pdFALSE )
            {
                FreeRTOS_NetworkDown();
            }
#endif
        }
    }

Which hides internals of the stack from the user.

ipconfigSUPPORT_NETWORK_DOWN_EVENT with the code solution can be later reused in the other network drivers,

What do you think about this?

[zipperowiec]

@htibosch ?

[htibosch]

Hi @zipperowiec, sorry to respond so late. We were very busy with the integration of IPv6 into mainline. Also in the new release there will be the possibility to use multiple interfaces.

I'm not sure if FreeRTOS TCP stack supports it but in general DHCP standard allows to request previously assigned IP address.

Yes there is this function:

uint32_t vDHCPSetPreferredIPAddress( uint32_t ulIPAddress );

The DHCP client will communicate this IP-address with the server and most probably get it honoured.

But I see your point here, however I have a different proposition for that, because I have some code related concerns regarding your solution. FreeRTOS_NetworkDown() network function is define in FreeRTOS_Ip_Private.h header and using it in user code breaks the stack interface and there is no garantee that function will not be changed/deleted in the future (like it is an stack internals).

Yes, you've got a point there. I'm also happy with:

#if( ipconfigSUPPORT_NETWORK_DOWN_EVENT != 0 )
{
    if( xGetPhyLinkStatus() == pdFALSE )
    {
        FreeRTOS_NetworkDown();
    }
}
#endif /* ( ipconfigSUPPORT_NETWORK_DOWN_EVENT != 0 ) */

By default ipconfigSUPPORT_NETWORK_DOWN_EVENT will not be defined -> there won't be any changes in driver behavior.

In FreeRTOS projects, we rarely use #ifdef but in stead we use #if( X != 0 ) as in my example here above. And also we like to have all config variables defined, often as 0 or pdFALSE.

First the compiler will see the private FreeRTOSIPConfig.h, and then it will see FreeRTOSIPConfigDefaults.h.

Which hides internals of the stack from the user.

Agreed!

ipconfigSUPPORT_NETWORK_DOWN_EVENT with the code solution can be later reused in the other network drivers

Would be good.

Thanks so far!
Hein

[shubnil]

I agree with Hein. Let us add a macro ipconfigSUPPORT_NETWORK_DOWN_EVENT.

[shubnil]

Please add the ipconfigSUPPORT_NETWORK_DOWN_EVENT macros check and then we would be fine.

[oleszekf]

Hi, @htibosch @shubnil

Thanks for your replies, it seems that we have solution :)
I will change code accordingly later this week

[moninom1]

Hi @oleszekf, Thank you again for the changes. If you would like can we go ahead and make this minor update and get the change merged?

[oleszekf]

Hi, @moninom1
Code changes from my side will be ready today evening ( CET time :) )

[zipperowiec]

/bot run uncrustify

[oleszekf]

/bot run uncrustify

[tony-josi-aws]

/bot run uncrustify

[oleszekf]

Hi @tony-josi-aws,

What is the reason for closing my PR?

[tony-josi-aws]

Hi @oleszekf, It seems the PR was autoclosed (by Github) when the formatting changes was force pushed to this PR. Can you please reopen this. Sorry for the inconvenience.