FreeRTOS-TCP improvement?

Kernel
1

fizzyaid wrote on Tuesday, March 06, 2018:

Hi,

I’m just in the process of testing out some stuff with FreeRTOS TCP and I have it working fine, but there are some changes that make it more useful when using the “event driven” mode, I’m wondering whether you would be open for me submitting changes for these.

Basically, the event driven stuff works fine, but it’s difficult to link it to other parts of the code, so I have tested the water by making a few changes.

Mainly I created a define “ipconfigSOCKET_HAS_USER_DATA” and added a field to the TCP socket (just like the user semaphore) that is a void * for user data to be stored.

I added a setsockopt field called FREERTOS_SO_SET_USERDATA which allows you to call setsockopt to set the value of this.

At the moment, I’m “fudging” getting this userdata by including the private ip header and accessing it directly from say the connection or rx callback, I can then cast this to (in my case a class) and use further callbacks to schedule operations to occur.

Ideally, there would be a getsockopt which would allow you to read the userdata value, but as getsockopt has not been implemented at all, I kind of went for my naughty but easy solution of accessing the field.

From my point of view it makes the callbacks much more useful because the socket carries around extra information which I can use to quickly inform other layers there is something to do, without having to traverse lists to find the data assosicated with a socket.

Is this something of interest?

2

heinbali01 wrote on Tuesday, March 06, 2018:

Hi Adrian, your technique is not an unusual one, passing a void pointer in a call-back. The callee will cast that pointer to a known structure type. Every FreeRTOS task gets a void pointer in the same way.

But we’re always kind of conservative when judging new changes. It should not be too deviant from BSD, it should not add too much work or complexity, it should not be prone to errors etc.

You are proposing to make the struct a member of a socket. I tend to reverse this relationship: I create my struct that owns a BSD socket.

The FreeRTOS+TCP call-back is a C-function, e.g. of this type:

void vOnConnected( Socket_t xSocket, BaseType_t xConnected );

I store all Socket_t's in a sorted tree, and the corresponding CSocket can be looked-up easily:

void vOnConnected( Socket_t xSocket, BaseType_t xConnected )
{
	CSocket *xCSocket = xLookupSocket( xSocket );
	if( xCSocket != NULL )
	{
		xCSocket->vOnConnected( xConnected );
    }
	else
	{
		/* strange to get here. */
	}
}

The addition that you propose doesn’t disturb users that do not use it. We can make it a conditional feature.
But the implementation would be complex though: we would get two sets of call-backs (application hooks): the old ones with Socket_t, and the new versions with the user void pointer. And therefore it would lead to a lot of new code and documentation.

But I’m curious to hear what Richard thinks of it.

Regards.

3

heinbali01 wrote on Tuesday, March 06, 2018:

I wrote:

But the implementation would be complex though:
we would get two sets of call-backs (application hooks):
the old ones with Socket_t, and the new versions with th
user void pointer

Mistake: that is not needed. Once called you can callFREERTOS_SO_GET_USERDATA to find your user struct.

4

fizzyaid wrote on Wednesday, March 07, 2018:

Thanks for the response.

I’m still unsure of the best way of achieving what I want, my first thought was exactly as you have suggested above, obviously that incurs a lookup penalty (depending on the method of storing).

I think it may be the way to go, it’s certainly cleaner than using a select on a group of sockets and easier to manage than using user semaphores.

The issue I have is that I have a virtual machine running in a task, it’s even driven so for the most part it’s “asleep”, but there could be various wake up conditions, i.e input changed, uart data, socket data, timer elapsed etc, so thinking about this more it’s probably cleaner to use an event group and search each time for any inputs, uarts, sockets that may have caused the condition. No issues about queues and not being able to store the “event” in the queue (so events getting “lost”), TCP windowing should manage the flow control.

I realise this is the reverse of what my original message said and the opposite of the changes I quickly cobbled together!

