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.

Sign up for GitHub

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

Tesla bugfixes #818

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+25 −20
Open

Tesla bugfixes #818

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
66c141c
Debug_log bugfix
josiahhiggs Jan 24, 2025
102db63
format
josiahhiggs Jan 24, 2025
19cd5c5
Update advanced_battery_html.cpp
josiahhiggs Jan 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 20 additions & 15 deletions Software/src/battery/TESLA-BATTERY.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -827,6 +827,7 @@ static const char* hvilStatusState[] = {"NOT OK",
"UNKNOWN(13)",
"UNKNOWN(14)",
"UNKNOWN(15)"};
static const char* noYes[] = {"No", "Yes"};

void clearIsolationFault() {
//CAN UDS messages to clear a latched isolation fault
Expand Down Expand Up @@ -1144,9 +1145,9 @@ void update_values_battery() { //This function maps all the values fetched via
logging.print(", setState: ");
logging.print(contactorState[battery_packContactorSetState]);
logging.print(", close allowed: ");
logging.print(battery_packCtrsClosingAllowed);
logging.print(noYes[battery_packCtrsClosingAllowed]);
Copy link
Collaborator

@mvgalen mvgalen Jan 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This use of indexing into an arrray has a great risk of array out-of-bounds accesses which can lead to crashes. Either and the input with 0x01, to reduce it to 0 or 1. Or, and perhaps better, define an inline function returning the string, but then as result of an if/else or switch statement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would this be acceptable option for an inline function?

inline const char* getContactorText(int index) {
    switch (index) {
        case 0: return "Open";
        case 1: return "Closed";
        default: return "Unknown";
    }
}

inline const char* getHvilStatusState(int index) {
    switch (index) {
        case 0: return "Disconnected";
        case 1: return "Connected";
        default: return "Unknown";
    }
}

inline const char* getContactorState(int index) {
    switch (index) {
        case 0: return "Inactive";
        case 1: return "Active";
        default: return "Unknown";
    }
}

inline const char* getNoYes(int index) {
    switch (index) {
        case 0: return "No";
        case 1: return "Yes";
        default: return "Unknown";
    }
}

void logStatus() {
    logging.print("STATUS: Contactor: ");
    logging.print(getContactorText(battery_contactor));  // Display what state the contactor is in
    logging.print(", HVIL: ");
    logging.print(getHvilStatusState(battery_hvil_status));
    logging.print(", NegativeState: ");
    logging.print(getContactorState(battery_packContNegativeState));
    logging.print(", PositiveState: ");
    logging.print(getContactorState(battery_packContPositiveState));
    logging.print(", setState: ");
    logging.print(getContactorState(battery_packContactorSetState));
    logging.print(", close allowed: ");
    logging.print(getNoYes(battery_packCtrsClosingAllowed));
    logging.print(", Pyrotest: ");
    logging.println(getNoYes(battery_pyroTestInProgress));
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, much better imho.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So this change is going to apply to the webserver as well. More of an improvement PR, I think I will work on this separately to make sure I test it on my bench. Sound good?

logging.print(", Pyrotest: ");
logging.println(battery_pyroTestInProgress);
logging.println(noYes[battery_pyroTestInProgress]);

logging.print("Battery values: ");
logging.print("Real SOC: ");
Expand Down Expand Up @@ -1174,11 +1175,16 @@ void update_values_battery() { //This function maps all the values fetched via
logging.print(battery_cell_deviation_mV);
logging.println("mV.");

print_int_with_units("High Voltage Output Pins: ", battery_dcdcHvBusVolt, "V");
logging.printf("High Voltage Output Pins: %.2f V", (battery_dcdcHvBusVolt * 0.146484));
logging.print(", ");
print_int_with_units("Low Voltage: ", battery_dcdcLvBusVolt, "V");
logging.println("");
print_int_with_units("DC/DC 12V current: ", battery_dcdcLvOutputCurrent, "A");
logging.printf("Low Voltage: %.2f V", (battery_dcdcLvBusVolt * 0.0390625));
logging.print(", ");
logging.printf("DC/DC 12V current: %.2f A", (battery_dcdcLvOutputCurrent * 0.1));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps better to combine the println(".") below with the printf on this line %.2f A.\n");

logging.println(".");

logging.printf("PCS_ambientTemp: %.2f°C, DCDC_Temp: %.2f°C, ChgPhA: %.2f°C, ChgPhB: %.2f°C, ChgPhC: %.2f°C",
PCS_ambientTemp * 0.1 + 40, PCS_dcdcTemp * 0.1 + 40, PCS_chgPhATemp * 0.1 + 40,
PCS_chgPhBTemp * 0.1 + 40, PCS_chgPhCTemp * 0.1 + 40);
logging.println("");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same for this one, just add \n to the printf string.


logging.println("Values passed to the inverter: ");
Expand Down Expand Up @@ -1230,6 +1236,7 @@ void handle_incoming_can_frame_battery(CAN_frame rx_frame) {
if (mux0_read && mux1_read) {
mux0_read = false;
mux1_read = false;
BMS352_mux = true; //Set flag to true
}
// older BMS <2021 without mux
battery_nominal_full_pack_energy = //BMS_nominalFullPackEnergy : 0|11@1+ (0.1,0) [0|204.6] "KWh" //((_d[1] & (0x07U)) << 8) | (_d[0] & (0xFFU));
Expand Down Expand Up @@ -1449,17 +1456,15 @@ void handle_incoming_can_frame_battery(CAN_frame rx_frame) {
BMS_pcsNoFlowRequest = ((rx_frame.data.u8[7] >> 6) & (0x01U)); // 62|1@1+ (1,0) [0|0] ""
BMS_noFlowRequest = ((rx_frame.data.u8[7] >> 7) & (0x01U)); //63|1@1+ (1,0) [0|0] ""
break;
case 0x2A4: //676 PCS_thermalStatus
PCS_chgPhATemp =
(((rx_frame.data.u8[1] & 0x07) << 8) | (rx_frame.data.u8[0] & 0xFF)); //0|11@1- (0.1,40) [0|0] "C"
case 0x2A4: //676 PCS_thermalStatus
PCS_chgPhATemp = (rx_frame.data.u8[0] & 0xFF) | ((rx_frame.data.u8[1] & 0x07) << 8); //0|11@1- (0.1,40) [0|0] "C"
PCS_chgPhBTemp =
(((rx_frame.data.u8[2] & 0x3F) << 5) | ((rx_frame.data.u8[1] >> 3) & 0x1F)); //11|11@1- (0.1,40) [0|0] "C"
PCS_chgPhCTemp = (((rx_frame.data.u8[2] & 0xC0) >> 6) | ((rx_frame.data.u8[3] & 0xFF) << 2) |
((rx_frame.data.u8[4] & 0x03) << 10)); //22|11@1- (0.1,40) [0|0] "C"
((rx_frame.data.u8[1] & 0xF8) >> 3) | ((rx_frame.data.u8[2] & 0x3F) << 5); //11|11@1- (0.1,40) [0|0] "C"
PCS_chgPhCTemp = ((rx_frame.data.u8[2] & 0xC0) >> 6) | (rx_frame.data.u8[3] << 2) |
((rx_frame.data.u8[4] & 0x01) << 10); //22|11@1- (0.1,40) [0|0] "C"
PCS_dcdcTemp =
(((rx_frame.data.u8[4] >> 1) & 0x1F) | ((rx_frame.data.u8[5] & 0x3F) << 5)); //33|11@1- (0.1,40) [0|0] "C"
PCS_ambientTemp =
(((rx_frame.data.u8[7] & 0x07) << 8) | (rx_frame.data.u8[6] & 0xFF)); //44|11@1- (0.1,40) [0|0] "C"
((rx_frame.data.u8[4] & 0xFE) >> 1) | ((rx_frame.data.u8[5] & 0x0F) << 7); //33|11@1- (0.1,40) [0|0] "C"
PCS_ambientTemp = ((rx_frame.data.u8[5] & 0xF0) >> 4) | (rx_frame.data.u8[6] << 4); //44|11@1- (0.1,40) [0|0] "C"
break;
case 0x2C4: // 708 PCS_logging: not all frames are listed, just ones relating to dcdc
mux = (rx_frame.data.u8[0] & (0x1FU));
Expand Down
10 changes: 5 additions & 5 deletions Software/src/devboard/webserver/advanced_battery_html.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -714,11 +714,11 @@ String advanced_battery_processor(const String& var) {
//content += "<h4>Brick Model Temp Max: " + String(BrickModelTMax) + " C</h4>";// Not giving useable data
//content += "<h4>Brick Model Temp Min: " + String(BrickModelTMin) + " C</h4>";// Not giving useable data
//0x2A4 676 PCS_thermalStatus
content += "<h4>PCS dcdc Temp: " + String(PCS_dcdcTemp, 2) + " DegC</h4>";
content += "<h4>PCS Ambient Temp: " + String(PCS_ambientTemp, 2) + " DegC</h4>";
content += "<h4>PCS Chg PhA Temp: " + String(PCS_chgPhATemp, 2) + " DegC</h4>";
content += "<h4>PCS Chg PhB Temp: " + String(PCS_chgPhBTemp, 2) + " DegC</h4>";
content += "<h4>PCS Chg PhC Temp: " + String(PCS_chgPhCTemp, 2) + " DegC</h4>";
content += "<h4>PCS dcdc Temp: " + String(PCS_dcdcTemp) + " DegC</h4>";
content += "<h4>PCS Ambient Temp: " + String(PCS_ambientTemp) + " DegC</h4>";
content += "<h4>PCS Chg PhA Temp: " + String(PCS_chgPhATemp) + " DegC</h4>";
content += "<h4>PCS Chg PhB Temp: " + String(PCS_chgPhBTemp) + " DegC</h4>";
content += "<h4>PCS Chg PhC Temp: " + String(PCS_chgPhCTemp) + " DegC</h4>";
//0x252 594 BMS_powerAvailable
content += "<h4>Max Regen Power: " + String(BMS_maxRegenPower) + " KW</h4>";
content += "<h4>Max Discharge Power: " + String(BMS_maxDischargePower) + " KW</h4>";
Expand Down
Loading