Add NetworkInterface.c for TMS320F2838x

[adrien-cardinale]

NetworkInterface.c for TMS320F2838x

Description

Add NetworkInterface.c for TMS320F2838x

Test Steps

Test with TMS320F28388d from texas instrument

Checklist:

• I have tested my changes. No regression in existing tests.
• I have modified and/or added unit-tests to cover the code changes in this Pull Request.

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]

@adrien-cardinale , thank you for this PR.

Could you write the text "/bot run formatting" (without the quotes) and send it as a comment. The source code will be formatted to the FreeRTOS standard. After that you can do a pull on your own PR to get the same formatting on your local repo.

[htibosch]

Hello @adrien-cardinale , I see that you already made changes after my more informal review on the forum.
In this review a few more proposals that should help making it more efficient.
Also I am proposing to think about the 2 streams: TX packets that have been sent, and RX packets that have been received.

[htibosch]

Can you explain why you test for '0'? What does that mean? Success/failure?

[htibosch]

Before an Output function can release a packet buffer, you must test the value of xReleaseAfterSend. If it is true, you can (must) release it after use.
In a system without zero-copy, xReleaseAfterSend will usually be false, which means that the Output routine may not release it.

[htibosch]

Now you do test xReleaseAfterSend, which is good.
But why do you set pxDescriptor->pucEthernetBuffer to NULL?
This may be a pointer, allocated with pvPortMalloc(). When you put it to NULL, you will get into trouble.

[htibosch]

In the case when if(ret == 0), you have already called vReleaseNetworkBufferAndDescriptor(). It would cause an erro to call it again.

[htibosch]

Here you are handling a EMAC_IF_RX_EVENT, meaning that a packet has been received. Wouldn't you check the validity of pPktDescGlobal here, see if validLength is within range and so?
Not need to copy ETHERNET_MAX_PACKET_LENGTH bytes, that would be a waste of time.
Also I wouldn't create a packet buffer before calling eConsiderFrameForProcessing() and FreeRTOS_MatchingEndpoint(): both functions might fail.
Have a look at this:

if( valid_packet( pPktDescGlobal )
{
    BaseType_t xWasAccepted = pdFALSE;

    /* Check if we should handle this packet. */
    if( eConsiderFrameForProcessing( pPktDescGlobal->dataBuffer ) == eProcessBuffer )
    {
        /* Find the proper endpoint. */
        NetworkEndPoint_t * pxEndPoint = FreeRTOS_MatchingEndpoint( pxMyInterface, pPktDescGlobal->dataBuffer );

        if( pxEndPoint != NULL )
        {
            NetworkBufferDescriptor_t * pxBufferDescriptor = pxGetNetworkBufferWithDescriptor( pPktDescGlobal->validLength, 0 );

            if( pxBufferDescriptor != NULL )
            {
                memcpy( pxBufferDescriptor->pucEthernetBuffer, pPktDescGlobal->dataBuffer, pPktDescGlobal->validLength );
                pxBufferDescriptor->xDataLength = pPktDescGlobal->validLength;

                pxBufferDescriptor->pxEndPoint = pxEndPoint;
                xRxEvent.eEventType = eNetworkRxEvent;
                xRxEvent.pvData = ( void * ) pxBufferDescriptor;

                if( xSendEventStructToIPTask( &xRxEvent, 0 ) == pdTRUE )
                {
                    xWasAccepted = pdTRUE;
                }
                else
                {
                    vReleaseNetworkBufferAndDescriptor( pxBufferDescriptor );
                }
            }
        }
    }

    if( xWasAccepted == pdTRUE )
    {
        iptraceNETWORK_INTERFACE_RECEIVE();
    }
    else
    {
        iptraceETHERNET_RX_EVENT_LOST();
    }
}

In other words, do not call pxGetNetworkBufferWithDescriptor() until you are sure that it will be used.

Question: How do we make sure that pPktDescGlobal isn't accessed before by interrupt code before we've handled the last packet?
I am thinking of adding a queue or a counting semaphore between the EMAC task and the callback functions:

Ethernet_receivePacketCallbackCustom()
Ethernet_releaseTxPacketBufferCallbackCustom()

A EMAC_IF_RX_EVENT event means: one or more packets have been received.
A EMAC_IF_TX_EVENT event means the one or more packets have been sent.
But while you are handling some EMAC_IF event, the interrupt may get triggered.

EDIT
I added the test if( pxBufferDescriptor != NULL ) to my sample code here above.

[htibosch]

You are saving the packet buffer because prvEMACDeferredInterruptHandlerTask() will release it later on.
Please check xReleaseAfterSend : when false, you may not release the buffer.
And also, a few lines later in this function, you are releasing pxDescriptor, without invalidating the copy in pxGlobalDescriptor.

[adrien-cardinale]

/bot run formatting

[AniruddhaKanhere]

@adrien-cardinale to fix the spell check, please add the words highlighted in the spell check report to the .cSpellWords.txt.

[htibosch]

Hello @adrien-cardinale, how is the driver going? I wonder if there is anything we can help you with? Is there anything unclear?
Or are you too busy to get on with it?

[adrien-cardinale]

Hello @htibosch , thank you for your interest and comments. I'm actually too busy at the moment to make the changes you suggested. This is why I wanted to publish the code as is so that it can serve as a basis for other people who would like to use the driver for the TMS320F2838x.

[amazonKamath]

@adrien-cardinale - Thanks for your efforts on this PR. To prevent stale PR's, we'll be closing this for now. Feel free to reopen when you're ready to address the comments.