autowp / arduino-mcp2515

Arduino MCP2515 CAN interface library
MIT License
795 stars 279 forks source link

A default for oscillator frequency can be determined by looking at F_CPU #20

Open leres opened 5 years ago

leres commented 5 years ago

I'm using something similar to:

#if F_CPU == 20000000UL
#define MCP_SPEED MCP_20MHZ
#elif F_CPU == 16000000UL
#define MCP_SPEED MCP_16MHZ
#elif F_CPU == 8000000UL
#define MCP_SPEED MCP_8MHZ
#else
#error "Unsupported F_CPU value"
#endif
[...]
mcp2515.setBitrate(CAN_500KBPS, MCP_SPEED);

But it wouldn't be difficult to change the default speed from 16MHz to a speed based on the arduino clock. I'm happy to make the changes and issue a pull request if this is interesting.

Also you could free up some program memory by changing setBitrate() to conditionally compiling the canClock cases.

autowp commented 5 years ago

Conditionally compiling sounds reasonable but breaks back compatibility. Maybe a new version?

About clocks. Same Arduino device may work with different clocked mcp's

leres commented 5 years ago

What I was thinking that by default the single argument version of setBitrate() would assume canSpeed is the same as F_CPU:

--- a/mcp2515.cpp
+++ b/mcp2515.cpp
@@ -185,7 +185,18 @@ MCP2515::ERROR MCP2515::setMode(const CANCTRL_REQOP_MODE mode)

 MCP2515::ERROR MCP2515::setBitrate(const CAN_SPEED canSpeed)
 {
-    return setBitrate(canSpeed, MCP_16MHZ);
+       /* Assume F_CPU is the same as canClock by default */
+#if F_CPU == 20000000UL
+#define MCP2515_SPEED MCP_20MHZ
+#elif F_CPU == 16000000UL
+#define MCP2515_SPEED MCP_16MHZ
+#elif F_CPU == 8000000UL
+#define MCP2515_SPEED MCP_8MHZ
+#else
+#error "Unsupported F_CPU value"
+#endif
+
+    return setBitrate(canSpeed, MCP2515_SPEED);
 }

 MCP2515::ERROR MCP2515::setBitrate(const CAN_SPEED canSpeed, CAN_CLOCK canClock)=

I guess that would break any sketches that use a 16 MHz canclock and and a non-16 MHz arduino clock (F_CPU set to something other than the default 16 MHz).

I see your point about conditionally compiling the canClock cases.

How about providing a convenience define?

--- a/mcp2515.h
+++ b/mcp2515.h
@@ -169,6 +169,15 @@
 #define MCP_20MHz_33k3BPS_CFG2 (0xFF)
 #define MCP_20MHz_33k3BPS_CFG3 (0x87)

+/* Define MCP_SPEED as a canClock based on F_CPU */
+#if F_CPU == 20000000UL
+#define MCP_SPEED MCP_20MHZ
+#elif F_CPU == 16000000UL
+#define MCP_SPEED MCP_16MHZ
+#elif F_CPU == 8000000UL
+#define MCP_SPEED MCP_8MHZ
+#endif
+
 enum CAN_CLOCK {
     MCP_20MHZ,
     MCP_16MHZ,

Then the user could just use:

    setBitrate(canSpeed, MCP_SPEED);

when the MCP2515 and arduino use the same clock?