-
Notifications
You must be signed in to change notification settings - Fork 49
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MCP251XFD CAN bus examples #467
base: main
Are you sure you want to change the base?
MCP251XFD CAN bus examples #467
Conversation
…as te last weeks work got lost when my WSL was corrupted.
… WSL's virtual disk went bye-bye so I need to redo that. Once that is working we need to get all of this into calling interface.h and MCP251XFD.hh and not having to include the *.cc files.
Finally, getting back some of that code we lost earlier. Also, it's a beautuiful, sunny day.
…nger have to include additional CC files. There are isseus with this but it does work. Issues that should be resolved going forwards: 1. Many static functions are now public 2. Some many #define statements that could be replaced with constexpr (apparantly that is a thing) and are never needed outside the namespace 3. should convert the MCP251XFD struct to a class, move the functions in as methods and make huge amounts of it private. 4. Once it's all in a class, we can drop the "MCP251XFD_" from everything because it will al be in a class called MCP251XFD in a namespace called MCP251XFD.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Some initial comments at a glance. Longer review still to come.
Please don't commit Windows' :Zone.Identifier
files.
uint16_t txCnt = 0; | ||
uint16_t rxCnt = 0; | ||
uint8_t dummy; | ||
while(rxCnt < len) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be nicer if this were IRQ-driven instead, if it can be, especially if the SPI is operating at low clock speeds. We should probably do the same with the UART driver, as another example...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I just combined the existing Tx and Rx examples. I've not got long left on this project but I may have time.
The result is XORed with 0x0000 | ||
http://reveng.sourceforge.net/crc-catalogue/16.htm | ||
|
||
History : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
License?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. It was part of the original driver that I adapted so I guess it's the same licence? I'm not quite ssure what the rules about changing a licence are.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You cannot change the license or remove copyright headers from code that you do not own.
It looks as if this is the original home: https://github.com/mommsen/evb/tree/master
This code comes with no license and so cannot be used for any purpose without express consent from the copyright owner (CERN?).
The page includes a disclaimer of liability but no license. It has an email address for the author at the bottom, so might be worth pinging the, and asking if we can use them under an MIT license? Otherwise this code would need rewriting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got the CRC from here and moved all the code into the header to make it easier to include with a single line: https://github.com/Emandhal/MCP251XFD/tree/master/CRC
This repo was licensed under MIT but it is possible that code was taken from elsewhere. I will message the author of the driver and ask about the license.
* @param[in] size Is the size of the pointed byte stream | ||
* @return The CRC computed | ||
*/ | ||
uint16_t ComputeCRC16CMS(const uint8_t* data, uint32_t size) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very minor: if you make this (and the tableless counterpart below) take the initial crc
value (and not do the "final xor" below, even though it's all zeros), then it can be used for streaming updates as well as whole-buffer operations, too.
It may be worth looking at the blocking_write() function too - since that appears to work the same way I'd originally done it (I didn't write that bit so I'm not sure why they wrote it that way fro a reason).
General Description
This is a generic MCP251XFD driver. The MCP2517FD and MCP2518FD are SPI to CAN drivers that support CAN2.0 and CAN FD. The driver is largely based on Emandhal's Generic CAN251XFD Driver and we have left the license intact.
The example is designed to run on the Sonata board, however, this is a generic driver with all the hardware specific changes located in one single file.
CAN Devices Supported
Code Layout
sdk/boards/include/driver/MCP251XFD
This is the generic driver. We've altered the code so that it is all based in header files so that you don't need to add any extra C to the
xmake.lua
file when building. The code includes a CRC in a sub folder. We've not tested this yet but the device does include an option to add CRC to all SPI messages.examples/12.CAN_driver/
Sonata specific: This is an implementation of the MCP251XFD driver. The sonata specific interface code is found in
/examples/12.CAN_driver/driver/interface.cc
and it merely defines some hardware specific SPI and timing code.examples/11.basic_spi
Sonata specific: This example shows how to read and write on the Sonata's SPI interface. It shows the use of the bi-directional SPI transfer function that we added. Bi-directional transfer is often used in SPI drivers so it makes sense to add support for it.
sdk/include/platform/sunburst/platform-spi.hh
Added a bi-directional blocking SPI transfer method (
void blocking_transfer(const uint8_t txData[], uint8_t rxData[], uint16_t len)
) as the driver (and many other SPI based drivers) expect a bi-directional transfer.We also added another version of chip_select_assert() that could take the Chip Select index as a parameter as it is not always possible to specify which chip select you will be using ahead of time. I'm sure that there is a way of implementing this driver without the need for this additional method but I'm running out of time on this project.
Items to be Tested
Suggested Improvements
struct MCP251XFD
could be converted to a class allowing us to have private and public methods and allowing us to no longer need to have to keep passing thestruct
around.#define
statements that could becomeconstexpr
. Many of these could safely be private to the class.MCP251XFD_
prefix (and occasional postfix) from everything.examples
folder. Maybe a hardware specific subfolder?