OpenEMS
Feature/pylontech battery implementation
Implementation of Pylontech Powercube M2 battery as OpenEMS component.
Design of battery implementation is based on Fenecon Commercial Battery implementation.
Hi Thomas,
I started with an implementation of the battery about two months ago and didn't realize there is already this PR. I will review it in the next days.
Best regards, Thomas
Hi Thomas,
I started with an implementation of the battery about two months ago and didn't realize there is already this PR. I will review it in the next days.
Best regards, Thomas
Hi Thomas,
Good to hear that you are also interested in this battery! Unfortunately I realised that I have not been disciplined enough with my development and have added extra commits to this branch/PR since finishing the battery development. I must remove my last commit from this PR as it is nothing to do with the Pylontech battery.
WIll do this an update the PR
@tsicking I have now updated the PR to remove the last commit. I have had the OpenEMS device working with the battery in practice for a couple of months now and it is working ok for us.
Hi Thomas, I still see all 7 commits and all 49 file changes here on github. I have reverted the branch to the second last commit on my local machine though. Also I ran the checks and it found checkstyle warnings. It would be nice if you could get rid of them. If you are using Eclipse, here is a guide on how to use the checkstyle plugin: https://openems.github.io/openems.io/openems/latest/contribute/coding-guidelines.html#_tool_support
Other than that I have had a brief look at your code and compared it with my code, but it will take some time until my review is completed. I see that you took care of the number of piles, which is good. In my implementation I ignored it as my test object only has one pile.
Also we have had some experience with alarm flags being raised, in particular low temperature alarms, as it is quite cold here. Maybe we can use the data I got later on to fully implement the BatteryProtection.
Best regards, Thomas
Hi Thomas,
Thank you for checking out and inspecting the code.
I will tidy up the code and push another commit in a while fixing the checkstyle issues. I will also remove the additional files as well. Strangely I had thought I removed the last commit on here but it is there again now. In any case I will tidy it up.
With regards to the low temperature alarms, we have also experienced these and I would like to know more about your experience with this. We also had this issue over the past few weeks where we were unable to charge/discharge the battery when the cell temperature was below 10 degrees.
I spoke with Pylontech's engineer who said that the behaviour of the battery is as follows:
- < 10°C - Low temperature alarm - The battery can still charge and discharge, but current is reduced
- < 0°C - Low temperature protection - The battery relay/contactor will open and the battery cannot charge/discharge
In my code I have encoded the temperature alarm as io.openems.common.channel.Level.FAULT, which means that the StateMachine prevents the battery and ESS from running when this is raised. This is probably not the correct approach, instead I think it would make sense to allow charge/discharge but ensuring that the current limits are respected by the EMS & Inverter.
What do you think? How have you handled this in your code?
More generally, I have encoded all the Pylontech "Alarm" channels as Level.FAULT. Given Pylontech's explanation of the difference between "Alarm" and "Protection", these should probably be Level.WARNING.
Yes, I also had them on Level.FAULT first and changed it to Level.WARNING so that the state machine doesn't go into error and you can still use the battery. I see that the charge max current goes down to 30A, and even to 2A if it is very cold. Then the Generic ESS will automatically take care of it by adjusting its allowed charge power.
@thomasjbhayes @tsicking
Just a short note: I am a bit unlucky about Pylontech naming the Temperature Flag as "TEMPERATURE_ALARM"
Because I would intuitivly map an ALARM to an OpenEMS Fault-State. Other BMSes provide warnings, errors and alarms.
Pylontech only offers ALARMS.
So to be more consistent on the OpenEMS side I would prefer to map the Temperature Alarm modbus registers to temperatureWarning OpenEMS Channels. and make them of type Level.WARNING. This confuses the developer looking at the pylontech driver bundle. But it is more intuitive for the guys operating different battery vendors.
Also I would probably add an UnderTemperature State.Fault channels when the temperature is below 0,1 or 2 °C to signal an error, when or before the battery goes offline.
@clehne @tsicking I agree that it is unfortunate that Pylontech named these registers as 'Alarm' when realistically they are warnings. I have changed all the alarm channels to Level.WARNING already but I think I should change the name too (to warning).
Pylontech use the term 'protection' for the statuses when the contactor/relay opens and the system shuts down due to an alarm condition (e.g temperature < 0). To me I feel intuitively that 'Alarm' is the best name for this status, but in this case I think that would invite too much confusion. Do you have a suggestion for these cases?
I have deleted the unneeded files for the other bundles from this PR in a new commit I added. Previously I tried to revert the old commit which just confused matters.
I have also ran the checkstyle plugin and the Eclipse code formatter and have dealt with almost all the issues. The outstanding issue is related to comments on very long lines of code. The Eclipse plugin is suggesting I do it one way (where the comment is split across multiple lines but all lines are aligned vertically), but the Checkstyle suggests something different. To me the code as it is now makes more sense from a style perspective. Is it ok if I keep the comment style as suggested by the Code Formatter or do I need to strictly follow the checkstyle?
It's my first time contributing to the public repo so I am trying to learn the proper convention.
I have pushed a couple of commits to rename the 'alarm' channels to 'warning' so that they will be more easily understood. I have left the 'Protection' channels as-is. I am open to renaming these if someone suggests a better name. I don't want to use 'Alarm' for these as I think it would be confusing given that Pylontech calls the 'Warnings' alarms.
I have fixed the checkstyle issues as well. Except for the ones I mentioned in my last comment where the checkstyle plugin is wanting me to change the indentation on the comments in a way that is contradictory to the Code Formatter. I will take your lead on the preferred styling for these comments.
One issue I am observing (related to the low temperature issue) is that I am not sure if the discharge current limits are being applied properly.
Currently I have the following in my debug log for the battery:
battery0[Running|SoC:96 %|Actual:905 V;0 A|Charge:907 V;1 A|Discharge:730 V;148 A|State:WARNING: Discharge low temperature warning.,Temperature Warning,Charge low temperature warning.]
We can clearly see that the charge current has been limited to 1A because of the low temperature (which seems to be what we would expect), but the max discharge current is 148A.
In the code for the battery, we are using the following to get the battery current limits:
m(BatteryProtection.ChannelId.BP_CHARGE_BMS, new SignedDoublewordElement(0x110A), ElementToChannelConverter.SCALE_FACTOR_MINUS_2), m(BatteryProtection.ChannelId.BP_DISCHARGE_BMS, new SignedDoublewordElement(0x110D), ElementToChannelConverter.chain(ElementToChannelConverter.SCALE_FACTOR_MINUS_2, ElementToChannelConverter.KEEP_NEGATIVE_AND_INVERT)),
I am using the BatteryProtection channels to apply the charge limits. I added the KEEP_NEGATIVE_AND_INVERT converter because the channel was reporting negative values.
It looks like the max discharge current is defaulting to the max value of 148A (which I believe is coming from the getInitialBmsMaxEverDischargeCurrent() which is set to 148A) side note: this needs to change: in the event that we have n battery stacks the max current will be n x 148A.
Is there something wrong with the code that reads the max discharge current? Does anyone have an alternative implementation that works better?
Hi, we are reading out the values from the device not using the BatteryProtection and it's the same behaviour: The charge max current goes down to 30A and then to 2A, but the discharge max current remains at 148A all the time.
Ok - I will validate with Pylontech about this behaviour just to be sure that the batteries are behaving correctly. Seems strange to me that there would only be limitations on charging and not discharging.
Have you experienced the behaviour of the battery at lower temperatures (below 0 degrees)? I believe the charge and discharge currents should go to zero, but I do not have the ability to cool the battery in our test setup to these temperatures so have not experienced it.
Hi @tsicking I have reviewed this with Pylontech and this is expected behaviour. The discharge current does not ramp down, due to the physical characteristics of the battery, the discharge current is relatively unaffected by temperature, until it reaches the threshold for the 'Protection' state at which it directly goes to zero.
I'm sorry; I accidentally pushed two commits... I can't find how to revert the PR to your last commit c4f4e5d527cd2ba5b7a3c193965b9d4d683b0569 (Fix checkstyle issues). Can you do it? Otherwise I'll find out tomorrow how I can do it. Best regards and sorry for the inconvenience...
Hi @tsicking I am not sure how to revert the commit on the PR - I will look again but if you have figured out how to do it that would be useful.
Hi @tsicking ,
Disregard my previous comment
I have now reverted the two most recent commits and force pushed to the branch.
Please try pulling the PR before making any changes
Hi @tsicking ,
Thanks for this initial review. I will work through these issues now and fix the comments too.
Thomas
Hi @tsicking Thank you for taking the time to review this PR. It was a very useful exercise as I realised there were several areas where I had not finished cleaning up code after my initial implementation. Especially with regards to the comments - most of them were obsolete and could be deleted (helping with the checkstyle too).
It is not passing the checkstyle tests and I have amended several aspects in accordance with your comments.
The BatteryProtectionDefinition is still one area where I am unsure - per my comment, we will not know the number of piles on initialisation and therefore I am not sure if we can put a current limit in the Definition.
More broadly, I have not tested the code that handles multiple piles in practice yet. We have only got one pile for testing, however we plan on testing with multiple over the coming months. I have received an MBMS from Pylontech and will soon test with this and a single pile (right now it is direct to pile).
My apologies, I realised I had left an occurrence of TimeLeapClock in one of the tests and this was causing the build to fail.
I am not sure why this did not cause my local build and tests to fail.
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
Hello,
My attention was away from this matter over the past couple of months. But I think the only outstanding thing was that I forgot to resolve one of @tsicking 's requested changes which I had previously actually worked on in the code.
I think I have marked all of the changes as done now - is it possible to merge the PR now?
As far as I can tell, the only unresolved issue is around the current limits in the BatteryProtection. I think it is best to not include these in BatteryProtection as the current will depend on the number of piles which is a dynamic variable read from the BMS - we will not have access to this when the BatteryProtection is instantiated so better to rely on the value coming from the BMS. In any case the BMS will provide this protection of the battery as well.
WDYT? @tsicking
Hello @tsicking ,
Just returning to this PR. Can you please let me know what you think about my logic around the Battery Protection in the comment above?
Thanks, Thomas
Codecov Report
Attention: Patch coverage is 88.19095% with 94 lines in your changes missing coverage. Please review.
Additional details and impacted files
@@ Coverage Diff @@
## develop #2388 +/- ##
=============================================
+ Coverage 58.50% 58.71% +0.22%
Complexity 159 159
=============================================
Files 2544 2556 +12
Lines 108999 109795 +796
Branches 8028 8049 +21
=============================================
+ Hits 63754 64454 +700
- Misses 42906 42983 +77
- Partials 2339 2358 +19
:rocket: New features to boost your workflow:
- :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
- :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
![codecov[bot] avatar](https://avatars.githubusercontent.com/in/254?v=4 "Published by a pub.dev verified publisher"){kind=small}
Hi @tsicking
I have updated the PR to reflect your comments above. I have decided to leave the issue of the number of piles / initial battery protection current for later.
I have updated the switch statement syntax etc.
I have also brought the branch up to date with origin/develop via a merge - there were a few changes to BatteryProtection and other APIs which have required a few changes to the code but all seems good.
The tests and builds are passing now, however 'codecov/project' is failing now.
I have tested the merged code with the physical device and it seems to be ok.