[Mcp1525] Added a CanController (read messages only)
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
- Fixes nanoFramework/Home#1600 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
- [x] Improvement (non-breaking change that improves a feature, code or algorithm)
- [] Bug fix (non-breaking change which fixes an issue with code or algorithm)
- [x] New feature (non-breaking change which adds functionality to code)
- [] Breaking change (fix or feature that would cause existing functionality to change)
- [] Config and build (change in the configuration and build system, has no impact on code or features)
- [] Dependencies (update dependencies and changes associated, has no impact on code or features)
- [] Unit Tests (add new Unit Test(s) or improved existing one(s), has no impact on code or features)
- [] Documentation (changes or updates in the documentation, has no impact on code or features)
Checklist:
- [x] My code follows the code style of this project (only if there are changes in source code).
- [] My changes require an update to the documentation (there are changes that require the docs website to be updated).
- [] I have updated the documentation accordingly (the changes require an update on the docs in this repo).
- [] I have read the CONTRIBUTING document.
- [] I have tested everything locally and all new and existing tests passed (only if there are changes in source code).
- [] I have added new tests to cover my changes.
Summary by CodeRabbit
-
Chores
- Updated several core framework dependencies to their latest versions for improved stability, performance, and compatibility across both main and sample configurations.
Walkthrough
This 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
| File(s) | Change Summary |
|---|---|
| devices/Mcp25xxx/.../packages.lock.json | Updated dependency versions and content hashes for: nanoFramework.CoreLibrary (1.15.5 β 1.16.11), nanoFramework.Runtime.Events (1.11.18 β 1.11.29), nanoFramework.System.Device.Gpio (1.1.41 β 1.1.53), nanoFramework.System.Device.Spi (1.3.52 β 1.3.73) |
Assessment against linked issues
| Objective | Addressed | Explanation |
|---|---|---|
| Create a CanController for the Mcp2515 and the SPI (#1600) | β | No implementation or modifications related to a CanController were added. |
Possibly related PRs
- nanoframework/nanoFramework.IoT.Device#1250: Updates the same dependencies (CoreLibrary, Runtime.Events, Gpio, and Spi) to new versions.
- nanoframework/nanoFramework.IoT.Device#1282: Contains identical dependency version updates in its packages.lock.json files.
- nanoframework/nanoFramework.IoT.Device#1265: Also updates the same dependencies with matching new version specifications.
Suggested labels
CI: Publish Release, Type: dependencies
π Recent review details
Configuration used: CodeRabbit UI Review profile: CHILL Plan: Pro
π₯ Commits
Reviewing files that changed from the base of the PR and between 226d11a579b81ecdb9425cfdc834ff0abab8fb32 and 77599d55f23d9e04e78f37ab65dd8917ca93e293.
β Files ignored due to path filters (13)
-
devices/Mcp25xxx/Can/CanController.csis excluded by none and included by none -
devices/Mcp25xxx/Can/CanEnums.csis excluded by none and included by none -
devices/Mcp25xxx/Can/CanMessage.csis excluded by none and included by none -
devices/Mcp25xxx/Can/CanMessageFrameType.csis excluded by none and included by none -
devices/Mcp25xxx/Can/CanMessageIdType.csis excluded by none and included by none -
devices/Mcp25xxx/Can/ICanController.csis excluded by none and included by none -
devices/Mcp25xxx/Mcp25xxx.csis excluded by none and included by none -
devices/Mcp25xxx/Mcp25xxx.nfprojis excluded by none and included by none -
devices/Mcp25xxx/Mcp25xxx.slnis excluded by none and included by none -
devices/Mcp25xxx/Register/Interrupt/CanIntF.csis excluded by none and included by none -
devices/Mcp25xxx/packages.configis excluded by none and included by none -
devices/Mcp25xxx/samples/Mcp25xxx.Samples.nfprojis excluded by none and included by none -
devices/Mcp25xxx/samples/packages.configis excluded by none and included by none
π Files selected for processing (2)
-
devices/Mcp25xxx/packages.lock.json(1 hunks) -
devices/Mcp25xxx/samples/packages.lock.json(1 hunks)
π Additional comments (9)
devices/Mcp25xxx/samples/packages.lock.json (4)
7-9: Dependency Update: nanoFramework.CoreLibrary Version BumpThe "requested" and "resolved" versions have been updated to "1.16.11," and the content hash has been updated accordingly. This appears to be a straightforward upgradeβjust ensure that all components using this library remain compatible.
13-15: Dependency Update: nanoFramework.Runtime.Events Version BumpThe version for nanoFramework.Runtime.Events has been updated to "1.11.29" with a new content hash. Verify that any event-related functionality continues to operate as expected with this updated version.
19-21: Dependency Update: nanoFramework.System.Device.Gpio Version BumpThe "requested" and "resolved" fields for the Gpio dependency are now set to "1.1.53" with an updated content hash. Please double-check that any GPIO-related hardware interactions are validated under the new version.
25-27: Dependency Update: nanoFramework.System.Device.Spi Version BumpThe SPI dependency has been updated to "1.3.73," and the corresponding content hash has been changed. Ensure this update is tested thoroughly, particularly since SPI is critical for communication with devices like the Mcp2515.
devices/Mcp25xxx/packages.lock.json (5)
7-9: Dependency Update: nanoFramework.CoreLibrary Version BumpThe dependency details for nanoFramework.CoreLibrary now reflect version "1.16.11" along with an updated content hash. This aligns with the changes seen in the samples file. Consistency across files is maintained.
13-15: Dependency Update: nanoFramework.Runtime.Events Version BumpThe version update to "1.11.29" for nanoFramework.Runtime.Events is clearly applied and consistent. Please verify that all event handling functionality expecting the previous version works as intended with the update.
19-21: Dependency Update: nanoFramework.System.Device.Gpio Version BumpHere, the Gpio dependency now updates to "1.1.53" with a new content hash. This update is in line with the corresponding sample file and should be validated against the dependent code.
25-27: Dependency Update: nanoFramework.System.Device.Spi Version BumpThe update for nanoFramework.System.Device.Spi to version "1.3.73" with the new content hash is correctly applied. Since SPI functionality is essential for interfacing with the Mcp2515, thorough testing is recommended.
29-33: Unchanged Dependency: Nerdbank.GitVersioningThe Nerdbank.GitVersioning dependency remains at version "3.7.112," which is expected as it was not part of this upgrade. Verify that its unchanged state does not conflict with the updated dependencies.
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?
πͺ§ Tips
Chat
There are 3 ways to chat with CodeRabbit:
- Review comments: Directly reply to a review comment made by CodeRabbit. Example:
-
I pushed a fix in commit <commit_id>, please review it. -
Generate unit testing code for this file. -
Open a follow-up GitHub issue for this discussion.
-
- Files and specific lines of code (under the "Files changed" tab): Tag
@coderabbitaiin a new review comment at the desired location with your query. Examples:-
@coderabbitai generate unit testing code for this file. -
@coderabbitai modularize this function.
-
- PR comments: Tag
@coderabbitaiin a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:-
@coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase. -
@coderabbitai read src/utils.ts and generate unit testing code. -
@coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format. -
@coderabbitai help me debug CodeRabbit configuration file.
-
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)
-
@coderabbitai pauseto pause the reviews on a PR. -
@coderabbitai resumeto resume the paused reviews. -
@coderabbitai reviewto trigger an incremental review. This is useful when automatic reviews are disabled for the repository. -
@coderabbitai full reviewto do a full review from scratch and review all the files again. -
@coderabbitai summaryto regenerate the summary of the PR. -
@coderabbitai generate docstringsto generate docstrings for this PR. (Beta) -
@coderabbitai resolveresolve all the CodeRabbit review comments. -
@coderabbitai configurationto show the current CodeRabbit configuration for the repository. -
@coderabbitai helpto get help.
Other keywords and placeholders
- Add
@coderabbitai ignoreanywhere in the PR description to prevent this PR from being reviewed. - Add
@coderabbitai summaryto generate the high-level summary at a specific location in the PR description. - Add
@coderabbitaianywhere in the PR title to generate the title automatically.
CodeRabbit Configuration File (.coderabbit.yaml)
- You can programmatically configure CodeRabbit by adding a
.coderabbit.yamlfile to the root of your repository. - Please see the configuration documentation for more information.
- If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation:
# yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json
Documentation and Community
- Visit our Documentation for detailed information on how to use CodeRabbit.
- Join our Discord Community to get help, request features, and share feedback.
- Follow us on X/Twitter for updates and announcements.
@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:
- The main additions in this PR is to create a copy of the nanoFramework.Device.Can code in this project with a different namespace and a 100% .NET implementation. The nanoFramework.Device.Can library requires a native component that uses the CAN-features of the microcontroller.
- Rik and I were comparing notes, and we thought that someone who is using the MCP2515 is probably using that as an alternative for a microcontroller-based CAN implementation, or to have an additional CAN bus.
- Unless you are creating your own PCB with microcontroller and MCP2515 chip and you use a microcontroller-board and MCP2515-board instead, the MCP25xx library is overly complicated. The MCP2515-boards all look the same
and expose only a few of the ports of the MCP2515 chip: SPI for data communication and INT to signal a message has been received. The Mcp25xx library supports connecting to all pins of the MCP2515 and lacks the INT-to-event code. - The Mcp25xx library is also a bit more complicated because it is quite low-level. The more user-friendly CAN layer added by Rik only covers the MCP2515, because:
- There are other models (e.g., MCP2518) that support more advanced features like CAN-FD, which is not supported by nanoFramework.Device.Can (and also not by ESP32) . The MCP2518 chips and boards
are more than five times as expensive as the MCP2515-boards, so you would probably buy a MCP2518 only if you want to use the extra features for which nanoFramework has no support.
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:
- If Rik would create a MCP2515-variant of nanoFramework.Device.Can with the same namespace but an implementation specific for the MCP2515, it could be used as a drop-in replacement for the native nanoFramework.Device.Can library. As this is MCP2515-specific (not for all Mcp25xx), it would seem to be a better idea to create this as a nanoFramework.Device.Can.MCP2515 library than as an addition to the Mcp25xx library.
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:
- Can't we split the current nanoFramework.Device.Can into two libraries: one, e.g, nanoFramework.Device.Can.Core that has shared interfaces/classes, and one nanoFramework.Device.Can.Native for the implementation? Rik could then create a nanoFramework.Device.Can.Mcp2515.
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:
- Can't we split the current nanoFramework.Device.Can into multiple libraries: one, e.g, nanoFramework.Device.Can.Core that has shared interfaces/classes, and one nanoFramework.Device.Can.device per device. Rik could then create a nanoFramework.Device.Can.Mcp2515. The current C# nanoFramework.Device.Can library would be renamed nanoFramework.Device.Can.STM32. And I would create the nanoFramework.Device.Can.ESP32.
(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:
- A library that is independent of hardware called "Core". So, defining an interface and functions that can be used regardless of the hardware. Something similar that I did for the Azure lib to work both on a system with wifi/ethernet connection and also on an AT Modem. It went up to having to split the MQTT lib into a core element that was common with both side. Reason I called that lib "Core" is mainly because that's the pattern we've been following in those cases (at least tried to).
- Specific libraries for different platform when there is a native support. This will use the "Core" library
- We can also add a generic implementation in managed code if possible independent of the hardware, that one we can call it "Managed" to also try to follow the pattern.
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", the new "Iot.Device.Mcp2515.CanController" should use the "nanoFramework.CAN.ICanController" and "nanoFramework.CAN.CanMessage".
Or should it be "nanoFramework.Device.Can.MCP2515" like @frobijn suggests?
- "nanoFramework.Device.Can" will be removed.
- "nanoFramework.Device.Can.ESP32" will be using "nanoFramework.CAN"
- "nanoFramework.Device.Can.STM32" (currently "nanoFramework.Device.Can") and using "nanoFramework.CAN".
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:
- "nanoFramework.Device.OneWire"
- No more "nanoFramework.Device.Can"
- "nanoFramework.Device.Bluetooth" ?
Does the "Iot" namespace, instead of "nanoFramework" have a historical reason? From the time it was copied?
- yes for the idea of the Core nuget and the specific ones.
- namespace wise, yes, we can use
nanoFramework.Device.Canto align with the rest. - for the sub devices, it's ok to use the same namespace except if things are very specific. It's actually better to just use the Can namespace as root.
- You can drop the Iot part of the namespace, it's just a convention for anything that is binding related but as for this case, we do have exception to make things more integrated.
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 Can in the namespace (not CAN). π
@frobijn yes, it makes sense to have all these grouped in the same repo. For tests, integration and productivity helper.
And all nanoFramework.Device.CAN libraries (.Core .Stm32 .Esp32 .Mcp2515) will be part of the nanoFramework.Device.Can github repo, right?
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?
Should nanoFramework.Device.Can.Mcp2515 use the IoT.Device.Mcp25xx library?
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.
- yes for the idea of the Core nuget and the specific ones.
- namespace wise, yes, we can use
nanoFramework.Device.Canto align with the rest. - for the sub devices, it's ok to use the same namespace except if things are very specific. It's actually better to just use the Can namespace as root.
So Frank and I came up with the following context diagram:
Is this something we can agree on?
I'm good with that. @Ellerbach ?
@rikkreeftenberg @frobijn and @josesimoes I'm all good with the proposition! Thanks a lot for the collaborative and productive work!
The setup of the new repository structure may not be so easy... see PR. Let's continue the discussion in that PR.