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

[Request] charging to SoC target with defined power after "Immediate charging requested" by Pylontech #723

Closed
gitisgreat2023 opened this issue Mar 7, 2024 · 19 comments
Assignees
Labels
enhancement New feature or request

Comments

@gitisgreat2023
Copy link

gitisgreat2023 commented Mar 7, 2024

Is your feature request related to a problem? Please describe.

When the SoC, for whatever reason, drops so low that an immediate charge is requested by the Pylontech battery, to avoid damage, currently nothing happens. Though a controllable AC charger is present and could charge the battery again to safe level.

Describe the solution you'd like

When "Immediate charging requested" switches from "No" to "Yes" the AC charger should charge to an specified SoC Target with a to be specified power.
Also it would be good if any of these situations would be logged.

Describe alternatives you've considered

Use an external script (like NodeRed) which monitors via MTTQ the "Immediate charging requested" state and sets via MTTQ the charge power until the target SoC is reached.

Additional context

Currently I use a minimal SoC of 20%. Implementing this automatically charging feature would enable to put for example a SoC of 15% as minimum, of even 10%. With the logging one could decide to top balance (charging 100%) or put the minimal SoC higher to avoid such immediate charge request.

@gitisgreat2023 gitisgreat2023 added the enhancement New feature or request label Mar 7, 2024
@schlimmchen
Copy link
Member

@MalteSchm Is this something you'd like to takle?

@MalteSchm
Copy link

MalteSchm commented Mar 10, 2024

My system tells me: yes I should address that... Here is why:

I just checked my dashboard and found all alarms red. It seemed that the inverter operated at full power with communication between openDTU and the inverter being broken. Soft and hard restarts of the ESP were unsuccessful. I got the system back by issuing an inverter reset command.

I think that resetting the inverter when communication is broken for a long time and starting up the PSU when charging is requested would be countermeasures that should be implemented.

So in general: the battery SoC drops to that level it is likely that the inverter runs mad. Implementing only the PSU solution is not sufficient IMHO.
I think inverter restart and an ESP reset should be carried out in this case

@schlimmchen
Copy link
Member

Meh... You, too? There are other users who recently started reporting that their inverter is somehow stuck and must be revived through a reboot command, as even with my more recent changes that repeat limit updates and power requests their inverter somehow manages to keep in standby... This is weird...

The DPL could (quite easily with the recent changes) detect if the inverter is not reaching the desired state for a longer time and trigger issuing a reboot message. Should we do that? Can you open a respective issue if you think that's desirable?

and an ESP reset

That would be a last resort, i.e., after issuing the reboot command did not do the trick.

@gitisgreat2023
Copy link
Author

gitisgreat2023 commented Mar 11, 2024

@MalteSchm
That's sounds a quite scary, an inverter going rogue... how low went the SoC on your Pylontech, did he shutdown due to undervoltage? Any idea why the inverter got in that situation?

To be prepared: "I got the system back by issuing an inverter reset command.": how do I do this? This is not possible in the WebUI, right?

@MalteSchm
Copy link

MalteSchm commented Mar 11, 2024

@schlimmchen

Yes I had that twice now. There could have been other cases that went without me noticing. The inverter is reset in regular intervals at night so the issue will resolve over time. The battery stays fully discharged during this period, potentially even disconnected as the BMS will cut the cord at some point.

I think changing the DPL this way is desirable yes. And I also think we should trigger an ESP reset as a last resort

@gitisgreat2023

First time: 0%, yesterday 2%. I think the inverter did shut off at this stage but: Once the PSU started charging the inverter also started up again.
You can reset ("Neustart") the inverter in the web-ui. This is accessible from the buttons in the main page / dashboard. Right next to the buttons to turn the inverter off and on

@MalteSchm
Copy link

I just checked my dashboard and found all alarms red. It seemed that the inverter operated at full power with communication between openDTU and the inverter being broken. Soft and hard restarts of the ESP were unsuccessful. I got the system back by issuing an inverter reset command.