As an aside, it would be nice if on the FreeRTOS documentation page these callbacks were actually documented rather than just saying “for advanced users”, I had to obviously go digging through code to find out the exacty implementation details for the callbacks.

It’s also so nice to be back using FreeRTOS, it was always a brilliant piece of software but I think the last version I used was 4 or 5 so there have been a lot of improvements since then, especially the addition of the TCP stack! I’m not a fan of LwIP so happy to never see that again!

5

fizzyaid wrote on Wednesday, March 07, 2018:

And just like that…I hit a wall.

Is something broken in the way the recv callback is used?

As far as I understand it (from the note you attached), returning 0 from it should cause the data to be stored in the socket so it can be retrieved later using a recv, except it doesn’t, it just endlessly loops around inside “lTCPAddRxdata” because no bytes are consumed and it therefore calls the callback again and again and again and again…

						if( pxSocket->u.xTCP.pxHandleReceive( (Socket_t *)pxSocket, ( void* )ucReadPtr, ( size_t ) ulCount ) != pdFALSE )
						{
							uxStreamBufferGet( pxStream, 0ul, NULL, ( size_t ) ulCount, pdFALSE );
						}

to get the behaviour as described in that note I had to alter it to:

						if( pxSocket->u.xTCP.pxHandleReceive( (Socket_t *)pxSocket, ( void* )ucReadPtr, ( size_t ) ulCount ) != pdFALSE )
						{
							uxStreamBufferGet( pxStream, 0ul, NULL, ( size_t ) ulCount, pdFALSE );
							continue;
						}

						break;
					}
				} //else
			#endif /* ipconfigUSE_CALLBACKS */

The addition of a continue in the case where the callback consumed, and a break if it didn’t and the removal of the else so that the standard packet processing gets called.

This allows me to use the callback to basically generate a “data available” which can then be used to wake up a task (without having to do a select or involving multiple semaphores).

I haven’t completely tested this (i.e I haven’t issued the recv), but given that it effectively works like the callback isn’t there, it should all be fine.

I’m just in the process of adding extra functionality to test this, I just thought I’d ask the question above based on my observation.

6

fizzyaid wrote on Wednesday, March 07, 2018:

Pretty sure this my modification here won’t work for all folks, it does in my case because I never consume inside the callback.

I implemented a bytes available function in the virtual machine and as expected it keeps going up according to the bytes I send to the socket.

Still reading that note you attached the implementation as far as I can see does not match what the note says should happen.

Now, I’m almost considering going the semaphore route, and keeping track of the sockets and polling them when a socket event happens, slower and I have to maintain a list of sockets that are in use. (in my case, there’s a virtual machine that is using the sockets)

7

heinbali01 wrote on Wednesday, March 07, 2018:

Hi Adrian, thanks for your comments.

As far as I understand it (from the note you attached), returning 0 from it should
cause the data to be stored in the socket so it can be retrieved later using a recv,
except it doesn’t, it just endlessly loops around inside “lTCPAddRxdata” because no
bytes are consumed and it therefore calls the callback again and again…

You’re right, at this moment it only works if your function returns ‘true’. I wasn’t aware of that. I should look-up if the code got broken at some point.

to get the behaviour as described in that note I had to alter it to:

>                 if( pxSocket->u.xTCP.pxHandleReceive( (Socket_t *)pxSocket, ( void* )ucReadPtr, ( size_t ) ulCount ) != pdFALSE )
>                 {
>                     uxStreamBufferGet( pxStream, 0ul, NULL, ( size_t ) ulCount, pdFALSE );
>                     continue;
>                 }
> 
>                 break;
>             }
>         } //else
>     #endif /* ipconfigUSE_CALLBACKS */

Yes, good idea to solve it this way. But it needs some rethinking.

Pretty sure that my modification here won’t work for all folks,
it does in my case because I never consume inside the callback.

As the call-back feature is not documented, there won’t be many people using it. And it would work if the callback function returns a non-zero value.

I implemented a bytes available function in the virtual machine
and as expected it keeps going up according to the bytes I send
to the socket.

There are some (non-BSD) functions check the buffers of a socket:

    /* Return the number of bytes in the RX buffer. */
    BaseType_t FreeRTOS_rx_size( Socket_t xSocket );

    /* Return the sizeof of available space in the TX buffer. */
    BaseType_t FreeRTOS_tx_space( Socket_t xSocket );

    /* Return the number of bytes in the TX buffer. */
    BaseType_t FreeRTOS_tx_size( Socket_t xSocket );

Still reading that note you attached the implementation as far as
I can see does not match what the note says should happen.

You are right that when the “receive callback” returns zero, the behaviour is not as expected.
But I don’t know of other abnormalities.

Now, I’m almost considering going the semaphore route, and keeping
track of the sockets and polling them when a socket event happens,
slower and I have to maintain a list of sockets that are in use.
(in my case, there’s a virtual machine that is using the sockets)

That is indeed another way. Which is very hybrid, in a sense that any task in your system can wake-up the task ( by giving to it’s semaphore ).

8

fizzyaid wrote on Wednesday, March 07, 2018:

Thanks Hein…I’m not going mad then!

9

fizzyaid wrote on Wednesday, March 07, 2018:

I think I’m going to archive this branch in the morning, revert back to plain old FreeRTOS and use a sempahore, queue and queue set to achieve what I want, for the most part the changes will be confined to one section of code. It does mean a “poll” of the sockets to see which ones need servicing and why they need servicing, but I don’t envisage many sockets being created anyway.

The same mechanism can be used for other things like serial ports etc as well.

10

thomask wrote on Wednesday, March 07, 2018:

Instead of a runtime lookup, you can just use a container_of macro:

struct mysocket {
int foo;
float bar;
Socket_t socket;
char *baz;
}

void vOnConnected( Socket_t xSocket, BaseType_t xConnected )
{
struct mysocket *s = container_of(xSocket, struct mysocket, socket);
…
}

(see https://code.woboq.org/linux/linux/include/linux/kernel.h.html#928 for an implementation)

11

heinbali01 wrote on Wednesday, March 07, 2018:

Hi Thomas, that is (more or less) how I looked up a socket reference.
But Adrian’s proposal would be cheaper: looking up the user pointer in a socket.

12

fizzyaid wrote on Wednesday, March 07, 2018:

i dont think that would work in this case seeing as freertos creates the socket, the extra data won’t be there to be accessed.

13

fizzyaid wrote on Thursday, March 08, 2018:

Thanks for all your help and thoughts on this, hopefully if I’ve achieved anything it’s that I found a bug in the callback system.

I just reverted the tcp stack back to original (i.e removed all my changes) and modified my socket code so that for every socket that is created, an assosicated semaphore and task is created. I don’t imagine many sockets being created, and I need to see what level of stack I can get away with to minimise the pain of having a task associated with every socket.

What this means is that the socket task is effectively very simple, upon starting it reads the rx length and current connection state and then waits forever on the semaphore (for the moment), when an event happens it checks if the rx length or connected state has changed, if so it fires off a callback which starts a chain of evens via a message queue which results in the VM processing the event.

I sat and thought about using a singleton semaphore and task, but that complicates matters because then I have to poll each socket to find out which one(s) have caused an event, in the end I went for the easier and less error prone option.

What would be a really nice addition would be (a non standard funtion!) something like select, but for a single socket:

pseudo code:

xEvent = FreeRTOS_waitforsocketevent(socket, portMAX_DELAY);

switch(xEvent)
{
         case EVT_CONNECTION_STATUS_CHANGED:
             break;
             
         case EVT_RX:
            break;
            
          case EVT_TX:
            break;
            
}

I’ve effectively created this function using a task.

Anyway…