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

Add support for bm5v2 #3126

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Conversation

Murphy-Cameron
Copy link

Add support for BM5 v2.0 12V Battery Monitor

@zuckschwerdt
Copy link
Collaborator

This looks really good, thanks! Some minor style changes, then good to merge.

  • First line of the doc-comment is a heading. The next sentence (Sold as…) should have a blank line before it.
  • if blocks should always be in braces {…}
  • Line 66 ){ should have a space: ) {
  • Float ops need to be float, prefer multiplication if possible, e.g. * 0.000625f; // divide by 1600
  • Model needs to be in alphanum-dash-alphanum format, e.g. BM5-v2
  • keys should be somewhat descriptive (longer) and have a unit symbol as suffix, e.g. "health_pct", "charge_pct", "battery_V", "starting_V"
  • parts of the name/description that are generic or a class should be lower case, e.g. "BM5 v2.0 12V battery monitor"

@Murphy-Cameron
Copy link
Author

Thanks for the feedback. I've made the requested changes.

@zuckschwerdt
Copy link
Collaborator

Perfect. Line 84 needs to be inside the braces :)
And "device_id", "Device_ID", should be "id", "Device ID",, that's it.

@zuckschwerdt
Copy link
Collaborator

Oh and the two voltages likely should have a format of "%.2f V",, right?

@zuckschwerdt
Copy link
Collaborator

Sorry, havn't noticed this before:

  • Shifts need parens for clear operator precedence , e.g. int volt1 = (b[7] << 8) | b[6]; // Current voltage
  • If you really have signed char for temperature you need int temp = (int8_t)b[5]; // Temperature in C, signed char in byte 6

@Murphy-Cameron
Copy link
Author

Thanks for catching that. My C skills are pretty rusty :-)

@Murphy-Cameron
Copy link
Author

Murphy-Cameron commented Dec 29, 2024

Your comment about the signed temperature value prodded me test my assumptions. (We haven't had any temperatures quite below zero C yet). So, I pulled the sensor out, and put it in the freezer. Turns out the temperature is stored as sign-magnitude in byte 6 of the data packet.

I fixed the temperature handling, and added one more sample signal to the tests repository covering the sensor at -1_C, with a cranking error triggered. (pulled the EFI fuse to simulate a failed start attempt).

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