-
Notifications
You must be signed in to change notification settings - Fork 127
libiso15118: d2: ChargeParameterDiscovery #1747
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: feat/adding-iso2-support
Are you sure you want to change the base?
libiso15118: d2: ChargeParameterDiscovery #1747
Conversation
26677a9 to
e21ac4a
Compare
3e7b5e4 to
5c7dded
Compare
Added ChargeParameterDiscovery req/res with tests. There are still a couple of TODOs in the convert functions. These will be resolved after settling the conversation in EVerest#1678. Signed-off-by: Kacper Dalach <[email protected]>
Added bounds checking for ChargeParameterDiscovery messages. Signed-off-by: Kacper Dalach <[email protected]>
5c7dded to
73f3a93
Compare
SebaLukas
left a comment
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.
Overall looking good 👍
See my comments
| std::optional<data_types::AcEvChargeParameter> ac_ev_charge_parameter; | ||
| std::optional<data_types::DcEvChargeParameter> dc_ev_charge_parameter; |
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.
Instead of two optionals I would like to have one std::variant with the Ac and DC EvChargeParameter.
| std::optional<data_types::AcEvseChargeParameter> ac_evse_charge_parameter; | ||
| std::optional<data_types::DcEvseChargeParameter> dc_evse_charge_parameter; |
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.
Same here
| auto& schedule_out = out.SAScheduleList.SAScheduleTuple.array[i]; | ||
| convert(schedule_in, schedule_out); | ||
| } | ||
| out.SAScheduleList.SAScheduleTuple.arrayLen = in.sa_schedule_list->size(); |
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.
Should be sa_schedule_list_max_length, not in.sa_schedule_list->size()
| entry_out.RelativeTimeInterval_isUsed = true; | ||
| convert(entry_in.p_max, entry_out.PMax); | ||
| } | ||
| out.PMaxSchedule.PMaxScheduleEntry.arrayLen = in.pmax_schedule.size(); |
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.
pmax_schedule_max_length instead of .size()
| auto& entry_out = out.SalesTariff.SalesTariffEntry.array[i]; | ||
| convert(entry_in, entry_out); | ||
| } | ||
| out.SalesTariff.SalesTariffEntry.arrayLen = in.sales_tariff->sales_tariff_entry.size(); |
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.
Same as above
| auto& cost_out = out.ConsumptionCost.array[i]; | ||
| convert(cost_in, cost_out); | ||
| } | ||
| out.ConsumptionCost.arrayLen = in.consumption_cost.size(); |
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.
Same problem
| entry_out.amount = entry_in.amount; | ||
| CPP2CB_ASSIGN_IF_USED(entry_in.amount_multiplier, entry_out.amountMultiplier); | ||
| } | ||
| out.Cost.arrayLen = in.cost.size(); |
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.
Same problem
Describe your changes
Added ChargeParameterDiscovery req/res with tests.
There are still a couple of TODOs in the convert functions regarding the array bounds checking. These will be resolved after we decide on how to handle this in #1678.
Because these messages can get quite complex and are quite different in AC and DC charging, the tests were split into multiple cases. Testing the SASchedules was also split into separate test, because this structure gets deep and complicated.
Issue ticket number and link
#1727
Checklist before requesting a review