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

Check new CANDecoder implementation #1

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Conversation

vurhd2
Copy link

@vurhd2 vurhd2 commented Oct 9, 2024

This pull request is to notify you that I finished the first (untested) versions of the 'candecoder.h' and 'candecoder.cpp' files so far.
Please let me know if the code seems to be as per your guidelines or if there are any issues and thus changes that I need to make.

@vurhd2 vurhd2 requested a review from bquan0 October 9, 2024 02:14
Copy link
Contributor

@bquan0 bquan0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good start! I think we can make the code a little less repetitive though...
The main difference between the first readHandler() and sendSignal() and the second pair of functions is just the values you send and the CAN IDs of the messages. Thus, we could just define 2 sets of macros at the top, then use those macros in the functions:

#if IS_FIRST_CAN
#define CAN_MSG_ID_BASE 0x000
#define FLOAT_VAL 3.2
#define UINT8_VAL 98
...
#else
#define CAN_MSG_ID_BASE 0x100
#define FLOAT_VAL 12.3
#define UINT8_VAL 45
...
#endif

void CANDecoder::readHandler(CAN_data msg) {
   switch (msg.id) {
      case (CAN_MSG_ID_BASE + 0):
...
int CANDecoder::sendSignal() {
   uint8_t test8int = UINT8_VAL;

Some other tips:

  • In general, I recommend compiling the code and trying to fix the errors before pushing any commits.
  • Don't forget to resolve the comments after you've fixed them. It helps with the reviewing process!

src/candecoder.cpp Outdated Show resolved Hide resolved
src/candecoder.cpp Outdated Show resolved Hide resolved
src/candecoder.cpp Outdated Show resolved Hide resolved
src/main.cpp Outdated Show resolved Hide resolved
src/main.cpp Outdated Show resolved Hide resolved
src/main.cpp Show resolved Hide resolved
include/candecoder.h Outdated Show resolved Hide resolved
include/candecoder.h Outdated Show resolved Hide resolved
src/candecoder.cpp Outdated Show resolved Hide resolved
src/candecoder.cpp Outdated Show resolved Hide resolved
@bquan0
Copy link
Contributor

bquan0 commented Oct 12, 2024

Also, can you switch to the FW-221/canmanager branch in the embedded-pio submodule? We will want to test with the actual canmanager.h/cpp implementations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants