Re: thread related issue
- From: Joseph M. Newcomer <newcomer@xxxxxxxxxxxx>
- Date: Wed, 06 Sep 2006 11:34:04 -0400
Many problems here...
On 5 Sep 2006 23:13:43 -0700, "harshalshete@xxxxxxxxx" <harshalshete@xxxxxxxxx> wrote:
hi all,****
i am having one problem with threading
i created one dialog based application whose work is to read the data
from serial port and display it.so i created one thred for this work.
means now i got two threads one is application thread and other is
serial communication thread which is created in OnInitDialog.
now everything is working i mean the thread got created it read some
bytes
from port and now it wants to show the data so need to send this data
to main
thread. so i am doing this with ::SendMessage() but it is giving access
violation. i don't know why this is happening and currently i am not
sending the data also i.e. buffer so please help me
below is code (stepwise)
1)In Oninitdialog
THREADPARAM *ptp = new THREADPARAM;
if(ptp)
{
memset(ptp->array,0,30);
WHAT IS 30? How did you arrive at this magical value?
::ZeroMemory(ptp->array, sizeof(ptp->array));
would make more sense
****
strcpy(ptp->array,"Harshal");****
This is unsafe. Use StringCchCopy or strcpy_s (if you are using VS.NET 2005)
#include <strsafe.h"
StringCchCopy(ptp->array, _T("Harshal"), sizeof(ptp->array)/sizeof(TCHAR));
*****
ptp->appHandle = this->m_hWnd;****
hSerialThread = CreateThread( NULL,
0,
(LPTHREAD_START_ROUTINE)
SerialCommunication,
(LPVOID) NULL,
0,
&dwSerialThreadID);
This is very scary. If you are using MFC, you should be using AfxBeginThread. Do NOT use
::CreateThread unless you want to see the MFC library mess you over because it has not
been properly initialized for the thread.
****
}****
2)Thread function
UINT SerialCommunication(LPVOID pParam)
{
int write_req=0;
THREADPARAM *ptp = (THREADPARAM *)pParam;
while(1)
while(TRUE)
Do not use integers to represent boolean values
****
{****
if(write_req == 2)// send write request and read it
What in the world does "2" mean? Have you ever heard of #define? enum?
<<It is worse that I could have imagined. You are trying to do a state machine using an
increment. The code here is so complex I can't imagine how it works. You have made a
simple problem difficult>>
****
{****
DWORD length=0;
DWORD dwRead=0;
if (WriteFile(hCom,
SEND_CMD, // pointer to data to write to
file
NOOFBYTE, // number of bytes to write
&length, // pointer to number of bytes
written
NULL) == 0)
{// write failed
write_req = 0;
continue;
}
// writefile succeeded issue readfile
char buffer[MAX_MESSAGE];
memset(buffer,0,MAX_MESSAGE);
Since you are about to read stuff into it, why are you clearing it?
****
if (!ReadFile(hCom, buffer, MAX_MESSAGE, &dwRead,****
NULL))
MAX_MESSAGE-1, otherwise you won't have a properly terminated string and all sorts of
things will start to go wrong
****
{*****
write_req = 0;
continue;
}
if(dwRead > 0)
{
//MessageBox(NULL,buffer,"Data with packet",MB_OK);
Never issue a MessageBox from a worker thread.
Also, you should do
buffer[dwRead] = '\0';
to properly terminate the buffer. Note that if the sender sent a message of exactly
MAX_MESSAGE, it would not be terminated properly.
*****
****
::SendMessage(ptp->appHandle,MY_SHOW_DATA,0,0);//
how does this know where the buffer is? (LPARAM)ptp might be a Really Good Idea here
****
error is here****
}
write_req = 0;
}
else // only read from port and increment counter
{
DWORD dwRead=0;
char buffer[MAX_MESSAGE];
memset(buffer,0,MAX_MESSAGE);
if (!ReadFile(hCom, buffer, MAX_MESSAGE, &dwRead,
NULL))
Same problem as above; MAX_MESSAGE-1, no need to clear the buffer.
Also, you should tend to use ::ZeroMemory instead of memset
****
{****
write_req++;
continue;
}
if(dwRead > 0)
{// you are in thread you need to send message to
// main application
//char a;
HWND hwnd = (HWND)AfxGetApp()->m_pMainWindow;
This is wrong; it converts the CWnd* value to be an HWND handle, which is meaningless.
Why not do
AfxGetApp()->m_pMainWindow->SendMessage(...)
and in any case, since you are using the ptp->appHandle value, the above line is useless
****
::SendMessage(ptp->appHandle,MY_SHOW_DATA,0,0);****
//MessageBox(NULL,buffer,"Data without
packet",MB_OK);
}
write_req++;
}
}
return 0;
}
Generally, you should use two threads, one to send, and one to receive. Trying to fold
everything into a single thread results in the complex code you have here, with odd
counters trying to maintain state. In an asynchronous event-driven world, you shouldn't
try to keep concepts like sequential modeling of asynchronous events.
****
3) Sent message function
LRESULT CReadDataDlg::OnShowDataMessage(UINT wParam, LONG lParam)
{
return 0;
}
Why in the world would you use weird types like UINT and LONG? The proper types are
*always* WPARAM and LPARAM, always, no exceptions, ever. Note that this code could not
port to Win64, because UINT and LONG are 32-bit, always, but WPARAM and LPARAM will be 64
bits. Never, ever use UINT or LONG for these parameters.
Take a look at my essay on serial port I/O on my MVP Tips site.
****
Joseph M. Newcomer [MVP]
Thanks and regards
Harshal
email: newcomer@xxxxxxxxxxxx
Web: http://www.flounder.com
MVP Tips: http://www.flounder.com/mvp_tips.htm
.
- References:
- thread related issue
- From: harshalshete@xxxxxxxxx
- thread related issue
- Prev by Date: Re: When presses ENTER on Edit BOX
- Next by Date: Re: Opening Large Files
- Previous by thread: Re: thread related issue
- Next by thread: Combo Box Control
- Index(es):
Relevant Pages
|
