mcp25xx add read and write methods

[Andrew25455]

Test only on mcp2515
For mcp25xx add methods for setting, work with all buffers, simple read and write
Tested. It works correctly with board - https://www.waveshare.com/wiki/2-CH_CAN_HAT

Microsoft Reviewers: Open in CodeFlow

[dnfadmin]

All CLA requirements met.

[Andrew25455]

/azp run dotnet.iot

[azure-pipelines]

Commenter does not have sufficient privileges for PR 1959 in repo dotnet/iot

[pgrawehr]

@Andrew25455 You can ignore failures of the build step "Run Helix tests". These are hardware tests that are unfortunately a bit flaky. Unless you change something in the System.Device.Gpio assembly, it's very unlikely you break those.

[Andrew25455]

Thank you Patrick! I wanted green pipeline for publish nuget for tests, but ok, I will test with dll's.

[Andrew25455]

Tested. It works correctly with board - https://www.waveshare.com/wiki/2-CH_CAN_HAT

[pgrawehr]

Looks good overall, thank you so far!

Just a few comments and questions. How did you test this, by the way? What did you connect to the CAN Bus interface?

[Andrew25455]

Thank you for your review! I fixed comments. I am refactored Bitrates enum to method with data in Dictionary.

I was tested this library with two Rpi with two CAN hat (photo) . One of this used your library and another - can-utils. In another test i connected Rpi with hat to our custom board with with C++ firmware, and it was worked too.

[pgrawehr]

One question still: How does one know how many bytes to read? Is the transmission just like a stream or is it packet-based?

[Andrew25455]

CAN bus is packet-based protocol. In mcp2515 received message will be in buffer. But buffer will contain cropped message only with id, data length code and data. Max size of this cropped message is id = 32bit, data length = 4bit and data can be 8*8bit, total - 12,5byte. I added TransmitBufferMaxSize constant 14byte, because it is max chip SRAM per buffer.

[pgrawehr]

CAN bus is packet-based protocol. In mcp2515 received message will be in buffer. But buffer will contain cropped message only with id, data length code and data. Max size of this cropped message is id = 32bit, data length = 4bit and data can be 8*8bit, total - 12,5byte. I added TransmitBufferMaxSize constant 14byte, because it is max chip SRAM per buffer.

Ok, that makes sense. What reason would one then have to call ReadMessages() with an argument of less than TransmitBufferMaxSize? The reason I'm asking is just because we should, if possible, avoid to offer features that could cause unnecessary confusion for the users.

[pgrawehr]

Sorry for the delay, i thought a lot about your message. As result, i change methods contract from bytes to CanMessage object. In this object i also add common can Id and extended Id, witch i have forgotten

No hurry, this is going to get very good. I like the new approach, but you should put an eye on possible error cases. Now an invalid message may cause ReadMessage() to throw an ArgumentException, which is confusing, as this is clearly not an user error. Maybe you add some unit tests for these error cases?

[Andrew25455]

I think it was bad idea - return CanMessage object from read method, because if converting of first message throw exception then second message will be lost. I see only one solution - return raw byte array to user.
Patrick, do you have a solution, maybe?

[pgrawehr]

I think it was bad idea - return CanMessage object from read method, because if converting of first message throw exception then second message will be lost. I see only one solution - return raw byte array to user. Patrick, do you have a solution, maybe?

Honestly, I don't like it very much that the method currently returns a List of elements which were possibly received from different interfaces. Most of the time, the two interfaces will probably connect to very different networks, and the user wants to know on which interface a message was received. So if you split that (add a "interfaceNo" parameter to the ReadMessage function), an error would throw exactly where it should be.

[Ellerbach]

I would go for something in line with Patrick's suggestion. In that case the exception should be transformed into an error message. So you would return a list of a class that may look like this (just as an idea):

List<CanMessageList> ReadMessages()

class CanMessageList 
{
    public CanMessage CanMessage { get; set; } // Can be null when there is an error
    public ReceivedMessageType ReceivedMessageType {get; set; }
    public CanError CanError { get; set; }
}

enum CanError
{
    NoError,
    SomeError,
    Etc;
}

[Andrew25455]

Thank you for your ideas,
Patrick, in your solution our users need to know about buffers and their quantity. I think we need to provide more high-level interface for our users.
Laurent, honestly, I don't like this pattern as much as the pattern to throw an exception :)

When I tried to add your solutions in code, I found more elegant solution, as I think. I separated CanMessage to two models. One of this is model for sending message and another - for received message. First model can throw exception when model is initialising , another - can throw exception when user call methods GetId or GetData.
What about this solution?

[Andrew25455]

@pgrawehr Could you review this?

[Ellerbach]

Looks good to me now. Thanks!

[pgrawehr]

@Andrew25455 Is it intentional that now still ReadMessages() throws an exception when illegal data is received? If so, it must at least be documented.

Additionally, I would like to see a ReadMessage(bufferNo) method that returns only the data of one buffer. Your method doesn't allow finding out on which port the message was received. You could also add the receiving interface as a property to ReceivedCanMessage.

[Andrew25455]

Patrick, I can not find exceptions, with will throw when illegal data is received. I extracted all checks and exceptions to GetId() and GetData() methods, for user call after receive message.

I think that ReadMessage(bufferNo) method will be repeat ReadRxBuffer(RxBufferAddressPointer addressPointer, int byteCount = 1) method and my goal was hide technical details like buffer. Because of this i don't want to add type of buffer property to ReceivedCanMessage.

[pgrawehr]

Patrick, I can not find exceptions, with will throw when illegal data is received. I extracted all checks and exceptions to GetId() and GetData() methods, for user call after receive message.

I need to take a deeper look into this.

I think that ReadMessage(bufferNo) method will be repeat ReadRxBuffer(RxBufferAddressPointer addressPointer, int byteCount = 1) method and my goal was hide technical details like buffer. Because of this i don't want to add type of buffer property to ReceivedCanMessage.

I see your point, but I think the receiver channel is not a technical detail (since the channels could be connected to two entirely different networks). I will take a look at it.

[pgrawehr]

@Andrew25455 I had a first look into this. I did update the test a bit, as you where never really testing reads. Your simulation device tested the readBuffer as output, but it should be an input (for the class under test).

I can confirm I didn't get it to crash on wrong returns.

For the other problem, I'm still checking options. Looking at the main class, we have the following methods:

public ReceivedCanMessage[] ReadMessages();
public List<byte[]> ReadAllBuffers(int byteCount);
public byte[] ReadRxBuffer(RxBufferAddressPointer addressPointer, int byteCount = 1);
public byte Read(Address address);

The first three all do basically the same. For the user, this is confusing, there should normally only be one way to do things, unless there's a reason. And the last one should probably be private anyway.

I'll try to provide a suggestion later.

[Andrew25455]

Patrick, thanks for this test, i missed it.
I add ReceiveBuffer property to ReceivedCanMessage, because you are right it is important. Masks and filters can be added to this buffers and this property will help to separate messages in this situation.
I also merged ReadAllBuffers and ReadMessages methods, for this realisation

Last method (Read(Address address)) can not be private, because it is reading register, not messages from buffer.

[pgrawehr]

/azp run dotnet.iot

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[pgrawehr]

@Andrew25455 Thanks, I think this is looking good now.

[Andrew25455]

Patrick, Laurent, thank you, for your help, guys!

I need to do something with this PR?

[pgrawehr]

@Andrew25455 no, not at the moment. I'll bring it up in tomorrow's team call, and if nobody has any reservations or wants to take another look, it will be merged.