I‘ve been working with CAN since NETMF 4.1, and I‘ve ran into numerous issues with it. I think we need a new, better CAN implementation from GHI 
So, here are my suggestions:
- Initialization shouldn‘t start raising events immediately. That leads to all kinds of issues I‘m fighting ever since I‘ve started coding NETMF. For example:
a. If I dont‘t attach an event handler to CanDataReceived event immediately after calling constructor, G400 has problems with high traffic, as it stacks incoming messages somewhere internally until all memory goes to BINARY_BLOB_HEAD and G400 locks up before even the program starts normally. EMX, on the other hand, isn‘t raising events at all until it receives 127 messages — which is problematic in low traffic conditions.
b. Sometimes I want to turn CAN off for a while, but once it‘s on there‘s no way to stop it. Especially when I stop the program for debugging and peripherals continue working, I end up with thousands of RXOVER errors that paralyzes the program once I continue its execution.
c. There are some smaller issues that I will skip to save typing.
So I think constructor should only set the rates and stuff, and Enable()/Disable() functions should set the state of the CAN. Would be very handy, in my opinion.
- PostMessages() function is very misleading. No matter the count argument passed to it, it only sends one message. I don‘t know if it‘s a bug or a hardware limitation, but the same happens on EMX/G120/G400, so I think it should either be fixed or renamed to PostMessage(). Better to have both. I want to send only a single message quite often, but I have to construct an array of messages with one element in it to do so.
- Initialize() is misleading, too. It accepts bitrate argument, which is actually a register of the chip, but it‘s not written anywhere. It‘s not bitrate, it‘s something that is different for every platform. It would be good to have more Initialize() overloads, one might accept predefined enumeration of possible CAN bus speeds, other be left as it is for those who have specific needs (note camel casing in enums):
var can=new Can(CAN.Channel.Channel_1, G400CanSpeed.125k)
var can=new Can(CAN.Channel.Channel_1, EmxCanSpeed.125k)
var can=new Can(CAN.Channel.Channel_1, 0x7F1235AA)
- The whole implementation should follow official Framework Design Guidelines more strictly (like CANDataReceivedArgs should become CanDataReceivedArgs and so on).
Here’s some scaffolding that illustrates my points:
//==== Usage ====
//Creating a CAN object
var myCan = new Can(CanChannel.One, CanSpeed.EmxAt250k);
//Attaching event handler (CAN is not running yet!)
myCan.MessagesReceived += (sender, e) => {
Debug.Print("Something arrived!");
};
//Enabling can, it can now send and receive
myCan.Enable();
Debug.Print(myCan.IsEnabled.ToString()); //should print: true
for (int i = 0; i < 10; i++) {
//Fetching some messages from CAN. Important: it should never return null, if there's no messages, then an empty array should be returned
var receivedMessages = myCan.GetMessages();
Debug.Print("Number of received messages: "+ receivedMessages.Length.ToString());
Thread.Sleep(100);
}
//Posting a single message (it's quite a frequent operation for me. I don't always want to send a batch of messages
myCan.PostMessage(new CanMessage(12, 4, new byte[] { 1, 2, 3, 4 }));
//Posting a batch of messages
var messagesToPost = new CanMessage[5];
myCan.PostMessages(messagesToPost);
//Posting a selected range
myCan.PostMessages(messagesToPost, 1, 3);
//Disabling can, it won't receive or send
myCan.Disable();
Debug.Print(myCan.IsEnabled.ToString()); //should print: false
//==== Classes ====
public class CanMessage {
public Int32 Id { get; set; }
public Int32 DataLength { get; set; }
public byte[] Data { get; set; }
public CanMessage(Int32 id, Int32 DataLength, byte[] data) { }
}
public class Can {
public Can(CanChannel channel, CanSpeed speed) { }
public Can(CanChannel channel, UInt32 speedRegisterValue) { }
public void Enable() { }
public void Disable() { }
public void PostMessages(CanMessage[] messagesToPost) { }
public void PostMessages(CanMessage[] messagesToPost, Int32 offset, Int32 count) { }
public void PostMessage(CanMessage messageToPost) { }
public CanMessage[] GetMessages() { return new CanMessage[1]; }
public Boolean IsEnabled { get; set; }
public event MessagesReceivedEventHandler MessagesReceived;
}
public delegate void MessagesReceivedEventHandler(Can sender, MessagesReceivedEventArgs e);
public class MessagesReceivedEventArgs { }
public enum CanChannel {
One,
Two
}
public enum CanSpeed {
G400At100k,
G400At200k,
EmxAt250k,
G120At1000k
}
I think it follows the .NET spirit better than the current implementation (please don’t beat me).
