Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clumsy Code #8

Open
Elmue opened this issue Dec 19, 2024 · 4 comments
Open

Clumsy Code #8

Elmue opened this issue Dec 19, 2024 · 4 comments

Comments

@Elmue
Copy link

Elmue commented Dec 19, 2024

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
Copy link
Owner

klasyc commented Dec 19, 2024

  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
Copy link
Author

Elmue commented Dec 21, 2024

  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
Copy link
Owner

klasyc commented Dec 22, 2024

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

@Elmue
Copy link
Author

Elmue commented Dec 23, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants