[Mcp1525] Added a CanController (read messages only) #1292
Conversation
nfbot
changed the title
WalkthroughThis pull request updates dependency versions in the packages.lock.json files for both the main Mcp25xxx project and its samples. The updates bump nanoFramework.CoreLibrary from 1.15.5 to 1.16.11, nanoFramework.Runtime.Events from 1.11.18 to 1.11.29, nanoFramework.System.Device.Gpio from 1.1.41 to 1.1.53, and nanoFramework.System.Device.Spi from 1.3.52 to 1.3.73, along with corresponding content hash changes. No new APIs or control flow modifications are introduced. Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (13)
📒 Files selected for processing (2)
🔇 Additional comments (9)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
josesimoes
changed the title
josesimoes
changed the title
josesimoes
changed the title
|
|
||
| namespace Iot.Device.Mcp25xxx.Can | ||
| { | ||
| public interface ICanController |
There was a problem hiding this comment.
please document with intellisense all the public functions.
It's also recommended to do it for interfaced and use the inehitdoc as it makes things easier.
| /// </summary> | ||
| public class CanController : ICanController | ||
| { | ||
| byte MCP_SIDH = 0; |
There was a problem hiding this comment.
we are explicit and we use private.
Also, we do use Pascal Case for the variables.
Those are also constants, so please use const as well.
| /// <param name="message">CAN mesage to write in CAN Bus.</param> | ||
| public void WriteMessage(CanMessage message) | ||
| { | ||
|
|
There was a problem hiding this comment.
My intention was to first build the Read functionality in this PR, and then add more functionality in subsequent PRs. Like Write, RemoteRequest, Events. Is this the way to do things?
There was a problem hiding this comment.
Then throw a not implemented. And add a comment in the code that this feature is not implemented yet and should be implemented in the future.
Like this, at least, it's clear.
And yes, it's indeed absolutely possible to do it step by step!
| var status = _mcp25xxx.ReadStatus(); | ||
|
|
||
| if (status.HasFlag(ReadStatusResponse.Rx0If)) | ||
| { // message in buffer 0 |
There was a problem hiding this comment.
weput comments on their own lines, please adjust for all the following ones.
(sorry the linter is not yet on this binding)
| // receives all valid messages using either Standard or Extended Identifiers that | ||
| // meet filter criteria. RXF0 is applied for RXB0, RXF1 is applied for RXB1 | ||
| _mcp25xxx.BitModify(Address.RxB0Ctrl, | ||
| 0x60 | 0x04 | 0x07, // moet die laatste niet 0x03 zijn? Geprobeerd maar doet niets.. |
There was a problem hiding this comment.
we are also using English only in the comments. So please translate and also place them on their own line.
| return true; | ||
| } | ||
|
|
||
| void PrepareId(SpanByte buffer, bool ext, uint id) |
|
|
||
| void PrepareId(SpanByte buffer, bool ext, uint id) | ||
| { | ||
| ushort canid = (ushort)(id & 0x0FFFF); |
There was a problem hiding this comment.
couple of comments for all those lies and magic numbers would be perfect.
There was a problem hiding this comment.
The reason there are no comments is because I just copied it, without wanting (and needing) to know the details. But I will find out and put in comments. No worries.
| Address reg; | ||
| switch (mask) | ||
| { | ||
| case MASK.MASK0: reg = Address.RxM0Sidh; break; |
There was a problem hiding this comment.
1 line per instructions, so 3 lines for each case please
|
|
||
| SetOperationMode(mode); | ||
| return true; | ||
| } |
| /// </summary> | ||
| public uint Id | ||
| { | ||
| get { return _id; } |
There was a problem hiding this comment.
why are you using private variables and not just autoproperties?
There was a problem hiding this comment.
Good point! I just copied nanoFramework.Device.Can.CanMessage and I expect that's from before 2007, when there were no autoproperties. I will change it. Much cleaner.
|
@rikkreeftenberg you also have conflicts which appeared. |
|
@Ellerbach Rik also ask a question that is for the core team to answer: how should this be done in relation to the nanoFramework.Device.Can library? I've looked at the code in this PR, Mcp25xx library and the Mcp25xx data sheets. A few observations:
For a nanoFramework user it would be nice if there is a single set of interfaces/classes that can be used regardless of the device used to connect to the CAN bus. Some thoughts:
It would be even better if the shared interfaces/classes are in a separate 100% .NET library. It would than be possible to develop the application-specific CAN communication as device independent and debug/test it on a Virtual nanoDevice. It is than also possible to use the MCP2515 for a CAN-bus in addition to one controlled by the microcontroller. Then you would have a structure like:
I still intend to create an ESP32-version of the native implementation (hopefully quite soon). It doesn't look too complicated in IDF, but I don't yet fully understand how to add it to the CLR. But I think I'm quite unhappy with the way settings are passed as that seems to be quite device dependent. ESP32 has additional settings (e.g., message buffer sizes) and more user-friendly configuration options, e.g., specify speed in baud/kbps rather than as time dividers. So my question would be:
(I think I can do/help with the nanoFramework.Device.Can.Core and nanoFramework.Device.Can.STM32 code, but I can't test that and I don't know what the consequences for the STM32 are - I think none as they seem to have plenty of memory. A Mcp2515 is expected to arrive within two weeks) |
|
@frobijn you are right and I agree. Also with the discussion on Discord about having:
This work, can be done separately. That's what I did for the MQTT/Azure libs. And in a second time. |
|
So @Ellerbach, just like there is a "nanoFramework.M2Mqtt.Core" assembly at the moment, you suggest creating a "nanoFramework.CAN.Core" assembly (Namespace "nanoFramework.CAN" and also a new "nanoFramework.CAN" NuGet package) with all common CAN stuff? And just like the "Iot.Device.AtModem.Mqtt.Sim7080MqttClient" uses the "nanoFramework.M2Mqtt.IMqttClient", Or should it be "nanoFramework.Device.Can.MCP2515" like @frobijn suggests?
We will leave the "Iot.Device.Mcp25xxx" as is. (I can't imagine people actually use it. In the comment we can recommend them to use "Iot.Device.Mcp2515" instead) Is this in tune with:
Does the "Iot" namespace, instead of "nanoFramework" have a historical reason? From the time it was copied? |
Thanks! |
|
@Ellerbach And all nanoFramework.Device.CAN libraries (.Core .Stm32 .Esp32 .Mcp2515) will be part of the nanoFramework.Device.Can github repo, right? |
|
@rikkreeftenberg mind the @frobijn yes, it makes sense to have all these grouped in the same repo. For tests, integration and productivity helper. |
The Mcp2515 may stay on the IoT as it can be used for other purposes. Similar like the AT Modem sitting in the IoT Repo as it can do more than just MQTT. |
|
@Ellerbach @josesimoes How do you see the relation between the IoT.Device.Mcp25xx and the nanoFramework.Device.Can.Mcp2515 library? Should nanoFramework.Device.Can.Mcp2515 use the IoT.Device.Mcp25xx library? Then I think it is easier to create the nanoFramework.Device.Can.Mcp2515 in the nanoFramework.Device.Can repo and leave the IoT.Device.Mcp25xx where it is. If the nanoFramework.Device.Can namespace should be added to IoT.Device.Mcp25xx (like Rik has done in this PR) then there is no reason to move the IoT.Device.Mcp25xx to the nanoFramework.Device.Can repo. Right? |
If it's "only" using the standard elements, then yes, it can be moved to the Can repo. If like for the AT Modem, it is deeply integrated, then, it should stay in the IoT repo. |
|
|
So Frank and I came up with the following context diagram: |
|
I'm good with that. @Ellerbach ? |
|
@rikkreeftenberg @frobijn and @josesimoes I'm all good with the proposition! |
|
The setup of the new repository structure may not be so easy... see PR. Let's continue the discussion in that PR. |
Description
This is the start of an implementation of a CanController that uses the Mcp2515 to receive CAN messages.
This implementation derives from the following cpp Github repo's. Both having an MIT License:
• https://github.com/autowp/arduino-mcp2515
• https://github.com/sandeepmistry/arduino-CAN
The following classes were copied from the "nanoFramework.Device.Can" library.
• CanMessage
• CanMessageFrameType
• CanMessageIdType
Ideally these classes should be put in a separate library that is used by:
• Iot.Device.Mcp25xxx
• nanoFramework.Device.Can
The CanController hardly makes use of the "Iot.Device.Mcp25xxx.Register" classes. One of the reasons is that they are readonly. I only changed that in the Iot.Device.Mcp25xxx.Register.Interrupt.CanIntF class as an example.
Another reason is that you often need two registers to do anything usefull and this functionality is not created.
The CanController is not finished. At the moment you can only read.
Things I want to add are:
• Remote transmission request
• Writing CAN messages
• MessageRead Eventhandler
• Sample
• Readme documentation
Motivation and Context
With this change you can use the Mcp2515 for CAN communication over the SPI for any microcontroller. Also those that don't (yet) implement the CAN protocol natively.
How Has This Been Tested?
I have tested this functionality by reading CAN messages and writing them to the console.
Screenshots
Types of changes
Checklist:
Summary by CodeRabbit