[Mcp1525] Added a CanController (read messages only)#1292
[Mcp1525] Added a CanController (read messages only)#1292rikkreeftenberg wants to merge 8 commits into
Conversation
nfbot
changed the title
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a CAN abstraction and basic MCP25xxx controller integration (ICanController, CanController, CanMessage) and updates dependency versions in two Changes
Sequence Diagram(s)sequenceDiagram
participant App as Client
participant Ctrl as CanController
participant Dev as Mcp25xxx
participant Bus as CAN_Bus
rect rgba(100,150,255,0.5)
App->>Ctrl: Initialize(mcp)
Ctrl->>Dev: Reset() / Configure filters & masks
Dev-->>Ctrl: ACK
end
rect rgba(100,255,150,0.5)
App->>Ctrl: GetMessage()
Ctrl->>Dev: Read CanStat (check RX0/RX1 flags)
Dev-->>Ctrl: RX flag set
Ctrl->>Dev: Read RX buffer registers (SIDH/SIDL/DLC + data)
Dev-->>Ctrl: Raw frame bytes
Ctrl->>Ctrl: Parse ID, DLC, payload -> CanMessage
Ctrl-->>App: CanMessage or null
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Suggested labels
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches⚔️ Resolve merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
josesimoes
changed the title
josesimoes
changed the title
josesimoes
changed the title
Ellerbach
left a comment
Ellerbach
left a comment
There was a problem hiding this comment.
Thanks a lot for this addition. Couple of comments, mainly related to our linting way and variables naming. And it would be also perfect if you can add few things on how to use what you've been adding in the readme. Thank!
|
|
||
| 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@devices/Mcp25xxx/Can/CanController.cs`:
- Around line 39-51: The XML doc comments for the CanController methods (e.g.,
Initialize and WriteMessage) contain a stray ">" (`/// <inheritdoc/>>`) causing
them to render as literal text; edit every occurrence of this malformed tag in
CanController.cs (and other locations in the file where `inheritdoc` appears)
and replace `/// <inheritdoc/>>` with the correct `/// <inheritdoc/>` so the
comments properly inherit documentation.
- Around line 1-397: This file duplicates core CAN abstractions (ICanController,
CanMessage, CanMessageIdType, CanMessageFrameType) inside the device namespace;
remove the duplicated types from Iot.Device.Mcp25xxx.Can and reference the
shared hardware-agnostic package instead: replace local type definitions/usages
in CanController (methods Initialize, WriteMessage, GetMessage, ReadDataMessage,
PrepareId, SetFilter, SetFilterMask, SetBitRate, GetOperationMode,
SetOperationMode) to use the canonical types from nanoFramework.Device.Can (or
the agreed core namespace), update using/import statements and any type names as
needed, and confirm namespace casing is "Can" not "CAN" so the device
implementation becomes purely device-specific and relies on the central
nanoFramework.Device.Can abstractions.
- Line 140: Replace the bare Exception thrown in CanController where it checks
dataLengthCode (>8) with a specific exception type (e.g.,
InvalidOperationException or a domain-specific CanException) and include
contextual details (the offending dataLengthCode and the buffer number variable
used in this scope) in the message so callers can catch the specific exception
and logs/debugging show which buffer caused the failure.
- Around line 197-215: In ClearInterrupt(int bufferNumber) remove the leftover
trailing comment "// intFlag);" on the ClearInterrupt call and update the
misleading comment above the write so it correctly states that you're writing
the modified CanIntF register (not CanCtrl); i.e., change the comment to
something like "Write the modified value back to the CanIntF register" adjacent
to the _mcp25xxx.WriteByte(canIntf) call and keep the existing behavior of
writing canIntf.
- Line 91: Add the [Flags] attribute to the InterruptEnable enum declaration in
CanEnums.cs so combined bitwise values are represented correctly; locate the
InterruptEnable enum type (used by CanController.cs where
_mcp25xxx.WriteByte(Address.CanIntE, (byte)(InterruptEnable.RXB0 |
InterruptEnable.RXB1 | InterruptEnable.ERR | InterruptEnable.MSG_ERR)) is
called) and annotate its declaration with [Flags] directly above the enum
keyword, keeping existing enum member values unchanged.
- Around line 272-277: The filter setup contains a copy/paste bug and SetBitRate
mutates filters unexpectedly: change the second SetFilter call so MASK.MASK1
configures RXF.RXF2 (i.e. replace the duplicate RXF.RXF0 with RXF.RXF2 in the
block that calls SetFilterMask/SetFilter), and remove any code in SetBitRate
that writes acceptance filters or masks (undo the hard-coded 0x154/0x7ff
writes); instead, initialize any sane default filters/masks inside Reset() or
leave filter configuration to callers via SetFilter/SetFilterMask so SetBitRate
only configures timing parameters.
- Line 7: The OperationMode enum is declared under the Tests namespace causing
CanController.cs to import a test namespace; update OperationMode.cs to declare
namespace Iot.Device.Mcp25xxx.Register.CanControl (instead of
Iot.Device.Mcp25xxx.Tests.Register.CanControl) so the enum lives in production
code, then remove the using Iot.Device.Mcp25xxx.Tests.Register.CanControl;
statement from CanController.cs and ensure any references (e.g., OperationMode)
resolve to Iot.Device.Mcp25xxx.Register.CanControl.
- Around line 52-71: The GetMessage/ReadDataMessage path currently performs
separate Read(header) and Read(payload) calls plus a manual ClearInterrupt;
replace that sequence by using the existing Mcp25xxx.ReadRxBuffer method to
stream the entire RX buffer (header + payload) in one CS-asserted burst and let
the device auto-clear the RX interrupt. Locate ReadDataMessage (called from
GetMessage) and change its implementation to call _mcp25xxx.ReadRxBuffer for the
selected buffer index (0 or 1) instead of separate Read(sidhReg, 5) and
Read(dataReg, dataLengthCode) calls and remove the manual ClearInterrupt call,
ensuring the returned CanMessage is still constructed from the single buffer
read.
- Around line 227-270: The lookup builds a large local CnfMapper each call and
leaves cnf null if no match, causing a NullReferenceException at
_mcp25xxx.WriteByte(Address.Cnf1, cnf[0]);; stop allocating the table per-call
by hoisting the table into a private static readonly field (rename to cnfMapper
for camelCase) in the CanController class, then after the foreach (or by using a
switch/Lookup) validate the result and throw an ArgumentException with a clear
message (include clockFrequency and baudRate) if cnf is still null before
calling _mcp25xxx.WriteByte.
In `@devices/Mcp25xxx/Can/CanMessage.cs`:
- Around line 28-46: The XML docs on the Message field claim an
ArgumentOutOfRangeException is thrown for buffers >8 bytes but the CanMessage
constructor doesn't enforce this; update the constructor CanMessage(uint id,
CanMessageIdType identifierType, CanMessageFrameType frameType, byte[] message)
to validate message != null and message.Length <= 8 and throw
ArgumentOutOfRangeException (with a clear message) when exceeded, then assign
Message; also correct the misspellings "lenght" → "length" in the XML comments
for Message and the exception tag (or remove the exception tag if you prefer to
keep validation elsewhere like CanController.ReadDataMessage), ensuring callers
cannot create invalid CAN frames.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: nanoframework/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9766d710-3544-46cb-be4c-f556287ba115
📒 Files selected for processing (3)
devices/Mcp25xxx/Can/CanController.csdevices/Mcp25xxx/Can/CanMessage.csdevices/Mcp25xxx/Can/ICanController.cs
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| using Iot.Device.Mcp25xxx.Register; | ||
| using Iot.Device.Mcp25xxx.Register.CanControl; | ||
| using Iot.Device.Mcp25xxx.Register.Interrupt; | ||
| using Iot.Device.Mcp25xxx.Tests.Register.CanControl; | ||
| using System; | ||
| using System.Diagnostics; | ||
| using System.Threading; | ||
|
|
||
| namespace Iot.Device.Mcp25xxx.Can | ||
| { | ||
| /// <summary> | ||
| /// Represents a CAN controller on the system. | ||
| /// </summary> | ||
| public class CanController : ICanController | ||
| { | ||
| byte MCP_SIDH = 0; | ||
| byte MCP_SIDL = 1; | ||
| byte MCP_EID8 = 2; | ||
| byte MCP_EID0 = 3; | ||
| byte MCP_DLC = 4; | ||
| byte DLC_MASK = 0x0F; | ||
| byte TXB_EXIDE_MASK = 0x08; | ||
|
|
||
| private Mcp25xxx _mcp25xxx; | ||
|
|
||
| /// <summary> | ||
| /// private class to hold the baud rate and frequency configuration. Used in the "SetBitRate" method. | ||
| /// </summary> | ||
| private class BaudAndFrequencyConfig | ||
| { | ||
| public long ClockFrequency { get; set; } | ||
| public long BaudRate { get; set; } | ||
| public byte[] Cnf { get; set; } | ||
| } | ||
|
|
||
| /// <inheritdoc/>> | ||
| public void Initialize(Mcp25xxx mcp25xxx) | ||
| { | ||
| _mcp25xxx = mcp25xxx; | ||
| } | ||
|
|
||
| /// <inheritdoc/>> | ||
| public void WriteMessage(CanMessage message) | ||
| { | ||
| // This method will be implemented in a future PR | ||
| } | ||
|
|
||
| /// <inheritdoc/>> | ||
| public CanMessage GetMessage() | ||
| { | ||
| var status = _mcp25xxx.ReadStatus(); | ||
|
|
||
| if (status.HasFlag(ReadStatusResponse.Rx0If)) | ||
| { | ||
| // message in buffer 0 | ||
| return ReadDataMessage(0); | ||
| } | ||
| else if (status.HasFlag(ReadStatusResponse.Rx1If)) | ||
| { | ||
| // message in buffer 1 | ||
| return ReadDataMessage(1); | ||
| } | ||
| else | ||
| { | ||
| // no messages available | ||
| return null; | ||
| } | ||
| } | ||
|
|
||
| /// <inheritdoc/> | ||
| public void Reset() | ||
| { | ||
| Debug.WriteLine("Reset Instruction"); | ||
| _mcp25xxx.Reset(); | ||
|
|
||
| Thread.Sleep(10); | ||
|
|
||
| // Clear control buffers | ||
| byte[] data14 = new byte[14]; | ||
| _mcp25xxx.Write(Address.TxB0Ctrl, data14); | ||
| _mcp25xxx.Write(Address.TxB1Ctrl, data14); | ||
| _mcp25xxx.Write(Address.TxB2Ctrl, data14); | ||
|
|
||
| _mcp25xxx.WriteByte(Address.RxB0Ctrl, 0); | ||
| _mcp25xxx.WriteByte(Address.RxB1Ctrl, 0); | ||
|
|
||
|
|
||
| _mcp25xxx.WriteByte(Address.CanIntE, (byte)(InterruptEnable.RXB0 | InterruptEnable.RXB1 | InterruptEnable.ERR | InterruptEnable.MSG_ERR)); | ||
|
|
||
| // 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, | ||
| 0x00 | 0x04 | 0x00); | ||
| _mcp25xxx.BitModify(Address.RxB1Ctrl, | ||
| 0x60 | 0x07, | ||
| 0x00 | 0x01); | ||
|
|
||
| // clear filters and masks | ||
| // do not filter any standard frames for RXF0 used by RXB0 | ||
| // do not filter any extended frames for RXF1 used by RXB1 | ||
| byte[] zeros12 = new byte[12]; | ||
| _mcp25xxx.Write(Address.RxF0Sidh, zeros12); | ||
| _mcp25xxx.Write(Address.RxF3Sidh, zeros12); | ||
|
|
||
| byte[] zeros8 = new byte[8]; | ||
| _mcp25xxx.Write(Address.RxM0Sidh, zeros8); | ||
| } | ||
|
|
||
| private CanMessage ReadDataMessage(int bufferNumber) | ||
| { | ||
| var sidhReg = bufferNumber == 0 ? Address.RxB0Sidh : Address.RxB1Sidh; | ||
| var sidlReg = bufferNumber == 0 ? Address.RxB0Sidl : Address.RxB1Sidl; | ||
| var dlcReg = bufferNumber == 0 ? Address.RxB0Dlc : Address.RxB1Dlc; | ||
|
|
||
| var ctrlReg = bufferNumber == 0 ? Address.RxB0Ctrl : Address.RxB1Ctrl; | ||
| var dataReg = bufferNumber == 0 ? Address.RxB0D0 : Address.RxB1D0; //.RXB0DATA RXB1DATA | ||
| var intFlag = bufferNumber == 0 ? ReadStatusResponse.Rx0If : ReadStatusResponse.Rx1If; | ||
|
|
||
| // read 5 bytes | ||
| var buffer = _mcp25xxx.Read(sidhReg, 5); | ||
|
|
||
| int id = (buffer[MCP_SIDH] << 3) + (buffer[MCP_SIDL] >> 5); | ||
|
|
||
| bool isExtended = false; | ||
|
|
||
| // check to see if it's an extended ID | ||
| if ((buffer[MCP_SIDL] & TXB_EXIDE_MASK) == TXB_EXIDE_MASK) | ||
| { | ||
| id = (id << 2) + (buffer[MCP_SIDL] & 0x03); | ||
| id = (id << 8) + buffer[MCP_EID8]; | ||
| id = (id << 8) + buffer[MCP_EID0]; | ||
| isExtended = true; | ||
| } | ||
|
|
||
| byte dataLengthCode = (byte)(buffer[MCP_DLC] & DLC_MASK); | ||
| if (dataLengthCode > 8) throw new Exception($"DLC of {dataLengthCode} is > 8 bytes"); | ||
|
|
||
| // see if it's a remote transmission request | ||
| //var isRemoteTransmitRequest = false; | ||
| //var ctrl = ReadRegister(ctrlReg)[0]; | ||
| //if ((ctrl & RXBnCTRL_RTR) == RXBnCTRL_RTR) | ||
| //{ | ||
| // isRemoteTransmitRequest = true; | ||
| //} | ||
|
|
||
| // create the CANMessag | ||
| CanMessage frame; | ||
|
|
||
| if (isExtended) | ||
| { | ||
| //if (isRemoteTransmitRequest) | ||
| //{ | ||
| // frame = new ExtendedRtrFrame | ||
| // { | ||
| // ID = id, | ||
| // }; | ||
| //} | ||
| //else | ||
| //{ | ||
| frame = new CanMessage((uint)id, | ||
| CanMessageIdType.EID, | ||
| CanMessageFrameType.Data, | ||
| _mcp25xxx.Read(dataReg, dataLengthCode)); | ||
|
|
||
| //} | ||
| } | ||
| else | ||
| { | ||
| //if (isRemoteTransmitRequest) | ||
| //{ | ||
| // frame = new StandardRtrFrame | ||
| // { | ||
| // ID = id, | ||
| // }; | ||
| //} | ||
| //else | ||
| //{ | ||
| frame = new CanMessage((uint)id, | ||
| CanMessageIdType.SID, | ||
| CanMessageFrameType.Data, | ||
| _mcp25xxx.Read(dataReg, dataLengthCode)); | ||
|
|
||
| // read the frame data | ||
| //} | ||
| //} | ||
|
|
||
|
|
||
| } | ||
| // clear the interrupt flag | ||
| //if (InterruptPort != null) | ||
| //{ | ||
| //ClearInterrupt(intFlag); | ||
| ClearInterrupt(bufferNumber); // intFlag); | ||
|
|
||
| return frame; | ||
| } | ||
|
|
||
| private void ClearInterrupt(int bufferNumber) | ||
| { | ||
| var canIntf = new CanIntF(_mcp25xxx.Read(Address.CanIntF)); | ||
| if (bufferNumber == 0) | ||
| { | ||
| canIntf.ReceiveBuffer0FullInterruptFlag = false; | ||
| } | ||
| else | ||
| { | ||
| canIntf.ReceiveBuffer1FullInterruptFlag = false; | ||
| } | ||
|
|
||
| // Write the modified value back to the CanCtrl register | ||
| _mcp25xxx.WriteByte(canIntf); | ||
| } | ||
|
|
||
| /// <inheritdoc/> | ||
| public void SetBitRate(int baudRate, int clockFrequency) | ||
| { | ||
| var mode = GetOperationMode(); | ||
| if (mode != OperationMode.Configuration) | ||
| { | ||
| SetOperationMode(OperationMode.Configuration); | ||
| } | ||
|
|
||
| var CnfMapper = new BaudAndFrequencyConfig[] | ||
| { | ||
| new() { ClockFrequency = 8_000_000L, BaudRate = 1_000_000L, Cnf = new byte[] { 0x00, 0x80, 0x00 } }, | ||
| new() { ClockFrequency = 8_000_000L, BaudRate = 666_666L, Cnf = new byte[] { 0xC0, 0xB8, 0x01 } }, | ||
| new() { ClockFrequency = 8_000_000L, BaudRate = 500_000L, Cnf = new byte[] { 0x00, 0x90, 0x02 } }, | ||
| new() { ClockFrequency = 8_000_000L, BaudRate = 250_000L, Cnf = new byte[] { 0x00, 0xB1, 0x05 } }, | ||
| new() { ClockFrequency = 8_000_000L, BaudRate = 200_000L, Cnf = new byte[] { 0x00, 0xB4, 0x06 } }, | ||
| new() { ClockFrequency = 8_000_000L, BaudRate = 125_000L, Cnf = new byte[] { 0x01, 0xB1, 0x05 } }, | ||
| new() { ClockFrequency = 8_000_000L, BaudRate = 100_000L, Cnf = new byte[] { 0x01, 0xB4, 0x06 } }, | ||
| new() { ClockFrequency = 8_000_000L, BaudRate = 80_000L, Cnf = new byte[] { 0x01, 0xBF, 0x07 } }, | ||
| new() { ClockFrequency = 8_000_000L, BaudRate = 50_000L, Cnf = new byte[] { 0x03, 0xB4, 0x06 } }, | ||
| new() { ClockFrequency = 8_000_000L, BaudRate = 40_000L, Cnf = new byte[] { 0x03, 0xBF, 0x07 } }, | ||
| new() { ClockFrequency = 8_000_000L, BaudRate = 20_000L, Cnf = new byte[] { 0x07, 0xBF, 0x07 } }, | ||
| new() { ClockFrequency = 8_000_000L, BaudRate = 10_000L, Cnf = new byte[] { 0x0F, 0xBF, 0x07 } }, | ||
| new() { ClockFrequency = 8_000_000L, BaudRate = 5_000L, Cnf = new byte[] { 0x1F, 0xBF, 0x07 } }, | ||
| new() { ClockFrequency = 16_000_000L, BaudRate = 1_000_000L, Cnf = new byte[] { 0x00, 0xD0, 0x82 } }, | ||
| new() { ClockFrequency = 16_000_000L, BaudRate = 666_666L, Cnf = new byte[] { 0xC0, 0xF8, 0x81 } }, | ||
| new() { ClockFrequency = 16_000_000L, BaudRate = 500_000L, Cnf = new byte[] { 0x00, 0xF0, 0x86 } }, | ||
| new() { ClockFrequency = 16_000_000L, BaudRate = 250_000L, Cnf = new byte[] { 0x41, 0xF1, 0x85 } }, | ||
| new() { ClockFrequency = 16_000_000L, BaudRate = 200_000L, Cnf = new byte[] { 0x01, 0xFA, 0x87 } }, | ||
| new() { ClockFrequency = 16_000_000L, BaudRate = 125_000L, Cnf = new byte[] { 0x03, 0xF0, 0x86 } }, | ||
| new() { ClockFrequency = 16_000_000L, BaudRate = 100_000L, Cnf = new byte[] { 0x03, 0xFA, 0x87 } }, | ||
| new() { ClockFrequency = 16_000_000L, BaudRate = 80_000L, Cnf = new byte[] { 0x03, 0xFF, 0x87 } }, | ||
| new() { ClockFrequency = 16_000_000L, BaudRate = 50_000L, Cnf = new byte[] { 0x07, 0xFA, 0x87 } }, | ||
| new() { ClockFrequency = 16_000_000L, BaudRate = 40_000L, Cnf = new byte[] { 0x07, 0xFF, 0x87 } }, | ||
| new() { ClockFrequency = 16_000_000L, BaudRate = 20_000L, Cnf = new byte[] { 0x0F, 0xFF, 0x87 } }, | ||
| new() { ClockFrequency = 16_000_000L, BaudRate = 10_000L, Cnf = new byte[] { 0x1F, 0xFF, 0x87 } }, | ||
| new() { ClockFrequency = 16_000_000L, BaudRate = 5_000L, Cnf = new byte[] { 0x3F, 0xFF, 0x87 } }, | ||
| }; | ||
|
|
||
| byte[] cnf = null; | ||
|
|
||
| foreach (var mapper in CnfMapper) | ||
| { | ||
| if (mapper.ClockFrequency == clockFrequency && mapper.BaudRate == baudRate) | ||
| { | ||
| cnf = mapper.Cnf; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| _mcp25xxx.WriteByte(Address.Cnf1, cnf[0]); | ||
| _mcp25xxx.WriteByte(Address.Cnf2, cnf[1]); | ||
| _mcp25xxx.WriteByte(Address.Cnf3, cnf[2]); | ||
|
|
||
| SetFilterMask(MASK.MASK0, false, 0x7ff); | ||
| SetFilter(RXF.RXF0, false, 0x154); | ||
| SetFilterMask(MASK.MASK1, false, 0x7ff); | ||
| SetFilter(RXF.RXF0, false, 0x154); | ||
|
|
||
| SetOperationMode(OperationMode.NormalOperation); | ||
| } | ||
|
|
||
| /// <inheritdoc/> | ||
| public bool SetFilter(RXF num, bool ext, uint ulData) | ||
| { | ||
| var mode = GetOperationMode(); | ||
| SetOperationMode(OperationMode.Configuration); | ||
|
|
||
| Address reg; | ||
|
|
||
| switch (num) | ||
| { | ||
| case RXF.RXF0: | ||
| reg = Address.RxF0Sidh; | ||
| break; | ||
| case RXF.RXF1: | ||
| reg = Address.RxF1Sidh; | ||
| break; | ||
| case RXF.RXF2: | ||
| reg = Address.RxF2Sidh; | ||
| break; | ||
| case RXF.RXF3: | ||
| reg = Address.RxF3Sidh; | ||
| break; | ||
| case RXF.RXF4: | ||
| reg = Address.RxF4Sidh; | ||
| break; | ||
| case RXF.RXF5: | ||
| reg = Address.RxF5Sidh; | ||
| break; | ||
| default: | ||
| return false; | ||
| } | ||
|
|
||
| SpanByte tbufdata = new byte[4]; | ||
| PrepareId(tbufdata, ext, ulData); | ||
| _mcp25xxx.Write(reg, tbufdata); | ||
|
|
||
| SetOperationMode(mode); | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
| private void PrepareId(SpanByte buffer, bool ext, uint id) | ||
| { | ||
| ushort canid = (ushort)(id & 0x0FFFF); | ||
|
|
||
| if (ext) | ||
| { | ||
| buffer[MCP_EID0] = (byte)(canid & 0xFF); | ||
| buffer[MCP_EID8] = (byte)(canid >> 8); | ||
| canid = (ushort)(id >> 16); | ||
| buffer[MCP_SIDL] = (byte)(canid & 0x03); | ||
| buffer[MCP_SIDL] += (byte)((canid & 0x1C) << 3); | ||
| buffer[MCP_SIDL] |= TXB_EXIDE_MASK; | ||
| buffer[MCP_SIDH] = (byte)(canid >> 5); | ||
| } | ||
| else | ||
| { | ||
| buffer[MCP_SIDH] = (byte)(canid >> 3); | ||
| buffer[MCP_SIDL] = (byte)((canid & 0x07) << 5); | ||
| buffer[MCP_EID0] = 0; | ||
| buffer[MCP_EID8] = 0; | ||
| } | ||
| } | ||
|
|
||
| private bool SetFilterMask(MASK mask, bool ext, uint ulData) | ||
| { | ||
| var mode = GetOperationMode(); | ||
| SetOperationMode(OperationMode.Configuration); | ||
|
|
||
| SpanByte tbufdata = new SpanByte(new byte[4]); | ||
| PrepareId(tbufdata, ext, ulData); | ||
|
|
||
| Address reg; | ||
| switch (mask) | ||
| { | ||
| case MASK.MASK0: | ||
| reg = Address.RxM0Sidh; | ||
| break; | ||
| case MASK.MASK1: | ||
| reg = Address.RxM1Sidh; | ||
| break; | ||
| default: | ||
| return false; | ||
| } | ||
|
|
||
| _mcp25xxx.Write(reg, tbufdata); | ||
|
|
||
| SetOperationMode(mode); | ||
| return true; | ||
| } | ||
|
|
||
| private OperationMode GetOperationMode() | ||
| { | ||
| byte value = _mcp25xxx.Read(Address.CanStat); | ||
| var canStat = new CanStat(value); | ||
| return canStat.OperationMode; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Sets the operation mode of the MCP25xxx device. | ||
| /// </summary> | ||
| /// <param name="mode">The desired operation mode.</param> | ||
| private void SetOperationMode(OperationMode mode) | ||
| { | ||
| // Read the current value of the CanCtrl register | ||
| var canCtrlValue = new CanCtrl(_mcp25xxx.Read(Address.CanCtrl)); | ||
|
|
||
| var newCanCtrlValue = new CanCtrl( | ||
| canCtrlValue.ClkOutPinPrescaler, | ||
| canCtrlValue.ClkOutPinEnable, | ||
| canCtrlValue.OneShotMode, | ||
| canCtrlValue.AbortAllPendingTransmissions, | ||
| mode); | ||
|
|
||
| // Write the modified value back to the CanCtrl register | ||
| _mcp25xxx.WriteByte(newCanCtrlValue); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Architectural reminder: align with the planned nanoFramework.Device.Can split.
Per the linked issue (nanoframework/Home#1600) and the PR discussion, the direction is a hardware-agnostic core (nanoFramework.Device.Can with ICanController, CanMessage, CanMessageIdType, CanMessageFrameType) plus device-specific implementations (nanoFramework.Device.Can.Mcp2515, …). Today this PR duplicates CanMessage/CanMessageIdType/CanMessageFrameType under Iot.Device.Mcp25xxx.Can and also defines ICanController there. Before merging, please confirm the final location with the reviewers so consumers don't have to change namespaces/types shortly after. Note the casing convention is "Can" (not "CAN").
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@devices/Mcp25xxx/Can/CanController.cs` around lines 1 - 397, This file
duplicates core CAN abstractions (ICanController, CanMessage, CanMessageIdType,
CanMessageFrameType) inside the device namespace; remove the duplicated types
from Iot.Device.Mcp25xxx.Can and reference the shared hardware-agnostic package
instead: replace local type definitions/usages in CanController (methods
Initialize, WriteMessage, GetMessage, ReadDataMessage, PrepareId, SetFilter,
SetFilterMask, SetBitRate, GetOperationMode, SetOperationMode) to use the
canonical types from nanoFramework.Device.Can (or the agreed core namespace),
update using/import statements and any type names as needed, and confirm
namespace casing is "Can" not "CAN" so the device implementation becomes purely
device-specific and relies on the central nanoFramework.Device.Can abstractions.
| using Iot.Device.Mcp25xxx.Register; | ||
| using Iot.Device.Mcp25xxx.Register.CanControl; | ||
| using Iot.Device.Mcp25xxx.Register.Interrupt; | ||
| using Iot.Device.Mcp25xxx.Tests.Register.CanControl; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find which symbol(s) from Tests.Register.CanControl CanController depends on,
# and confirm whether equivalents already exist in the production namespace.
rg -n --type=cs 'namespace\s+Iot\.Device\.Mcp25xxx\.Tests\.Register\.CanControl'
rg -n --type=cs 'namespace\s+Iot\.Device\.Mcp25xxx\.Register\.CanControl'
rg -nP --type=cs -C2 '\b(CanStat|CanCtrl)\b' devices/Mcp25xxx/Can/CanController.csRepository: nanoframework/nanoFramework.IoT.Device
Length of output: 1352
🏁 Script executed:
#!/bin/bash
# Show all using statements in CanController.cs
head -20 devices/Mcp25xxx/Can/CanController.cs
# Search for which symbols are actually used and determine their source
echo "=== Checking OperationMode usage ==="
rg -n 'OperationMode' devices/Mcp25xxx/Can/CanController.cs | head -20
# Check what's in the production Register.CanControl namespace
echo "=== Files in production Register.CanControl ==="
ls -la devices/Mcp25xxx/Register/CanControl/ 2>/dev/null || echo "Dir structure varies"
# Check OperationMode source
echo "=== OperationMode definition location ==="
rg -n 'enum OperationMode' --type=csRepository: nanoframework/nanoFramework.IoT.Device
Length of output: 2042
🏁 Script executed:
#!/bin/bash
# Check the actual namespace in OperationMode.cs
head -15 devices/Mcp25xxx/Register/CanControl/OperationMode.csRepository: nanoframework/nanoFramework.IoT.Device
Length of output: 501
OperationMode enum is declared in the Tests namespace but should be in the production namespace.
OperationMode.cs is located in devices/Mcp25xxx/Register/CanControl/ but declares namespace Iot.Device.Mcp25xxx.Tests.Register.CanControl. This forces CanController.cs to import from the Tests namespace to use OperationMode, creating an unwanted production → tests dependency.
Change the namespace declaration in OperationMode.cs to Iot.Device.Mcp25xxx.Register.CanControl, then remove the using Iot.Device.Mcp25xxx.Tests.Register.CanControl; line from CanController.cs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@devices/Mcp25xxx/Can/CanController.cs` at line 7, The OperationMode enum is
declared under the Tests namespace causing CanController.cs to import a test
namespace; update OperationMode.cs to declare namespace
Iot.Device.Mcp25xxx.Register.CanControl (instead of
Iot.Device.Mcp25xxx.Tests.Register.CanControl) so the enum lives in production
code, then remove the using Iot.Device.Mcp25xxx.Tests.Register.CanControl;
statement from CanController.cs and ensure any references (e.g., OperationMode)
resolve to Iot.Device.Mcp25xxx.Register.CanControl.
| /// <inheritdoc/>> | ||
| public void Initialize(Mcp25xxx mcp25xxx) | ||
| { | ||
| _mcp25xxx = mcp25xxx; | ||
| } | ||
|
|
||
| /// <inheritdoc/>> | ||
| public void WriteMessage(CanMessage message) | ||
| { | ||
| // This method will be implemented in a future PR | ||
| } | ||
|
|
||
| /// <inheritdoc/>> |
There was a problem hiding this comment.
/// <inheritdoc/>> has a stray >.
All three sites render as literal text instead of inheriting docs. Replace with /// <inheritdoc/> (this also occurs elsewhere in the file where inheritdoc appears).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@devices/Mcp25xxx/Can/CanController.cs` around lines 39 - 51, The XML doc
comments for the CanController methods (e.g., Initialize and WriteMessage)
contain a stray ">" (`/// <inheritdoc/>>`) causing them to render as literal
text; edit every occurrence of this malformed tag in CanController.cs (and other
locations in the file where `inheritdoc` appears) and replace `///
<inheritdoc/>>` with the correct `/// <inheritdoc/>` so the comments properly
inherit documentation.
| public CanMessage GetMessage() | ||
| { | ||
| var status = _mcp25xxx.ReadStatus(); | ||
|
|
||
| if (status.HasFlag(ReadStatusResponse.Rx0If)) | ||
| { | ||
| // message in buffer 0 | ||
| return ReadDataMessage(0); | ||
| } | ||
| else if (status.HasFlag(ReadStatusResponse.Rx1If)) | ||
| { | ||
| // message in buffer 1 | ||
| return ReadDataMessage(1); | ||
| } | ||
| else | ||
| { | ||
| // no messages available | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# See whether the base Mcp25xxx already exposes the "Read RX Buffer" instruction.
rg -nP --type=cs '\bRead(Rx|RX)Buffer\b|InstructionFormat\.ReadRxBuffer|Read Rx Buffer'Repository: nanoframework/nanoFramework.IoT.Device
Length of output: 764
🏁 Script executed:
#!/bin/bash
# Check the ReadDataMessage implementation in CanController
rg -n "ReadDataMessage" devices/Mcp25xxx/Can/CanController.cs -A 15Repository: nanoframework/nanoFramework.IoT.Device
Length of output: 1539
🏁 Script executed:
#!/bin/bash
# Check if there's a ReadDataMessage method in Mcp25xxx base class
rg -n "ReadDataMessage" devices/Mcp25xxx/Mcp25xxx.cs -A 20Repository: nanoframework/nanoFramework.IoT.Device
Length of output: 64
🏁 Script executed:
#!/bin/bash
# Look at the ClearInterrupt calls in CanController
rg -n "ClearInterrupt" devices/Mcp25xxx/Can/CanController.cs -B 2 -A 2Repository: nanoframework/nanoFramework.IoT.Device
Length of output: 452
🏁 Script executed:
#!/bin/bash
# Get the complete ReadDataMessage method
sed -n '113,200p' devices/Mcp25xxx/Can/CanController.csRepository: nanoframework/nanoFramework.IoT.Device
Length of output: 3243
Consider using ReadRxBuffer to consolidate header and payload reads.
ReadDataMessage issues one Read(sidhReg, 5) for the header, then a second Read(dataReg, dataLengthCode) for the payload, followed by a manual ClearInterrupt that triggers another read. The MCP2515 provides a dedicated ReadRxBuffer instruction (already available in the base Mcp25xxx class) that streams the entire buffer in a single CS-asserted burst and automatically clears the RX-full interrupt, eliminating multiple read calls and improving throughput and atomicity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@devices/Mcp25xxx/Can/CanController.cs` around lines 52 - 71, The
GetMessage/ReadDataMessage path currently performs separate Read(header) and
Read(payload) calls plus a manual ClearInterrupt; replace that sequence by using
the existing Mcp25xxx.ReadRxBuffer method to stream the entire RX buffer (header
+ payload) in one CS-asserted burst and let the device auto-clear the RX
interrupt. Locate ReadDataMessage (called from GetMessage) and change its
implementation to call _mcp25xxx.ReadRxBuffer for the selected buffer index (0
or 1) instead of separate Read(sidhReg, 5) and Read(dataReg, dataLengthCode)
calls and remove the manual ClearInterrupt call, ensuring the returned
CanMessage is still constructed from the single buffer read.
| _mcp25xxx.WriteByte(Address.RxB1Ctrl, 0); | ||
|
|
||
|
|
||
| _mcp25xxx.WriteByte(Address.CanIntE, (byte)(InterruptEnable.RXB0 | InterruptEnable.RXB1 | InterruptEnable.ERR | InterruptEnable.MSG_ERR)); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm InterruptEnable is a [Flags] enum with power-of-two members.
rg -nP -B2 -A20 '\benum\s+InterruptEnable\b' --type=csRepository: nanoframework/nanoFramework.IoT.Device
Length of output: 2267
Add [Flags] attribute to the InterruptEnable enum in CanEnums.cs.
The InterruptEnable enum (in devices/Mcp25xxx/Can/CanEnums.cs) is used with bitwise OR operations but lacks the [Flags] attribute. While the code functions correctly because values are powers of two, the attribute is a required best practice for enums that combine flags. Without it, debuggers and reflection-based tools won't properly display combined flag values.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@devices/Mcp25xxx/Can/CanController.cs` at line 91, Add the [Flags] attribute
to the InterruptEnable enum declaration in CanEnums.cs so combined bitwise
values are represented correctly; locate the InterruptEnable enum type (used by
CanController.cs where _mcp25xxx.WriteByte(Address.CanIntE,
(byte)(InterruptEnable.RXB0 | InterruptEnable.RXB1 | InterruptEnable.ERR |
InterruptEnable.MSG_ERR)) is called) and annotate its declaration with [Flags]
directly above the enum keyword, keeping existing enum member values unchanged.
| } | ||
|
|
||
| byte dataLengthCode = (byte)(buffer[MCP_DLC] & DLC_MASK); | ||
| if (dataLengthCode > 8) throw new Exception($"DLC of {dataLengthCode} is > 8 bytes"); |
There was a problem hiding this comment.
Don't throw bare Exception.
Throw a specific type (InvalidOperationException or a domain-specific CanException) so callers can handle it meaningfully. Also provide the buffer number in the message to aid debugging.
🛡️ Proposed fix
- if (dataLengthCode > 8) throw new Exception($"DLC of {dataLengthCode} is > 8 bytes");
+ if (dataLengthCode > 8)
+ {
+ throw new InvalidOperationException(
+ $"DLC of {dataLengthCode} in RXB{bufferNumber} exceeds the 8-byte CAN limit.");
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@devices/Mcp25xxx/Can/CanController.cs` at line 140, Replace the bare
Exception thrown in CanController where it checks dataLengthCode (>8) with a
specific exception type (e.g., InvalidOperationException or a domain-specific
CanException) and include contextual details (the offending dataLengthCode and
the buffer number variable used in this scope) in the message so callers can
catch the specific exception and logs/debugging show which buffer caused the
failure.
| ClearInterrupt(bufferNumber); // intFlag); | ||
|
|
||
| return frame; | ||
| } | ||
|
|
||
| private void ClearInterrupt(int bufferNumber) | ||
| { | ||
| var canIntf = new CanIntF(_mcp25xxx.Read(Address.CanIntF)); | ||
| if (bufferNumber == 0) | ||
| { | ||
| canIntf.ReceiveBuffer0FullInterruptFlag = false; | ||
| } | ||
| else | ||
| { | ||
| canIntf.ReceiveBuffer1FullInterruptFlag = false; | ||
| } | ||
|
|
||
| // Write the modified value back to the CanCtrl register | ||
| _mcp25xxx.WriteByte(canIntf); |
There was a problem hiding this comment.
Misleading comment in ClearInterrupt.
Line 214 says "Write the modified value back to the CanCtrl register" but the register being written is CanIntF. Also, line 197's trailing // intFlag); leftover is noise — remove it.
♻️ Proposed fix
- ClearInterrupt(bufferNumber); // intFlag);
+ ClearInterrupt(bufferNumber);
...
- // Write the modified value back to the CanCtrl register
+ // Write the modified value back to the CanIntF register.
_mcp25xxx.WriteByte(canIntf);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@devices/Mcp25xxx/Can/CanController.cs` around lines 197 - 215, In
ClearInterrupt(int bufferNumber) remove the leftover trailing comment "//
intFlag);" on the ClearInterrupt call and update the misleading comment above
the write so it correctly states that you're writing the modified CanIntF
register (not CanCtrl); i.e., change the comment to something like "Write the
modified value back to the CanIntF register" adjacent to the
_mcp25xxx.WriteByte(canIntf) call and keep the existing behavior of writing
canIntf.
| var CnfMapper = new BaudAndFrequencyConfig[] | ||
| { | ||
| new() { ClockFrequency = 8_000_000L, BaudRate = 1_000_000L, Cnf = new byte[] { 0x00, 0x80, 0x00 } }, | ||
| new() { ClockFrequency = 8_000_000L, BaudRate = 666_666L, Cnf = new byte[] { 0xC0, 0xB8, 0x01 } }, | ||
| new() { ClockFrequency = 8_000_000L, BaudRate = 500_000L, Cnf = new byte[] { 0x00, 0x90, 0x02 } }, | ||
| new() { ClockFrequency = 8_000_000L, BaudRate = 250_000L, Cnf = new byte[] { 0x00, 0xB1, 0x05 } }, | ||
| new() { ClockFrequency = 8_000_000L, BaudRate = 200_000L, Cnf = new byte[] { 0x00, 0xB4, 0x06 } }, | ||
| new() { ClockFrequency = 8_000_000L, BaudRate = 125_000L, Cnf = new byte[] { 0x01, 0xB1, 0x05 } }, | ||
| new() { ClockFrequency = 8_000_000L, BaudRate = 100_000L, Cnf = new byte[] { 0x01, 0xB4, 0x06 } }, | ||
| new() { ClockFrequency = 8_000_000L, BaudRate = 80_000L, Cnf = new byte[] { 0x01, 0xBF, 0x07 } }, | ||
| new() { ClockFrequency = 8_000_000L, BaudRate = 50_000L, Cnf = new byte[] { 0x03, 0xB4, 0x06 } }, | ||
| new() { ClockFrequency = 8_000_000L, BaudRate = 40_000L, Cnf = new byte[] { 0x03, 0xBF, 0x07 } }, | ||
| new() { ClockFrequency = 8_000_000L, BaudRate = 20_000L, Cnf = new byte[] { 0x07, 0xBF, 0x07 } }, | ||
| new() { ClockFrequency = 8_000_000L, BaudRate = 10_000L, Cnf = new byte[] { 0x0F, 0xBF, 0x07 } }, | ||
| new() { ClockFrequency = 8_000_000L, BaudRate = 5_000L, Cnf = new byte[] { 0x1F, 0xBF, 0x07 } }, | ||
| new() { ClockFrequency = 16_000_000L, BaudRate = 1_000_000L, Cnf = new byte[] { 0x00, 0xD0, 0x82 } }, | ||
| new() { ClockFrequency = 16_000_000L, BaudRate = 666_666L, Cnf = new byte[] { 0xC0, 0xF8, 0x81 } }, | ||
| new() { ClockFrequency = 16_000_000L, BaudRate = 500_000L, Cnf = new byte[] { 0x00, 0xF0, 0x86 } }, | ||
| new() { ClockFrequency = 16_000_000L, BaudRate = 250_000L, Cnf = new byte[] { 0x41, 0xF1, 0x85 } }, | ||
| new() { ClockFrequency = 16_000_000L, BaudRate = 200_000L, Cnf = new byte[] { 0x01, 0xFA, 0x87 } }, | ||
| new() { ClockFrequency = 16_000_000L, BaudRate = 125_000L, Cnf = new byte[] { 0x03, 0xF0, 0x86 } }, | ||
| new() { ClockFrequency = 16_000_000L, BaudRate = 100_000L, Cnf = new byte[] { 0x03, 0xFA, 0x87 } }, | ||
| new() { ClockFrequency = 16_000_000L, BaudRate = 80_000L, Cnf = new byte[] { 0x03, 0xFF, 0x87 } }, | ||
| new() { ClockFrequency = 16_000_000L, BaudRate = 50_000L, Cnf = new byte[] { 0x07, 0xFA, 0x87 } }, | ||
| new() { ClockFrequency = 16_000_000L, BaudRate = 40_000L, Cnf = new byte[] { 0x07, 0xFF, 0x87 } }, | ||
| new() { ClockFrequency = 16_000_000L, BaudRate = 20_000L, Cnf = new byte[] { 0x0F, 0xFF, 0x87 } }, | ||
| new() { ClockFrequency = 16_000_000L, BaudRate = 10_000L, Cnf = new byte[] { 0x1F, 0xFF, 0x87 } }, | ||
| new() { ClockFrequency = 16_000_000L, BaudRate = 5_000L, Cnf = new byte[] { 0x3F, 0xFF, 0x87 } }, | ||
| }; | ||
|
|
||
| byte[] cnf = null; | ||
|
|
||
| foreach (var mapper in CnfMapper) | ||
| { | ||
| if (mapper.ClockFrequency == clockFrequency && mapper.BaudRate == baudRate) | ||
| { | ||
| cnf = mapper.Cnf; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| _mcp25xxx.WriteByte(Address.Cnf1, cnf[0]); | ||
| _mcp25xxx.WriteByte(Address.Cnf2, cnf[1]); | ||
| _mcp25xxx.WriteByte(Address.Cnf3, cnf[2]); |
There was a problem hiding this comment.
Null-deref when (clockFrequency, baudRate) is not in the table, and large array re-allocated per call.
Two problems in this block:
- Critical: If no mapper entry matches (e.g. the user passes 20 MHz, or a common variant like 125 kbps @ 20 MHz),
cnfstaysnull, and lines 268–270 will throwNullReferenceException. The failure mode for a user mis-typing a rate is thus a cryptic NRE after the device has already been put into Configuration mode and never taken out. Validate the lookup and throw a descriptiveArgumentException. - Duplicate (memory): The
CnfMapperarray is allocated — 26 objects + 26 byte[3] arrays — on every call. It is a pure constant table; hoist it to aprivate static readonlyfield (or aswitchexpression as suggested previously) so it is built once.
Also, local CnfMapper should be camelCase (cnfMapper) to match repo convention.
♻️🛡️ Proposed fix
- public void SetBitRate(int baudRate, int clockFrequency)
+ private static readonly BaudAndFrequencyConfig[] s_cnfMapper = new BaudAndFrequencyConfig[]
+ {
+ new() { ClockFrequency = 8_000_000L, BaudRate = 1_000_000L, Cnf = new byte[] { 0x00, 0x80, 0x00 } },
+ // ... remaining entries ...
+ new() { ClockFrequency = 16_000_000L, BaudRate = 5_000L, Cnf = new byte[] { 0x3F, 0xFF, 0x87 } },
+ };
+
+ public void SetBitRate(int baudRate, int clockFrequency)
{
var mode = GetOperationMode();
if (mode != OperationMode.Configuration)
{
SetOperationMode(OperationMode.Configuration);
}
- var CnfMapper = new BaudAndFrequencyConfig[] { /* ... 26 entries ... */ };
-
byte[] cnf = null;
- foreach (var mapper in CnfMapper)
+ foreach (var mapper in s_cnfMapper)
{
if (mapper.ClockFrequency == clockFrequency && mapper.BaudRate == baudRate)
{
cnf = mapper.Cnf;
break;
}
}
+
+ if (cnf == null)
+ {
+ throw new ArgumentException(
+ $"Unsupported combination: clockFrequency={clockFrequency}, baudRate={baudRate}.");
+ }
_mcp25xxx.WriteByte(Address.Cnf1, cnf[0]);
_mcp25xxx.WriteByte(Address.Cnf2, cnf[1]);
_mcp25xxx.WriteByte(Address.Cnf3, cnf[2]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@devices/Mcp25xxx/Can/CanController.cs` around lines 227 - 270, The lookup
builds a large local CnfMapper each call and leaves cnf null if no match,
causing a NullReferenceException at _mcp25xxx.WriteByte(Address.Cnf1, cnf[0]);;
stop allocating the table per-call by hoisting the table into a private static
readonly field (rename to cnfMapper for camelCase) in the CanController class,
then after the foreach (or by using a switch/Lookup) validate the result and
throw an ArgumentException with a clear message (include clockFrequency and
baudRate) if cnf is still null before calling _mcp25xxx.WriteByte.
| SetFilterMask(MASK.MASK0, false, 0x7ff); | ||
| SetFilter(RXF.RXF0, false, 0x154); | ||
| SetFilterMask(MASK.MASK1, false, 0x7ff); | ||
| SetFilter(RXF.RXF0, false, 0x154); | ||
|
|
||
| SetOperationMode(OperationMode.NormalOperation); |
There was a problem hiding this comment.
Likely copy/paste bug in filter setup, and SetBitRate is also hard-coding an acceptance filter.
Two concerns:
- The second
SetFilterafterMASK.MASK1passesRXF.RXF0again.MASK1governs RXB1 which uses filtersRXF2–RXF5, so the intended call is almost certainlySetFilter(RXF.RXF2, false, ...). As written,RXF0is overwritten twice and no RXB1 filter is ever configured. - Setting an acceptance filter of
0x154with mask0x7ffinsideSetBitRatesilently restricts reception to a single CAN ID — directly contradicting the "receive all valid messages" behavior established byReset(). This mixes two unrelated concerns (bit-timing vs. acceptance filtering) and is a surprising side effect of callingSetBitRate. Filter setup should be left to the caller viaSetFilter/SetFilterMask.
🛡️ Proposed fix
- SetFilterMask(MASK.MASK0, false, 0x7ff);
- SetFilter(RXF.RXF0, false, 0x154);
- SetFilterMask(MASK.MASK1, false, 0x7ff);
- SetFilter(RXF.RXF0, false, 0x154);
-
SetOperationMode(OperationMode.NormalOperation);(Move any default filter/mask initialization into Reset() or require callers to configure filters explicitly.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@devices/Mcp25xxx/Can/CanController.cs` around lines 272 - 277, The filter
setup contains a copy/paste bug and SetBitRate mutates filters unexpectedly:
change the second SetFilter call so MASK.MASK1 configures RXF.RXF2 (i.e. replace
the duplicate RXF.RXF0 with RXF.RXF2 in the block that calls
SetFilterMask/SetFilter), and remove any code in SetBitRate that writes
acceptance filters or masks (undo the hard-coded 0x154/0x7ff writes); instead,
initialize any sane default filters/masks inside Reset() or leave filter
configuration to callers via SetFilter/SetFilterMask so SetBitRate only
configures timing parameters.
| /// <summary> | ||
| /// Message data. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// Maximum lenght of data buffer is 8. | ||
| /// </remarks> | ||
| /// <exception cref="ArgumentOutOfRangeException">If the message buffer exceeds the maximum allowed lenght.</exception> | ||
| public byte[] Message; | ||
|
|
||
| /// <summary> | ||
| /// Creates a CAN message. | ||
| /// </summary> | ||
| public CanMessage(uint id, CanMessageIdType identifierType, CanMessageFrameType frameType, byte[] message) | ||
| { | ||
| Id = id; | ||
| IdentifierType = identifierType; | ||
| FrameType = frameType; | ||
| Message = message; | ||
| } |
There was a problem hiding this comment.
Docs advertise an exception that is never thrown; also validate the payload length.
The <exception> tag on Message claims ArgumentOutOfRangeException is thrown when the buffer exceeds the maximum allowed length, but the constructor performs no validation. Either enforce the contract in the constructor (CAN frames are max 8 data bytes), or remove the misleading <exception> tag. Also: "lenght" → "length" (twice).
Currently only ReadDataMessage in CanController.cs validates DLC (line 140), so any external caller can construct an invalid CanMessage.
🛡️ Proposed fix
/// <remarks>
- /// Maximum lenght of data buffer is 8.
+ /// Maximum length of data buffer is 8.
/// </remarks>
- /// <exception cref="ArgumentOutOfRangeException">If the message buffer exceeds the maximum allowed lenght.</exception>
- public byte[] Message;
+ public byte[] Message { get; }
...
/// <summary>
/// Creates a CAN message.
/// </summary>
+ /// <exception cref="ArgumentOutOfRangeException">If <paramref name="message"/> exceeds 8 bytes.</exception>
public CanMessage(uint id, CanMessageIdType identifierType, CanMessageFrameType frameType, byte[] message)
{
+ if (message != null && message.Length > 8)
+ {
+ throw new ArgumentOutOfRangeException(nameof(message), "CAN data payload cannot exceed 8 bytes.");
+ }
Id = id;
IdentifierType = identifierType;
FrameType = frameType;
Message = message;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@devices/Mcp25xxx/Can/CanMessage.cs` around lines 28 - 46, The XML docs on the
Message field claim an ArgumentOutOfRangeException is thrown for buffers >8
bytes but the CanMessage constructor doesn't enforce this; update the
constructor CanMessage(uint id, CanMessageIdType identifierType,
CanMessageFrameType frameType, byte[] message) to validate message != null and
message.Length <= 8 and throw ArgumentOutOfRangeException (with a clear message)
when exceeded, then assign Message; also correct the misspellings "lenght" →
"length" in the XML comments for Message and the exception tag (or remove the
exception tag if you prefer to keep validation elsewhere like
CanController.ReadDataMessage), ensuring callers cannot create invalid CAN
frames.
5 participants
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: