Testability

This is my main concern when creating an API that talks to external references, in this case The CAN library. The problem with this library is that we are forced to consume a static class PCANbasic. There is no interface provided for us to work with. Testing our own code depends on a connection to the CAN bus.

// static class is a tight dependency, hard to mock out
PCANBasic.Write(_handle, ref msg);

Therefore, it is up to us to create an interface, an implementation using the static library and use a variable of this interface _canBusService in CanbusConnection. This way, we can

• mock the library for unit tests
• use a different version of the library (considering the signatures remain)

interface ICanService

public interface ICanService : IDisposable
{ 
    TPCANHandle Handle { get; }
    bool Initialize();
    TPCANStatus Read(out TPCANMsg message);
    void Write(ref TPCANMsg message);
}

implementation CanService

public class CanService : ICanService
{
    public TPCANHandle Handle { get; private set; }

    public bool Initialize()
    {
        // Handle = [redacted]
        return true;
    }

    public TPCANStatus Read(out TPCANMsg message)
    {
        return PCANBasic.Read(Handle, out message);
    }

    public void Read(ref TPCANMsg message)
    {
        PCANBasic.Write(Handle, ref message);
    }

    public void Dispose()
    {
        PCANBasic.Uninitialize(Handle);
        Handle = 0;
    }
}

Data Integrity

You have stated in the comments that the library is half-duplex, non-concurrent. This simplifies design, but still requires measures to enforce thread-safety.

Consider using a lock for Open and WriteCanMsg to avoid race conditions.

public object SyncRoot { get; } // .. create a new object() in the constructor

method Open

Since no lock is used, another thread can call the method after we think it is not opened.

if (IsOpen) { return true; }
// .. other code that assumes IsOpen == false

We no longer store the handle and use the newly created interface.

IsOpen = TryInitializePCANChannel(out _handle);

IsOpen = _canBusService.Initialize();

What happens if the call was not succesfull?

if(IsOpen)
{
    _canbusListenerThread = new Thread(new ThreadStart(CanbusListener));
    _canbusListenerThread.Start();
} // else ??

Invert the condition and trow on failure.

if (!IsOpen)
{
    throw new IOException("Failure initializing CAN bus");
}

Use throw to keep the original stacktrace.

catch(Exception ex)
{
    Log.Error("Error opening PCAN connection: {Message}, Error: {Error}", ex.Message, ex);
    throw ex;
}

catch(Exception ex)
{
    Log.Error("Error opening PCAN connection: {Message}, Error: {Error}", ex.Message, ex);
    throw;   // preserves stacktrace
}

refactor Open

public bool Open()
{
    lock (SyncRoot)
    {
        if (IsOpen) { return true; }

        try
        {
            IsOpen = _canBusService.Initialize();
            if (!IsOpen)
            {
                throw new IOException("Failure initializing CAN bus");
            }
            _canbusListenerThread = new Thread(new ThreadStart(CanbusListener));
            _canbusListenerThread.Start();
        }
        catch(Exception ex)
        {
            Log.Error("Error opening PCAN connection: {Message}, Error: {Error}", ex.Message, ex);
            throw;
        }
        return success;
    }
}

method WriteCanMsg

Guard against bad user input.

msg = msg ?? throw new ArgumentNullException(nameof(msg));

Same tips as with Open: use lock, interface, throw

refactor WriteCanMsg

public void WriteCanMsg(TPCANMsg msg)
{
    msg = msg ?? throw new ArgumentNullException(nameof(msg));
    lock (SyncRoot)
    {
        try
        {
            _canBusService.Write(ref msg);
        }
        catch (Exception ex)
        {
            Log.Error("Error writing CAN message: {Error}, Bytes: {Bytes}", ex.Message, String.Join(" ", msg.DATA));
            throw;
        }
    }
}

The commands should reuse the lock. For instance, CommandBase

 //lock (connection)
 lock (connection.SyncRoot)
 {
     connection.CanbusPacketReceived += Connection_CanbusPacketReceived;
     connection.WriteCanMsg(msg);
 }

refactor CanbusListener to use _canBusService

// ..
while(_canBusService.Read(out msg) == TPCANStatus.PCAN_ERROR_OK)
{
    // ..
}
// ..
Thread.Sleep(100); // <- I am surprised this is required. 
                   //    I would expect `_canBusService.Read` to be blocking.

Resource Management

Let Close do the actual work.

public void Close()
{  
    lock (SyncRoot) 
    {
        if (IsOpen)
        {
           _canBusService.Dispose();
           if(_canbusListenerThread.IsAlive && !_canbusListenerThread.Join(200))
           {
               _canbusListenerThread.Abort();
           }
        }
    }
}

Have Dispose calling Close. You could also refactor Dispose to use the dispose pattern. Clear event listeners and provide a destructor.

public void Dispose()
{
    Dispose(true);
    GC.SuppressFinalize(this);
}

~CanbusConnection()
{
    Dispose(false);
}

protected virtual void Dispose(bool disposing)
{
    if (disposing)
    {
        CanbusPacketReceived = null;
    }
    Close();
}

Separation of Concerns

• I would not expect CommandBase to have an operation protected async Task<bool> ExecuteCommandBase(CanbusConnection connection, TPCANMsg msg). This should be a method on CanbusConnection as public async Task<bool> ExecuteCommand(CommandBase command).
• Each specific command should provide a method BuildMessage() which would create the TPCANMsg from own state.

code CanbusConnection.ExecuteCommand

public class CanbusConnection
{
    // ..

    public async Task<bool> ExecuteCommand(CommandBase command)
    {
        var message = command.BuildMessage();
        // continue implementation with 'message' ..
    }
}

code CmdMpuSerialNumber.BuildMessage

public class CmdMpuSerialNumber : CommandBase
{
    // ..
    
    public TPCANMsg BuildMessage()
    {
        var msg = new TPCANMsg();
        msg.DATA = new byte[] { 0xAA, 0xAA, 0x08, 0x08, 0, 0, 0, 0 };
        msg.LEN = 8;
        msg.MSGTYPE = TPCANMessageType.PCAN_MESSAGE_EXTENDED;
        msg.ID = (uint)CanbusAddress.PC;
        return msg;
    }
}