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

[SDL 0173] - Read Generic Network Signal Data Implementation #2977

Merged
merged 29 commits into from
Aug 30, 2019

Conversation

LuxoftAKutsan
Copy link
Contributor

@LuxoftAKutsan LuxoftAKutsan commented Jul 29, 2019

Fixes #2840

This PR is ready for review.

Risk

This PR makes major API changes.

Testing Plan

Manual testing is done with smartdevicelink/sdl_hmi#210
Automated testing is done with smartdevicelink/sdl_atf_test_scripts#2217
Testing reports attached : RGNSD_test_reports.zip

Summary

Implementation of SDL evolution issue: smartdevicelink/sdl_evolution#768

Enhancements
  • Implemented generating of Policy enums from RPC spec xml. This is done to avoid hight coupling of generated API schema to application manager and policies. Also was done the additional generation of schemas to validate enum string according to RPC spec.

  • VehicleInfo commands and command factory refactored to get struct with the list of dependencies in constructor.

Bug Fixes
Known issues

CLA

@LuxoftAKutsan
Copy link
Contributor Author

@atiwari9 please approve feature implementation

@atiwari9
Copy link

Ford has reviewed and Approved the changes. Ready for Livio review

@LuxoftAKutsan LuxoftAKutsan changed the title Feature/read generic network signal data [SDL 0173] - Read Generic Network Signal Data Implementation Jul 29, 2019
@mkorniichuk mkorniichuk force-pushed the feature/read_generic_network_signal_data branch from 60b281d to 3ade2bc Compare August 1, 2019 11:39
@jacobkeeler
Copy link
Contributor

jacobkeeler commented Aug 23, 2019

A few error flows that I found while testing

  1. any mode

    • Connect app1
    • Send SubscribeVehicleData with any allowed vehicle data type: SUCCESS response is received
    • Disconnect app1, then reconnect again
    • Send SubscribeVehicleData with any allowed vehicle data type: expected SUCCESS, got DATA_NOT_AVAILABLE
  2. PROPRIETARY mode

    • Add custom vehicle data to preloaded policy table
    • Start Core
    • Connect an app with custom vehicle data permissions, then restart Core while still connected. Repeat this process several times (shouldn't need more than 10)
    • Check contents of policy table, vehicle_data_item_definition and
      vehicle_date_item_parameters are empty

This issue seems to be tied to the following message in the Core logs, and as implied by the multiple restarts it only happens sometimes:

ERROR [23 Aug 2019 15:55:14,244][0x7f8ca7fff700][Policy] /home/jacobkeeler/sdl_core/src/components/policy/policy_regular/src/sql_pt_representation.cc:2609 SQLPTRepresentation::InsertVehicleDataItem: Vehicle data item is not initialized.
TRACE [23 Aug 2019 15:55:14,244][0x7f8ca7fff700][Policy] /home/jacobkeeler/sdl_core/src/components/policy/policy_regular/src/sql_pt_representation.cc:1754 SQLPTRepresentation::SaveVehicleDataItems: Exit
TRACE [23 Aug 2019 15:55:14,244][0x7f8ca7fff700][Policy] /home/jacobkeeler/sdl_core/src/components/policy/policy_regular/src/sql_pt_representation.cc:1728 SQLPTRepresentation::SaveVehicleData: Exit

Edit: revised flow of issue 2

@LuxoftAKutsan
Copy link
Contributor Author

@jacobkeeler
Unable to reproduce the issues you specified.
Tried manually and also with the attached script.
Could you please attach logs for analysis?

All other comments addressed

@jacobkeeler
Copy link
Contributor

Issue 1: SmartDeviceLinkCore.log

Issue 2: SmartDeviceLinkCore.log

@LuxoftAKutsan
Copy link
Contributor Author

LuxoftAKutsan commented Aug 27, 2019

Issue 1 is an HMI issue fixed in smartdevicelink/sdl_hmi@cb240f4

Issue 2: It is known SDL issue: #2282
With fix available: #2501
Should we include this fix in this feature?

@jacobkeeler
Copy link
Contributor

jacobkeeler commented Aug 27, 2019

@LuxoftAKutsan
Looks like both of the links you provided are related to issue 1.

Issue 2 is a policy table corruption issue, which is completely unrelated to the HMI logic and #2282.

Considering it is an existing issue, I will review #2501 separately, rather than including it in this feature

@LuxoftAKutsan
Copy link
Contributor Author

LuxoftAKutsan commented Aug 27, 2019

@jacobkeeler

Issue 2 :
Should I include already an existing fix for #2282: #2501 to develop into feature branch?

@jacobkeeler
Copy link
Contributor

jacobkeeler commented Aug 27, 2019

#2282 is not related to issue 2, as mentioned.

#2501 should remain targeted against develop

@jacobkeeler
Copy link
Contributor

@LuxoftAKutsan Are you still investigating the second issue?

  1. PROPRIETARY mode
  • Add custom vehicle data to preloaded policy table
  • Start Core
  • Connect an app with custom vehicle data permissions, then restart Core while still connected. Repeat this process several times (shouldn't need more than 10)
  • Check contents of policy table, vehicle_data_item_definition and
    vehicle_date_item_parameters are empty

This issue seems to be tied to the following message in the Core logs, and as implied by the multiple restarts it only happens sometimes:

ERROR [23 Aug 2019 15:55:14,244][0x7f8ca7fff700][Policy] /home/jacobkeeler/sdl_core/src/components/policy/policy_regular/src/sql_pt_representation.cc:2609 SQLPTRepresentation::InsertVehicleDataItem: Vehicle data item is not initialized.
TRACE [23 Aug 2019 15:55:14,244][0x7f8ca7fff700][Policy] /home/jacobkeeler/sdl_core/src/components/policy/policy_regular/src/sql_pt_representation.cc:1754 SQLPTRepresentation::SaveVehicleDataItems: Exit
TRACE [23 Aug 2019 15:55:14,244][0x7f8ca7fff700][Policy] /home/jacobkeeler/sdl_core/src/components/policy/policy_regular/src/sql_pt_representation.cc:1728 SQLPTRepresentation::SaveVehicleData: Exit

@alexkutsan
Copy link
Contributor

@jacobkeeler we reproduced the issue, found the root cause.
The fix is ready, regression check is in progress.

@alexkutsan
Copy link
Contributor

alexkutsan commented Aug 28, 2019

@jacobkeeler please see the fix for Issue 2 :
6564f61

Right now we are in progress of rebasing feature branch on the latest develop and resolve conflicts.
Checking that feature tests are not affected by new commits in develop.
The feature does not introduce regression in delivered features.

Looks like all comments addressed. Please let me know if you have additional questions.
And are you OK with forcing push to feature branch (rebased to develop with commit squashing) after your approval?

@jacobkeeler
Copy link
Contributor

@LuxoftAKutsan I'm fine with squashing and merging, I'd prefer to avoid force pushing before that though.

@jacobkeeler
Copy link
Contributor

@LuxoftAKutsan please fix merge conflicts

@LitvinenkoIra LitvinenkoIra force-pushed the feature/read_generic_network_signal_data branch from 6564f61 to 13465f4 Compare August 29, 2019 17:21
@alexkutsan
Copy link
Contributor

alexkutsan commented Aug 29, 2019

@jacobkeeler Additional merge conflicts were found after delivery of RPC message protection to develop branch.

Conflicts are rather significant and should be resolved carefully to avoid regression in both features.
RPC message protection and RGNSD tests are in regression area, so should be checked.

Conflicts resolving will be ready for tomorrow.

Policy manager HMI level enum reorder
Set default version of custom vehicle data mapping
SQL storage implementation for VehicleDataItems

Fix validation of double values in policy table

Change isDouble to isNumeric during validation double values

"0" should be validated as correct double value

Add validation vehicle data during PTU

Send in PT snapshot only version of custom VDI

 - During snapshot generation remove vehicle data items section
 - Add validation for Policy table depended on PT type

Fix wrong check for vehicle data snapshot

Allow empty vehicle data in PTU

Change max value of string for URL from 255 to INT_MAX
Vehicle info params added to hmi commands
LuxoftAKutsan and others added 21 commits August 30, 2019 14:14
Allow unknown params for Subscribe VD
GetPolicyCOnfigurationData Implementation
Fixes : #2961

fix sending OnPermissionChanged after PTU with removing functional
group. OnPermissionChange notification is now sent after applying PTU

Replace ModuleConfig with Whole PT
Fixes #2962

Add additional check for functional group content
before OnPermissionChangeNotification.

* rename comparing functions to make more clear
their return value sense
* change variable names
* extra check for null values in HasNewGroups()

New unit test to cover changes within functional group
Change max value url for external policy

fixes for external flow
* since unti validation for the database
* empty vehicle data validation
Introduces validation of vehicle_data_item
name and key validation:
* they should not contain spaces;
* they should not be empty or consist only spaces;
* they should not contain invalid chars like '!@#$%^&*'.
* Avoid converting to Json non object values

Json::write adds additional `"` to string type as json value

* Fix unit tests
to Subscribe, Unsubscribe vehicle data
Extract SMember from CObjectSchemaItem

Add methods to ISchemaItem (Using composite pattern)
 - GetMemberSchemaItem
 - AddMemberSchemaItem

Add implementation of VehicleDataItemSchema class
Add appropriate unit tests

Add creation of vehicle_data items schemes on policy event
@mkorniichuk mkorniichuk force-pushed the feature/read_generic_network_signal_data branch from 13465f4 to ce477b8 Compare August 30, 2019 12:54
@alexkutsan
Copy link
Contributor

@jacobkeeler feature branch rebased on latest develop

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.

7 participants