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

Pytes battery: Add support for native CAN protocol #1196

Merged
merged 3 commits into from
Oct 10, 2024

Conversation

ranma
Copy link

@ranma ranma commented Aug 25, 2024

The recently added PytesCanReceiver.cpp implements the Victron CAN protocol.

This change additionally adds support for the native Pytes CAN protocol messages.

Features only supported in Pytes protocol:

  • High-resolution state of charge / full and remaining mAh
  • Charge cycle counter
  • Balancing state

Features only supported in Victron protocol:

  • FW version
  • Serial number

Note that the only known way to select the native Pytes protocol is via the serial console (Cisco-compatible cables work):

login config
setprt PYTES
logout

to return to Victron protocol use:

login config
setprt VICTRON
logout

The protocol switch will take effect immediately.

Tested on Pytes E-Box 4850

See #1188

@AndreasBoehm
Copy link
Member

AndreasBoehm commented Aug 26, 2024

Thanks for your PR, cool that we can get more details about the battery by switching it to a different protocol.
I will test this once my console cable arrives.

Things that we should adjust:

  • Figure out what 'balance' actually represents and add it to the UI
  • If we can't figure out state/warning flags we should remove the section from the UI, but it also doesn't feel right for me to switch to a different protocol but then we loose important things like warnings
  • Remove any information from the UI the JSON that is generated by the BatteryStats for Pytes that is not available anymore when the protocol is switched (Should still be available when the 'Victron' protocol is used)

@schlimmchen
Copy link
Member

Remove any information from the UI that is not available anymore

The UI is generic, as far as I remember. Only info that is put into the respective JSON array is displayed in the UI. So I am not sure why you mention this. We have to take care that in the backend, only relevant info is serialized into JSON. The respective BatteryProvider implementation is responsible for that.

@AndreasBoehm
Copy link
Member

@schlimmchen That is exactly what i meant with my comment. Don't touch the UI code, adjust the BatteryStats for 'Pytes' to not add values into the JSON that are not available.

@ranma ranma force-pushed the pytes-can-protocol branch from 6a95a5c to 9a59048 Compare August 29, 2024 20:50
@ranma
Copy link
Author

ranma commented Aug 29, 2024

Updated the pull request with support for alarms, warnings and online/offline module count (based on Ghidra analysis).

src/BatteryStats.cpp Outdated Show resolved Hide resolved
src/BatteryStats.cpp Outdated Show resolved Hide resolved
src/BatteryStats.cpp Outdated Show resolved Hide resolved
src/BatteryStats.cpp Outdated Show resolved Hide resolved
src/PytesCanReceiver.cpp Outdated Show resolved Hide resolved
src/PytesCanReceiver.cpp Show resolved Hide resolved
src/PytesCanReceiver.cpp Outdated Show resolved Hide resolved
@ranma ranma force-pushed the pytes-can-protocol branch 2 times, most recently from ad32ed8 to 6bc1cc6 Compare September 1, 2024 19:33
@ranma
Copy link
Author

ranma commented Sep 1, 2024

With this change and Victron protocol:
screenshot victron

With this change, Victron protocol and below 10% charge ("immediate charging requested" via 0x360 val 0xff):
screenshot victron immediate charging requested

With this change, Pytes protocol and low charge state:
screenshot pytes low charge

With this change, Pytes protocol, fully charged and balancing active:
screenshot pytes fully charged and balancing

@ranma ranma force-pushed the pytes-can-protocol branch from 6bc1cc6 to a16a92c Compare September 5, 2024 18:32
@ranma
Copy link
Author

ranma commented Sep 5, 2024

Added HASS auto-discovery for new fields.

@ranma ranma force-pushed the pytes-can-protocol branch 3 times, most recently from 9e97dea to da43028 Compare September 14, 2024 11:51
@schlimmchen schlimmchen force-pushed the development branch 2 times, most recently from 91cc2fc to 8ff94e7 Compare September 16, 2024 14:10
@AndreasBoehm
Copy link
Member

@ranma could you do me a favor and update the description of the PR to reflect what features/changes we can expect from this PR?
Its a bit hard to follow the comments and the code does not match the description anymore (e.g. support of alarms)

Thank you.

src/PytesCanReceiver.cpp Show resolved Hide resolved
src/PytesCanReceiver.cpp Outdated Show resolved Hide resolved
src/PytesCanReceiver.cpp Show resolved Hide resolved
@AndreasBoehm
Copy link
Member

I switched to the PYTES protocol and the web ui is showing a strange value for voltage sometimes, the value shown was 1,70 V. In the live view on top and in the battery card on the bottom.

It happens randomly and i could not find anything strange in the CAN messages, they all looked fine and showed the correct voltage of 53,xx

Did you have the same experience @ranma ?

@ranma
Copy link
Author

ranma commented Sep 20, 2024

I switched to the PYTES protocol and the web ui is showing a strange value for voltage sometimes, the value shown was 1,70 V. In the live view on top and in the battery card on the bottom.

I don't remember seeing any drops like that, not in the UI nor in the MQTT data.

@ranma ranma force-pushed the pytes-can-protocol branch 2 times, most recently from 849d249 to dc5ae84 Compare September 20, 2024 19:35
@ranma
Copy link
Author

ranma commented Sep 22, 2024

I switched to the PYTES protocol and the web ui is showing a strange value for voltage sometimes, the value shown was 1,70 V. In the live view on top and in the battery card on the bottom.

I can currently reproduce this in testing right now, I sometimes see 2.90 V instead of 53.15 V.

@ranma ranma force-pushed the pytes-can-protocol branch from dc5ae84 to 091fca3 Compare September 22, 2024 18:16
@ranma
Copy link
Author

ranma commented Sep 22, 2024

I can currently reproduce this in testing right now, I sometimes see 2.90 V instead of 53.15 V.

There was a stray setVoltage() fatfingered into the message 0x402 handler at some point, removed that to fix the issue.

Also verified the charge inhibited count with replayed messages from when the battery was full, it shows Charging blocked 1 now:
image

@AndreasBoehm
Copy link
Member

Everything works fine now. I will re-review this PR after my holidays and then we can finally merge it, thanks for providing this :)

@AndreasBoehm AndreasBoehm self-assigned this Oct 8, 2024
ranma added 2 commits October 10, 2024 17:57
This one-byte message is set to 0xff to request charging below a
 certain SoC threshold (10% in my tests).
The recently added PytesCanReceiver.cpp implements the Victron CAN protocol.

This change additionally adds support for the native Pytes CAN
protocol messages.

Features only supported in Pytes protocol:
- High-resolution state of charge / full and remaining mAh
- Charge cycle counter
- Balancing state

Features only supported in Victron protocol:
- FW version
- Serial number

Note that the only known way to select the native Pytes protocol is
via the serial console (Cisco-compatible cables work):

```
login config
setprt PYTES
logout
```

to return to Victron protocol use:
```
login config
setprt VICTRON
logout
```

to retrun to DIP-switch based protocol setting:
```
login config
setprt DIP
logout
```

The protocol switch should take effect immediately.

Tested on Pytes E-Box 4850
@AndreasBoehm AndreasBoehm merged commit c523f06 into hoylabs:development Oct 10, 2024
9 checks passed
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new discussion or issue for related concerns.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants