-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Vehicles: add maximum power planner hint #25303
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
base: master
Are you sure you want to change the base?
Vehicles: add maximum power planner hint #25303
Conversation
|
@anndig this may fix the 1p/3p discussion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
For what purpose? |
|
Planner will estimate correct time. For example my BMW iX3 can only charge with maximum 11kW. But with single phase it can be charged with 32A. |
|
It would be nice to have the discussion in #25079 and honor #25079 (comment) before we build partial solutions. |
|
Ah yes I see. |
|
We could merge both solutions couldn't we? This solution is better for 1p/3p maximum current mismatch that some vehicles have. |
Based on how I understand the impact of the proposed changes: Is it really necessary to include the calculation of the current based on maximum vehicle power into setLimit for that purpose? Wouldn't it be enough to change effectiveMaxPower? Look at e.g. #25239, the car has a limit of 7.4 kW, regardless of 3 or 1 phase(s). This results in a 3 phase current of 10.72 A. When using a charger without mA-controll, this results in 10 A. To actually achieve the 7.4 kW, the vehicle even needs at least 12 A signalled current. Even usual 11 kW vehicles typically charge with about 10.4 to 10.6 kW on an 11 kW charger, without mA control, this seems to limit the sognalled charge current to 15 A, this charging with an even lower speed. |
|
That's correct, changing effectiveMaxPower would be sufficient. The other changes are just for not seeing the gap between the offered current and the consumed current in the UI. Regarding the consumed current you are right that there are losses, but we should offer the correct current anyway -> so for 11kW 16A at 3p and for 7,3kW 32A at 1p or 11A at 3p ( I know it would be 7,5kW). This way we can make sure that always enough current will be provided. We can leave the efficiency at 0,9 and this will make sure that the planner will start slightly before the "perfect" start time. Trying to estimate the correct efficiency will never work. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3 issues found across 8 files
Prompt for AI agents (all 3 issues)
Understand the root cause of the following 3 issues and fix them.
<file name="i18n/de.json">
<violation number="1" location="i18n/de.json:579">
Fix the typo in the German help text so "Fahrzeug" is spelled correctly and the UI copy is accurate.</violation>
</file>
<file name="core/loadpoint_effective.go">
<violation number="1" location="core/loadpoint_effective.go:178">
`maxCurrent` is no longer limited when the vehicle reports only a max current or only a max power value, allowing the charger to exceed the vehicle spec.</violation>
<violation number="2" location="core/loadpoint_effective.go:243">
`lp.vehicle` is dereferenced without a nil check, causing a panic whenever the loadpoint has no active vehicle.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
|
@cubic-dev-ai review this PR |
@marcelGoerentz I've started the AI code review. It'll take a few minutes to complete. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 issue found across 8 files
Prompt for AI agents (all 1 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="core/loadpoint_effective.go">
<violation number="1" location="core/loadpoint_effective.go:235">
Calling lp.vehicle.OnIdentified() before verifying lp.vehicle is non-nil will panic when no vehicle is attached. Check for lp.vehicle == nil before dereferencing.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
b9f5917 to
54ad821
Compare
|
I think it's a good solution for the 1p 3p charge current problem. More elegant than the often requested different maxCurrent settings for 1p and 3p, while at the same time, it also improves the accuracy of the planner. |
|
This is an improvement to the planner, as it now respects the vehicles charging power limit. It simplifies the handling for 1-phase and 3-phase charging, since the user doesn't need to manually switch the loadpoint configuration. |
Could you clarify? Would someone line to propose a description and help text for such a parameter if it were added? /cc @premultiply wdyt? |
|
I think that sounds like a viable option, at least. |
Sure. Currently, you need to adjust the loadpoint's max current for the Planner to plan correctly when the maximum current differs between 1‑phase (32A) and 3‑phase (16A). |
|
I don‘t understand. It will dtill switch to fast charging and try to set 3p@32A, so you MUST limit to 16A. Doesn‘t this create more confusion? |
|
There are a couple of vehicles that allow 1 phase charging with up to 32 A (e.g. our BMW i4), but only 16 A on 3 phases. This is great since it closes that gap between 3.6 and 4.1 kw. With the current maxCurrent settings, you have to decide to either not use more than 16 A on one phase or inaccurate planning. With maxPower, evcc will know that there is a limit on 3 phases but is still able to use the full range for 1 phase. |
|
The other thing it resolves is planning for vehicles that charge with significantly lower power that offered. E.g. #25239, the car has a limit of 7.4 kW, regardless of 3 or 1 phase(s). This results in a 3 phase current of 10.72 A. When using a charger without mA-control, this results in 10 A. To actually achieve the 7.4 kW, the vehicle needs at least 12 A signalled current. With maxPower, you can signal those 12 A or more while the planner calculates with the correct 7.4 kW. The only way to do this currently is to increase the configured capacity, but people don't understand this, so there are quite some issues and discussions here asking on how to deal with this issues. I think allowing to configure the charge power is easier to understand. |
|
@mfuchs1984 Yeah, you are right. |
...so you'd statically configure 32A in this case?
It would still only do this for the planner when estimating. It would still switch to 32A. Imho this is prone to a lot of confusion! This (amongst others) is why I've asked:
I still don't see if this really is a vehicle setting or a planner hint or a no-go. |
Me too, I think this is how it implemented @marcelGoerentz |
|
Correct, it is only for the Planner when estimating. |
|
|
@marcelGoerentz I think you should change the title of the PR. It's misleading since |
Signed-off-by: Marcel Goerentz <[email protected]>
Signed-off-by: Marcel Goerentz <[email protected]>
Signed-off-by: Marcel Goerentz <[email protected]>
Signed-off-by: Marcel Goerentz <[email protected]>
Signed-off-by: Marcel Goerentz <[email protected]>
Signed-off-by: Marcel Goerentz <[email protected]>
Signed-off-by: Marcel Goerentz <[email protected]>
Signed-off-by: Marcel Goerentz <[email protected]>
54ad821 to
d75654a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes - here's some feedback:
- The new
maxPowerparameter is declared asname: maxpowerindefaults.yamlbut referenced asmaxPowerelsewhere (templates, presets, Go structs), which is likely to break binding; align the naming consistently (including case) across all usages. - Consider adding
maxPowerto theinitialValuesobject inVehicleModal.vueso that the form has a defined default and behaves consistently with the other configurable vehicle fields.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `maxPower` parameter is declared as `name: maxpower` in `defaults.yaml` but referenced as `maxPower` elsewhere (templates, presets, Go structs), which is likely to break binding; align the naming consistently (including case) across all usages.
- Consider adding `maxPower` to the `initialValues` object in `VehicleModal.vue` so that the form has a defined default and behaves consistently with the other configurable vehicle fields.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@naltatis could you check regarding UI? I'm surprised these fields actually need hard-coding? |
Signed-off-by: Marcel Goerentz <[email protected]>
|
@naltatis please check ui part/ tempalte. LGTM. |
Added missing periods to the help descriptions for maximum vehicle charging power.
Fix #23509
Make maximum power consumption configurable for vehicles