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

prevent shaper to control charge_current if divert mode is active #478

Merged
merged 2 commits into from
Nov 15, 2022

Conversation

KipK
Copy link
Collaborator

@KipK KipK commented Nov 15, 2022

This should fix some priority issues I'had been reported.
There's no need of shaper doing it's job when divert is active in eco mode

@jeremypoulter
Copy link
Collaborator

Been meaning to mention this before but the current shaper should be setting the max_charge_current rather than the charge _current to stop issues like this.

@KipK
Copy link
Collaborator Author

KipK commented Nov 15, 2022

Been meaning to mention this before but the current shaper should be setting the max_charge_current rather than the charge _current to stop issues like this.

There's a good reason why.

If you claim max_current from /override , the shaper will be ignored,
if you claim max_current from /claim using HTTP api, ( priority 500 ) , then the shaper will have the priority, but will ignore the max_current user has set before.

By using charge_current, this allows setting a limit for shaper too. Whatever the cpriority of other services controling charge_current.
Means setting a max_current claim at 20A on the UI, will limit shaper 20A max, then shaper will works in the lower range.

Also doing this way allows any client_id with any priority to set max_current as a limit, that will be taken into account.

@KipK
Copy link
Collaborator Author

KipK commented Nov 15, 2022

btw, don't merge yet, I have an issue, not sure coming from this one.
Have to investigate first

@KipK
Copy link
Collaborator Author

KipK commented Nov 15, 2022

spotted & fixed the issue. All good sir 👍

@glynhudson glynhudson merged commit f6ca6f5 into OpenEVSE:master Nov 15, 2022
@jeremypoulter
Copy link
Collaborator

The UI should not be using a claim/override to set the current, that is a device configuration so should be using /config

@KipK
Copy link
Collaborator Author

KipK commented Nov 16, 2022

It can set only a lower value than current_max_soft.
This is to allow limiting manually without touching the max_current_conf as it can auto_release

@KipK KipK deleted the shaper branch November 16, 2022 10:23
@KipK
Copy link
Collaborator Author

KipK commented Nov 16, 2022

There's some case for that. I have a second EVSE charger running on another car for a guest.
I need to temporary limit one to half the available current, and allow the other use shaper with the rest available.

Or I just need for a short moment during a charge for whatever reason ( ex It's too hot outdoor ) to limit it to 6A

@KipK
Copy link
Collaborator Author

KipK commented Nov 16, 2022

@jeremypoulter , sending an /override with max_current properties is not visible in the /claims/target endpoint
Other props are ok.
Also openevse don't take it in accout.
doing it with /claims works

Is it a bug ?

I can change the shaper behavior to use max_current, and use charge_current on ui side, but we won't be able then to set the charge_current when divert is on .

Use case: one would want to limit current temporary to let some free energy for other stuff in the house. He don't need to use 100% all the time.

If I switch to charge_current this use case has no solution then. But I can leave without ( has no solar here ), u tell me

@KipK
Copy link
Collaborator Author

KipK commented Nov 16, 2022

seems related to shaper, as property appears in claims/target/ if shaper & work is off. I have to find why

edit: mhh no it's worst, that's a bug.
I'll open a ticket

@KipK
Copy link
Collaborator Author

KipK commented Nov 16, 2022

opened ticket here: #483

I've found where it comes from , dunno if I've introduced it or it was there before, will give a look

@KipK
Copy link
Collaborator Author

KipK commented Nov 16, 2022

@jeremypoulter
Copy link
Collaborator

I think you wrote that bit, not sure what you were trying to occomplish, but feel free to change.

I can change the shaper behavior to use max_current, and use charge_current on ui side, but we won't be able then to set the charge_current when divert is on .

Use case: one would want to limit current temporary to let some free energy for other stuff in the house. He don't need to use 100% all the time.

IMHO I think this is correct, the manual override should set the charge current (including overriding divert). The user is saying that they want to charge at a specific current and that is the current that it should charge at. They are making a consious desission to do that and will (hopefully) understand the consiquencies. They also need the ability to configure the (soft) max current separately as a global config setting via /config, so the expectation is there is a factory/install hardware maximum, a user configured maximum (preserved over reboot), and a dynamic maximum for load balancing/safety (not preserved) I think that coveres the use cases you have mentioned.

The other option (and I think we have discussed this before) is to make the max_charge_current select the lowest value rather that the highest priority value. This might help when implementing things like tempurature throtaling or another safety feature so it mixes correctly, this does however add an inconsistency with the rest of the properties, so may be better to split into a separate interface/endpoint.

@KipK
Copy link
Collaborator Author

KipK commented Nov 17, 2022

Ok I'll go this way 👍
This will breaks a bit some installs, we will have to display a change warning for the next realease

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.

3 participants