Update ARP cache only when necessary and use ARP cache to translate IP to MAC

[AniruddhaKanhere]

Description

This PR removes the call to refresh ARP cache entry when a UDP/TCP packet is received. It replaces that call with a function to only update the ARP entry age.
Additionally, the translation of IP->MAC must be done through ARP cache. Thus this PR translates an IP to MAC using ARP cache instead of switching the source and destination MAC address before sending any packet.

Test Steps

Related Issue

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

[htibosch]

Hello Aniruddha, thank you again for this PR. Except for some minor things it looks good to me. I will include in my integration testing before approving it.
Thanks,

[htibosch]

The function memcmp() is called, xMatchingMAC will be set, but during most loops it will not be used.
Maybe rewrite it to:

        for( x = 0; x < ipconfigARP_CACHE_ENTRIES; x++ )
        {
            /* Does this line in the cache table hold an entry for the IP
             * address being queried? */
            if( xARPCache[ x ].ulIPAddress == ulIPAddress )
            {
                /* Does it have the same MAC-address? */
                if( memcmp( xARPCache[ x ].xMACAddress.ucBytes, pxMACAddress->ucBytes, sizeof( pxMACAddress->ucBytes ) ) == 0 )
                {
                    /* The IP address and the MAC matched, update the entry age. */
                    xARPCache[ x ].ucAge = ( uint8_t ) ipconfigMAX_ARP_AGE;
                    break;
                }
            }
        }

The existing function vARPRefreshCacheEntry() is slightly different: xMatchingMAC is (almost) always used.

[htibosch]

At the top of this function vReturnEthernetFrame(), 2 variables are defined:

/* memcpy() helper variables for MISRA Rule 21.15 compliance*/
    const void * pvCopySource;
    void * pvCopyDest;

We might as well move the down to this compound block with the other local variables.

[htibosch]

Just thinking out loud:

/* Try to find a MAC address corresponding to the destination IP
 * address. */
eResult = eARPGetCacheEntry( &ulDestinationIPAddress, &xMACAddress );

When given an IP-address outside the netmask, eARPGetCacheEntry() will set xMACAddress to the MAC-address of the gateway, which is good.
In case there is no gateway, or in case the gateway's IP-address is not yet resolved, it will set ulDestinationIPAddress to the IP-address of the gateway, and return eARPCacheMiss. In that case we'll use the earlier method: just swap the MAC-address in the packet.

Ergo, it looks all good to me!

[htibosch]

Add some comment?

/**
 * @brief Lookup an IP- and MAC-address couple in the ARP cache.
 *        If a perfect match is found, the age field will be reset.
 *        When not found, no action will be taken.
 * @param[in] pxMACAddress: The MAC-address
 * @param[in] ulIPAddress: The IP-address
 */
    void vARPRefreshCacheEntryAge( const MACAddress_t * pxMACAddress,
                                   const uint32_t ulIPAddress );

[htibosch]

I have tested the changes in embedded projects. It all worked well.

Also I did measurements to see if this will affect performance a lot. When the ARP table is small, like 32 entries, the extra lookups will cost a few uS on a mid-size CPU (168 MHz). The extra lookup is only needed when a packet is being sent.

We have been thinking of a more efficient approach: keep all entries sorted on age, the newest entries in the front. The free entries will be in the back. This would add quite a bit of code and complexity though.

Some more text about this PR 366:

prvTCPReturnPacket

When returning a TCP packet in prvTCPReturnPacket(), the MAC-address of the peer must be looked up. This address was taken from the field xSourceAddress of the Ethernet header.
This works well, it is efficient, but formally not correct. For every outgoing packet, the ARP cache table must be consulted.

Before replying, the peer's IP-address in the ARP cache table. Normally the entry should be there. Every time when the peer sends a packet, the timer of the entry is updated so it gets a longer life.

If the entry disappears for whatever reason, the value xSourceAddress of the Ethernet packet will be used.

vReturnEthernetFrame

The same happens when a (non-TCP) packet is returned, in vReturnEthernetFrame(), the ARP table will be consulted for IP-packets. ARP-packets are returned to the MAC-address found in the Ethernet header.

Touch timer of ARP entries

And finally: when receiving a packet, the corresponding in the entry in the ARP table will be touched. When the entry is unknown, nothing will be done. The latter is quite unlikely, because in an earlier PR, we added the xARPResolutionTimer in PR #338. Before processing a packet from an unknown IP-address, an ARP request will be done. When it is replied, the message will be handled.

And finally, FreeRTOS_ARP.c received a new function vARPRefreshCacheEntryAge(). It will touch the timer of an ARP entry to extends it validity. It will be called on reception of any IP packet.

I approve this pull request.

[htibosch]

My compiler says that xMatchingMAC is not used anymore.

[htibosch]

Here you added declarations in the middle of a function. Would it be possible to create a compound block for this?

+        {
             MACAddress_t xMACAddress;
             uint32_t ulDestinationIPAddress = pxIPHeader->ulDestinationIPAddress;
             eARPLookupResult_t eResult;

             eResult = eARPGetCacheEntry( &ulDestinationIPAddress, &xMACAddress );

             if( eResult == eARPCacheHit )
             {
                 pvCopySource = &xMACAddress;
             }
             else
             {
                 pvCopySource = &pxEthernetHeader->xSourceAddress;
             }
+        }