Skip to content

Conversation

@dhruv-aron
Copy link

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.

@dhruv-aron dhruv-aron 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!

@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.

3 participants