Initial progress dump for Mcp25xxx binding

[shaggygi]

This adds a device binding for the Microchip Mcp25xxx CAN controller.

Notes:

• This is only an initial dump of project files and current API structure.
• The intent is to provide a basic binding for CAN to allow team to work with CAN hardware in order to learn and implement CAN interfaces as we did with Mcp23xxx and IGpioController. That effort will not be in this PR.
• This PR only includes the basics of working with commands and registers. There are areas that can be improved to configure registers for communications. For example, you must assign correct bits for Extended Identifier (EID) in multiple registers (TXBxSIDL, TXBxEID8). Improved helper methods will be added in a future PR as more progress is made on CAN interfaces mentioned above.
• The API should work with Mcp2510 and Mcp25625 devices.
• This offers a way for Linux and Windows 10 IoT to interface with CAN devices via SPI.
• Little testing has been performed against hardware as I'm still reviewing tooling, datasheet, hardware, etc. I'll focus on this next.
• A future PR will be added for samples and Fritzing. Basic links and references have been added for now.

cc: @krwq @joperezr

[krwq]

Nice @shaggygi! Will take a look whenever I got time 😄

[shaggygi]

@krwq i reached a point for majority of initial register tests and changed GetAddress() method to Address property. If time allows, I'll try to focus on the instructions and more interaction with hardware over the weekend.

[krwq]

Sounds good @shaggygi - Once you get to the point where you send can frames don't build more on top of that except simple tests since that will have overlap with the stuff which I'm doing right now so we can merge from there - I was able to send couple of frames over the wire and learning details about what can be done and how we should expose it.

[shaggygi]

@krwq I now have the commands added. I was able to perform a few interactions with the device and appear you can read/write to registers. Sadly, I have not been successful on transmitting/receiving over the wire. This is mostly like because I'm configuring the device wrong. I'll keep tinkering when I get a chance.

Please go ahead and skim over the current code. I would like to go ahead an check in the progress as team should be able to play around with these bits enough to discuss how/if a generic CAN abstraction could be used with this and other implementations.

I want to take a breather from this binding for a few days to study some other things.

Let me know if you find mistakes (I'm sure there are a few) 😄 Thx

[shaggygi]

@joperezr no prob. I still believe it covers most of the feedback for now. As mentioned, it is expected to have future enhancement PRs by community as we figure out a good CAN structure for other implentations. I have some ideas to help troubleshoot this with TIOT project once I recover.

[krwq]

use $(SystemDeviceGpioPackageVersion) instead of the specific version (recently changed everywhere)

[shaggygi]

Will update.

[shaggygi]

I'm going to hold off on this until the end. There are some other changes in the repo that have since been changed and break my local build when doing this.

[krwq]

optional: I think would smplify a bit if you converted this to array and just simply use indexer and IndexOf (I think Span has it somewhere) for the other method

[krwq]

same comment for similar classes (actually wondering if we should unify them into one class)

[shaggygi]

This would be better approach. I started to update and get an error I can't seem to understand though.

This example works (not related to binding):

ReadOnlySpan<byte> value = new byte[] { 0x00, 0x01, 0x02 };
value.IndexOf<byte>(0x00);

Below I get the following and not sure why it is trying to use the overloaded method call:

Error CS1503 Argument 2: cannot convert from 'Iot.Device.Mcp25xxx.Register.Address' to 'System.ReadOnlySpan<Iot.Device.Mcp25xxx.Register.Address>'

ReadOnlySpan<Address> addresses = new Address[] {Address.TxB0Sidl, Address.TxB1Sidl, Address.TxB2Sidl };
addresses.IndexOf<Address>(Address.TxB0Sidl);  // Get red squiggles here.

Have any thoughts on this?

[krwq]

I think you can workaround by creating stackalloc Address[1] { whateverYouSearchFor } since this is what it seems to expect. I would presume searching for single element should work though. cc: @ahsonkhan @GrabYourPitchforks

[shaggygi]

I tried this and works, but is not ideal.

Address address = Address.RxB1Dlc;

ReadOnlySpan<byte> addresses = stackalloc byte[2]
{
    (byte)Address.RxB0Dlc,
    (byte)Address.RxB1Dlc
};

int index = addresses.IndexOf((byte)address);

if( index < 0)
{
    throw new ArgumentException($"Invalid address value {address}.", nameof(address));
}

return (byte)index;

[shaggygi]

@joperezr do you have any thoughts on how to fix this? I still see error in newest VS2019 preview, as well.

[joperezr]

The reason why this happens is because the extension method IndexOf requires that the T type should be IEquatable, and an Enum will derive from System.Enum which is not IEquatable. You could theoretically cast it as a byte if you wanted which should work, or alternatively, instead of having Address be an Enum, you could have it be a struct that does extends byte and has some default static values. For that suggestion, you can take a look at what we did with PinValue here: https://github.com/dotnet/iot/blob/master/src/System.Device.Gpio/System/Device/Gpio/PinValue.cs

[ahsonkhan]

As an aside, I am not sure if it makes sense to use IndexOf to search through a compile-time known enum set like this and the switch/case makes more sense to me. Why incur the cost of searching through the enum array every time? What's the benefit?

[shaggygi]

@ahsonkhan Thanks for feedback. This change was based on other feedback for the PR. @krwq If you're good with current code, I believe there are no more outstanding issues with PR.

[krwq]

Overall looks good to me and I'm fine with merging as is.

Long term all public abbreviated names (mostly thinking about registers and their properties) should be expanded and clear - ideally we should not expose registers at all though

[shaggygi]

@krwq thanks for another round of feedback. I plan to address the issues, but as mentioned to @joperezr, I'm out with an injury 🤕 and most likely for another week or so. Leave open until I get a chance to complete. Thx

[krwq]

No worries @shaggygi, get better!

[shaggygi]

ideally we should not expose registers at all though

I think this makes sense for some very simple bindings, but I look at it 2 ways for binding such as this Mcp25xxx.

1. Some devs will want to interact with the binding directly and get down to the details of setting/getting registers/parameters. Gives them more control on what they want with the capabilities of the component's internals.

2. Some devs (and probably in most cases) will want to interact with the binding using an interface. Once we determine this (e.g. ICanController or similar), they will use this and perform the general Reads/Writes of CAN messages. The registers/parameters would be set/get at a level they care to see.

[krwq]

@shaggygi fine with me - if possible you can try to make them protected so that by default you won't see them and if someone needs them they can derive - fine to leave as is for now (please expand names though)

[krwq]

LGTM so far, let me know once you address all feedback

[shaggygi]

Updated the register properties extended names. Still have to review the address array feedback and package version.

[joperezr]

LGTM! thanks for the hard work on this @shaggygi ! 🎆