|
|
Dear Daniel the only question remaining I have is that other class may call OpenPort method (or similar ClosePort - which has similar lock as OpenPort inside with same lock object). Please see the updated question.
Does this change anything?
modified 8-Feb-16 6:19am.
|
|
|
|
|
For a given port, OpenPort and ClosePort should be called only once. Calling OpenPort twice (with no intervening ClosePort) is an error.
One way to handle this would be to create a separate class which calls OpenPort in its constructor and ClosePort in its destructor. This would guarantee that each function is called only once. It is still your responsibility to ensure that only one instance of the class is created at any one time.
Search for the Singleton pattern for more details on how to do this.
If you have an important point to make, don't try to be subtle or clever. Use a pile driver. Hit the point once. Then come back and hit it again. Then hit it a third time - a tremendous whack.
--Winston Churchill
|
|
|
|
|
As you can see from my HASP class calling OpenPort twice is not possible because of m_isOpen flag. (please see updated question).
|
|
|
|
|
Not the answer to your question but some comments:
Because a serial port can be opened only once, there is no need to lock inside your OpenPort function. If the port is already opened, SerialPort::Open() will throw an exception.
There is also no need to have the m_isOpen member variable. Just use SerialPort::IsOpen() instead.
Regarding the locking question:
If you want to ensure that a series of transfers is not interrupted by another thread, you must lock like shown in your Tester class.
But then there is no need to lock also inside the Query() function. So you can use the locking object of your serial class instead by providing public access.
|
|
|
|
|
Thanks a lot foe your feedback but I didn't get your last sentence 
But then there is no need to lock also inside the Query() function. So you can use the locking object of your serial class instead by providing public access.
You meant locking object of my Tester class?
|
|
|
|
|
Yes. You can provide a function to return that object and pass it to lock().
|
|
|
|
|
didn't get you again sorry
|
|
|
|
|
In your Tester class lock using the object of your serial class (HASPClass::m_locker) and remove the locking inside your Query() function.
|
|
|
|
|
Thanks for suggested improvement. (PS in your case I'd need to make HASPClass lock public?)
Otherwise my case is also correct right?
|
|
|
|
|
It must be public then.
Your case is correct if Tester is the only class using the serial class (as answered by Daniel).
But if you have another class that uses the serial communication from within another thread too, it would be not safe. The general rule is: All locking should use the same object.
|
|
|
|
|
Other Class may ONLY call OpenPort (or Similar ClosePort method - which has similar lock inside as OpenPort) method.
Otherwise indeed only tester uses that.
What you say now?
|
|
|
|
|
Member 12061600 wrote: What you say now?
Nothing else than I had before: Then it would be safe.
But:
Using it that way (a locking object in the Tester class) is bad style. It would not work anymore if you (or even worse anyone else) decide later to use it from within another class.
Again:
There is no need to lock upon opening (and closing). The exclusive behaviour of serial ports makes it unnecessary. If you want to open the port from different code parts, you must handle the execptions (they should be handled anyway). Using locking there does not prevent the exception. It may just delay it.
Conclusion:
Your current code is actually thread safe. But there are unnecessary operations, missing error checks upon opening, and it is prone to be not thread safe when changed.
|
|
|
|
|
"Using locking there does not prevent the exception. It may just delay it."
It prevents because if port is already opened I don't reopen it.
PS did you see the updated version of question? (with open and close calls). Thanks a lot!
PPS I understood your conclusion too, thanks.
|
|
|
|
|
Member 12061600 wrote: It prevents because if port is already opened I don't reopen it.
OK. You are right because you are checking the open state first. But it just returns without any error code or message.
But there may be other reasons that opening fails which should be handled (not existing, already opened by another application or another instance of your application).
So when handling the exception, there is no need for locking.
|
|
|
|
|
So we agree that although my approach maybe redundant at places
at least in its current form it should work (including the Open and Close methods being called like in updated question)
Thanks a lot for help
|
|
|
|
|
I have slightly updated my question with how open port and close port methods could be also called from other methods. Please give feedback on that if it is Okay.
|
|
|
|
|
See my above post. No need to lock when closing and (as noted in my initial answer) use SerialPort::IsOpen() instead of your own member variable.
|
|
|
|
|
I still prefer to have lock in OpenPort to avoid that exception you mention
|
|
|
|
|
Locking protects only against simultaneous calls. Because serial ports are exclusive, they are usually opened only once from within the main thread. When doing so, you would never have the situation of simultaneous calls from within your app. But you may have other open errors that should be handled.
|
|
|
|
|
To be honest, if I were doing this, I'd look at using a different architecture altogether because you're mixing a lot of responsibilities in your code here. What I would be tempted to go with is to have your API exposed so that you have separate writer and reader implementations, and the actual management of the port operation would be the responsibility of a completely separate class altogether. As I tend to compose objects together with IoC, I would constrain the port functionality to be container controlled so that all classes get the same instance and I would put the checking of the port along with its creation in there. I would also hide this as an internal implementation detail so you would have this as an internal interface with an internal implementation. That way, your client code only sees the separate reader and writer interfaces. Something like this internally:
internal interface IHaspPort
{
SerialPort Port { get; }
}
internal class HaspPort : IHaspPort
{
private readonly object SyncLock = new object();
private SerialPort port;
public SerialPort Port
{
get
{
if (port == null)
{
lock(SyncLock)
{
if (port == null)
{
}
}
}
return port;
}
}
}Then, you simply inject this instance into your reader and writer classes and let it take care of itself.
This space for rent
|
|
|
|
|
thanks but I am slightly beginner with .NET and I'd rather stick with my approach.
You can have a look I slightly updated question, if you have opinion - whether my scenario is OK now I'd appreciate. (I added open and close calls)
|
|
|
|
|
I thought about posting this question in the "Article Writing" forum, but decided it would get more "exposure" here, even if I risk this being seen as slightly off-topic for this forum.
The reason I ask is that I am thinking of doing an article for CodeProject that will demonstrate what explicit use of 'ApplicationContext can enable in terms of multiple window management, over-all application structure, and, more specifically, the all-too-common task of "communication" (passing data, raising/receiving event notifications, etc.) between Forms.
I've been using 'ApplicationContext (which is, as I think you know, being automatically instantiated/used "under-the-hood" of any default new Win Forms project) explicitly for years; couldn't live without it. But, I'm unclear as to how often this is used by other programmers.
And, is it, possibly, the case that at this late date in the life-time of Win Forms, such a topic has a small possible "audience" ?
thanks, Bill
«In art as in science there is no delight without the detail ... Let me repeat that unless these are thoroughly understood and remembered, all “general ideas” (so easily acquired, so profitably resold) must necessarily remain but worn passports allowing their bearers short cuts from one area of ignorance to another.» Vladimir Nabokov, commentary on translation of “Eugene Onegin.”
|
|
|
|
|
BillWoodruff wrote: And, is it, possibly, the case that at this late date in the life-time of Win Forms, such a topic has a small possible "audience" ?
Likely. Definitely smaller than the all the rage web technologies. But as I'm doing mainly WinForms as well I would be interested in reading it!
cheers, Sascha
If the brain were so simple we could understand it, we would be so simple we couldn't. — Lyall Watson
|
|
|
|
|
Thanks, Sascha, if a "bulb as bright" as you are still working in WinForms, that encourages me to write the article.
cheers, Bill
«In art as in science there is no delight without the detail ... Let me repeat that unless these are thoroughly understood and remembered, all “general ideas” (so easily acquired, so profitably resold) must necessarily remain but worn passports allowing their bearers short cuts from one area of ignorance to another.» Vladimir Nabokov, commentary on translation of “Eugene Onegin.”
|
|
|
|
General 
News 
Suggestion 
Question 
Bug 
Answer 
Joke 
Praise 
Rant 
Admin 
Submit an article or tip
