Clumsy Code

[Elmue]

1.)
There is no reason to use a GCHandle in your code.
If you read the documentation that this is for the opposite purpose: When UNmanaged code accesses Managed resources.
But you not do this in your code.

2.)
There is absolutely no need to ever use the keyword "unsafe" in your code and require to compile you code as unsafe.

3.)
It does not make sense to use hundreds of "async" and "Task" if you use "await" in the same function.
You did not understand asynchronous programming.
If you use "await" in the same function you block the calling thread. This is not asynchronous programming anymore.
The idea of asynchronous programming is that one thread waits for another and not one function blocks itself.

4.)
Why pass a CancellationToken to each and every function if you have a timeout of 1500 ms ?
Who will ever cancel such a short operation ?

[klasyc]

1. What solution would you recommend me, when I need to convert a byte array to struct or vice versa? Is this better in your opinion?

2. I also don't like the unsafe block, but it is necessary because of using of the NativeOverlapped structure and WriteFile() function in the asynchronous mode. When I delete it, I get CS0214 error. How can I get rid of it?

3. Please recommend me a better approach - e.g. use separate Task for the whole read/write operation and make the internal implementation blocking?

4. Yes, CancellationToken is overkill for such a short timeout.

[Elmue]

1. Your code is correct but you can store the memory pointer directly in an IntPtr. GcHandle is not for this purpose. GcHandle is only required if you pass the IntPtr then to native code which you do not.

2. Unsafe is not necessary. I already rewrote your code and it works perfectly without that. Why do you need unsafe for Overlapped but not for SP_DEVICE_INTERFACE_DATA. What is the difference? Both are simple structures!

3. You already are using non-blocking Overloapped I/O operations. Therefore your code will never block eternally. So you need neither Tasks nor await. All you need is a simple timeout. 1 second is enough because USB is very fast. You call a function of your class and pass a timeout. Either the function returns faster when the operation has finished or it waits a maximum of 1 second and throws a timeout exception. That is the standard programming. I already rewrote your code and it works perfectly without Task and async and await and CancellationToken.

I tell you that I completely rewrote your code. It is much shorter and clearer now. (approx half the length of yours). I also fixed some other issues.
It is one class in a bigger project that communicates directly with the oscilloscope over USB.
I will publish it on GitHub and my homepage.
This project will also show the channel voltages comfortably with zoom in the GUI, can store them in PNG images and can load CSV files, it has a logic analyzer, can directly import from the memory of my Rigol DS1074Z, and more.
I will inform you when it is ready.

[klasyc]

Looking forward to your project. If you already fixed my code, you could also create a pull request.

[Elmue]

I did not fix your code.
I completely rewrote it.

[ShortArrow]

@Elmue I am Noob of SCPI now.
I like the concept of ScpiNet, but I'm confused by the conversation here. Is there any new information of your new repo?

[Elmue]

Hello Short Arrow
I'am working on a new project that will allow to copy waveforms from an oscilloscope over USB without any additional installation and then allows to view and convert and decode the signals.
This is a HUGE work in progress and when it is ready I will publish it on my homepage with a detailed manual and also on Github.

[Elmue]

Hello klasyc: When I publish my project which downloads data via USB from the oscilloscope using your completely rewritten code, would you set a link to my project in your project description? It does much more than copying wave forms from an oscilloscope. It displays analog and digital channels and has decoders for several data formats like for example UART, SPI, I2C, CAN Bus, Math operations, noise filter and much more.

[klasyc]

@Elmue: If you create a pull request to fix the issues you addressed, I will reference your project in my library. Currently, you are only criticizing my project and advertising your own, which I don't like.

[ShortArrow]

@Elmue In many cases, to ensure transparency, third parties or end users can read the historical background of the project in the form of Inspired by ~ or Thank you for pioneering to ~ or Alternative to ~. Of course, this rule is not binding. I think this is the OSS spirit.

[Elmue]

@klasyc:
It is your choice. So I will also not mention your project in the description of my software. That is OK for me.

I already wrote you that I can NOT make a pull request.
I already wrote you that I COMPLETELY rewrote your code from the scratch.
My issues are still open after 2 months.
So I see that you are not motivated (or not able) to create a better version of your code.
You want others to fix your issues and do this work for you.
So if I would upload my completely rewritten class as a pull request you would publish my code under your name.
That is not my idea.

@ShortArrow:

background of the project in the form of Inspired by ~ or Thank you for pioneering to ~

Exactly!
I left a small comment in the source code, But I will not mention his project in my detailed project description because klasyc is angry that I report the several issues with his code. He takes a report of an issue (that he himself created) as criticizing him! So what is the functionality of Issues on Github for, if not criticizing bugs and badly written code??

[klasyc]

OK Elume, thank you for your feedback. I was hoping that you will show us your code.

Finally, I got some time to fix the addressed issues myself:

• The GCHandle is now replaced by simple IntPtr.
• I reworked the async/await hell to a single task, which is definitely nicer solution.
• I couldn't find any way, how to get rid of the unsafe code and still use the overlapped API. Of course, I could switch to the synchronous approach when I am still using separate Task for each request, but in the past I used to have issues with the synchronous approach (I could not cancel frozen requests when the instrument froze, which can sometimes happen).
• I am leaving CancellationToken as is. It is an optional parameter, and I am not forcing anyone to use it. Also, the timeout can be (and sometimes it has to be) extended, e.g. when you are downloading a large dataset from the instrument. Then, possibility of cancelling makes sense.

[Elmue]

I was hoping that you will show us your code.

I will publich it as soon at it is ready.
I'am still working on this huge project.

I couldn't find any way, how to get rid of the unsafe code and still use the overlapped API.

I already wrote you how to do this!

I wrote on 21 december:

Why do you need unsafe for Overlapped but not for SP_DEVICE_INTERFACE_DATA.
What is the difference?
Both are simple structures!

So look at the DllImport of SetupDiEnumDeviceInterfaces()
How do you pass the structure SP_DEVICE_INTERFACE_DATA ???

Why are you not able to change the DllImport of WriteFile() to pass the structure NativeOverlapped in exactly the same way and get rid of the useless unsafe ???

It is sooooooooo easy !

[klasyc]

Yes, I know that I can pass the NativeOverlapped structure as reference, but I am struggling with the overlapped.pack() function, which returns a pointer to the NativeOverlapped*. Also, the IOCompletionCallback uses NativeOverlapped* pointer. I was searching the documentation and trying to find any alternative which would accept a reference type, but I found nothing. I don't think it is so easy...

[Elmue]

The reason is that you are mixing native Windows API with .NET Task stuff.
You dont need Overlapped. Just NativeOverlapped is enough.
You dont need Pack() nor Unpack() nor Free() nor IOCompletionCallback
All this clumsy stuff is unnecessary when you switch to use plain Windows API.

Have you ever programmed Overlapped Read/Write in C++ before?
There is nothing of this .NET Pack() stuff.
Create Member variable NativeOverlapped.
Assign to EventHandle a new event created with CreateEvent().
Pass this member variable NativeOverlapped by ref to ReadFile, WriteFile.
After ReadFile, WriteFile, use WaitForSingleObject and GetOverlappedResult
Thats all you need.
Its really very straight forward and very simple code. Same as programming in C++.

[klasyc]

Thank you for your suggestion, I cannot understand, why I didn't get that idea myself... My previous C++ version of the code looked exactly like that... So the library is finally without unsafe secitons.

[Elmue]

Excellent!