jcurl / RJCP.DLL.SerialPortStream

SerialPortStream is an independent implementation of System.IO.Ports.SerialPort and SerialStream for better reliability and maintainability. Default branch is 2.x and now has support for Mono with help of a C library.
Microsoft Public License
631 stars 197 forks source link

Expose Win32 COMMTIMEOUTS values as platform-specific settings #107

Closed mdavis closed 2 years ago

mdavis commented 4 years ago

This adds a pair of methods (Get/SetPlatformSpecificSettings()) to allow the user to configure settings that are only relevant to specific platforms in a generic manner by exposing them through an IDictionary<string,object>.

This then fixes #74 by making the Win32 COMMTIMEOUTS values configurable via these methods.

The keys for the COMMTIMEOUTS settings are:

You can configure the COMMTIMEOUTS values with the following code:

var settings = new Dictionary<string, object>();
settings["CommTimeouts.ReadIntervalTimeout"] = 100;
settings["CommTimeouts.ReadTotalTimeoutConstant"] = 0;
settings["CommTimeouts.ReadTotalTimeoutMultiplier"] = 0;
settings["CommTimeouts.WriteTotalTimeoutConstant"] = 0;
settings["CommTimeouts.WriteTotalTimeoutMultiplier"] = 0;
stream.SetPlatformSpecificSettings(settings);
jcurl commented 4 years ago

Thanks for the patch! I do have concerns about cross platform support, as it now makes it very "windows" centric. I have to think about this. In particular, I see advantages to using strings for the API for stability, but there's no feedback for any way the user can query what settings are actually supported (say that this could be done on Linux, but there are different settings?)

If I were to integrate this, I think there needs to be:

In addition to what you've got:

stream.PlatformSpecificSettings["win32.CommTimeouts.ReadTotalTimeoutMultiplier"] = 0;
foreach (string setting in stream.PlatformSpecificSettings.SupportedKeys) {
  Console.WriteLine("Setting: {0} = {1}", setting, stream.PlatformSpecificSettings[setting]);
}
mdavis commented 4 years ago

I did include a GetPlatformSpecificSettings() method to retrieve the current values of those settings, though with my current implementation, I think you'd get an empty dictionary unless the port is already open. It probably wouldn't be hard to change the implementation so that it puts default values for the properties into the dictionary (possibly by moving the default values out of the CommTimeouts constructor and into WinNativeSerial).

I do have one concern with exposing the settings dictionary with a property. Suppose you were to update all of the COMMTIMEOUTS values one at a time that way while the port is already open. Which behavior would you want to see?

  1. The changes are applied immediately, so each property update results in calling SetCommTimeouts() on the port to update the value you just changed (for a total of 5 calls).
  2. The changes are buffered until you explicitly call some other method that will apply them all at once.

I'm open to other suggestions as to how to handle that better.

TheBeardedQuack commented 3 years ago

I would argue that programming with properties and code structures is much cleaner and easier to manage than a dictionary of string settings. For something like the WinAPI COMMTIMEOUT feature, I'd stick with having a single structure to contain all the info and you provide only one property to update, thus the call to SetCommTimeouts() is only required once.

However I'm not sure I can provide a decent way of adding OS-specific features to a cross-platform library and personally I would strive to have consistency across platforms rather than feature completeness.

jcurl commented 3 years ago

To let you know, I'm considering a different approach on how to do this, and expose the values via a property. This will result in multiple classes (I'll be using Generics)

We all know the current implementation SerialPortStream. Then add a new class:

public class SerialPortStream<T> : SerialPortStream {
  public T Settings { get; }

And T is a type that could be an interface for setting the properties. For this, I'm thinking of doing something (I have to look in the details, still got some other work to do first), that there's a "Native" library.

public class SerialPortStream<T> : SerialPortStream {
  public SerialPortStream(INativeSerialPort<T>) { ... }

  public T Settings { get { return nativeSerial.Settings; } }

This way, it works with implementations already using SerialPortStream. When writing code, the compiler can check and ensure the properties work. A user can derive or provide their own implementation for the native parts (e.g. integration tests that doesn't need specific hardware).

To get our very specific class, one would have to construct it themselves (which I don't like), but I could think about a factory type method.

Comments?

tcsaddul commented 3 years ago

That is an excellent idea.

jcurl commented 2 years ago

I've provided a different solution now on the master branch (v3.x) with 76253971d133791332e2f3b6df5d2fbee83dcd87. I still need to upload all the other support repositories to build the master. But this PR is now closed.