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

Improvement/tesla advanced battery #693

Merged

Conversation

josiahhiggs
Copy link
Contributor

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.

Tesla more battery info 1 Tesla More Battery Info 2 Tesla More Battery Info 3 Tesla More Battery Info 4 Tesla More Battery Info 5

@@ -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) {
Copy link
Owner

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?

Copy link
Contributor Author

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;
Copy link
Owner

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!

Copy link
Contributor Author

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?

Copy link
Owner

@dalathegreat dalathegreat Dec 22, 2024

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 :)

Copy link
Owner

@dalathegreat dalathegreat left a 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!

@dalathegreat dalathegreat merged commit e2fb70e into dalathegreat:main Dec 28, 2024
46 checks passed
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