buffer fixes to the NetworkInterface for the LCP54018 #41

n9wxu · 2020-10-20T22:11:52Z

Buffer fixes for the LPC54018 ethernet

Description

fixed the zerocopy buffer allocation. Both in alignment and in initializing the first entry to the descriptor pointer.

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

htibosch · 2020-10-21T08:49:53Z

@lundinc2 , @n9wxu , thank you for the new network driver!

Although the PR is merged and closed, I do have some remarks.

The function xSendEventStructToIPTask() returns either pdPASS or pdFAIL. Although these values are the same as pdTRUE and pdFALSE, it is custom to compare the result of xSendEventStructToIPTask() with pdPASS or pdFAIL.

More serious: I do not understand why Joseph has changed the logic. As it is now: a packet that is transferred to the IP-task will be released, whereas packets that could not be delivered will cause memory/buffer leaks.
Note: the IP-task will release all network buffers that it receives with the eNetworkRxEvent message.

So unless I am missing some logic, I think that the code should be like this:

    if( eConsiderFrameForProcessing( pxBufferDescriptor->pucEthernetBuffer ) == eProcessBuffer )
    {
        xRxEvent.eEventType = eNetworkRxEvent;
        xRxEvent.pvData = ( void * ) pxBufferDescriptor;

        if( xSendEventStructToIPTask( &xRxEvent, 0 ) == pdFAIL )
        {
            /* Sending the packet failed, release the network buffer. */
            vReleaseNetworkBufferAndDescriptor( pxBufferDescriptor );
            iptraceETHERNET_RX_EVENT_LOST();
        }
        else
        {
            /* Now the IP-task owns the network buffer, and will release it. */
            iptraceNETWORK_INTERFACE_RECEIVE();
        }
    }

The comment was not correct:

/* put this back if using bufferallocation_2.c */

vReleaseNetworkBuffer() does 2 things:

Call vPortFree() to release pucEthernetBuffer ( indeed bufferallocation_2.c only )
Add the network buffer descriptor to the list of available descriptors ( both bufferallocation_1 and bufferallocation_2.c ).

I added iptraceNETWORK_INTERFACE_RECEIVE(), as it is common in all network drivers.

I would reorganise rx_task() as follows, using a while in stead of an if:

while( pdTRUE )
{
    ulTaskNotifyTake( pdTRUE, portMAX_DELAY );

    while( pdTRUE )
    {
        status = ENET_GetRxFrameSize( EXAMPLE_ENET_BASE, &g_handle, &length, 0 );
        if( ( status != kStatus_Success ) || ( length == 0U ) )
        {
            break;
        }
        pxBufferDescriptor = pxGetNetworkBufferWithDescriptor( length, 0 );

        if( pxBufferDescriptor != NULL )
        {
            /* Send the bufffer to the IP-task. */
            prvSendDescriptor( pxBufferDescriptor );
        }
        else
        {
            /* I think that ENET_ReadFrame() must still be called.
             * If not, it might keep on triggering task.
             * Maybe call ENET_GetRxFrameSize() with NULL as a parameter and
             * change the code to allow for NULL?
             * The packet must be dropped.
             */
        }
    } /* while( pdTRUE ) */
    /* Check the status of the PHY regularly. When packet are sent while the
     * PHY status is low, it may hold up the task for a long time, because of
     * time-outs.
     */
} /* while( pdTRUE ) */

n9wxu · 2020-10-21T14:27:52Z

This is very helpful. Working on this interface was a learning experience so I am happy to have your feedback. I will correct and submit a new PR shortly.

n9wxu · 2020-10-21T14:33:29Z

One question concerning vReleaseNetworkBuffer
I cannot find that in BufferAllocation_1
However: vReleaseNetworkBufferAndDescriptor is present in BufferAllocation_1. Does that the actual function required?

n9wxu · 2020-10-21T15:01:46Z

At the end of the recommended receive loop, you have a comment on checking the PHY and the caution is to prevent TX timeouts. It seems to be that checking the link state before TX is a better choice. It looks like I could adjust the notifyTake in the receiver to timeout every so often and poll the link state but I don't see an API to push the link state into the TCP stack. Instead there is a xGetPhyLinkStatus to allow the TCP stack to pull the state. I think checking the link before TX is the better choice instead of checking the link state in the receiver and then signaling the transmitter.