I think that resetting the inverter when communication is broken for a long time and starting up the PSU when charging is requested would be countermeasures that should be implemented.

Created #748

@gitisgreat2023
Copy link
Author

@MalteSchm
0% and 2% ain't good... yes we definitely need a feature coping with this. We should under all circumstances avoid that the inverter goes rogue and the PSU starts charging, creating a high power circular flow. If one is on holidays that could get dangerous.

First time: 0%, yesterday 2%. I think the inverter did shut off at this stage but: Once the PSU started charging the inverter also started up again. You can reset ("Neustart") the inverter in the web-ui. This is accessible from the buttons in the main page / dashboard. Right next to the buttons to turn the inverter off and on

I'm stupid I guess: next to the turn on/off there's the grid info:
image

image

Or do you meant the on/off is the inverter reset?

@MalteSchm
Copy link

MalteSchm commented Mar 12, 2024

@MalteSchm 0% and 2% ain't good... yes we definitely need a feature coping with this. We should under all circumstances avoid that the inverter goes rogue and the PSU starts charging, creating a high power circular flow. If one is on holidays that could get dangerous.

First time: 0%, yesterday 2%. I think the inverter did shut off at this stage but: Once the PSU started charging the inverter also started up again. You can reset ("Neustart") the inverter in the web-ui. This is accessible from the buttons in the main page / dashboard. Right next to the buttons to turn the inverter off and on

I'm stupid I guess: next to the turn on/off there's the grid info: image

image

Or do you meant the on/off is the inverter reset?

Click on this on/off button. There is more...

@MalteSchm
Copy link

@schlimmchen
How do I get the PylontechBatteryStats needed for _chargeImmediately

This did not work:
auto stats = static_cast<PylontechBatteryStats>(Battery.getStats());

@schlimmchen
Copy link
Member

You can't. Battery returns a std::shared_ptr<BatteryStats>, i.e., a pointer to the base class BatteryStats, and if you were to cast this to a PylontechBatteryStats, you would need some fancy std::dynamic_cast I guess... The reason I don't know is because I avoid stuff like this at all cost. We shouldn't use it or do it like that. And if we were, we would need to be sure that the instance is actually an instance of the derived PylontechBatteryStats before we attempt such a cast.

Your request is valid, of course, so lets work out a solution.

The goal is to know the value of _chargeImmediately in HuaweiCanClass. The value is generated in PylontechCanReceiver and then saved in PylontechBatteryStats.

We can't ask PylontechCanReceiver, as there is no public instance. Class Battery manages that instance in a private std::unique_ptr<BatteryProvider>.

That might sound cumbersome, but this kind of encapsulation, separation of concern, and ownership rules, saves us headaches in other contexts.

So I think the best way is to define a new method bool needsCharging() in the BatteryStats base class. In particular, this gives the other BatteryStats derived classes a chance to implement the same logic based on their battery provider implementation, i.e., the JK BMS can also implement this method and return true to facilitate this feature. Same for the SmartShunt, which might also want to trigger this feature.

This answer war quite verbose, but maybe you appreciate knowing my train of thought.

Please rebase onto 56353e4 😊

@gitisgreat2023
Copy link
Author

@MalteSchm 0% and 2% ain't good... yes we definitely need a feature coping with this. We should under all circumstances avoid that the inverter goes rogue and the PSU starts charging, creating a high power circular flow. If one is on holidays that could get dangerous.

First time: 0%, yesterday 2%. I think the inverter did shut off at this stage but: Once the PSU started charging the inverter also started up again. You can reset ("Neustart") the inverter in the web-ui. This is accessible from the buttons in the main page / dashboard. Right next to the buttons to turn the inverter off and on

I'm stupid I guess: next to the turn on/off there's the grid info: image
image
Or do you meant the on/off is the inverter reset?

Click on this on/off button. There is more...

