Prev: TabControl vs CPropertySheet & CPropertyPage
Next: side by side configuration - what does this really mean?
From: Joseph M. Newcomer on 31 May 2010 12:46
See below...
On Mon, 31 May 2010 00:55:17 -0700 (PDT), bernd <bernd.schuster12(a)googlemail.com> wrote:

>thanks for the answers.
>
>
>/* transmit eth-messages to the specific com port (is working) */
>unsigned char* buffer=new unsigned char[400];
>memcpy((void *)&buffer[0], (void *)&ptr->Data[0], 400);
****
Why 400? Why is this a magic number? Why are you not just copying the data that you want
to send? Why do you allocate a fixed-size buffer that is not equal to the length of the
data (maybe +1, if you want to transmit a terminal NUL)? Why are you alway transmitting
400 bytes of data?

>m_Ports[number].m_serialWriteThread-
>>PostThreadMessageA(UWM_SEND_DATA2, (WPARAM)&buffer, 400);
*****
This is erroneous. You are sending the address of the pointer called 'buffer', which is
on your local stack, across a thread. THis means that when that value is examined in the
thread, it will have whatever value is in that location at the time, which might be total
and complete garbage. You should do
(WPARAM)buffer
to send the actual buffer POINTER that you allocated. Why do you claim the buffer has 400
bytes when it probably doeasn't have 400 valid bytes in it? And why are you using memcpy
of a fixed size? If the source data is a pointer to a string, you will copy all kinds of
garbage into the buffer; if the string happens to be near the end of a page, this can give
you an access fault if the following bytes do not exist. I get very scared when I see
memcpy used like this; this is one of the reasons that in some companies, strcpy, strcat,
sprintf, and memcpy are all firable offenses if found in your code.
*****
>
>void CSerialWriter::OnSendData(WPARAM wParam, LPARAM lParam)
>{
>unsigned char * schar = (unsigned char *)wParam;
****
Why not
LPBYTE scha = (LPBYTE)wParam;
This is, after all, Windows, and we have nice types like LPBYTE so we don't have to
program like it was 1975.

And what does the 's' stand for? signed?
****
>UINT length = strlen((const char *)wParam); //<- 3 instead of 400
****
OK, it is worse than I imagined! Your memcpy of 400 is ALWAYS going to be wrong, and
possibly fatally wrong!

There ia no need to do the cast; you could have written
UINT length = strlen(schar);
because you already had a character pointer!

the correct calling sequence would have been

CStringA * s = new CStringA(ptr->data);
m_Ports[number].m_serialWriteThread->PostThreadMessageA(UWM_SEND_DATA2, (WPARAM)s);

and your handler should be

CStringA * s = (CStringA *)wParam;

*****
>
>BOOL ok = ::WriteFile(parms->hCom,(unsigned char
>*)schar,lParam,&bytesWritten,&ovl);
>//....
****
this would now be
BOOL ok = ::WriteFile(parms->hCom, (LPCSTR)*s, s.GetLength(), &bytesWritte, &ovl);
You have written code that is needlessly complicated, and in fact, given the magical 400,
is absolutely incorrect and fatally so.
****
>
>delete [] schar;
>}
>
>How can I delete the unsigned char array at the end of the
>OnSendData() function?
****
Well, because you did new[] to allocate it, the delete [] schar is already correct. In
the rewrite, using a CString*, I would have written
delete s;
joe

****
>
>
>best regards
>Bernd
Joseph M. Newcomer [MVP]
email: newcomer(a)flounder.com
Web: http://www.flounder.com
MVP Tips: http://www.flounder.com/mvp_tips.htm
From: bernd on 2 Jun 2010 09:38
On 28 Mai, 17:54, "Scott McPhillips [MVP]" <org-dot-mvps-at-scottmcp>
wrote:
> You don't need and don't want thread-ID.
>
> When you create the threads save the pointer that is returned by
> AfxBeginThread.  To post a message to the thread you can then use:
>
> pointertothread->PostThreadMessage(....);
>
> >>> How is it possible for me to know which thread which com port is
>
> using?
>
> When you create a thread initialize it to TELL it which com port to use.
> You're in charge.
>
> >>> Or is it much better to use one thread for all 8 com ports
>
> (transmitting the data)?
>
> A transmit thread and a receive thread for each com port is a good plan.
>
> --
> Scott McPhillips [VC++ MVP]

A small additional question: is it enough to have one thread for the
ethernet communication (socket) or could it be much better to have one
thread for the receive and one for the transmit part? Or another way:
using one receive-list and one transmit-list...

best regards
Bernd
From: ScottMcP [MVP] on 2 Jun 2010 10:19
On Jun 2, 9:38 am, bernd <bernd.schuste...(a)googlemail.com> wrote:
> A small additional question: is it enough to have one thread for the
> ethernet communication (socket) or could it be much better to have one
> thread for the receive and one for the transmit part? Or another way:
> using one receive-list and one transmit-list...

If you are using CAsyncSocket (or equivalent in winsock) then you
can't use one ethernet port in two threads. The socket notifications
are, by design, all posted to one thread's message queue. The calls
you make to socket functions are non-blocking (they all return
quickly) so there would be no advantage to using two threads.

From: Joseph M. Newcomer on 2 Jun 2010 10:22
See below...
On Wed, 2 Jun 2010 06:38:17 -0700 (PDT), bernd <bernd.schuster12(a)googlemail.com> wrote:

>On 28 Mai, 17:54, "Scott McPhillips [MVP]" <org-dot-mvps-at-scottmcp>
>wrote:
>> You don't need and don't want thread-ID.
>>
>> When you create the threads save the pointer that is returned by
>> AfxBeginThread. �To post a message to the thread you can then use:
>>
>> pointertothread->PostThreadMessage(....);
>>
>> >>> How is it possible for me to know which thread which com port is
>>
>> using?
>>
>> When you create a thread initialize it to TELL it which com port to use.
>> You're in charge.
>>
>> >>> Or is it much better to use one thread for all 8 com ports
>>
>> (transmitting the data)?
>>
>> A transmit thread and a receive thread for each com port is a good plan.
>>
>> --
>> Scott McPhillips [VC++ MVP]
>
>A small additional question: is it enough to have one thread for the
>ethernet communication (socket) or could it be much better to have one
>thread for the receive and one for the transmit part? Or another way:
>using one receive-list and one transmit-list...
****
For networking, it is impossible to have one thread for receive and one thread for send if
you are using CAsyncSocket, because this relies on the handle map to map incoming
notifications to the CAsyncSocket-derived class, and the handle map is per-thread.
Therefore, you can have one thread per connection (although with CAsyncSocket, this is
unnecessary) but you cannot split a connection's two operations (send/receive) across two
threads. It works for serial ports because only the HANDLE object needs to be shared
across the threads, and MFC doesn't come into the picture.

If you do raw SOCKET programming, you are on your own (not a recommended practice, because
of the complexity), and therefore could do the split, but it doesn't actually buy much to
allow it.

joe
****
>
>best regards
>Bernd
Joseph M. Newcomer [MVP]
email: newcomer(a)flounder.com
Web: http://www.flounder.com
MVP Tips: http://www.flounder.com/mvp_tips.htm
First  |  Prev  | 
Pages: 1 2 3
Prev: TabControl vs CPropertySheet & CPropertyPage
Next: side by side configuration - what does this really mean?