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

CAN bus implementation #5120

Draft
wants to merge 23 commits into
base: mega
Choose a base branch
from

Conversation

bclbruno
Copy link

@bclbruno bclbruno commented Sep 11, 2024

  • Add CAN hardware implementation
    • Added CAN bus configuration to HardwarePage: TxPin, RxPin, Baudrate, NodeId
    • Added CAN bus configuration to Settings structure
    • Added CAN bus pins to markup and string generation
    • Added sendcan and sendtocan commands
  • [C20] Add CAN controller
  • [P155] Add CAN importer plugin

TODOs:

  • Set the plugin and controller IDs (It's set as P155 and C20 just as a place holder)
  • doc?

@tonhuisman
Copy link
Contributor

@bclbruno I have assigned Plugin ID P174 for the CAN Importer plugin and Controller ID C022 for the CAN Controller, as these are the next available IDs. Please update your code, as the current IDs are already in use.

Comment on lines 85 to 90
for (int i = 0; i < valueCount; ++i) {
if (lines[i].length() == 0) {
lines[i] = concat(F("val"), i + 1);
}
ExtraTaskSettings.setTaskDeviceValueName(i, lines[i]);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use Uncrustify to format the sources (exception: external libraries), to avoid mis-alignments like these.

Comment on lines 36 to 37
String log_debug = F("Disconnected");
addLogMove(LOG_LEVEL_INFO, log_debug);
Copy link
Contributor

Choose a reason for hiding this comment

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

To preserve code-size and memory use, this can be a single statement, no need to instantiate that String first.
addLog(LOG_LEVEL_INFO, F("Disconnected"));

Comment on lines 479 to 482

case ESPEasy_cmd_e::sendcan: COMMAND_CASE_A(Command_CAN_SendCAN, -1); //CAN.h
case ESPEasy_cmd_e::sendtocan: COMMAND_CASE_A(Command_CAN_SendToCAN, -1); //CAN.h

Copy link
Contributor

Choose a reason for hiding this comment

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

  • There's a missing #if FEATURE_CAN here
  • Commands are in this switch statement in alphabetical order, by default

@@ -276,6 +276,10 @@ const char Internal_commands_w[] PROGMEM =
"wdconfig|"
"wdread|"
#endif // ifndef LIMIT_BUILD_SIZE
#ifdef FEUTER_CAN
Copy link
Contributor

Choose a reason for hiding this comment

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

  • That's probably a typo, should better be #if FEATURE_CAN
  • We've stopped using #ifdef for feature-flags, to make handling defaults possible, that can also be controller from a Custom.h file, to be explicitly enabled (1) or disabled (0)

Comment on lines 1455 to 1459
#ifdef FEATURE_CAN
#define USES_C020 //CAN
#define USES_C021
#define USES_P155
#endif
Copy link
Contributor

@tonhuisman tonhuisman Sep 11, 2024

Choose a reason for hiding this comment

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

This should be like:

#ifndef FEATURE_CAN
  #ifdef ESP32
    #define FEATURE_CAN 1
  #endif
  #ifdef ESP8266
    #define FEATURE_CAN 0 // Default disabled for ESP8266 for size reasons
  #endif
#endif
#if FEATURE_CAN
  #ifndef USES_C022 // CAN Controller
    #define USE_C022
  #endif
  #ifndef USES_P174
    #define USES_P174 // CAN Import plugin
  #endif
#endif

(For Plugin, Controller and Notifiers we still use #ifdef USES_... checks...)
Not sure why you also try to enable Controller C021? If that's one you also created yourself, then it should probably get ID C023, as C021 is reserved for a (planned) LoRa controller 🤔

Comment on lines 574 to 577
uint8_t CAN_Tx_pin = -1;
uint8_t CAN_Rx_pin = -1;
long CAN_baudrate = DEFAULT_CAN_BAUDRATE;
int CAN_node_id = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this belongs in the CAN Controller, where the CAN Importer can get it's pins from, if needed, as the CAN Controller seems t o be a requirement for the importer to function.

@TD-er TD-er force-pushed the can_implementation branch from 44f2f69 to 0bc2b45 Compare September 22, 2024 13:27
Comment on lines +3466 to +3469
#if FEATURE_CAN
#define USES_C022 //CAN
#define USES_P174
#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a duplicate for lines 3431..3441

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.

4 participants