This reverts commit 1a070f5.

htibosch · 2020-10-21T18:37:04Z

However: vReleaseNetworkBufferAndDescriptor is present in
BufferAllocation_1. Does that the actual function required?

Oops on my side: I was mistaken between vReleaseNetworkBuffer() (BufferAllocation_2 only), and vReleaseNetworkBufferAndDescriptor(), which exists in both BufferAllocation_1 and 2. I corrected my sample code here above.

@htibosch

* Repair asynchronous DNS lookup (#37) Co-authored-by: Hein Tibosch <[email protected]> * WIFINetworkParams_t is changing. Update to conform (#36) * WIFINetworkParams_t is changing. Update to conform * uncrustify (v0.66) Co-authored-by: Aniruddha Kanhere <[email protected]> * added a network interface for the LPC54018 ethernet (#39) * added a network interface for the LPC54018 ethernet Co-authored-by: Joseph Julicher <[email protected]> * FreeRTOS+TCP Zynq: check frame type before calculating ICMP checksum (v4) (#19) Co-authored-by: Hein Tibosch <[email protected]> Co-authored-by: Aniruddha Kanhere <[email protected]> * A network driver for Xilinx UltraScale+ 64-bits(v2) (#22) * A network driver for Xilinx UltraScale+ 64-bits(v2) * Uncrustify Co-authored-by: Hein Tibosch <[email protected]> Co-authored-by: AniruddhaKanhere <[email protected]> Co-authored-by: Aniruddha Kanhere <[email protected]> * Adding a network interface for STM32H7xx(v2) (#21) * Adding a network interface for STM32H7xx(v2) * Uncrustify * Uncrustify v2 * Remove trailing whitespace Co-authored-by: Hein Tibosch <[email protected]> Co-authored-by: AniruddhaKanhere <[email protected]> Co-authored-by: Aniruddha Kanhere <[email protected]> * FreeRTOS+TCP compatibility with 64-bit platforms (v2) (#20) * FreeRTOS+TCP compatibility with 64-bit platforms (v2) * Uncrustify Co-authored-by: Hein Tibosch <[email protected]> Co-authored-by: Aniruddha Kanhere <[email protected]> Co-authored-by: AniruddhaKanhere <[email protected]> * buffer fixes to the NetworkInterface for the LCP54018 (#41) * added a network interface for the LPC54018 ethernet * fixed the buffers so DHCP is now working * network buffers fixed * network buffers fixed Co-authored-by: Aniruddha Kanhere <[email protected]> * Cleanup as a result of Hein's code review (#43) * added a network interface for the LPC54018 ethernet * fixed the buffers so DHCP is now working * network buffers fixed * network buffers fixed * added a network interface for the LPC54018 ethernet * improvements from Hein's comments, thanks * final changes based upon Hein's review * uncrustifying the NetworkInterface.c for LPC54018 Co-authored-by: Aniruddha Kanhere <[email protected]> * Add spell check (#23) * Add the spell-check files * Add lexicon.txt file after generating it. * Update the allowed name list * Added some more words to the lexicon * final revision of additions * Update ci.yml * Update ci.yml * Delete lexicon.txt * Rename temp.txt to lexicon.txt * Ignore the portable directory * Update ci.yml * Update ci.yml * Update ci.yml * Update find-unknown-comment-words * Update ci.yml * Make all letters lowercase * Update the lexicon and fix some typos * Add more details and then add lexicon * remove spell directory from tools * Remove duplicate instances and sort the words * Update lexicon after Gary's comments (and test) * Update the words list * Correct word spelling * Update lexicon.txt Co-authored-by: Gary Wicker <[email protected]> * Synchronized the MAC address with main.c * Uncrustify LPC54018 network interface * removed extra prints (#48) * Uncrustify (#44) * Create manifest.yml * Remove trailing whitespace * Update manifest.yml * Corrected a typo * Uncrustify C files * Uncrustify header files * Update ci.yml * Update ci.yml * Uncrustify with version 0.67 * Spelling corrections * uncrustify run * - check for pxDuplicateNetworkBufferWithDescriptor returning NULL (#47) * - check for pxDuplicateNetworkBufferWithDescriptor returning NULL * Uncrustify the sources Co-authored-by: Robert Korn <[email protected]> Co-authored-by: Aniruddha Kanhere <[email protected]> Co-authored-by: AniruddhaKanhere <[email protected]> * Added CODEOWNERS to require PRBR review (#50) * Add comments for Doxygen and add doxygen check (#46) * Create manifest.yml * Remove trailing whitespace * Update manifest.yml * Corrected a typo * Add the config file * Update doxygen and Update some files * Add the CI check * Add documentation for doxygen * Uncrustify * temp * Add the files individually * Update * Added 2 c files * Added 2 c files v2 * Added DNS and IP * Added more comments for doxygen * Added more comments * Sockets.c added * Sockets and TCP_IP * Added UDP_IP.c * Remove doxygen output * Remove a typo * Spelling corrections * Some spelling corrections * Update ci.yml * Add the TCP_WIN.c file * Add missing function description * update lexicon and fix some typos * Removed duplicates, sort and make lower-case * Update lexicon * Uncrustify * Minor Uncrustify changes * Update lexicon * removed extra prints (#48) * Uncrustify * Fix conflict resolution issues * Uncrustify * Add missing briefs and update based on @htibosch's comments. * Fix spellings and update lexicon * uncrustify * Minor fixes Co-authored-by: root <[email protected]> Co-authored-by: Joseph Julicher <[email protected]> Co-authored-by: Hein Tibosch <[email protected]> Co-authored-by: Hein Tibosch <[email protected]> Co-authored-by: David Chalco <[email protected]> Co-authored-by: Carl Lundin <[email protected]> Co-authored-by: Joseph Julicher <[email protected]> Co-authored-by: Gary Wicker <[email protected]> Co-authored-by: RolinBert <[email protected]> Co-authored-by: Robert Korn <[email protected]> Co-authored-by: root <[email protected]>

n9wxu added 5 commits October 19, 2020 15:57

added a network interface for the LPC54018 ethernet

97486f8

fixed the buffers so DHCP is now working

6f4230a

network buffers fixed

3ada0e5

network buffers fixed

3f9dcee

Merge branch 'master' of https://github.com/n9wxu/FreeRTOS-Plus-TCP

9123c99

lundinc2 approved these changes Oct 20, 2020

View reviewed changes

AniruddhaKanhere approved these changes Oct 21, 2020

View reviewed changes

AniruddhaKanhere added 2 commits October 20, 2020 22:36

Merge branch 'master' into master

cf2e2fe

Merge branch 'master' into master

0baa5c7

AniruddhaKanhere merged commit 1a070f5 into FreeRTOS:master Oct 21, 2020

AniruddhaKanhere added a commit that referenced this pull request Oct 21, 2020

Revert "buffer fixes to the NetworkInterface for the LCP54018 (#41)"

835d5c9

This reverts commit 1a070f5.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

buffer fixes to the NetworkInterface for the LCP54018 #41

buffer fixes to the NetworkInterface for the LCP54018 #41

Uh oh!

n9wxu commented Oct 20, 2020

Uh oh!

htibosch commented Oct 21, 2020 •

edited

Loading

Uh oh!

n9wxu commented Oct 21, 2020

Uh oh!

n9wxu commented Oct 21, 2020

Uh oh!

n9wxu commented Oct 21, 2020

Uh oh!

htibosch commented Oct 21, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

buffer fixes to the NetworkInterface for the LCP54018 #41

buffer fixes to the NetworkInterface for the LCP54018 #41

Uh oh!

Conversation

n9wxu commented Oct 20, 2020

Description

Uh oh!

htibosch commented Oct 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

n9wxu commented Oct 21, 2020

Uh oh!

n9wxu commented Oct 21, 2020

Uh oh!

n9wxu commented Oct 21, 2020

Uh oh!

htibosch commented Oct 21, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

htibosch commented Oct 21, 2020 •

edited

Loading