- 
                Notifications
    
You must be signed in to change notification settings  - Fork 8.2k
 
drivers: can: mcp2515: Add driver for MCP2515 CAN controller #8304
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
Conversation
| 
           @alexanderwachter Some notes on this driver: 
  | 
    
          Codecov Report
 @@            Coverage Diff             @@
##           master    #8304      +/-   ##
==========================================
- Coverage   52.93%   52.92%   -0.02%     
==========================================
  Files         309      309              
  Lines       45268    45268              
  Branches    10451    10451              
==========================================
- Hits        23961    23956       -5     
- Misses      16542    16544       +2     
- Partials     4765     4768       +3
 Continue to review full report at Codecov. 
  | 
    
| 
           @karstenkoenig The timing should be in in the device-tree to have the ability to set different speeds on multiple CAN controllers. Moreover for various boards with different clock speeds its annoying to change the Kconfig for every board. For the filtering you can maybe use a clever masking to reduce the number of interrupts.  | 
    
| 
           Having CAN timing information in device tree files is straightforward for on MCU devices, as you can just toss the information into the existing board files, but right now for 'external peripherals' it is harder to provide a straight forward sample until dts overlays/shields are enabled. Most sensors etc all still have that sort of information in KConfig files I am not really sure under which circumstance the stm will feedback tx errors now, won't it also try forever? Or will it pretty much report the error and still have transmitted the message? The XOR filter idea is really nice, but to configure the filters on the MCP2515 it needs to be put out of operation into configuration mode, so everytime the filters get changed it needs to be recalculated and applied while stopping actual operation, so it might also create unexpected behavior.  | 
    
| 
           Now I get your point with the device-tree. I think it's ok for the external controller to have this in Kconfig. There is no error feedback. The controller will try sending forever as long as there is no ACK . I think that is the behaviour recommended by the ISO standard. The API for reading the bus states (Error active, Error passive and bus off) is still pending. Also recovery from bus off is not possible with the current API. Could you use the Acceptance Filter as they are and just "or" the masking together? In my opinion it is sufficient ho have only six filter banks. The stm32 driver stops operation of the filters too for the short period of changing them. Not using the hardware filtering at all will result in a heavy CPU load depending on the overall bus load.  | 
    
        
          
                drivers/can/mcp2515.c
              
                Outdated
          
        
      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.
form line 16 to here, you could move that to a dedicated header file.
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.
Done
        
          
                drivers/can/mcp2515.c
              
                Outdated
          
        
      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.
About all the above spi related function: would there be a way to make it in one function? (just asking, and if you prefer that way, just keep it.)
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 thought about it, but the methods are quite different and I don't want to throw distinguishing it to the callers. I renamed them with cmd to indicate they are doing remote calls on the mcp2515.
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.
ok
        
          
                drivers/can/mcp2515.c
              
                Outdated
          
        
      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.
rewrite that:
u8_t opcode_buf = { ..., ... };
Apply that comment everywhere. (you did it properly in soft-reset function above actually)
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.
Done
        
          
                drivers/can/mcp2515.c
              
                Outdated
          
        
      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.
0x07, 5: could you create macro to these with relevant names?
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 tried to do my best, there is currently 3 functions left where it is still using unnamed bit positions and masks
mcp2515_convert_can_msg_to_mcp2515_frame
mcp2515_convert_mcp2515_frame_to_can_msg
mcp2515_configure
They won't really become easier to read nor to follow, as you need to look at the datasheet if you want to know the details, and then it is easier to check for correctness when having the bare positions spilled out instead of double checking defines.
I can change the rest too if it is policy or just frowned upon.
        
          
                drivers/can/mcp2515.c
              
                Outdated
          
        
      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.
Never declare a variable in the middle of a code block, always a the beginning of it. Apply that everywhere.
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.
Done
        
          
                drivers/can/mcp2515.c
              
                Outdated
          
        
      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.
