Add Support for BMS Tiltback #27

Relys · 2024-11-25T01:27:52Z

    (var bms-fault-codes '(
        (NONE . 0)
        (BMS_COMM_TIMEOUT . 1) < 2 SECONDS. Not currently used in Refloat. Not sure where to put to notify user.
        (BMS_OVER_TEMP . 2) CELL_OVER_TEMP +10
        (CELL_OVER_VOLTAGE . 3)  bms-vmax-limit-start
        (CELL_UNDER_VOLTAGE . 4) bms-vmin-limit-start
        (CELL_OVER_TEMP . 5) bms-t-limit-start
        (CELL_UNDER_TEMP . 6) ; Not used
        (CELL_BALANCE . 7) <= 0.5v difference. Not currently used in Refloat. Not sure how to alert user.
    ))

The other values are from conf-get function in lisp. They can be found in Motor Config -> BMS. While we want the limit boxes unchecked, we can use these fields to customize limits for our own use. Note: I also check if any limits are enabled and disable them (since we want to do that with pushback not limiting motor current).
'bms-t-limit-start      ; Temperature where current starts to get reduced
'bms-vmin-limit-start   ; VCell where current starts to get reduced
'bms-vmax-limit-start   ; VCell where regen current starts to get reduced

Now package.lisp spawns a new process bms-state-loop which handles looking at user config values and BMS data to determine BMS state. It will search for a bms-can-id within first 10 seconds of boot and load the bms-state library if found. BMS faults are stored in an u32 as bitfields. They are sent to the main Refloat C loop with ext-bms-set-fault. In the areas that handle pushback bitfield checks with bool bms_is_fault_set(uint32_t fault_mask, BMS_FAULT_CODES fault_code) are now performed. Minimal code was touched in Refloat itself besides the additional checks, and BeepReason was expanded to handle additional error states.

lukash

You're not handling the Cell Balance and Comm Timeout faults on the C side and you removed the Cell Under Temp handling altogether (I feel this should be addressed in some way).

The commit message is chaotic. Is it really signed off by @surfdado? Please add a Feature: git trailer for the changelog.

package.lisp

src/bms-state.lisp

src/conf/datatypes.h

src/utils.c

ui.qml.in

Relys · 2024-12-19T02:43:26Z

Added some changes, but am unable to test right now since my board is down.

I added beeps (no tiltback) for balance and communication errors. I also re-added CELL_UNDER_TEMP when cell goes below 0 degrees (as per recommended by Battery Mooch).

Instead of keeping logic where I loop through bms voltages, I was able to get accessing the min/max cell values through lisp pulled upstream into main bldc branch: vedderb/bldc@07128ea

Instead of hard coding a specific version, I can just check if that function exists before continuing:

