Seeed-Studio / Seeed_Arduino_CAN

Seeed Arduino CAN-BUS library - MCP2518FD&MCP2515&MCP2551
MIT License
826 stars 440 forks source link

Examples Do Not Compile #138

Open devindumont opened 1 year ago

devindumont commented 1 year ago

The following examples do not compile: send send_Blink send_receive send_receiveFD send_sleep sendFD

Compiler Error: no matching function for call to 'mcp2515_can::sendMsgBuf(int, int, int, unsigned char [8])'

Hardware: Arduino Uno Arduino Nano

Software: Arduino IDE v1.8.19

Opinion: This library seems to be the most commonly used on help forums and blogs and has healthy responses to issues and pull requests. It seems to be the best library for a new programmer to get started with. However, the basic usage examples do not compile out of the box making it extremely difficult on most Arduino users. The simpler examples do compile with an older version of the library. These examples need to be revised so that a basic sketch with only the bare minimal code compiles.

nilsfast commented 1 year ago

Seems like pull #136 deprecated the sendMsgBuf function that takes four arguments (the ones called in the example sketches) which results in the error you were getting.

Solution Modify your function call as follows: sendMsgBuf(arg1, arg2, 0, arg3, arg4, true); to achieve the behaviour of the four-argument version. Note values 0 and true inserted after argument two and four.

techie66 commented 1 year ago

It doesn't look like that was the intent of #136. It's probably worth looking at if that PR should have been handled different. It's causing me problems now as I update my version of the library.

Edit: An additional workaround is to call the function as MCP_CAN::sendMsgBuf(arg1, arg2, arg3, arg4)

m516 commented 1 year ago

Hello, I'm the one to blame for writing #136, and it probably should have been handled differently. I didn't mean to override or mask any existing four-argument sendMsgBuf function, and deprecating any existing features was certainly not my goal, so I apologize for the bad PR and for the trouble it caused.

I saw three CAN implementations in this library: mcp2518fd_can, mcp2515_can, and can-serial. Each implementation had a six-parameter sendMsgBuf implementation, but only mcp2518fd_can and mcp2515_can wrapped that six-parameter function into a four-parameter version. Both assumed rtr is zero and wait_sent is true.

The following code was in both impementations:

/* wrapper */
    inline byte sendMsgBuf(unsigned long id, byte ext, byte len, const byte *buf) {
        return sendMsgBuf(id, ext, 0, len, buf, true);
    }

My goal for #136 was to isolate redundant code and make this four-parameter wrapper available to every implementation of MCP_CAN, but I failed and broke existing codebases.

139 fixes all the examples by replacing every instance of CAN.sendMsgBuf(0x00, 0, 8, stmp); with CAN.MCP_CAN::sendMsgBuf(0x00, 0, 8, stmp);, but it doesn't fix any external codebases.

I propose to make a new PR where every implementation includes using MCP_CAN::sendMsgBuf, explicitly unmasking the method so it's available again.

For example:

class mcp2518fd : public MCP_CAN {
public:

  ...

  virtual byte trySendMsgBuf(unsigned long id, byte ext, byte rtr, byte dlc,
                             const byte *buf, byte iTxBuf = 0xff);
  virtual byte sendMsgBuf(byte status, unsigned long id, byte ext, byte rtr,
                          byte dlc, volatile const byte *buf);
  virtual byte sendMsgBuf(unsigned long id, byte ext, byte rtr, byte dlc,
                          const byte *buf, bool wait_sent = true);
  using MCP_CAN::sendMsgBuf;

  ...

}

Initial testing looks promising, but I'll wait a few days for comments/concerns here before submitting a PR in case there are any glaring issues I'm forgetting about. PRs in this repo tend to be accepted quickly without much internal testing.

techie66 commented 1 year ago

That's exactly what I would do. using MCP_CAN::sendMsgBuf; is the right approach as it doesn't require code changes anywhere else and achieves the goal of only defining the wrapper in one place.

m516 commented 1 year ago

OK, patch available and requested in #146. Thoughts?