Async SerialPort Wrapper

Question

I've been working with the SerialPort class for a while, trying to figure out the best way to work with it, and especially adding support for Async-Await in C#. I also started before 4.5, so I didn't have access to the BaseStream member (so no BaseStream.ReadAsync()). I wanted to get some opinions on it before I started relying on it too heavily. Performance considerations and stability are high priority, but also figuring out if I'm reinventing the wheel.

The code is designed to wrap around a SerialPort to provide Async-Await operations. It does this by intercepting calls to Write and the SerialPort.DataRecieved events and relaying the data to a pair of Reactive Subjects that client code can subscribe to. Note this code requires the nuget package Rx-Main.

using System.IO.Ports;
using System.Reactive;
using System.Reactive.Linq;
using System.Reactive.Subjects;
namespace Core.IO {
    public sealed class ComPort : IDisposable, INotifyPropertyChanged{ 
        private Subject<byte[]> _txDataSubject;
        private Subject<byte[]> _rxDataSubject;
        private SerialPort _port;

        public String PortName {
            get {
                return _port.PortName;
            }
            set {
                _port.PortName = value;
                RaisePropertyChanged("PortName");
                RaisePropertyChanged("IsOpen");
            }
        }
        public Boolean IsOpen {
            get {
                return _port.IsOpen;
            }
        }
        public IObservable<byte[]> DataTx {
            get {
                return _txDataSubject.AsObservable();
            }
        }
        public IObservable<byte[]> DataRx {
            get {
                return _rxDataSubject.AsObservable();
            }
        }
        public ComPort() {
            _port = new SerialPort();
            _txDataSubject = new Subject<byte[]>();
            _rxDataSubject = new Subject<byte[]>();
            _port.DataReceived+=(sender,args)=>{
#if DEBUG
                Console.WriteLine("({0}) RX on thread {1}",PortName, System.Threading.Thread.CurrentThread.ManagedThreadId);
#endif
                _rxDataSubject.OnNext(_port.Encoding.GetBytes(_port.ReadExisting()));
            };
        }
        public void Open() {
            _port.Open();
            RaisePropertyChanged("IsOpen");
        }
        public void Close() {
            _port.Close();
            RaisePropertyChanged("IsOpen");
        }
        private void RaisePropertyChanged(string propertyName) {
            if (PropertyChanged != null) {
                PropertyChanged(this, new PropertyChangedEventArgs(propertyName));
            }
        }
        public event PropertyChangedEventHandler PropertyChanged;
        public Boolean IsDisposed {
            get;
            private set;
        }
        public void Write(string data) {
            _txDataSubject.OnNext(_port.Encoding.GetBytes(data));
            _port.Write(data);
        }
        private void Dispose(bool isDisposing) {
            if (IsDisposed) return;
            if (isDisposing) {
                _port.Dispose();
                _txDataSubject.Dispose();
            }
            IsDisposed = true;
        }
        public void Dispose() {
            Dispose(true);
            GC.SuppressFinalize(this);
        }
        ~ComPort() {
            Dispose(false);
        }
    }
}

Answer 1

_port.Encoding.GetBytes(_port.ReadExisting())

If you want to work with byte[], then your data should never appear as string, even temporarily, because some byte sequences are likely going to be invalid in your encoding.

I believe you should be able to use the byte[] overload of Read() for this. (Though switching to byte[] and async methods of the BaseStream is probably a better choice in the long run.)

---

Your class is schizophrenic about whether it wants to use string or byte[] (the observables use byte[], Write() uses string). You should figure out which choice makes more sense and stick to it.

---

What is the purpose of DataTx? I think that being able to read what you have written is not commonly needed.

---

I realize that Tx and Rx are abbreviations used in networking, but I find them confusing. Are you sure users of this library will immediately understand them? If not, you should rename the properties.

---

Boolean
String

It's common to write those types in their keyword form: bool, string.

---

~ComPort() {
    Dispose(false);
}

You don't have any unmanaged resources, so Dispose(false) basically doesn't do anything. But this means there is no reason for this class to have a finalizer.

The Dispose Pattern makes sense when you're writing a class that is going to be inherited. But since your class is sealed (nice, most people don't bother with that), I think there is also no reason to have the Dispose(bool) overload, just a single Dispose() method will be enough.