[WIP] Fix smart charger OCTT test cases #424
Conversation
…hedule even without active transaction
|
I will add unit tests for the other 3 TC so we can easily port the fixes to the Also, according to "chargingSchedulePeriod": {
"type": "array",
"items": {
"type": "object",
"properties": {
"startPeriod": {
"type": "integer"
},
"limit": {
"type": "number",
"multipleOf" : 0.1
},
"numberPhases": {
"type": "integer"
}
},
"additionalProperties": false,
"required": [
"startPeriod",
"limit"
]
}
},... so we cannot (or should not) return BUT, to keep the library CP agnostic, after discussion with @matth-x we agreed to make those configurable as defines MO_, still |
…ine s instead of hardcoded -1 to pass the OCTT SmartCharging tests
matth-x
left a comment
matth-x
left a comment
There was a problem hiding this comment.
@razvanphp Thanks again for the valuable fixes! Only change request is to keep the SmartChargingOutput pasing -1s if no limit is defined
| #ifndef MO_MaxChargingLimitNumberPhases | ||
| #define MO_MaxChargingLimitNumberPhases -1 | ||
| #endif | ||
|
|
There was a problem hiding this comment.
Good and simple solution for most use cases
| nextChange = basis + period->startPeriod; | ||
| nextChange = std::min(nextChange, basis + period->startPeriod); | ||
| const Timestamp candidate = basis + period->startPeriod; | ||
| nextChange = std::min(nextChange, candidate); |
|
|
||
| //if no TxProfile limits charging, check the TxDefaultProfiles for this connector | ||
| if (!txLimitDefined && trackTxStart < MAX_TIME) { | ||
| if (!txLimitDefined) { |
There was a problem hiding this comment.
This fixes the composite schedules. But also affects the ordinary charge limit output if no transaction is running. I cannot think of scenarios where this would be super bad though.
The usage of the global trackTxStart in calculateLimit() seems not to work well with composite schedules. In MO v2, it would be a possibility to add a new function parameter to calculateLimit() to alter the limit determination logic depending on where the output is used
Looks good for now!
| limit.nphases != std::numeric_limits<int>::max() ? limit.nphases : -1); | ||
| limit.power != std::numeric_limits<float>::max() ? limit.power : MO_MaxChargingLimitPower, | ||
| limit.current != std::numeric_limits<float>::max() ? limit.current : MO_MaxChargingLimitCurrent, | ||
| limit.nphases != std::numeric_limits<int>::max() ? limit.nphases : MO_MaxChargingLimitNumberPhases); |
There was a problem hiding this comment.
Would be nice to preserve the -1 value for "undefined" limit. It could make a difference on a charger if no charging schedule is set at all, or if a charging schedule is set and its limit just happens to match the electric rating
There was a problem hiding this comment.
But do you really think it matters? What would the user-land code do? it still needs to set that limit on the PWM duty-cycle, no matter if default or set.
Anyhow, do you have an idea how to preserve this AND also keep the fixes for OCTT? It would be strange to use those defs only in GetCompositeSchedule....
There was a problem hiding this comment.
For sure it doesn't matter much. There are few edge cases I can think of:
- You want to show on a device screen that Smart Charging is being used
- There are competing load balancing mechanisms and OCPP should overrule other mechanisms, if a schedule is defined
- I'm feeling a bit insecure about further edge cases I just don't know (happened a few times when I made breaking changes which put things to the worse for some users)
On the other hand, negative values as undefined values are easy to understand. And the application firmware needs some logic to sanitize the values anyway, because MO doesn't do a range check (you could define a 100A limit on a 32A charger at the moment).
The GetCompositeSchedule use case alone is a super valid reason to add a new build flag. Maybe a name like MO_CompositeScheduleDefaultPower would be a better fit then?
| periods.back().numberPhases = limit.nphases != std::numeric_limits<int>::max() ? limit.nphases : MO_MaxChargingLimitNumberPhases; | ||
| periods.back().startPeriod = periodBegin - startSchedule; | ||
| lastLimit = limit; | ||
| } |
| limit.nphases != std::numeric_limits<int>::max() ? limit.nphases : -1); | ||
| limit.power != std::numeric_limits<float>::max() ? limit.power : MO_MaxChargingLimitPower, | ||
| limit.current != std::numeric_limits<float>::max() ? limit.current : MO_MaxChargingLimitCurrent, | ||
| limit.nphases != std::numeric_limits<int>::max() ? limit.nphases : MO_MaxChargingLimitNumberPhases); |
2 participants
This PR makes all those tests pass:
BUT, it will only work for v1.1.0 and v1.2.0, it is not compatible with the upcoming develop/remodel-api branch. I think there @matth-x already fixed those bugs.
We should also have the max A of the charging station and number of phases as configuration variables probably, as OCTT expects a positive value there instead of
-1when there is no Profile installed.I could/should also add unit tests for this...
closes #360