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

Fixing JK BMS black version. #24

Merged
merged 5 commits into from
Mar 18, 2024
Merged

Conversation

p0l0us
Copy link

@p0l0us p0l0us commented Mar 15, 2024

My previouse change related to Louisvdw#950 for JK bms can broke older "Black" JK bms devices. See fix in jkbms_can.py.

JK BMS cells are updating very slowly via can bus and cell count can be increased few times before all detected. This causes exception in dbushelpre.py at line self._dbusservice[cellpath % (str(i + 1))] = voltage. In the end the cell voltage values were missing and not updated in VenusOS. So I added option to preset cell count for JK CAN JKBMS_CAN_CELL_COUNT which resolves the issue. Original logic is kept so the parameter is optional and not necessary if it works without.

Last change in this PR fixes the reinstall shell script. It may happen that user has can2, but not can0 used by dbus-serialbattery. The original detection for can0 didn't work in such case. I included same fix for BLE even it may not be necessary.

Adding JKBMS_CAN_CELL_COUNT to predefine number of cells.
Fixing CAN reinstallation process.
@mr-manuel
Copy link
Owner

Could you please make sure that your GitHub Actions are working and completing with success? You find more informations here: https://louisvdw.github.io/dbus-serialbattery/general/supported-bms#add-by-opening-a-pull-request

; ----------Cell count ------------
; Predefine cell count for Jkbms_can
; Should be autodetected if not set
JKBMS_CAN_CELL_COUNT = 1
Copy link
Owner

Choose a reason for hiding this comment

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

Please move this under ; --------- BMS specific settings ---------

Copy link
Author

Choose a reason for hiding this comment

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

done

@p0l0us
Copy link
Author

p0l0us commented Mar 18, 2024

Could you please make sure that your GitHub Actions are working and completing with success? You find more informations here: https://louisvdw.github.io/dbus-serialbattery/general/supported-bms#add-by-opening-a-pull-request

https://github.com/p0l0us/dbus-serialbattery/actions/runs/8324561682/job/22776438759
I fixed my issues, now if I understand correctly actions are failing due:

 /opt/hostedtoolcache/Python/3.8.13/x64/bin/flake8 .
./etc/dbus-serialbattery/bms/daly.py:39:121: E501 line too long (125 > 120 characters)
./etc/dbus-serialbattery/bms/daly.py:762:16: F821 undefined name 'exception'
Error: The process '/opt/hostedtoolcache/Python/3.8.13/x64/bin/flake8' failed with exit code 1

But daly.py changes are not part of this PR. It seems to be caused by those two commits:
87e99ae.
ff9bcbe

@mr-manuel mr-manuel merged commit c925a1e into mr-manuel:master Mar 18, 2024
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