FreeRTOS+TCP minor bug-finds

rwt33 wrote on Wednesday, November 26, 2014:

I’ve been enjoying deploying FreeRTOS+TCP (from the labs) on a STM32F103 + enc28j60 and have found a few gotchas along the way. I’m rating FreeRTOS+TCP a hole-in-one for an IP stack on micros - a great improvement over the two leading alternatives. Thankfully the findings are all relatively minor.

  1. The docs for BufferManagement_1.c don’t mention the need to implement vNetworkInterfaceAllocateRAMToBuffers
  2. The network buffers returned to BufferManagement_1.c must be 8-byte aligned. Which if not leads to next bug:
  3. If a buffer is not aligned to 8-byte boundary, the DHCP buffer alloc fails (pxUDPPayloadBuffer_to_NetworkBuffer returns null), but when vReleaseNetworkBufferAndDescriptor is called (from FreeRTOS_ReleaseUDPPayloadBuffer in prvSendDHCPRequest), there is no null check and so vReleaseNetworkBufferAndDescriptor crashes.
  4. configASSERT_VOID in a couple of files but with no def.

rtel wrote on Wednesday, November 26, 2014:

Thank you for your valuable feedback. I would be extremely grateful if you could post your code to the FreeRTOS Interactive site - I have just created a forum for this, a link to which is below. If you are unable to post the project then please at least post the driver files. Thanks.

http://interactive.freertos.org/forums/21211265-FreeRTOS-TCP

The docs for BufferManagement_1.c don’t mention the need to implement
vNetworkInterfaceAllocateRAMToBuffers

I will fix.

The network buffers returned to BufferManagement_1.c must be 8-byte aligned.

Hein - can you please confirm and if so we can add an assert() into the code to catch this, and so bring peoples attention to it immediately without having to debug first. I will also then add it to the documentation.

If a buffer is not aligned to 8-byte boundary, the DHCP buffer alloc fails
(pxUDPPayloadBuffer_to_NetworkBuffer returns null), but when
vReleaseNetworkBufferAndDescriptor is called (from
FreeRTOS_ReleaseUDPPayloadBuffer in prvSendDHCPRequest), there is no null
check and so vReleaseNetworkBufferAndDescriptor crashes.

Again I think there needs to be some documentation on this point, and some asserts will help to. Sometimes it is necessary to add an offset into the network buffer at the driver level - that may or may not be related.

configASSERT_VOID in a couple of files but with no def.

I think I have that one already.

Regards.

rtel wrote on Wednesday, November 26, 2014:

Just writing the documentation for vNetworkInterfaceAllocateRAMToBuffers() now, and it occurs to me the function would be better named vNetworkInterfaceAllocateRAMToDescriptors(), as that is what it is doing.

Regards.

heinbali01 wrote on Wednesday, November 26, 2014:

Hi Robert,

a great improvement

Thanks. We were all hoping that FreeRTOS+TCP will be simple (to use, to adopt) and at least equally powerful. It has the great advantage that it is build on top of FreeRTOS. This makes the code a lot simpler.

deploying FreeRTOS+TCP (from the labs) on a STM32F103 + enc28j60

Please share, in return you will get valuable feedback :slight_smile:

Maybe we should make a check-list for “Writing a NIC driver for FreeRTOS+TCP”.
I’m thinking of these options, I’m sure you’ve seen them all:

  • The choice between BufferManagement_1 / 2
    Version 1 is indeed preferred if your system has enough memory
    Provide an allocation function for version 1.

  • Optional: ipconfigUSE_LLMNR : add the LLMNR multicast MAC address 01:00:5E:00:00:FC
    to the list of MAC-addresses.
    LLMNR is very useful for name resolution on a LAN, especially when using DHCP.
    All browsers use LLMNR as a preferred way to find names (without a dot)

  • Optional: ipconfigDRIVER_INCLUDED_TX_IP_CHECKSUM

  • Optional: ipconfigDRIVER_INCLUDED_RX_IP_CHECKSUM
    The NIC driver calculates the TX and/or RX checksum, either automatically within the
    EMAC peripheral, in which case it is called “checksum offloading”.
    Or by calling usGenerateChecksum().

  • Optional: ipconfigETHERNET_DRIVER_FILTERS_PACKETS
    If you want to filter incoming messages within the NIC driver, enable this option.
    The advantage: you might want to stop many UDP broadcast messages as early as
    possible. W32, discovery protocols, PnP, simple network maintenance and other inventions
    can occupy quite some resources: allocate a buffer, send it to the IP-task, which
    will analyse and drop it.
    See FreeRTOS_Sockets.c:

        /* Function returns true if there is a UDP socket bound to
        the given port number. */
  	BaseType_t xPortHasUdpSocket( uint16_t usPortNr )
  • Optional: ipconfigPACKET_FILLER_SIZE
    This option is a bit tricky: it makes sure that all 32-bit fields in the network
    packets are 32-bit aligned. This means that the 14-byte Ethernet header should
    start at a 16-bit offset. Therefore ipconfigPACKET_FILLER_SIZE is defined a 2 (bytes).
    I think that most EMAC’s have an option to set this 2-byte offset for
    both incoming and outgoing packets.

  • Optional: ipconfigUSE_LINKED_RX_MESSAGES
    An optimisation: Collect received packets in a loop and link them through a pointer
    called ‘pxNextBuffer’. Several messages will be forwarded to the IP-stack within
    a single ‘eNetworkRxEvent’ message to the IP-stack

  • Optional: what I would do is check the Link-Status continuously. As long as the LS is
    low, it doesn’t make sense to send messages. Check the PHY frequently, to have a quick
    response once it gets connected.
    As long as LS is high, check it e.g. every 15 seconds, unless packets are received,
    which already proves that the Link-Status is high.

I’m repeating the word “Optional”. All these points aren’t necessary, but they can all improve the performance.

I will come back on the “8-byte alignment” issue, thanks for reporting that.

‘configASSERT_VOID’ was my not-so-useful invention, the idea was:

#define configASSERT_VOID(statement)	if( ( statement ) == pdFALSE ) return

used in functions returning ‘void’.

Regards,
Hein

rwt33 wrote on Wednesday, November 26, 2014:

Hi,
Thanks for the reply. I will contribute the driver once it’s done also.
The buffer alignment issue wasn’t completely fixed by aligning to 8-byte boundaries, rather the buffer is rejected after the UDP offset is applied (ipUDP_PAYLOAD_OFFSET) which unfortunately is a non-8-byte size (42) so the buffer is never going to end up 8-byte aligned.
Is there a fix for this yet? I’m wondering why there is even the requirement to align to 8-byte boundaries for the packet? On the CM3 8-byte alignment is only required for stack pointers, no?
Hein might have found something similar with what he’s referring to re the ipconfigPACKET_FILLER_SIZE

Regards,
Robert

rwt33 wrote on Wednesday, November 26, 2014:

Hi guys, I don’t suppose there’s a git repo from which I can pull latest updates?

rwt33 wrote on Wednesday, November 26, 2014:

I’m starting to get a bit suspicious as to why the problem I’m having hasn’t been seen before, and assume it’s due to some combo of settings + Buffer_1 that hasn’t been tested before.
I’m on a STM32F103, and the crash is happening right during the first time through the prvIPTask, long before any packets are sent/received, during the first sending (attempt) of a DHCP packet.

rtel wrote on Wednesday, November 26, 2014:

Hi guys, I don’t suppose there’s a git repo from which I can pull
latest updates?

Currently the sources are in a private SVN repository - although they
will be moved into the main FreeRTOS SVN repository at some point. You
have have noticed both the source code directory structure and the web
pages are organised to fit with the standard model.

Regards.

rtel wrote on Wednesday, November 26, 2014:

I’m starting to get a bit suspicious as to why the problem I’m having
hasn’t been seen before, and assume it’s due to some combo of
settings + Buffer_1 that hasn’t been tested before. I’m on a
STM32F103, and the crash is happening right during the first time
through the prvIPTask, long before any packets are sent/received,
during the first sending (attempt) of a DHCP packet.

You could be right of course, although when Hein and I work on this Hein
uses Buffer_1 and I use Buffer_2. Likewise Hein uses big endian and I
use little endian. That way we are using both of both options all the time.

Your comments about this being related to UDP make me suspect your
problem could be related to padding bytes that are required at the start
of the frame. I will look through my notes on that, it might take a
while to find (I have a lot of notes!).

Regards.

heinbali01 wrote on Wednesday, November 26, 2014:

Hi Robert,

If you like you send what you have now to info at freertos point org. Then we can have a look at it (without revealing any code).

Or maybe this text will contain a hint:

The ‘ipconfigPACKET_FILLER_SIZE’ may indeed be a bit confusing. It corresponds to what is named ‘ETH_PAD_SIZE’ in lwIP.

    uint8_t *pucBuffer;

What is sizeof( pucBuffer ) on your platform ? If it is 64-bits then indeed the (internal) function like ‘pxUDPPayloadBuffer_to_NetworkBuffer’ will only work with 8-byte aligned buffers.
At the other hand, the library is not yet prepared for 64-bit pointers.

Every network buffer descriptor has this memory:

    0x00 4 bytes  pointer to the network buffer descriptor
    0x04 4 bytes  reserved for flags
    0x08 2 bytes  Alignment filler (ipconfigPACKET_FILLER_SIZEbytes)
    0x0a 'ipTOTAL_ETHERNET_FRAME_SIZE' bytes
                  Here the packet begins with 'xDestinationAddress'

    #define ipTOTAL_ETHERNET_FRAME_SIZE  ( ipconfigNETWORK_MTU + \
        ipSIZE_OF_ETH_HEADER + ipSIZE_OF_ETH_CRC_BYTES + \
        ipSIZE_OF_ETH_OPTIONAL_802_1Q_TAG_BYTES )

Please note that the packet data:

    uint8_t *pucEthernetBuffer

points to offset 0x0a (== ipBUFFER_PADDING) in the allocated space.

In your NetworkInterface.c you should allocate ‘ipTOTAL_ETHERNET_FRAME_SIZE + ipBUFFER_PADDING’ bytes per network buffer.

    #define UNIT_SIZE ( ipTOTAL_ETHERNET_FRAME_SIZE + ipBUFFER_PADDING )
    /* Here the aligned(8) attribute is used because DMA likes it. */
    static unsigned char network_packets[ ipconfigNUM_NETWORK_BUFFER_DESCRIPTORS * UNIT_SIZE ] __attribute__ ((aligned (8)));

    void vNetworkInterfaceAllocateRAMToBuffers( pxNetworkBuffers[ ] )
    {
    unsigned char *ucRamBuffer = network_packets;
    int x;
        for (x = 0; x < ipconfigNUM_NETWORK_BUFFER_DESCRIPTORS; x++) {
            /* Add the 10 bytes */
            pxNetworkBuffers[ x ].pucEthernetBuffer = ucRamBuffer + ipBUFFER_PADDING;
            /* Set the pointer to the network buffer descriptor */
            *((unsigned*)ucRamBuffer) = (unsigned)(&pxNetworkBuffers[x]);
            /* and point to the next space to assign. */
            ucRamBuffer += UNIT_SIZE;
        }
    }
}

Regards,
Hein

rwt33 wrote on Wednesday, November 26, 2014:

Am I able to have read access to the SVN repo? Of course it it’s an internal-only repo then no problem :slight_smile:

rtel wrote on Wednesday, November 26, 2014:

Am I able to have read access to the SVN repo? Of course it it’s an
internal-only repo then no problem :slight_smile:

Unfortunately not at the moment as there are some other new, and as yet
confidential, developments in the same repository. We should aim to
expedite moving the TCP code into the public SVN repository, but I’m
somewhat hesitant as having it separate means we can change it rapidly -
rather than just between FreeRTOS releases.

Regards.

rwt33 wrote on Wednesday, November 26, 2014:

Ahh that buffer allocation looks like it might be promising. What I have at present does not allow for the padding:

static uint8_t ucBuffers[ipconfigNUM_NETWORK_BUFFER_DESCRIPTORS][ipTOTAL_ETHERNET_FRAME_SIZE] __attribute__((aligned(8)));

void vNetworkInterfaceAllocateRAMToBuffers(xNetworkBufferDescriptor_t pxNetworkBuffers[ipconfigNUM_NETWORK_BUFFER_DESCRIPTORS])
{
	BaseType_t x;

	for (x = 0; x < ipconfigNUM_NETWORK_BUFFER_DESCRIPTORS; x++)
	{
		pxNetworkBuffers[x].pucEthernetBuffer = &(ucBuffers[x][0]);
	}
}

After not finding any example of vNetworkInterfaceAllocateRAMToBuffers in FreeRTOS+TCP, I happened to find that snippet in FreeRTOS8.1.2 FreeRTOS-Plus-UDP/portable/NetworkInterface/WinPCap/NetworkInterface.c

I will try the changes in ~12 hours and reply.
Thanks for the support guys

rtel wrote on Wednesday, November 26, 2014:

The ‘ipconfigPACKET_FILLER_SIZE’ may indeed be a bit confusing.

Just checking this page:
http://www.freertos.org/FreeRTOS-Plus/FreeRTOS_Plus_TCP/TCP_IP_Configuration.html

I find that constant is not even mentioned. We need to add it in asap
and update the buffer_1 documentation pages accordingly. I updated them
today once already following Robert’s feedback.

Regards.

heinbali01 wrote on Thursday, November 27, 2014:

You were still missing some things, like the ‘ipBUFFER_PADDING’:

#define UNIT_SIZE ( ipTOTAL_ETHERNET_FRAME_SIZE + ipBUFFER_PADDING )

// The 10 bytes of ipBUFFER_PADDING should be added
// so the declaration becomes:

static uint8_t ucBuffers[ ipconfigNUM_NETWORK_BUFFER_DESCRIPTORS ]
	[ UNIT_SIZE ] __attribute__((aligned(8)));

It doesn’t make much sense to align the first buffer only, so let’s round up the size of each buffer:

#define UNIT_SIZE_1 ( ipTOTAL_ETHERNET_FRAME_SIZE + ipBUFFER_PADDING )
#define UNIT_SIZE_8 ( ( UNIT_SIZE_1 + 7 ) & ~7 )

static uint8_t ucBuffers[ ipconfigNUM_NETWORK_BUFFER_DESCRIPTORS ]
	[ UNIT_SIZE_8 ] __attribute__((aligned(8)));

// And finally make the double link, from descriptor to buffer and from
// buffer to descriptor:

void vNetworkInterfaceAllocateRAMToBuffers2( xNetworkBufferDescriptor_t
	pxNetworkBuffers[] )
{
	BaseType_t x;
	
	for (x = 0; x < ipconfigNUM_NETWORK_BUFFER_DESCRIPTORS; x++)
	{
		/* Not like this: */
/*		pxNetworkBuffers[ x ].pucEthernetBuffer = &(ucBuffers[ x ][ 0 ] ); */
		/* But like this: */
		pxNetworkBuffers[ x ].pucEthernetBuffer = &(ucBuffers[ x ][ ipBUFFER_PADDING ] );
		/* And set a pointer back to the descriptor: */
		*( ( uint32_t * ) &ucBuffers[ x ][ 0 ] ) = ( uint32_t ) &pxNetworkBuffers[x];
	}
}

There is no requirement to have the actual buffers 8-byte aligned, 4-byte is enough. But some DMA peripherals do like an 8-byte alignment or higher. A better alignment may lead to a higher throughput.

Regards,
Hein

rtel wrote on Thursday, November 27, 2014:

Hi Hein - I think there is scope to make this easier for the end user.
For example, setting the pointer back to the descriptor as:

*( ( uint32_t * ) &ucBuffers[ x ][ 0 ] ) = ( uint32_t )
&pxNetworkBuffers[x];

could be done inside BufferAllocation_1.c (in fact I thought it was!).
Ideally we don’t want the end user to know anything about how the space
is used, that should be hidden. In fact even the use of
ipBUFFER_PADDING could be hidden by providing a different constant that
has the padding already added. Something like:

#define ipBUFFER_SIZE ( ipTOTAL_ETHERNET_FRAME_SIZE + ipBUFFER_PADDING )

except using a more descriptive name than 'BUFFER_SIZE', then the user 
can use the BUFFER_SIZE constant without even knowing ipBUFFER_PADDING 
has been added.

Lets try and do this before the next labs release.

Regards.

rwt33 wrote on Thursday, November 27, 2014:

Bingo, that was it.
I first sorted the ipBUFFER_PADDING which made satisfied the alignment, but then hit the snag of no reference back to the descriptor. The above post fixed it.
Definitely let me know when you bundle up the next labs release :slight_smile:
Thanks guys. Will continue code stepping…

heinbali01 wrote on Saturday, November 29, 2014:

Hi Robert,

Bingo, that was it.

I’m glad to hear.

I first sorted the ipBUFFER_PADDING which made satisfied the alignment,
but then hit the snag of no reference back to the descriptor.

This mechanism was already present in the earlier FreeRTOS+UDP. The double pointers are necessary for the ZERO_COPY feature. It allows to pass around a “const char*” whereas in fact it is NetworkBuffer.

In the next release, the double pointers and also the ipBUFFER_PADDING + ipconfigPACKET_FILLER_SIZE defines will be totally hidden, i.e. moved to the library.

The network driver will only supply a pointer to a space big enough to hold all Network Buffers.

Definitely let me know when you bundle up the next labs release :slight_smile:

You will be the first one to hear it :slight_smile:

Note that the current release (141019) is not a first try-out. The development already started more than a year ago and this library has already been applied in commercial applications.

It will remain in FreeRTOS Labs as long as: “… we are still refining its design, and the source code and documentation do not yet meet Real Time Engineers Ltd’s strict quality standards”.

To be expected in the new release:

- Sockets in listening mode, using the
  'FREERTOS_SO_REUSE_LISTEN_SOCKET', can now also
  be used with select() and accept().
  The reuse option means that the same instance of
  the listening socket will get connected.

- The above change: hiding the PADDING and FILLING
  bytes and setting the double links with pointers.

- Extending the zero-copy option for TCP: the sending
  routine may pass all packet buffers directly to the DMA,
  in stead of first copying it.

- Adapted documentation

Thanks guys. Will continue code stepping…

Same to you.

Btw are you still stepping through the code or running it by now?

Regards,
Hein

rwt33 wrote on Sunday, November 30, 2014:

Hi guys,
New update: have a polled version of the enc28j60 driver working sweetly. UDP loop tests are showing about 1Mb/s performance before packets start dropping. It’s pretty crude in that it currently has to poll the chip (SPI transaction) to check if a received packet is waiting. I’m midst enabling interrupts and DMA. Transferring a 1500 byte packet over 10MHz SPI really is screaming for DMA! The more hardware peripheral features I pull in (SPI, GPIO Interrupt & DMA) the more my frustration rises at keeping things moderately portable. I started out using libmaple with it’s plethora of cpp libs and have been slowly FreeRTOS-ing them.

Just a quick thing I noticed, in the porting examples, the basic packet receiving example doesn’t set the xNetworkBufferDescriptor_t.xDataLength with xBytesReceived.

Cheers guy,
Robert

rtel wrote on Sunday, November 30, 2014:

Just a quick thing I noticed, in the porting
examples, the basic packet receiving example
doesn’t set the xNetworkBufferDescriptor_t.xDataLength
with xBytesReceived.

Fixed!

Regards.