-
Notifications
You must be signed in to change notification settings - Fork 167
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
Improvement/tesla advanced battery #693
Improvement/tesla advanced battery #693
Conversation
Update all CAN ID's battery is sending, reorganize and add comments for reference. Change names to match DBC files. Need to update DOUBLE BATTERY.
Update battery2 info to match.
@@ -1227,14 +2654,14 @@ the first, for a few cycles, then stop all messages which causes the contactor | |||
#endif //defined(TESLA_MODEL_SX_BATTERY) || defined(EXP_TESLA_BMS_DIGITAL_HVIL) | |||
|
|||
//Send 30ms message | |||
if (currentMillis - previousMillis30 >= INTERVAL_30_MS) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it really OK to change the 30ms sending to 50ms? Should we test on more batteries before merging?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot that I made that correction in the code. I have tested it at 50ms for the past month with no problems. All the DBC and CAN dumps I have looked at indicate the 50ms is correct. For the context of this PR, I will return it back to 30ms and introduce the 50ms in another PR that aligns with it.
static uint32_t battery_moduleType = 0; | ||
static uint32_t battery_reservedConfig = 0; | ||
//Fault codes | ||
static uint8_t battery_packCtrsOpenNowRequested = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To increase readability and make the logic easier, all values that hold either a true/false could instead of being defined as:
static uint8_t battery_packCtrsOpenNowRequested = 0;
Instead be defined as:
static bool battery_packCtrsOpenNowRequested = false;
Minor improvement for easier maintainability!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense, I can make the changes for that. What would be better, false/True or no/Yes, displayed on the webserver?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a user perspective, I think
Error explanation: Not active / Active!
makes sense. But anything is fine there! I just worry about maintainability :)
Change values that hold either a true/false could instead of being defined as: static uint8_t battery_packCtrsOpenNowRequested = 0; Instead be defined as: static bool battery_packCtrsOpenNowRequested = false;
…om/josiahhiggs/Battery-Emulator into improvement/tesla-advanced-battery
Changed the displayed data format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Nice improvement for people wanting more data out of their Tesla batteries!
What
This PR implements Tesla advanced battery info to the webserver, reorganize of data and labelling for reference.
Why
Why does it do it? There is so much information that is able to be read from the battery, but was not coded to read it or display the data.
How
How does it do it? All data frames in the TESLA-BATTERY.cpp were updated, adding information to the datalayer_extended and adding to the webserver under the advanced_battery.html. Factoring was moved from the Tesla-BATTERY.cpp to the advanced_battery.html using float, also added static const char to correctly display data in the webserver. Also separated the 0x352 with mux and labeled in the webserver. This data has not been completely verified as accurate.