Skip to content
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

Examples Do Not Compile #138

Open
devindumont opened this issue Jan 3, 2023 · 5 comments
Open

Examples Do Not Compile #138

devindumont opened this issue Jan 3, 2023 · 5 comments
Labels
Seeed_Arduino_CAN Label for Seeed_Arduino_CAN UAY Unassigned yet

Comments

@devindumont
Copy link

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
Copy link

nilsfast commented Jan 8, 2023

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
Copy link
Contributor

techie66 commented Aug 3, 2023

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
Copy link
Contributor

m516 commented Aug 3, 2023

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
Copy link
Contributor

techie66 commented Aug 3, 2023

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
Copy link
Contributor

m516 commented Aug 8, 2023

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

@MatthewJeffson MatthewJeffson added UAY Unassigned yet Seeed_Arduino_CAN Label for Seeed_Arduino_CAN labels Oct 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Seeed_Arduino_CAN Label for Seeed_Arduino_CAN UAY Unassigned yet
Projects
Status: No status
Development

No branches or pull requests

5 participants