6
\$\begingroup\$

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);
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ Right now, your post only contains code. Please add more context explaining what your code does and what you want reviewed. \$\endgroup\$ – SirPython Mar 17 '15 at 21:50
  • \$\begingroup\$ Is that enough? \$\endgroup\$ – Benjamin Mar 17 '15 at 22:09
3
\$\begingroup\$
_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.

\$\endgroup\$
  • \$\begingroup\$ The SerialPort.ReadExisting method returns a string. I'd like to use the normal SerialPort.Read(byte[],....)' method, but I've read that the BytesToRead` property is unreliable, meaning I either have to allocate a large buffer to get everything, wasting memory and probably time, or I risk loosing data, which is unacceptable. I should be able to guarantee that the encoding is at LEAST UTF8, but you're right, that's a major issue. \$\endgroup\$ – Benjamin Apr 13 '15 at 20:03
  • \$\begingroup\$ The class isn't so much schizophrenic as incomplete. I wanted it to work with byte[]'s internally but provide shortcuts for string, since this is the most common method of in and out. \$\endgroup\$ – Benjamin Apr 13 '15 at 20:09
  • \$\begingroup\$ DataTx is there because I need the ability to log both input and output from the port. Making the port itself the point of access for this information seemed logical. \$\endgroup\$ – Benjamin Apr 13 '15 at 20:09
  • \$\begingroup\$ I'm currently the only one using the library, but I think the standard abbriviations for Transmit and Recieve are well known enough to deal with for most people. If I get more feedback saying their confusing I might change it. \$\endgroup\$ – Benjamin Apr 13 '15 at 20:09
  • \$\begingroup\$ The full dispose pattern was implemented basically to get VS to stop yelling at me about it. I'll change that. \$\endgroup\$ – Benjamin Apr 13 '15 at 20:09

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.