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

Fix tc secc ac vtb power delivery 010 #150

Merged
merged 1 commit into from
Nov 16, 2022

Conversation

ikaratass
Copy link
Collaborator

@ikaratass ikaratass commented Oct 19, 2022

fix AB#2853

[V2G2-225] The SECC shall send the negative ResponseCode
FAILED_ChargingProfileInvalid in the PowerDelivery response message if the EVCC sends a ChargingProfile which
is not adhering to the PMax values of all PMaxScheduleEntry elements according
to the chosen SAScheduleTuple element in the last ChargeParameterDiscoveryRes
message sent by the SECC.

iso15118/secc/states/iso15118_2_states.py Outdated Show resolved Hide resolved
Comment on lines 206 to 211
def get_v2g_message_power_delivery_req_charge_start_invalid_charging_profile():
charging_profile = ChargingProfile(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to add two tests that covers the above mentioned use case as well - which is return a charging profile that doesn't fall on the same boundary as the chosen schedule - but is in line with the limits and one that is not.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

ResponseCode.FAILED_CHARGING_PROFILE_INVALID,
)
return

# TODO We should also do a more detailed check of the charging profile
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line could be removed as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

iso15118/secc/states/iso15118_2_states.py Outdated Show resolved Hide resolved
@ikaratass ikaratass force-pushed the fix_TC_SECC_AC_VTB_PowerDelivery_010 branch from 7e423a9 to 13d255b Compare October 24, 2022 16:19
iso15118/secc/states/iso15118_2_states.py Outdated Show resolved Hide resolved
iso15118/secc/states/iso15118_2_states.py Outdated Show resolved Hide resolved
iso15118/secc/states/iso15118_2_states.py Outdated Show resolved Hide resolved
@ikaratass ikaratass requested a review from tropxy October 28, 2022 12:12
tropxy
tropxy previously approved these changes Oct 29, 2022
Comment on lines 1650 to 1699
if sa_profile_entry_start <= ev_profile_entry.start <= sa_profile_entry_end: # noqa
ev_entry_pmax = (ev_profile_entry.max_power.value
* 10 ** ev_profile_entry.max_power.multiplier) # noqa
if ev_entry_pmax > sa_entry_pmax:
return False
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can add an else clause and break the loop. I know you said that it would stop also the outer loop, but cant be, I just tested:

In [1]: test_array = [1]*10

In [2]: test_array
Out[2]: [1, 1, 1, 1, 1, 1, 1, 1, 1, 1]

In [3]: test_array_2 = [2]*20

In [4]: for idx1 in test_array:
   ...:     count = 0
   ...:     for idx2 in test_array_2:
   ...:         count += 1
   ...:         print("the count is %s" % count)
   ...:         if count == 5:
   ...:             break
   ...:
the count is 1
the count is 2
the count is 3
the count is 4
the count is 5
the count is 1
the count is 2
the count is 3
the count is 4
the count is 5
the count is 1
the count is 2
the count is 3
the count is 4
the count is 5
the count is 1
the count is 2
the count is 3
the count is 4
the count is 5
...

as you see, with the code above, the inner loop stops at the count of 5, but the outer loop continues until the end

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, I went over this and realized that that was not the problem. the code was breaking the loop, but it needed some more logic to work as expected. I added it and also included a more elaborated test and looks good ;)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I tested as well, the problem wasn't this, we should check all profile entries to find the correct range to be checked. it passed all unit tests. Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, we had to check all the entries with the logic before, but the more EV profile entries more time it would take as in the worst case scenario, if we have N EV profile entries and M SA schedule entries, we would do NxM checks all the time. And the rationale is that the EV entries we have already checked during one iteration, do not need to be reevaluated for the next one.

@ikaratass ikaratass force-pushed the fix_TC_SECC_AC_VTB_PowerDelivery_010 branch from fbd1f2c to 68d1595 Compare October 31, 2022 15:09
@shalinnijel2
Copy link
Contributor

shalinnijel2 commented Oct 31, 2022

I think we have missed one use case where the ev profile spans over two sa schedule entries.
In my test, the below given use case passed when it should have failed:
SA schedule:

[
PMaxScheduleEntry(p_max=PVPMax(value=11000, multiplier=0, unit=<UnitSymbol.WATT: 'W'>), time_interval=RelativeTimeInterval(start=0, duration=None)), 
PMaxScheduleEntry(p_max=PVPMax(value=7000, multiplier=0, unit=<UnitSymbol.WATT: 'W'>), time_interval=RelativeTimeInterval(start=1800, duration=1800))
]

EV Charging Profile:

[
ProfileEntryDetails(start=0, max_power=PVPMax(value=11000, multiplier=0, unit=<UnitSymbol.WATT: 'W'>), max_phases_in_use=3), 
ProfileEntryDetails(start=1200, max_power=PVPMax(value=11000, multiplier=0, unit=<UnitSymbol.WATT: 'W'>), max_phases_in_use=3), 
ProfileEntryDetails(start=1900, max_power=PVPMax(value=7000, multiplier=0, unit=<UnitSymbol.WATT: 'W'>), max_phases_in_use=3)
]

Notice the second entry with Pmax 11000W spans over the 1800secs mark from which point only 7000W is available.

@ikaratass ikaratass force-pushed the fix_TC_SECC_AC_VTB_PowerDelivery_010 branch from de1fe00 to 5d7dba2 Compare November 15, 2022 10:15
schedule p_max values are checked against if exceed the boundaryC

enhanced the ev profile profile validation by skipping entries that were already scanned and tested
@ikaratass ikaratass force-pushed the fix_TC_SECC_AC_VTB_PowerDelivery_010 branch from 5d7dba2 to 341988a Compare November 15, 2022 17:17
@ikaratass ikaratass merged commit 636fbb5 into master Nov 16, 2022
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