BufferedTcpObject and OSTaskCreate()

[awdorrin]

Is it possible to pass a BufferedTcpObject as a parameter into OSTaskCreate()?

I'm trying to define the BufferedTcpObject in the UserMain and pass it to a UserTask() function.
Code is compiling, but it does not appear as if the object is passed properly (or perhaps my C++ is rusty and I'm not de-referencing properly)

Code: Select all

void UserTask( void *pdata )
{
	BufferedTcpObject tcp = *(BufferedTcpObject*)pdata;

	while(1)
	{
		if (tcp.Connected()) {
			iprintf("t");
		}
		else
		{
			iprintf(".");
		}
		OSTimeDly(1);
	}
}

void UserMain(void *pd) {

	// Initialize TCP socket
	static BYTE buffer[256];
	BufferedTcpObject tcp(buffer, 256, NULL);

	BufferedTcpObject *p = &tcp;

	if ( OSTaskCreate(
			UserTask,
			(void*)&p,
			(void*)&UserTaskStack[USER_TASK_STK_SIZE],
			(void*)UserTaskStack,
			MAIN_PRIO + 1) != OS_NO_ERR)
	{
		iprintf("***Error starting A/D Task");
	}

	tcp.Connect(DestIP, DestPort, 40);

	while (1) {
		OSTimeDly(1);
	}
}

[tod]

Yes, but you shouldn't. I would say you're flirting with disaster. Sounds like you want to pass a pointer to a buffer in one task's stack to another task's stack. You better make sure you implement proper locking protection if you want to get valid reads and writes all the time.
Your first problem is with this line:

Code: Select all

BufferedTcpObject tcp = *(BufferedTcpObject*)pdata;

This is calling the assignment operator for a BufferedTcpObject. (I suspect it first calls a BufferedTcpObject constructor). That is it's the same as this

Code: Select all

BufferedTcpObject tcp;  //constructor call - construct object on the stack
tcp = *(BufferedTcpObject*)pdata; //assignment call

If the assignment operator for BufferedTcpObject was written correctly it will NOT merely do a memberwise assignment. It will most likely do a full copy of the pointed to data structures (but it could do a reference counted copy - I just find it unlikely). (If you want to know why it should do this see Scott Meyer's Effective C++ Item 11). In effect you've ended up with a copy of the data contained in the main task at the time the spawned task gets to that line.

You could try

Code: Select all

BufferedTcpObject* tcp = (BufferedTcpObject*)pdata; //just copy the pointer

and then use the -> syntax

Code: Select all

if (tcp->Connected)

Safer (but still suspect) would be to do this:

Code: Select all

const BufferedTcpObject* tcp =  (BufferedTcpObject*)pdata; //make  what is pointed to const --personally I would also make the ptr const

Now at least you're promising to just read (not write) the other task's stack data. Even that's not a good idea without some sort of locking.

I would say the canonical way to exchange data between tasks is to use OS Calls (like mailbox and semaphores and often a combination of the two). Other may have differing opinions.

[awdorrin]

Tod, thanks for the clarification - my pointer knowledge is rusty when dealing with objects and I wasn't thinking about constructor/copy.

I was trying to pass the pointer to the object, realize now I was not.

I simplified the code slightly, now in the OSTaskCreate() pass '&tcp' rather than the &p, since that was giving me the address of the pointer to the tcp object, rather than the address of the tcp object.

I do realize that risk of using the tcp object in both the UserMain and the UserTask and have implemented protection using USER_ENTER_CRITICAL() and USER_EXIT_CRITICAL() - probably not the most elegant approach - but the code I'm working on is meant to be quick/dirty - not intended for production.

The code appears to be working as intended now. Wish I'd have thought of just printing the pointer addresses yesterday.

Thanks for your help!

-Al

[tod]

If this is truly throw-away code then fine. I think NB should fix this because it strikes me as a disaster waiting to happen. They did not write custom copy constructors or operator= for this class. In my not so humble opinion, both of these should be left private and undefined. Making a copy of this class (in addition to violating good design rules about coupling and encapsulation) is just asking for problems. What happens when the copy goes out of scope (at a minimum the socket closes, won't the owner be surprised.) I didn't analyze the class that much but I bet there's a good chance that the pList and pList_Head stuff doesn't work correctly on the copy (after copying pList and PList_HEad do not point to the same thing). The worst thing though is that the original buffer can go out of scope but the copy can still read/write to that memory and it has absolutely NO WAY of knowing the memory has been released. NB should not write library code that allows this. Because of how they pass in the pre-allocated memory buffers, I don't see how they can write a robust copy constructor/operator= so that is why I think they should be private and undefined.

[pbreed]

I Agree these should NOT be copied.
I'll make the copy constructor private and undefined.

Paul

[awdorrin]

I agree that it should not be a copy of the object.

What I was looking for was a pointer to the BufferedTcpObject, which is what I have now. (I'll admit my C pointer knowledge is rusty, too much C# and Java coding the past few years and not enough C/C++)

I would have preferred being able to not have to use the -> notation within my task function. for consistency. However I should have been passing the pointer in originally instead of the BufferedTcpObject.

Due to the limits of the SBL2E, I am hesitant about using other methods to share the same BufferedTcpObject between the UserMain and the UserTask. I really didn't want to open two sockets (one within each task) and double the memory usage.

I think sharing the BufferedTcpObject and using the critical code sections should work fine, given the constraints of the hardware. Still, if someone has a more efficient approach, I would be interested in seeing what it might be.

Thanks,
Al

[pbreed]

Two ways around this...

1)Make it a global object. (Structured design people are now screaming in pain....)

2)Pass by reference....

If I have a function of the form

void MyFunctionThatActuallyDoesWork(BufferedTcpObject & tob)
{
//Inside here the object is just

to.whatever..... No -> needed
}


//then in the place you have the pointer but not the reference....
BufferedTcpObject * pObj;
MyFunctionThatActuallyDoesWord(*pObj);

Now it may be possible to make a BufferedTcpObject referece
and do this by casting rather than do it as a function, but the function overhead in this case is small
(12 bytes on the stack, 4 bytes for the call return, probably never used, 4 bytes for the frame pointer in the function, and 4 bytes for the pointer to the bufferedtcpobject.)

[tod]

That Paul is so smart! Now I hate global variables as much as the next guy but using a global makes your code express your intent much better. I think given the way you're passing the pointer I would make it a file static global variable. That prevents the linker from allowing any other module to "ext" to it, and puts it on the heap (right Paul?). Also I would try and make it as const correct as possible. The pointer you pass should be a const pointer to non-const object, (passing a reference achieves this as well, and it gives you syntactic sugar when using it). It would be nice to create the original object pointer as a const pointer also but that is a little tricky since you'll have to new up the object and the underlying buffer all in one step. Normally doing this would be a memory leak on the underlying buffer, but again this expresses your intent that the buffer will NEVER go away and that the pointer to BufferedTcpObject should never point to anything else.

You'll need to reduce your stack size by the corresponding amount to have a zero sum game on memory.

[pbreed]

all statics are allocated by the linker, they do not go on the stack.
so a file local static would not be on the stack.


Paul