Comments (15)
Hi @fmartens - nice work!
I've implemented the first part of your message which is a bug, thanks for going through the history and understanding what happened - really appreciate that!.
I'm not 100% convinced on the second bit, the Statuses correspond to status codes from libplctag core - the Status.ErrorTimeout has a specific meaning in terms of what libplctag core thinks that means, which might be different / diverge in the future from the async-timeout meaning.
Lets keep the conversation going because I'm not 100% against it either.
from libplctag.net.
If you hand the core control and tell it how long you are willing to wait and that time is exceeded, then you'll get that error status. This may get a little more nuanced as I rejigger the internals into a more event-driven logic flow, but that is really all PLCTAG_ERR_TIMEOUT means.
int rc = plc_tag_read(tag_handle_id, 3000);
if(rc == PLCTAG_ERR_TIMEOUT) {
fprintf(stderr, "Barf! No response from the PLC after 3 seconds. Did you plug in the Ethernet cable?\n");
exit(PLCTAG_ERR_TIMEOUT);
}
If you use your own async approach on top of the core's async calls, you can redefine what timeout means all you want. It has been a while since I looked through the .Net code but if you are not using the core's timeout mechanism, then by all means go ahead and make this more sane and idiomatic for C# users!
Basically if you only pass zero as the timeout argument to those functions that take one, you are purely running async and I am not going to return that error to you. In those circumstances the application/wrapper is saying that it will determine when a timeout occurs and the core library will just keep trying until you tell it to stop.
from libplctag.net.
@kyle-github and @timyhac , thank you both for the swift responses.
@kyle-github , thank you for your explanation. I went through the libplctag code and it gave me the impression that it would be perfectly fine to throw a LibPlcTagException(Status.ErrorTimeout).
@timyhac: #116 misses await Task.Delay(ASYNC_STATUS_POLL_INTERVAL, cts.Token);
, it still uses await Task.Delay(ASYNC_STATUS_POLL_INTERVAL, token);
@timyhac, is there any plan to update the nuget soon?
from libplctag.net.
Thanks for the pickup!
The only issue is that we'll need to separate the exception by its source,
- Timeout (after sleeping on it, I'm less against throwing a LibPlcTagException than yesterday - we do have a separation layer between the core statuses, and the wrapper statuses, so if we need to they can diverge in the future.)
- User-supplied CancellationToken was cancelled (it wouldn't make sense to throw a LibPlcTag exception in this case).
That use of Task.Delay is actually just a hack because we haven't implemented proper event-driven logic with the Read/Write/Init - it just waits 2ms. The timeout for these operations is driven by cts.CancelAfter(Timeout)
.
What if we do something like this:
public async Task InitializeAsync(CancellationToken token = default)
{
ThrowIfAlreadyDisposed();
ThrowIfAlreadyInitialized();
using (var cts = CancellationTokenSource.CreateLinkedTokenSource(token))
{
cts.CancelAfter(Timeout);
var attributeString = GetAttributeString();
var result = plctag.plc_tag_create(attributeString, 0);
if (result < 0)
throw new LibPlcTagException((Status)result);
else
tagHandle = result;
using (cts.Token.Register(() => {
Abort();
token.ThrowIfCancellationRequested(); // User supplied token
throw new LibPlcTagException(Status.ErrorTimeout); // Else it must have been a timeout
}))
{
while (GetStatus() == Status.Pending)
{
await Task.Delay(ASYNC_STATUS_POLL_INTERVAL, cts.Token);
}
}
ThrowIfStatusNotOk();
IsInitialized = true;
}
}
from libplctag.net.
You could use the callback API to have the core library tell you when status changes. I am happy to extend/add to it if there is something missing in it! As I remake the core logic around event-driven flows, it is clear that there is a lot of room for small improvements that will add up.
from libplctag.net.
@timyhac , i've tested your suggestion and it results in an unhandled exception.Wouldn't it be better to do:
using (cts.Token.Register(() =>Abort()))
{
while (GetStatus() == Status.Pending)
{
try
{
await Task.Delay(ASYNC_STATUS_POLL_INTERVAL, cts.Token);
}
catch (TaskCanceledException)
{
if (token.IsCancellationRequested)
{
throw; // User supplied token
}
throw new LibPlcTagException(Status.ErrorTimeout); // Else it must have been a timeout
}
}
}
I've read https://bengribaudo.com/blog/2018/02/08/4360/understanding-cancellation-callbacks and an unhandled exception handler is not what you are looking for I quess.
from libplctag.net.
Yep okay, it reads better as well.
Where does the exception propagate from btw?
from libplctag.net.
Sorry about the delay.
Not quite sure if I understand your question but it's the LibPlcTagException which is thrown from the timer thread that executes:
Abort();
token.ThrowIfCancellationRequested();
throw new LibPlcTagException(Status.ErrorTimeout);
from libplctag.net.
Hi @fmartens - I've started work on this and pushed this branch
.
It involves a reasonable refactor of this class, and one thing I would like to do before merging this change is to implement unit tests, particularly around the garbage collection, async behaviour which are the primary things that the .NET wrapper extends above the core library (async with timeout does not exist in core library).
This work has started: #121
from libplctag.net.
Hi @fmartens - I'd appreciate if you could help develop some unit tests to check for correct behaviour. The work has started here:
https://github.com/libplctag/libplctag.NET/blob/%23115/src/libplctag.Tests/AsyncTests.cs
from libplctag.net.
@timyhac, I'd be more than happy to contribute to this but I need to find some time to prepare and get familiar with Moq.
from libplctag.net.
Even helping develop the scenarios that need to be tested would be a big help.
I'm thinking that, in order to close out #115 we just develop tests that check that specific scenario. As a separate item we can look at increasing test coverage.
I've got 3 tests so far, I'm not confident the case is covered properly but it is better than nothing. With the old code we don't get a timeout, but with the new code we do (at least with the unit tests, I haven't tried any manual testing yet).
I've found it quite difficult to learn moq, the official documentation is a bit lacking. I guess there would be books/courses to buy, but I've been relying on free stuff found on the web so far.
from libplctag.net.
@fmartens - hi there. The work has been merged into master but its not going to be released until the string API has been tested working, and a second pair of eyes looks at the async work. I'd love if you could take a look/test https://github.com/libplctag/libplctag.NET/blob/master/src/libplctag/NativeTagWrapper.cs#L274
from libplctag.net.
@fmartens - hi there, there is a beta package available on nuget with fixes: https://www.nuget.org/packages/libplctag/1.0.4-beta.1
from libplctag.net.
I'm quite busy at the moment and I am still learning Moq but the unit tests are still on my todo list though.
from libplctag.net.
Related Issues (20)
- ReadAsync() not handling exception when wrong IP Address is provided. HOT 4
- Issue timeout on first read HOT 6
- CompactLogix Plc Communicate C# this code not working on compactlogix HOT 1
- Writing sting value from C# to AB compactlogix string_50 tag. HOT 9
- Can I read descriptions from tags HOT 2
- What does error not allowed mean? HOT 2
- Try to decouple NotifyValueChangedTag from application
- Performance Issue - AutoRead HOT 9
- Should (some specific) tag parameters be able to be modified after initialization? HOT 3
- Timeout Micro850 (2080-LC50-24QBB) HOT 4
- BoolPlcMapper not returning properly HOT 1
- Connecting to a topic name HOT 2
- Running on .NET Alpine docker image HOT 3
- ReadAsync + Dispose HOT 4
- Async call exception behavior varies HOT 5
- Why can't I read out the PLC data type after a "Read" although the data type is supplied with the read? HOT 2
- Read Performance for Numerous Tags HOT 1
- Determining which tag read/write operation a warning in the log is associated with HOT 16
- Unloading of plctag.dll stuck in Shutdown HOT 5
- ErrorNoData exceptions thrown by LibPlcTag are not able to be handled by Try/Catch. HOT 9
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from libplctag.net.