Thanks!
As an interim solution, until these features are built in and released I now are able to access remotely my OpenDTU-on-Battery, for example when I'm not at home.
I activated on my Fritz!Box VPN and know I can easily control my setup remotely as well by phone.
Note: if the phone disconnects from WiFi and no LTE appears (for example on my Google Pixel) while trying to connect to the Fritz!Box VPN server, then force on the mobile data settings of the phone ipv4, see for example here.

@Linos88
Copy link

Linos88 commented Mar 14, 2024

I also like the original idea with forced loading. This function can also be useful for a long service life in winter. Instead of using the SoC in the limit range, the voltage would be more suitable. The SoC can stop if there is only a small standby consumption and the voltage still continues to fall

@Manos1966
Copy link

@MalteSchm Do you have the dtata stored from the time your inverter went rogue?
Can it be that the Efficiency % rating of the inverter went from 96% down to something around 60%?

This is the reason why I have a SHELLY 1PM connected to the 230V side of the inverter.... 😉

@ottelo9
Copy link

ottelo9 commented Mar 15, 2024

@MalteSchm Do you have the dtata stored from the time your inverter went rogue? Can it be that the Efficiency % rating of the inverter went from 96% down to something around 60%?

This is the reason why I have a SHELLY 1PM connected to the 230V side of the inverter.... 😉

Really good idea. I'll probably install a Zigbee relay right now.

@CaCu15
Copy link

CaCu15 commented Mar 17, 2024

You can't. Battery returns a std::shared_ptr<BatteryStats>, i.e., a pointer to the base class BatteryStats, and if you were to cast this to a PylontechBatteryStats, you would need some fancy std::dynamic_cast I guess... The reason I don't know is because I avoid stuff like this at all cost. We shouldn't use it or do it like that. And if we were, we would need to be sure that the instance is actually an instance of the derived PylontechBatteryStats before we attempt such a cast.

Your request is valid, of course, so lets work out a solution.

The goal is to know the value of _chargeImmediately in HuaweiCanClass. The value is generated in PylontechCanReceiver and then saved in PylontechBatteryStats.

We can't ask PylontechCanReceiver, as there is no public instance. Class Battery manages that instance in a private std::unique_ptr<BatteryProvider>.

That might sound cumbersome, but this kind of encapsulation, separation of concern, and ownership rules, saves us headaches in other contexts.

So I think the best way is to define a new method bool needsCharging() in the BatteryStats base class. In particular, this gives the other BatteryStats derived classes a chance to implement the same logic based on their battery provider implementation, i.e., the JK BMS can also implement this method and return true to facilitate this feature. Same for the SmartShunt, which might also want to trigger this feature.

This answer war quite verbose, but maybe you appreciate knowing my train of thought.

Please rebase onto 56353e4 😊

@schlimmchen:

I would like to extend this approach with the following suggestion: Currently the automatic charge control (Huawei automatic charge control) does NOT consider the feedback from the BMS during charging, especially the "charge currrent limitation" as provided by the Pylontech BMS (don't know abut the other BMSs). In order to support this, it would be good to have an additional method float getChargeCurrentLimitation() that returns the respective value returned by the BMS (and currently by REST call api/batterylivedata/status). This would make this value available to the automatic power control in HuaweiCanClass::loop() to limit the charge like this:

        // Set the actual output limit
        float efficiency =  (_rp.efficiency > 0.5 ? _rp.efficiency : 1.0); 
        float outputCurrent = efficiency * (newPowerLimit / _rp.output_voltage);
        
        // new code
        // assuming getChargeCurrentLimitation returns something > than any current supported by the Huawei, e.g. 100A 
        // if not supported by BMS 
        float chargeCurrentLimitation = Battery.getStats().getChargeCurrentLimitation(); 
        outputCurrent = std::min(outputCurrent, chargeCurrentLimitation);

@MalteSchm
Copy link

@CaCu15

I already have that implemented but still need to test it

@MalteSchm
Copy link

Draft PR in #767

@schlimmchen
Copy link
Member

I consider this closed as completed since #767 was merged into development and will be released eventually.

Copy link

github-actions bot commented May 6, 2024

This issue 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 May 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

7 participants