Odd problem with UDPClient [modified] - Solved
-
This is my third attempt to leave this question. The first time I submitted (without keeping a copy of the text - Grrr!) the submission apparently failed and I was left staring at a white screen. The second time was similar, though I did keep a copy. I am writing a Class to monitor and manage a WiFi-enabled HVAC thermostat which communicates using UDP packets. It sends out a "status report" containing various data every 10 seconds, which I am able to receive (using an asynchronous thread) and decode with no difficulty. Because the thermostat and its WiFi module are relatively slow and single-threaded, I have set the Class up so that settings changes are processed in its main thread and the resulting commands are sent out synchronously from the receiving thread after the status report is received and processed. This is where I have run into som problems. It appears that if I do not insert pauses (using Thread.Sleep) before and/or between using .Send to transmit each command packet, some packets do not get transmitted (confirmed by using a packet sniffer on the network). In addition, the first packet of at least some combinations of commands appears not to be sent if I do not include an Application.DoEvents call before the pause that precedes it. Putting in breakpoints and/or stepping through the Class code abolishes all misbehavior! This worries me a) because I don't like behavior I don't understand, and b) because I had planned to make the Class into a Class Library, which appears to preclude the use of Application Methods. The current version of the code is as shown below. Does anyone have any ideas as to what may be going on?
Private Sub ReceiveMessages()
Try
Dim receiveBytes As [Byte]() = TCudpClient.Receive(RecEndpoint)
' Some error checking code omitted
Process_Heartbeat(receiveBytes)If NewSettings Then ' There are some queued commands UpDating = True ' Set a flag to prevent new commands being added while we are processing existing ones Send\_Commands() ' See below UpDating = False NewSettings = False End If ThreadReceive = New System.Threading.Thread(AddressOf ReceiveMessages) ThreadReceive.Start() Catch ex As System.Threading.ThreadAbortException ' this generally means that the thread was deliberately aborted, so we can ignore it Catch e As Exception If Not ShuttingDown Then Throw New -
This is my third attempt to leave this question. The first time I submitted (without keeping a copy of the text - Grrr!) the submission apparently failed and I was left staring at a white screen. The second time was similar, though I did keep a copy. I am writing a Class to monitor and manage a WiFi-enabled HVAC thermostat which communicates using UDP packets. It sends out a "status report" containing various data every 10 seconds, which I am able to receive (using an asynchronous thread) and decode with no difficulty. Because the thermostat and its WiFi module are relatively slow and single-threaded, I have set the Class up so that settings changes are processed in its main thread and the resulting commands are sent out synchronously from the receiving thread after the status report is received and processed. This is where I have run into som problems. It appears that if I do not insert pauses (using Thread.Sleep) before and/or between using .Send to transmit each command packet, some packets do not get transmitted (confirmed by using a packet sniffer on the network). In addition, the first packet of at least some combinations of commands appears not to be sent if I do not include an Application.DoEvents call before the pause that precedes it. Putting in breakpoints and/or stepping through the Class code abolishes all misbehavior! This worries me a) because I don't like behavior I don't understand, and b) because I had planned to make the Class into a Class Library, which appears to preclude the use of Application Methods. The current version of the code is as shown below. Does anyone have any ideas as to what may be going on?
Private Sub ReceiveMessages()
Try
Dim receiveBytes As [Byte]() = TCudpClient.Receive(RecEndpoint)
' Some error checking code omitted
Process_Heartbeat(receiveBytes)If NewSettings Then ' There are some queued commands UpDating = True ' Set a flag to prevent new commands being added while we are processing existing ones Send\_Commands() ' See below UpDating = False NewSettings = False End If ThreadReceive = New System.Threading.Thread(AddressOf ReceiveMessages) ThreadReceive.Start() Catch ex As System.Threading.ThreadAbortException ' this generally means that the thread was deliberately aborted, so we can ignore it Catch e As Exception If Not ShuttingDown Then Throw NewHi Peter, 1. the blank pages are a known problem, the powers that be are working hard to get it fixed. 2. I think you have some threading problems. Here are two things I don't like at all: 2a. Sub ReceiveMessages() is creating and launching a thread that executes ReceiveMessages(). So you seem to want the code to run repetitively, however instead of using a loop, you create a new thread all the time? 2b. Application.DoEvents() is evil; when used, AFAIK it should be called only by the main thread, not by any other thread. And I don't think you need it at all. 3. It is not obvious to me how you intend on using all the code shown, as it consists of subs only. If you need further help, I suggest you explain it a bit more and indicate how/where/when something gets called (e.g. ReceiveMessages() is called from the main form's constructor; or from a button_clicked handler; ...) Hope this helps. :)
Luc Pattyn [Forum Guidelines] [Why QA sucks] [My Articles] Nil Volentibus Arduum
Please use <PRE> tags for code snippets, they preserve indentation, and improve readability.
-
Hi Peter, 1. the blank pages are a known problem, the powers that be are working hard to get it fixed. 2. I think you have some threading problems. Here are two things I don't like at all: 2a. Sub ReceiveMessages() is creating and launching a thread that executes ReceiveMessages(). So you seem to want the code to run repetitively, however instead of using a loop, you create a new thread all the time? 2b. Application.DoEvents() is evil; when used, AFAIK it should be called only by the main thread, not by any other thread. And I don't think you need it at all. 3. It is not obvious to me how you intend on using all the code shown, as it consists of subs only. If you need further help, I suggest you explain it a bit more and indicate how/where/when something gets called (e.g. ReceiveMessages() is called from the main form's constructor; or from a button_clicked handler; ...) Hope this helps. :)
Luc Pattyn [Forum Guidelines] [Why QA sucks] [My Articles] Nil Volentibus Arduum
Please use <PRE> tags for code snippets, they preserve indentation, and improve readability.
2a I am following a model published elsewhere, though the logic did make sense to me. The .Receive call needs to be in an independent thread - it is first activated from initialisation code. When ReceiveMessages is invoked on its thread, the thread stalls (for up to 10 seconds) until the UDPClient.Receive call returns. It then does the necessary processing and ends after invoking a new copy of itself on a new thread. Only one ReceiveMessages thread is in existence at a time (except for the very short time after the reinvocation and before the invoking thread terminates), or at least, that is what I assumed. I am not sure why you feel that using a loop would be better, though I also don't think it would be worse. 2b You are not the only one who thinks that Application.DoEvents is evil, but it seems to be the only thing that produces consistent behavior in this code. I dont know why this should be so, and I would be delighted to replace it with something cleaner that also worked - that was what I was hoping that someone would come up with. 3) This is a moderately large and complex Class (because the Thermostat has lots of bells and whistles!). I should have made it clear that the initial Threading call to ReceiveMessages was from the Class constructor, but I thought that the code which I included, which is all that runs in the problem thread, showed how I had things set up without being excessively long and confusing.
-
2a I am following a model published elsewhere, though the logic did make sense to me. The .Receive call needs to be in an independent thread - it is first activated from initialisation code. When ReceiveMessages is invoked on its thread, the thread stalls (for up to 10 seconds) until the UDPClient.Receive call returns. It then does the necessary processing and ends after invoking a new copy of itself on a new thread. Only one ReceiveMessages thread is in existence at a time (except for the very short time after the reinvocation and before the invoking thread terminates), or at least, that is what I assumed. I am not sure why you feel that using a loop would be better, though I also don't think it would be worse. 2b You are not the only one who thinks that Application.DoEvents is evil, but it seems to be the only thing that produces consistent behavior in this code. I dont know why this should be so, and I would be delighted to replace it with something cleaner that also worked - that was what I was hoping that someone would come up with. 3) This is a moderately large and complex Class (because the Thermostat has lots of bells and whistles!). I should have made it clear that the initial Threading call to ReceiveMessages was from the Class constructor, but I thought that the code which I included, which is all that runs in the problem thread, showed how I had things set up without being excessively long and confusing.
OK, here are some thoughts: 1. I'm inclined to use less code; so I'll try and get rid of some. Simpler is better, assuming it works. 2. I would replace your current Sub ReceiveMessage() by something along these lines:
Private Sub ReceiveMessages()
Try
ThreadReceive = New System.Threading.Thread(AddressOf ReceiverMethod)
ThreadReceive.Start()
Catch ex As System.Threading.ThreadAbortException
' this generally means that the thread was deliberately aborted, so we can ignore it
Catch e As Exception
If Not ShuttingDown Then Throw New ApplicationException("Problem receiving data from Thermostat" & vbCrLf, e)
End Try
End SubPrivate Sub ReceiverMethod()
While Not ShuttingDown
Try
Dim receiveBytes As [Byte]() = TCudpClient.Receive(RecEndpoint)
' Some error checking code omitted
Process_Heartbeat(receiveBytes)If NewSettings Then ' There are some queued commands UpDating = True ' Set a flag to prevent new commands being added while we are processing existing ones Send\_Commands() ' See below UpDating = False NewSettings = False End If Catch ex As System.Threading.ThreadAbortException ' this generally means that the thread was deliberately aborted, so we can ignore it Catch e As Exception If Not ShuttingDown Then Throw New ApplicationException("Problem receiving data from Thermostat" & vbCrLf, e) End Try End WhileEnd Sub
This makes sure only one extra thread is used, and every execution of the original code is by that one thread (including the very first, which wasn't true at all in the original code). The original with a new thread every so often is expensive on system resources, and has the risk of two of those threads running at the same time, confusing you and making you add code to fix things that could have been avoided. Note: some details may need fixing (e.g. what exceptions to catch, and how to deal with them) [ADDED] Remark: make sure the while loop is blocking somehow; it should not just sit there spinning like mad, wasting lots of CPU cycles; if there is no way to make it block (i.e. wait on something), insert a time delay using Thread.Sleep [/ADDED] 3. Just remove the DoEvent stuff. 4. If you want to send the same thing twice, no need to have two for loops. This should
-
OK, here are some thoughts: 1. I'm inclined to use less code; so I'll try and get rid of some. Simpler is better, assuming it works. 2. I would replace your current Sub ReceiveMessage() by something along these lines:
Private Sub ReceiveMessages()
Try
ThreadReceive = New System.Threading.Thread(AddressOf ReceiverMethod)
ThreadReceive.Start()
Catch ex As System.Threading.ThreadAbortException
' this generally means that the thread was deliberately aborted, so we can ignore it
Catch e As Exception
If Not ShuttingDown Then Throw New ApplicationException("Problem receiving data from Thermostat" & vbCrLf, e)
End Try
End SubPrivate Sub ReceiverMethod()
While Not ShuttingDown
Try
Dim receiveBytes As [Byte]() = TCudpClient.Receive(RecEndpoint)
' Some error checking code omitted
Process_Heartbeat(receiveBytes)If NewSettings Then ' There are some queued commands UpDating = True ' Set a flag to prevent new commands being added while we are processing existing ones Send\_Commands() ' See below UpDating = False NewSettings = False End If Catch ex As System.Threading.ThreadAbortException ' this generally means that the thread was deliberately aborted, so we can ignore it Catch e As Exception If Not ShuttingDown Then Throw New ApplicationException("Problem receiving data from Thermostat" & vbCrLf, e) End Try End WhileEnd Sub
This makes sure only one extra thread is used, and every execution of the original code is by that one thread (including the very first, which wasn't true at all in the original code). The original with a new thread every so often is expensive on system resources, and has the risk of two of those threads running at the same time, confusing you and making you add code to fix things that could have been avoided. Note: some details may need fixing (e.g. what exceptions to catch, and how to deal with them) [ADDED] Remark: make sure the while loop is blocking somehow; it should not just sit there spinning like mad, wasting lots of CPU cycles; if there is no way to make it block (i.e. wait on something), insert a time delay using Thread.Sleep [/ADDED] 3. Just remove the DoEvent stuff. 4. If you want to send the same thing twice, no need to have two for loops. This should
Thanks! The short version is that your suggestions about reorganising the Threading certainly simplified things and may well have been 90% responsible for solving the problem (I'll come back to the 10%, below) The longer version: 1. Agreed! 2. Done, with very minor modifications. The While loop blocks "automatically" on the UDPClient.Receive call, which doesn't return until the next Status Report packet arrives, 10 seconds later. 3. Done (and in one other place in the Class) 4. It was actually only the first packet sent after the (initial and only) UDPClient.Connect call that seemed to have problems - only that packet was sent twice by my original code (the first For loop has an early exit). Your code sends every packet twice, which was not my intention! 5. Threads (up until recently) weren't my thing. Data structures are! There are (IMHO) good reasons for the way I am doing this. 6. I have resorted to this sort of approach once or twice (more when I was writing real-time data acquisition and control stuff), but haven't found it useful in general programming often enough to put the "hooks" for it in my code on a routine basis. 7. It appears from further experimentation with the modified code that you do need delays between at least some calls to UDPClient Methods, and that I wasn't leaving quite enough time between them in some cases. Sleeping for 200 msec seems to work reliably for my situation - 100 msec is not always enough. I think that it is possible that the Application.DoEvents calls were "working" simply because they were wasting more time, and the poor threading code may also have been only a contributing factor to the problems.
-
Thanks! The short version is that your suggestions about reorganising the Threading certainly simplified things and may well have been 90% responsible for solving the problem (I'll come back to the 10%, below) The longer version: 1. Agreed! 2. Done, with very minor modifications. The While loop blocks "automatically" on the UDPClient.Receive call, which doesn't return until the next Status Report packet arrives, 10 seconds later. 3. Done (and in one other place in the Class) 4. It was actually only the first packet sent after the (initial and only) UDPClient.Connect call that seemed to have problems - only that packet was sent twice by my original code (the first For loop has an early exit). Your code sends every packet twice, which was not my intention! 5. Threads (up until recently) weren't my thing. Data structures are! There are (IMHO) good reasons for the way I am doing this. 6. I have resorted to this sort of approach once or twice (more when I was writing real-time data acquisition and control stuff), but haven't found it useful in general programming often enough to put the "hooks" for it in my code on a routine basis. 7. It appears from further experimentation with the modified code that you do need delays between at least some calls to UDPClient Methods, and that I wasn't leaving quite enough time between them in some cases. Sleeping for 200 msec seems to work reliably for my situation - 100 msec is not always enough. I think that it is possible that the Application.DoEvents calls were "working" simply because they were wasting more time, and the poor threading code may also have been only a contributing factor to the problems.
I'm glad you got on top of it, well mostly anyway. 5. I've done lots of real-time stuff, even with queues. One very neat trick is this: - create two queues, lets call them FULL and EMPTY; FULL will represent the functional queue, EMPTY will be the store for messages currently not in use; - create N messages, put all of them in the EMPTY queue; - have the producer get a message from the EMPTY queue, fill it, and put it in the FULL queue; - have the consumer get a message from the FULL queue, execute it, and put it in the EMPTY queue. The net result is you don't need object creation/destruction once things have been initialized; and you get an automatic dataflow-limited behavior, as either queue will never hold more than N items. It works just great, provided all messages have the same type (or size). 6. Same here. In embedded systems, you typically want to observe without halting everything, hence logging fits the bill. What remains to be done IMO is figuring out why you need your delays. Is it related to the reaction time of your thermostat? (i.e. is it deaf while processing an earlier command or transmitting a result?) Timestamped logging should help out here as well. :)
Luc Pattyn [Forum Guidelines] [Why QA sucks] [My Articles] Nil Volentibus Arduum
Please use <PRE> tags for code snippets, they preserve indentation, and improve readability.
-
I'm glad you got on top of it, well mostly anyway. 5. I've done lots of real-time stuff, even with queues. One very neat trick is this: - create two queues, lets call them FULL and EMPTY; FULL will represent the functional queue, EMPTY will be the store for messages currently not in use; - create N messages, put all of them in the EMPTY queue; - have the producer get a message from the EMPTY queue, fill it, and put it in the FULL queue; - have the consumer get a message from the FULL queue, execute it, and put it in the EMPTY queue. The net result is you don't need object creation/destruction once things have been initialized; and you get an automatic dataflow-limited behavior, as either queue will never hold more than N items. It works just great, provided all messages have the same type (or size). 6. Same here. In embedded systems, you typically want to observe without halting everything, hence logging fits the bill. What remains to be done IMO is figuring out why you need your delays. Is it related to the reaction time of your thermostat? (i.e. is it deaf while processing an earlier command or transmitting a result?) Timestamped logging should help out here as well. :)
Luc Pattyn [Forum Guidelines] [Why QA sucks] [My Articles] Nil Volentibus Arduum
Please use <PRE> tags for code snippets, they preserve indentation, and improve readability.
5. In this case, all the messages are not the same size (the message header length is constant, but a message can contain 0, 1, 2, or an arbitrary (2 < n <= 26) number of additional bytes of data). Also, for a number of reasons related to the device and application, I want to be able to identify and (if necessary) modify a pending command, if another command of the same "class" is issued before the first one is executed, which may be up to 10 seconds later. The thermostat is relatively slow (in computer terms), and can only do one thing at a time, but I don't think that this can be causing the need for the delays. If I don't put them in, some of the UPD packets are never sent. It is as if the outgoing UDP port needs time to "clear" before it will send a new message.