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

Add comparator to track difference before and after #593

Conversation

AKalinich-Luxoft
Copy link
Contributor

@AKalinich-Luxoft AKalinich-Luxoft commented Aug 27, 2021

And send OnIVD notification only for case when some fields has
actually been changed.

Fixes #96

This PR is ready for review.

Testing Plan

Steps:

  1. mobile sends a valid SetInteriorVehicleData to HMI with some params that will be changed
  2. HMI apply that changes and send OnInteriorVehicleData notification with changed parameters
  3. mobile sends a valid SetInteriorVehicleData to HMI with the same params

Summary

Add comparator to track difference before and after SetIVD request for each module data and send OnIVD if some values were actually changed

CLA

And send OnIVD notification only for case when some fields has
actually been changed.
@AKalinich-Luxoft
Copy link
Contributor Author

@jordynmackool This PR is ready for Livio review

});
});

const changedFields = getDataDifference(data_before, lightControlData);
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't work correctly if only some properties are changed. For example sending SetInteriorVehicleData twice with params

{
	"moduleData": {
		"moduleType": "LIGHT",
		"lightControlData": {
			"lightState": [{
				"id": "FRONT_LEFT_HIGH_BEAM",
				"status": "ON",
				"density": 1
			}]
		}
	}
}

results in two OnInteriorVehicleData notifications being sent from the HMI. Probably due to extra params (like color) not being ignored by the if (undefined !== another[param] && check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ShobhitAd are you sure you are sending SetIVD twice with the same params? When I am trying to send the same params like you described above, I am getting only one OnIVD notification:

SDL -> HMI [14:10:59:485]: {"id":50,"jsonrpc":"2.0","method":"RC.SetInteriorVehicleData","params":{"appID":1111048898,"moduleData":{"lightControlData":{"lightState":[{"color":{"blue":0,"green":0,"red":0},"density":0,"id":"FRONT_LEFT_HIGH_BEAM","status":"ON"}]},"moduleId":"d38e4a05-b17c-28e3-9d38-e4a05b17c28e","moduleType":"LIGHT"}}}

RCRPC.js:225 FFW.RC.SetInteriorVehicleData Request
RCRPC.js:529 FFW.RC.OnInteriorVehicleData Notification
RPCClient.js:323 HMI -> SDL [14:12:17:114]: {"jsonrpc":"2.0","method":"RC.OnInteriorVehicleData","params":{"moduleData":{"moduleType":"LIGHT","moduleId":"d38e4a05-b17c-28e3-9d38-e4a05b17c28e","lightControlData":{"lightState":[{"color":{"blue":0,"green":0,"red":0},"density":0,"id":"FRONT_LEFT_HIGH_BEAM","status":"ON"}]}}}}
RPCClient.js:323 HMI -> SDL [14:12:17:114]: {"jsonrpc":"2.0","id":50,"result":{"code":0,"method":"RC.SetInteriorVehicleData","moduleData":{"moduleType":"LIGHT","moduleId":"d38e4a05-b17c-28e3-9d38-e4a05b17c28e","lightControlData":{"lightState":[{"color":{"blue":0,"green":0,"red":0},"density":0,"id":"FRONT_LEFT_HIGH_BEAM","status":"ON"}]}}}}
RPCClient.js:137 SDL -> HMI [14:12:23:144]: {"id":54,"jsonrpc":"2.0","method":"RC.SetInteriorVehicleData","params":{"appID":1111048898,"moduleData":{"lightControlData":{"lightState":[{"color":{"blue":0,"green":0,"red":0},"density":0,"id":"FRONT_LEFT_HIGH_BEAM","status":"ON"}]},"moduleId":"d38e4a05-b17c-28e3-9d38-e4a05b17c28e","moduleType":"LIGHT"}}}

RCRPC.js:225 FFW.RC.SetInteriorVehicleData Request
RPCClient.js:323 HMI -> SDL [14:16:6:789]: {"jsonrpc":"2.0","id":54,"result":{"code":0,"method":"RC.SetInteriorVehicleData","moduleData":{"moduleType":"LIGHT","moduleId":"d38e4a05-b17c-28e3-9d38-e4a05b17c28e","lightControlData":{"lightState":[{"color":{"blue":0,"green":0,"red":0},"density":0,"id":"FRONT_LEFT_HIGH_BEAM","status":"ON"}]}}}}

Copy link
Contributor

@ShobhitAd ShobhitAd Aug 30, 2021

Choose a reason for hiding this comment

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

@AKalinich-Luxoft I re-ran the test but I'm still seeing the same issue. I'm attaching my HMI console logs for the test
OnIVD_LIGHT.log

I think the issue occurs when one or more optional params are omitted from the request. It looks like if compareObjects is called with extra params in the second argument this condition https://github.com/smartdevicelink/sdl_hmi/blob/master/app/controller/sdl/Abstract/Controller.js#L408 will return a non-zero value(-1)

Could you retry the test you mentioned and send the SetInteriorVehicleData requests without the color parameter(only send the density, id and status params)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ShobhitAd thank you for the additional input, I see now. I also noticed that my SPT sends optional params even if I unchecked them in UI.
Looks like we should consider the field as changed only when the comparator returns positive 1. I updated the condition and now it works as expected. See 553aeba

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.

2 participants