adafruit / Adafruit_CircuitPython_MCP2515

A CircuitPython library for working with the MCP2515 CAN bus controller
MIT License
21 stars 14 forks source link

Adds support for non-standard bitrates and timing register adjustments #4

Closed kdschlosser closed 3 years ago

kdschlosser commented 3 years ago

This update removes the use of hard coded bit timing registers in favor of calculating the registers at runtime. The benefit to this is it allows for any baud rate to be supplied that is within the interfaces ability. It also allows for tweaking of the timing registers to improve network stability and performance. Being able to tweak the timing registers allows for an interface that has a 16MHz clock to work on a network that has devices with 20MHz clocks. Being able to adjust the sample point and the synchronization jump width (SJW) is what makes this possible.

There is only a single api breaking change and that is the baud rate property no longer returns an integer, it returns a bitrate.Bitrate instance.

I have removed all of the code that did not pertain specifically to the MCP2515 interface, If at some point this library changes and supports more then one interface chipset family then the code for tht family can be added easily. So if a question gets raised about why something is done the way that it is, keep in mind that it can be added to.

siddacious commented 3 years ago

@kdschlosser I'll take a look at the code, however if you'd like to help things along in the meanwhile, you can work on getting the build passing by using black and pylint to clean up the code.

If needed, this guide is available to help: https://learn.adafruit.com/improve-your-code-with-pylint?view=all

siddacious commented 3 years ago

@kdschlosser I looked over briefly; Wow, great contribution! It'll take me a while to do it justice.

Please add one or more examples to the examples directory that show how to use the new code. The documentation in the library looks great, and the example in the constructor is great, however it's nice to have a single file easily available to test with a single copy to the device.

Additionally it will help with testing!

Thanks!

kdschlosser commented 3 years ago

OK I have run both black and pylint nd there are 0 changes being made to the files locally. I am not sure why the build is failing. If there is a more verbose option for the build it would be really helpful to be able to see what is exactly wrong so it can be fixed.

This is not helpful for determining what the problems are.

black....................................................................Failed
- hook id: black
- files were modified by this hook

reformatted /home/runner/work/Adafruit_CircuitPython_MCP2515/Adafruit_CircuitPython_MCP2515/adafruit_mcp2515/bitrate.py
reformatted /home/runner/work/Adafruit_CircuitPython_MCP2515/Adafruit_CircuitPython_MCP2515/examples/bitrate_enumeration_example.py
reformatted /home/runner/work/Adafruit_CircuitPython_MCP2515/Adafruit_CircuitPython_MCP2515/examples/bitrate_example.py
reformatted /home/runner/work/Adafruit_CircuitPython_MCP2515/Adafruit_CircuitPython_MCP2515/adafruit_mcp2515/__init__.py
All done! ✨ 🍰 ✨
4 files reformatted, 9 files left unchanged.

reuse....................................................................Passed
Check Yaml...............................................................Passed
Fix End of Files.........................................................Passed
Trim Trailing Whitespace.................................................Passed
kdschlosser commented 3 years ago

There are a couple of changes black is making one of the things it is changing it shouldn't be. It is changing it to something that is not pep8 compliant and to be honest makes code harder to read. it is changing this

bitrates = Bitrate.get_bitrates(
    333000, 
    Bitrate.TimingConstants.MCP251x16Const(), 
    calc_tolerance=1.5
)

to this

bitrates = Bitrate.get_bitrates(
    333000, Bitrate.TimingConstants.MCP251x16Const(), calc_tolerance=1.5
)

when this is the pep8 compliant version of what it is doing

bitrates = Bitrate.get_bitrates(
    333000, Bitrate.TimingConstants.MCP251x16Const(), calc_tolerance=1.5)

the other thing is black is changing things I did not write. so it is altering code that i did not contribut and it shouldn't be doing that.

it is also changing double quotes to single quotes which is not a big deal.

kdschlosser commented 3 years ago

IDK why it is failing, I reformatted it with black yet it is still failing on __init\.py.

tannewt commented 3 years ago

@kdschlosser It may have been a different version of black. The easiest way to deal with pre-commit checks is to install them locally: https://pre-commit.com/#installation