Re: Is a Thread appropriate?

From: Joseph M. Newcomer (newcomer_at_flounder.com)
Date: 02/20/05


Date: Sat, 19 Feb 2005 19:29:04 -0500

See below...
On Fri, 18 Feb 2005 00:59:03 -0800, "Arno Kromke" <ArnoKromke@discussions.microsoft.com>
wrote:

>
>
>"Joseph M. Newcomer" wrote:
>
>> Yes. A thread is both appropriate and necessary.
>>
>> Now, if you are not doing dynamic loading of the DLL, you shouldn't need GetProcAddress.
>> If you are doing dynamic loading, get the address exactly once, when you load the DLL, not
>> every time you call the function. Assume that the lookup of the name is very, very,
>> expensive.
>
>Actually, I'm not responsible for the CSerialCom. However I used it without
>trying to understand ...
>Why is it bad to GetProcAddress every time? It takes much time to evaluate
>the address? Therefore it's better to store the result, right?

*****
GetProcAddress by name lookup is very slow. Assume that it will be seriously slow.
*****
>
>>
>> Note that it is foolish to create a char array, particularly a char array of a fixed size.
>> GetProcAddress would work fine as
>>
>> p = (LP2INT)GetProcAddress(hDLL, "READBYTE");
>> so why do you think you need to declare a separate variable for this purpose?
>>
>> If you need to declare a separate variable (which is inappropriate here), you could
>> declare it as
>>
>> const char szFuncName[] = "READBYTE";
>>
>> but there is no reason to create the array at all; you could have done
>>
>> const char * szFuncName = "READBYTE";
>>
>> but neither of these make much sense in this particular context. Why introduce a
>> completely unnecessary separate variable to solve a nonexistent problem?
>>
>Well, I'm not sure about that. The "READBYTE" has to be stored somewhere. If
>it isn't explicitly linked to a variable, the compiler will assign it to a
>place in memory. Moreover, change the array to a constant (const) doesn't
>change anything, does it? It's then just a constant and can't be changed
>contentwise. Besides the fact that memory is allocated everytime the function
>is called, why don't you like the array?
****
It is an issue of programming style. The general rule is to not introduce variables
without necessity. If the variable isn't necessary, don't introduce one. Yes, the string
has to be stored somewhere. So let the compiler store it (note that what you wrote
requires a copy each time the variable is initialized). Declaring it a const is good
practice, because you are not going to change it.

I don't like the array because it is a waste of conceptual information. It adds nothing
functionally, but introduces several new concepts: a name, an initialization, a use of the
name, which merely increase the entropy of your program, making it harder to read an
understand.

Do not introduce unnecessary variables into a program. This one is unnecessary.
*****
>
>char szFuncName[9] = "READBYTE";
>char *szFuncName = "READBYTE";
>
>Isn't it the same?
****
Absolutely not. The first form creates an array of 9 characters, then copies the constant
string into the array. The second form creates a pointer, then copies a pointer to the
string into the variable. If you don't understand this, please go back and study
introductory C language texts.
****
>
>> If you want to do both input and output concurrently, you need to use asynchronous I/O;
>> presumably your DLL is doing this (although the use of a DLL to do a trivial task is
>> already suspect. Asynchrnonous serial I/O is relatively trivial to write in such a case).
>>
>> I tend to use two threads: one for input, and one for output. Then if the ReadFile fails
>> on an ERROR_IO_PENDING I do a WaitForMultipleObjects on the event handle and a shutdown
>> event, where the shutdown event is the [0]th element of the array. This allows for
>> graceful shutdown. This allows you to set longer timeouts and get better response to
>> thread shutdown requests.
>>
>It is is a small project for my studies and I don't wanted to spend too much
>time on it. I'm not an expert either. I/O seems to me pretty complex. It's
>the first time that I'm using a thread and I just read a chapter in book
>about it. I don't have any practial experience on it.
>
>first it looked like:
>
>bool bThreadRunning = true;
>
>void CClass::Run()
>{
> while(bThreadRunning)
> {
> ..........
> }
>
>}
>
>void CClass::Stop()
>{
> bThreadRunning = false; // stop thread
>}
>
>
>The problem with that is the CPU load. I had to change it somehow ...
>
****
Then you got it wrong in the thread function. The thread function should block if there is
nothing to do. If it doesn't block, then it will consume 100% of the CPU. This indicates
an erroneous design. The fix is to fix the problem, not build a program that is even more
bizarre.
****
>
>> What I don't understand is why you are creating a thread each time you read a character.
>> This is horrendously inefficient. There is no need to do this.
>>
>> There is no need to test pParam as you have done. It is simply an error to pass in a NULL,
>> so you should do
>>
>> ASSERT(pParam != NULL)
>>
>> if(pParam == NULL)
>> return 0;
>>
>> Forget return values other than 0; nobody cares about thread return values.
>>
>ok, thanks
>
>> For more, see below...
>>
>> On Thu, 17 Feb 2005 10:51:02 -0800, "Arno Kromke" <ArnoKromke@discussions.microsoft.com>
>> wrote:
>>
>> >Hi,
>> >
>> >I'm working on a dialog based app that utilizes PC's serial port. Up to now,
>> >the app is just capable to send. I'd like to extend it by receive
>> >functionality. Actually, port I/O isn't the issue since I'm using a dll that
>> >does it for me. The issue I discovered is that I would lock my application
>> >for user commands while waiting for some data to be received. Data arrives
>> >now and then - might be within a second or might take longer like 5 minites
>> >...
>> >
>> > I applied a thread to workaround the locking of the application. But I
>> >don't know if that is an appropriate technique solve this kind of
>> >communication. I think it would be much better if my app gets notified by
>> >incomming data and only in this case the application reads from the port.
>> >Does anyone have some suggestions? Whats appropriate for that?
>> >
>> >Here's what I did:
>> >
>> >- there's a port.dll that I'm interfacing thru a wrapper class CSerialCom
>> >- CSerialCom got member functions for open, close, send and read port
>> >
>> >here's the member function that i use for reading from port in particular:
>> >
>> >int CSerialCom::ReadByte()
>> >{
>> >typedef int (CALLBACK* LP2INT)();
>> >char szFuncName[9] = "READBYTE";
>> >LP2INT p;
>> >p = (LP2INT)GetProcAddress(hDLL, szFuncName);
>> > return p();
>> >
>> >}
>> >
>> >
>> >The following code is a snippet of my dialog based app where I create the
>> >worker thread:
>> >...
>> >CWinThread* pThread = AfxBeginThread(threadHelperFunc, this);
>> >pThread->SetThreadPriority(THREAD_PRIORITY_BELOW_NORMAL);
>> >...
>> >
>> >// static function
>> >UINT CDspComDlg::threadHelperFunc(LPVOID pParam)
>> >{
>> >CDspComDlg* pApp = (CDspComDlg*) pParam;
>> >if (pApp)
>> >{
>> > pApp->threadReceive();
>> > return 1;
>> >}
>> >
>> >return 0;
>> >}
>> >
>> >
>> >
>> >void CDspComDlg::threadReceive(void)
>> >{
>> > chBuffer = pCom->ReadByte(); // pCom is object of
>> >CSerialCom
>> > if (bFirstByte) // lower byte to be expected?
>> > {
>> >
>> > if (LowerByte != chBuffer) // new data has been received
>> > {
>> > LowerByte = chBuffer;
>> > bFirstByte = false; // set flag, upper byte to be expected next
>> > }
>> > }
>> > else
>> > {
>> > // we expect upper byte
>> > if (UpperByte != chBuffer)
>> > {
>> > UpperByte = chBuffer; // received data complete
>> > bFirstByte = true; // next will be lower byte
>> >
>> > // alignment
>> > ReceivedValue = (short int)UpperByte;
>> > ReceivedValue = ReceivedValue <<8;
>> > ReceivedValue += (short int)LowerByte;
>> >
>> > ...
>> > etc.
>> > ....
>> > }
>> > }
>> > Sleep(1);
>> >}
>> >
>> >
>> This is about the most convoluted code I could imagine to accomplish a simple task.
>>
>> The code is incredibly simple:
>>
>> chBuffer[0] = pCom->ReadByte();
>> chBuffer[1] = pCom->ReadByte();
>> ReceivedValue = *(short int *) chBuffer;
>>
>> I have no idea why you have chosen to create the received value in such a complex fashion.
>> ReceivedValue = MAKEWORD(lowerByte, upperByte);
>
>I like your solution better ;-) very efficient in comparison to what I've
>done.
>
>>
>> would have done the job without any complexity, but the whole business about trying to
>> figure out what byte you are reading and where to put it is needlessly complex.
>>
>> Lose the Sleep(1). If your program doesn't work without it, you have such deep and
>> fundamental flaws that it isn't going to save you.
>>
>
>I think I can get rid of it. But what's wrong with Sleep? It's like a wait
>for the working thread and returns computation time to the main thread.
>
>> Note that this code (both yours and mine) doesn't recover from serial port I/O errors.
>> Presumably they throw an exception; otherwise, you have to assume that the I/O completed.
>> In the case of trying to do asynchronous shutdown, this won't work very well. If you are
>> doing syncrhonous I/O, you are in seirously deep trouble anyway (you can't do concurrent
>> I/O, you can't shut down your program), so it is reasonably safe to assume that
>> asynchronous I/O is mandatory.
>>
>> If the DLL isn't doing asynchronous I/O, scrap it. I posted the code for async I/O within
>> the last day on this newsgroup.
>> joe
>
>Ok, I'll have a look at your i/o code. Thanks reviewing my code and for all
>of your
>suggestions and qualified critics.
>
>> >Thanks & Regards,
>> >
>> >Arno
>>
>> Joseph M. Newcomer [MVP]
>> email: newcomer@flounder.com
>> Web: http://www.flounder.com
>> MVP Tips: http://www.flounder.com/mvp_tips.htm
>>
>
>Arno

Joseph M. Newcomer [MVP]
email: newcomer@flounder.com
Web: http://www.flounder.com
MVP Tips: http://www.flounder.com/mvp_tips.htm