Fix VD items subscriptions after PTU #3081
Conversation
AKalinich-Luxoft
changed the title
AKalinich-Luxoft
force-pushed
the
fix/fix_vd_items_subscription_after_ptu
branch
from
178a3b4
to
f2756a7
Compare
AKalinich-Luxoft
requested review from
LuxoftAKutsan,
AByzhynar,
IGapchuk,
LitvinenkoIra,
mked-luxoft,
mkorniichuk and
SNiukalov
| @@ -72,6 +72,7 @@ class VehicleInfoPlugin : public plugins::RPCPlugin { | |||
| VehicleInfoAppExtension& ext); | |||
|
|
|||
| private: | |||
| void UnsubscribeFromNotExistingVDItems(); | |||
There was a problem hiding this comment.
@AKalinich-Luxoft ...NonExisting...
There was a problem hiding this comment.
@AByzhynar fixed in 334284c
There was found an issue that SDL still kept subscriptions for custom vehicle data items even for case when they were removed during PTU. By that reason, SDL was crashing with DCHECK on attempt to unsubscribe from non existing custom data. There was added missing logic of internal unsubscribe in case some custom vehicle data was removed by policies.
AKalinich-Luxoft
force-pushed
the
fix/fix_vd_items_subscription_after_ptu
branch
from
f2756a7
to
334284c
Compare
Was added a missing part of logic of subscriptions control - cache manager was updated to keep the vehicle data which was removed by last PTU. Custom vehicle data manager was updated to consider the removed vehicle data and also prepare and send unsubscribe requests to HMI for case when some vehicle data was removed and some application was subscribed on it.
| ext.unsubscribeFromVehicleInfo(subscription_name); | ||
| typedef std::vector<std::string> StringsVector; | ||
|
|
||
| auto get_items_to_unsubscribe = [this](StringsVector& out_items) { |
There was a problem hiding this comment.
@AKalinich-Luxoft get ... name should return something. Make it return value and use like value = get...();
Or rename to write_items_to... (StringsVector& ..)
There was a problem hiding this comment.
@AByzhynar added a return type for a getter.
See 7aa5625
| }; | ||
|
|
||
| StringsVector items_to_unsubscribe; | ||
| get_items_to_unsubscribe(items_to_unsubscribe); |
There was a problem hiding this comment.
@AKalinich-Luxoft I would rather use it like
StringsVector items_to_unsubscribe = get_items_to_unsubscribe();Or even without get_ prefix StringsVector items_to_unsubscribe = ItemsToUnsubscribe()
There was a problem hiding this comment.
@AByzhynar I choose the option 1 with get_ prefix. It's a labmda and I am considering it as a local variable so kept appropriate naming. Please take a look at 7aa5625
| * @brief Gets removed vehicle data items | ||
| * @return Structure with vehicle data items | ||
| */ | ||
| virtual const std::vector<policy_table::VehicleDataItem> |
There was a problem hiding this comment.
@AKalinich-Luxoft What is the sense of returning of const by value? In returning by value - copying occurs.
There was a problem hiding this comment.
@AByzhynar good point. There is no sense while copying. Mostly it was a copy-past of the neighbour function :)
However, today I noticed one funny thing when removing const from return type - it makes sense when you are declaring smth like that:
auto& removed_oem_items = vehicle_data_provider_.GetRemovedVehicleDataItems();
So when const is removed from the return type, you will get a compilation error:
invalid initialization of non-const reference of type ‘std::vector<rpc::policy_table_interface_base::VehicleDataItem, std::allocator<rpc::policy_table_interface_base::VehicleDataItem> >&’ from an rvalue of type ‘std::vector<rpc::policy_table_interface_base::VehicleDataItem, std::allocator<rpc::policy_table_interface_base::VehicleDataItem> >’
That's because auto is deducted depending on function return type. So in that case it's enough just to add const if returned value is non-const:
const auto& removed_oem_items = vehicle_data_provider_.GetRemovedVehicleDataItems();
There was a problem hiding this comment.
@AByzhynar agree that there is no sense in const return type here. Removed it from everywhere in interfaces. See 7aa5625
| * @param items_before list of vehicle data items before PTU was applied | ||
| * @param items_after list of vehicle data items after PTU was applied | ||
| */ | ||
| void SetRemovedCustomVdItems( |
There was a problem hiding this comment.
@AKalinich-Luxoft I would recommend to divide this method into 2 :
- One calculates and returns the difference
- Second stores calculated difference into appropriate place
There was a problem hiding this comment.
@AByzhynar agree. Splitted on two separate functions. See 7aa5625
AKalinich-Luxoft
force-pushed
the
fix/fix_vd_items_subscription_after_ptu
branch
from
db06441
to
8b2a3e4
Compare
|
@AKalinich-Luxoft please advise when this is ready for Livio review |
|
@jordynmackool Please notice this PR is ready for review. |
|
Please notice our team decided to extend test coverage regarding related issue. |
|
@theresalech Please notice update is completed and PR is ready for review. |
...on_manager/rpc_plugins/vehicle_info_plugin/include/vehicle_info_plugin/vehicle_info_plugin.h
Outdated
Show resolved
Hide resolved
Fixes #3079 and #3080
This PR is ready for review.
Risk
This PR makes no API changes.
Testing Plan
Covered with ATF scripts
Summary
There was found an issue that SDL still kept subscriptions for custom vehicle data items even
for case when they were removed during PTU. By that reason, SDL was crashing with DCHECK on
attempt to unsubscribe from non existing custom data.
There was added missing logic of internal unsubscribe in case some custom vehicle data was removed by policies.
CLA