(if (eq (first (trap (get-bms-val 'bms-v-cell-min))) 'exit-ok)

lukash

Thanks for the update. I unresolved some comments which I felt weren't addressed well enough. I also think you missed 4 comments from the previous review because GitHub hid them... For those I made new comments in the current review, so you can just react to my current comments and ignore the old ones...

Sorry about the amount, but it is what it is 😅

And thanks for upstreaming the v-cell-min/max values, makes it cleaner on our side.

src/bms.h

lukash · 2024-12-21T13:51:03Z

src/bms.lisp

+    (var bms-vmax-limit-start (conf-get 'bms-vmax-limit-start))
+    (var bms-tmax-limit-start (- (conf-get 'bms-t-limit-start) 3)) ; Start 3 degrees before Motor CFG -> BMS limiting functionality would happen.
+    (var bms-tmin-limit-start 0)
+    (var bms-cell-balance-start 0.5)


Unaddressed comment from last review, 0.5 seems way too high to me. Not sure what the value should be though.

That's what dado recommended. Cells dip especially on split packet and where there's noise.

I'm wondering if I should just remove this entirely? Or maybe only beep when the board is not running and then we could lower that threshold?

Oh, I see. True that properly judging how the pack is balanced should only be done when idle, it can jump around a lot while riding. We could check it only when idle, question is what to do, is the board supposed to beep in that case? Is it supposed to beep continuously or once in a while? Maybe seeing the battery is imbalanced in the UI would be enough? But then, the dedicated BMS page in VESC Tool might be a better place for that...

FTR, propagating e.g. the engaged state to lisp and moving more of the logic over to there is exactly what I don't want to do.

So this brings me around to whether we should do anything at all, and if we should do anything, seems just checking the balance is not totally out of whack, like, having a 0.5V difference, makes some sense to me 😀 .

In that case, please add a comment along the lines of:

// Only check if the balance is not completely off (in idle or while riding), not meant as a proper balance check

Right now I'm only checking if the board state is READY (not RUNNING) in main.c: https://github.com/lukash/refloat/pull/27/files#diff-e0cf5b28d9b6b600f0af2bc78e8fd30ec675fd731a5da86f0c4283ffc0e40176R1497

Same for BMS connection issue. Should BMS connection actually be checked while running though, and balance only alert while idle?

lukash · 2024-12-21T13:56:47Z

src/bms.lisp

+    (var bms-vmin-limit-start (conf-get 'bms-vmin-limit-start))
+    (var bms-vmax-limit-start (conf-get 'bms-vmax-limit-start))
+    (var bms-tmax-limit-start (- (conf-get 'bms-t-limit-start) 3)) ; Start 3 degrees before Motor CFG -> BMS limiting functionality would happen.
+    (var bms-tmin-limit-start 0)
+    (var bms-cell-balance-start 0.5)
+    (var bms-ic-tmax-limit-start (+ bms-tmax-limit-start 15)) ; Set bms temp limit to +15C past cell limit


Just a note, looking at this 15 degree constant being pretty arbitrary. We should at least figure out a place to document these numbers so that users know how it works...

(I don't like the arbitrary number but don't have a better suggestion, seems reasonable at least to start with)

yeah idk +15 just seems like a reasonable overhead. for cell max 45 +15 would be 60 on bms (and bms is next to cell pack so you don't want it getting too hot) I could also just not look at this value.

I don't know, makes some sense to warn about this, if someone's discharge bms starts catching fire... I think we just need to set up some documentation page for this stuff...

src/bms.lisp

src/bms-state.lisp

src/bms.lisp

src/main.c

lukash · 2024-12-21T14:52:09Z

And please add the Feature: trailer to the commit description, that'll be turned into a changelog record (so phrase it that way) with an attribution to you.

Relys · 2024-12-22T05:41:37Z

Added another update. I'm unable to test right now still, so lmk if there's any errors in lisp or something.

Relys · 2024-12-22T06:12:19Z

Also just added support back in for 6.05 (since we don't have 6.06 API changes yet) after talking to @surfdado and Fungineers.

Relys · 2024-12-22T08:46:46Z

Another change to allow disabling this feature by setting Motor CFG -> BMS vmin-limit-start, vmax-limit-start or tmax-limit-start to 0.0

lukash · 2024-12-22T12:10:17Z

Another change to allow disabling this feature by setting Motor CFG -> BMS vmin-limit-start, vmax-limit-start or tmax-limit-start to 0.0

Hmm. I'm not sure about this, what's the motivation? FWIW IMO it'd be better if you brought up significant changes like this with me before doing them. If I have an issue with them, it makes me feel bad to bring it up because I don't want to make you go back and forth.

You can turn this BMS alerting functionality off by setting BMS Type to None, right? So what's the motivation?

What'd make more sense to me is disabling that one particular check by setting the limit to 0 (but we don't have a config value for every check we do that'd allow us to disable them individually 😞).

Relys · 2024-12-23T05:24:33Z

Another change to allow disabling this feature by setting Motor CFG -> BMS vmin-limit-start, vmax-limit-start or tmax-limit-start to 0.0

Hmm. I'm not sure about this, what's the motivation? FWIW IMO it'd be better if you brought up significant changes like this with me before doing them. If I have an issue with them, it makes me feel bad to bring it up because I don't want to make you go back and forth.

You can turn this BMS alerting functionality off by setting BMS Type to None, right? So what's the motivation?

What'd make more sense to me is disabling that one particular check by setting the limit to 0 (but we don't have a config value for every check we do that'd allow us to disable them individually 😞).

"You can turn this BMS alerting functionality off by setting BMS Type to None, right? So what's the motivation?"

You assume too much haha. No that's not exposed. @surfdado mentioned wanting some way to disable this feature, and so this was the only solution I could come up with the variables exposed in Lisp. I could take a look and see if BMS Type is exposed on the C side maybe? Edit: I checked and nope.

lukash

Hey, I'm very sorry about the long delay. Read through it another time, this time I have more questions than anything...

In addition, I think we need a master switch for this functionality in the Refloat Cfg -> Specs tab. If disabled, just don't even initialize the lisp code. The feature as a whole also needs all the thresholds documented, this new option's help text could be a good place. That is, all hardcoded thresholds should be listed there and the options that are used from VESC BMS config should be listed there along with the details (e.g. having a 3 degree offset etc...).

And if the option is enabled, it should only report BMSF_NONE if it's running properly and everything is ok. In case of unsupported FW version, it should report a special error, as well as in case of any other state than "all checks are ok". That way, when the user enabled the feature, they can be sure what's up with it. The way it is now, it seems to me you're kind-of hoping...

lukash · 2025-02-23T16:57:19Z

package.lisp

@@ -8,6 +8,17 @@
 ; Set firmware version:
 (apply ext-set-fw-version (sysinfo 'fw-ver))

+(def version_major (first (sysinfo 'fw-ver)))
+(def version_minor (second (sysinfo 'fw-ver)))
+(if (or (eq (first (trap (get-bms-val 'bms-v-cell-min))) 'exit-ok) (or (>= version_major 7) (and (>= version_major 6) (>= version_minor 5)))) {


This part of the condition: (>= version_major 6) should be just (= version_major 6), I think I suggested this in the first place but thought I corrected myself later.

Also, remind me, why do we need both (eq (first (trap (get-bms-val 'bms-v-cell-min))) 'exit-ok) and (well, logically it's or) (or (>= version_major 7) (and (>= version_major 6) (>= version_minor 5)))?

lukash · 2025-02-23T17:01:04Z

src/bms.lisp

+    (var vmin-limit)
+    (var vmax-limit)
+    (var tmax-limit)
+    (var tmin-limit -10)


Why did you silently change this to -10? When you change the semantics, could you please comment with an explanation?

lukash · 2025-02-23T17:02:05Z

src/bms.lisp

+    (var config-update-time)
+    (var fault 0u32)
+    (var v-cell-support (eq (first (trap (get-bms-val 'bms-v-cell-min))) 'exit-ok))
+    (var bms-data-version-support (and (eq (first (trap (get-bms-val 'bms-data-version))) 'exit-ok) (> (get-bms-val 'bms-data-version) 0)))


What does bms-data-version-support represent? Seems improperly named to me... Might need an explanation comment.

lukash · 2025-02-23T17:03:25Z

src/bms.lisp

+        (if (> (secs-since config-update-time) 1.0) {
+            (update-config)
+        })
+        (if (or (= vmin-limit 0.0) (= vmax-limit 0.0) (= tmax-limit 0.0)) {


What is this condition for? It doesn't look right to skip all the checks if one of them is 0.

Ok, I obviously didn't even read the last few messages when I came back with this review. But my message kind-of confirms this is confusing. The code is already running here, this check is not like an ordinary enable switch (in addition to it being hooked to three or-ed values). IMO a global in-package switch (as I described in the review) would be much better. Potentially any of the above being zero could be used to disable that particular check (for when e.g. the user has improperly configured thermistors or some other malfunction of one them).

lukash · 2025-02-23T17:05:39Z

src/bms.lisp

+                (if bms-data-version-support {
+                    (setq t-cell-min (get-bms-val 'bms-temps-adc 1))
+                    (setq t-cell-max (get-bms-val 'bms-temps-adc 2))
+                    (var t-mosfet-temp (get-bms-val 'bms-temps-adc 3))


What do the 1, 2 and 3 represent here? Why do you set t-cell-min to 1 and t-cell-max to 2?

lukash · 2025-02-23T17:07:00Z

src/bms.lisp

+                    (setq t-cell-min (get-bms-val 'bms-temps-adc 1))
+                    (setq t-cell-max (get-bms-val 'bms-temps-adc 2))
+                    (var t-mosfet-temp (get-bms-val 'bms-temps-adc 3))
+                    (if (> t-mosfet-temp -280) {


What is the -280 constant?

lukash · 2025-02-23T17:07:43Z

src/bms.lisp

+                    })
+                } {
+                    (setq t-cell-min (get-bms-val 'bms-temp-cell-max))
+                    (setq t-cell-max t-cell-min)


I think this logic around the t-cell-min and t-cell-max needs some comments. Not clear what's going on.

lukash · 2025-02-23T17:11:16Z

src/main.c

+            if (bms_is_fault_set(d->bms_fault, BMSF_CELL_BALANCE)) {
+                beep_alert(d, 3, true);
+                d->beep_reason = BEEP_CELL_BALANCE;
+            }


IIUC this is gonna be beeping pretty much continuously (not sure the 3 beeps actually make any difference, I'd say not). Did you test this?

Relys changed the title ~~Add support for BMS tiltback.~~ Add Support for BMS Tiltback Nov 25, 2024

Relys force-pushed the feature/bms-tiltback branch 3 times, most recently from db4730c to 77c35eb Compare November 25, 2024 20:10

Relys force-pushed the feature/bms-tiltback branch from 77c35eb to 39340a8 Compare December 12, 2024 05:58

Relys closed this Dec 12, 2024

Relys force-pushed the feature/bms-tiltback branch from 39340a8 to 1ad13ed Compare December 12, 2024 06:02

Relys reopened this Dec 12, 2024

Relys force-pushed the feature/bms-tiltback branch 6 times, most recently from 85570e4 to 7311911 Compare December 12, 2024 07:48

lukash requested changes Dec 12, 2024

View reviewed changes

Relys force-pushed the feature/bms-tiltback branch 2 times, most recently from dc8b405 to 7138dee Compare December 19, 2024 02:39

lukash requested changes Dec 21, 2024

View reviewed changes

Relys force-pushed the feature/bms-tiltback branch from 7138dee to 33ca415 Compare December 22, 2024 05:40

Relys force-pushed the feature/bms-tiltback branch from 33ca415 to 3d324d4 Compare December 22, 2024 06:11

Relys force-pushed the feature/bms-tiltback branch from 3d324d4 to 63902b0 Compare December 22, 2024 08:45

Relys force-pushed the feature/bms-tiltback branch 2 times, most recently from c529a23 to 102e622 Compare December 28, 2024 00:09

Feature: Add Support for BMS Tiltback

a39726a

Relys force-pushed the feature/bms-tiltback branch from 102e622 to a39726a Compare February 18, 2025 22:33

lukash reviewed Feb 23, 2025

View reviewed changes

Relys mentioned this pull request Mar 10, 2025

Feature: Add Support for BMS Tiltback 2.0 #37

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Support for BMS Tiltback #27

Add Support for BMS Tiltback #27

Relys commented Nov 25, 2024 •

edited

Loading

lukash left a comment

Relys commented Dec 19, 2024 •

edited

Loading

lukash left a comment

lukash Dec 21, 2024

Relys Dec 22, 2024

lukash Dec 22, 2024

Relys Dec 23, 2024

lukash Dec 21, 2024

Relys Dec 22, 2024

lukash Dec 22, 2024

lukash commented Dec 21, 2024

Relys commented Dec 22, 2024

Relys commented Dec 22, 2024

Relys commented Dec 22, 2024

lukash commented Dec 22, 2024

Relys commented Dec 23, 2024 •

edited

Loading

lukash left a comment

lukash Feb 23, 2025

lukash Feb 23, 2025

lukash Feb 23, 2025

lukash Feb 23, 2025

lukash Mar 7, 2025

lukash Feb 23, 2025

lukash Feb 23, 2025

lukash Feb 23, 2025

lukash Feb 23, 2025

Add Support for BMS Tiltback #27

Are you sure you want to change the base?

Add Support for BMS Tiltback #27

Conversation

Relys commented Nov 25, 2024 • edited Loading

lukash left a comment

Choose a reason for hiding this comment

Relys commented Dec 19, 2024 • edited Loading

lukash left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukash commented Dec 21, 2024

Relys commented Dec 22, 2024

Relys commented Dec 22, 2024

Relys commented Dec 22, 2024

lukash commented Dec 22, 2024

Relys commented Dec 23, 2024 • edited Loading

lukash left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Relys commented Nov 25, 2024 •

edited

Loading

Relys commented Dec 19, 2024 •

edited

Loading

Relys commented Dec 23, 2024 •

edited

Loading