add an empty line after '}' unless it's another } etc... Apply that everywhere.
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.
Done
        
          
                drivers/can/mcp2515.c
              
                Outdated
          
        
      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.
too many if imbrications.
I guess you could reverse that test and use continue, to reduce indentation of the lines below.
If necessary, it's also a good idea to create an inline function to de-clutter the code.
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.
Done
        
          
                drivers/can/mcp2515.c
              
                Outdated
          
        
      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.
For all of these sem/mutex maybe you could try using K_SEM_INITIALIZER()/K_MUTEX_INITIALIZER(). It will generate a little less code then. (and it's faster to initialize).
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.
These macros are flagged internal, does that mean I can use them in kernel drivers or is that just for really core components? Use throughout the rest of the tree seems to lean towards k_sem_init/k_mutex_init style initialization.
I more dislike the general amount of mutexes and semaphores, but couldn't come up with a smaller amount
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's ok to use these macros in native drivers, not in application.
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.
Ok, I changed the initializers to be in the mcp2515_data_1 initializer, thanks!
        
          
                samples/can/prj.conf.mcp2515
              
                Outdated
          
        
      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 could split that into another patch.
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.
Done
ff8325b    to
    b7dfbd1      
    Compare
  
    | 
           Thanks for the great feedback so far! I tried incorporating everything from @tbursztyka and updated the pull request.  | 
    
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.
just some minor comments, rest is fine to me.
        
          
                drivers/can/mcp2515.c
              
                Outdated
          
        
      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.
tiny alignement issue (should be below last '(' + 1 space)
apply that rule everywhere (I probably did not notice it the first time)
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.
Sorry I have to double check here, as the first time I did it checkpatch complained about space instead of tabs
You want any line breaks aligned under and after the last current open bracket right? It doesn't have to be exactly 1 behind the last open bracket, right? Anyways, I did just that and checkpatch didn't complain (but keeps seeing lines longer than 80 chars that aren't, does it tab extend to 8 space?)
        
          
                drivers/can/mcp2515.c
              
                Outdated
          
        
      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.
does that fit the 80chars limit?
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.
Looks good. Some minor comments.
        
          
                drivers/can/Kconfig
              
                Outdated
          
        
      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.
This option is specific to driver implementations and therefore should be kept in Kconfig.stm32 and Kconfig.mcp2515. Also the maximum in your driver is 32?
        
          
                drivers/can/Kconfig.mcp2515
              
                Outdated
          
        
      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.
Do you want to merge CAN_PROP_SEG and CAN_PHASE_SEG1? There is no point to keep them separate because it wont change the sampling point.
I don't mind to keep them separate but in device-tree they are merged.
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 wanted to avoid doing any unexpected math, and there is hard limits on the values, but I think you are right, I could just do the following to allow for full range and stay in the bounds, will keep it in mind when switching to dts proper
tq_prop = tq_prop_bs1 / 2
tq_bs = (tq_prop_bs1 / 2) + (tq_prop_bs1 % 2)
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 had
tq_bs1 = (tq_prop_bs1 -1) & 0x07
tq_prop = tq_prop_bs1 - tq_bs1 - 2
in mind but its the same :-)
But I think you can keep it separate anyway.
        
          
                drivers/can/Kconfig.mcp2515
              
                Outdated
          
        
      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.
this should be range 2 8
"PS1 is programmable from 1 – 8 TQ and PS2 is
programmable from 2 – 8 TQ" [MCP2515 datasheet]
        
          
                drivers/can/Kconfig.mcp2515
              
                Outdated
          
        
      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.
Wouldn't it be better to call this INT Pin?
        
          
                drivers/can/mcp2515.c
              
                Outdated
          
        
      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.
Could you write
u8_t cmd_buf[] = { MCP2515_OPCODE_BIT_MODIFY, reg_addr, mask, data };
        
          
                drivers/can/mcp2515.c
              
                Outdated
          
        
      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.
and
.buf = cmd_buf, .len = sizeof (cmd_buf),
        
          
                drivers/can/mcp2515.c
              
                Outdated
          
        
      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 would prefer sizeof (config_buf) instead of just 4
        
          
                drivers/can/mcp2515.c
              
                Outdated
          
        
      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 could use ARRAY_SIZE(tx_buf) here
| 
           Thanks for all the reviews and sorry for silently pushing, took it a bit slow. @alexanderwachter As there is only two filter masks on the mcp2515 the actual filtering will be very limited. My worry with random or'ing the masks together will be unintended filters such as worst cases ending up with 0 masks that won't filter. What do you think would be a decent strategy that allows application developers to pick sensible filters and leverage the limited filtering the best way using the current can interface? 
 I prefer either 1. or 2., as they are the easiest to explain what is happening and don't try to paper over limitations. Should I open a discussion issue ticket for this? Might be a better place then drag it out in a pull request  | 
    
| 
           @karstenkoenig I have to study the data sheet an think about the filters.  | 
    
| 
           @karstenkoenig Please have a look at https://github.com/alexanderwachter/zephyr/tree/can_extend_api  | 
    
| 
           @alexanderwachter On the bus error conditions: The get_state I'd rather name get_error_state. Right now there is no 'disable CAN device' function but there might be a future and that is properly the more relevant 'state'. Also do you specifically need the recover method? Couldn't you just configure it to auto-recover? I'd rather not expose device features until there is a use case for it, because that will make sure it at least gets used and tested and I can't see why I wouldn't want my CAN controller to auto recover for the most time.  | 
    
| 
           @karstenkoenig I think 1) should the preferred solution or a Kconfig option to switch from your actual solution to 1) or just leave it as it is now. On the bus-state API: I will change get_state to get_bus_state.  | 
    
| 
           Hi, I'd be willing to help, at least with testing.  | 
    
| 
           Sure.  | 
    
        
          
                drivers/can/mcp2515.c
              
                Outdated
          
        
      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 should use the new logger as the syslog is deprecated.
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.
Done, thanks
6c391db    to
    046eab9      
    Compare
  
    eaeb6f5    to
    60e2785      
    Compare
  
    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.
Just some nits.
d09e9c8    to
    462ca4f      
    Compare
  
    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.
Nits fixed.
| 
           @dwagenk apparently @alexanderwachter wrote some api test in tests/drivers/can/api, can you try running these as well please? Sorry I still don't have my stuff with me :-(  | 
    
462ca4f    to
    4696554      
    Compare
  
    | 
           Finally got to try out the current state of this PR, but didn't get it to work yet. It fails in   | 
    
| 
           I have access to my stuff again and fixed the driver. @dwagenk you are right and probably ran into SPI chipselect issues, I made a mistake with the devicetree connection. Fixed it and provided an overlay dts file in the sample project for reference. To be honest the interworking of different parts of devicetree/kconfig etc are kind of hard to follow.... the cs-gpios dts struct has to be part of the spi bus and not the bus member, and when mapped to defines in the build process they lose the plural s and end up being CS_GPIO_xyz etc :-( Anyways, I also ran the CAN api testsuite and it passes  | 
    
| 
           @karstenkoenig great that you picked it back up and have your Hardware for testing. I'll keep trying it out and hope it helps if I just document my issues. Iwill comment directly on the files causing issues for me.  | 
    
| 
           DeviceTree is so beautiful and so utterly frustrating at the same time...  | 
    
          Codecov Report
 @@           Coverage Diff           @@
##           master    #8304   +/-   ##
=======================================
  Coverage   52.87%   52.87%           
=======================================
  Files         309      309           
  Lines       45267    45267           
  Branches    10451    10451           
=======================================
  Hits        23934    23934           
  Misses      16558    16558           
  Partials     4775     4775Continue to review full report at Codecov. 
  | 
    
| 
           @karstenkoenig that fixed it! It compiles and runs the CAN sample in loopback mode now (stm32_min_dev board with external button). I'd propose merging it now and then doing little improvements later. LGTM  | 
    
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.
suggested changes for a follow-up PR
| 
               | 
          ||
| if CAN_MCP2515 | ||
| 
               | 
          ||
| config CAN_MCP2515_OSC_FREQ | 
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 guess these could/should go into device tree as well?
- CAN_MCP2515_OSC_FREQ
 - CAN_PROP_SEG
 - CAN_PHASE_SEG1
 - CAN_PHASE_SEG2
 - CAN_SJW
 
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.
Hm yes, the segment length ones are sort of defined already in the can devicetree, but seg1 and prop are merged together there but the mcp2515 allows specifing them independently. I'll try sorting out again why I didn't just use it before
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.
@karstenkoenig it should be save to merge CAN_PROP_SEG and CAN_PHASE_SEG1. Even the CAN standard says so. I have no idea why this is separate in the mcp2515. They have the same effect.
| /* CNF3, CNF2, CNF1, CANINTE */ | ||
| u8_t config_buf[4]; | ||
| 
               | 
          ||
| if (bitrate == 0) { | 
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.
Maybe we should make it more explicit somewhere, that bus-speed confgirued in device-tree will be ignored, if can_configure is called with bitrate != 0 ?
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.
Indeed. This should be added in the API documentation.
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.
haha actually I only found this because the api test tripped there and added it in after double checking with the stm32 can drivers :-)
| CONFIG_CAN_1=y | ||
| CONFIG_CAN_STM32=n | ||
| CONFIG_CAN_MCP2515=y | ||
| CONFIG_GPIO_LED_DEV="GPIOB" | 
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.
already discussed earlier but noted again here as a to do for later:
use led0 and sw0 from the board's device tree.
| status = "ok"; | ||
| cs-gpios = <&gpioa 4 GPIO_DIR_OUT>; | ||
| mcp2515@0 { | ||
| compatible = "microchip,mcp2515"; | 
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.
If we give it a label properties can be changed in additional (e.g. application specific) overlay files.
&spi1 {
	/* ... */
        mcp2515_can1: mcp2515@0 {
		/* ... */
	};
};
/* in different file, overlay or position: */
&mcp2515_can1 {
	bus-speed = <250000>;
};
| 
           @dwagenk Nice! Glad to hear it works for you as well!  | 
    
| 
           @karstenkoenig @dwagenk next big change is #13262  | 
    
The MCP2515 is a CAN controller that can be connected via SPI to an host MCU. This driver adds support for the MCP2515 as a new driver in the CAN subsystem. As it is a SPI peripheral it uses a thread for its interrupt handling and the received message filtering is done inside this interrupt thread, as the MCP2515 filter capabilities are not sufficient for the Zephyr CAN interface. The driver was validated with an external CAN logger and the adjusted CAN sample application. Signed-off-by: Karsten Koenig <[email protected]>
Adjusted the MCP2515 driver to switch from KConfig SPI configuration to DTS based configuration. Signed-off-by: Karsten Koenig <[email protected]>
Project configuration to run the CAN sample with the MCP2515 attached via SPI as the CAN controller. Signed-off-by: Karsten Koenig <[email protected]>
Fixed using chipselect with seperate chipselect GPIOs and how they were referenced from/in DeviceTree. Also configure the device during initialization so it's ready to go after init. Signed-off-by: Karsten Koenig <[email protected]>
The MCP2515 is a CAN controller that can be connected via SPI to an
host MCU. This driver adds support for the MCP2515 as a new driver in
the CAN subsystem.
As it is a SPI peripheral it uses a thread for its interrupt
handling and the received message filtering is done inside this
interrupt thread, as the MCP2515 filter capabilities are not sufficient
for the Zephyr CAN interface.
The driver was validated with an external CAN logger and the adjusted
CAN sample application.
Signed-off-by: Karsten Koenig [email protected]