Skip to content

[SDL-0262] New vehicle data SeatOccupancy#351

Merged
renonick87 merged 2 commits intosmartdevicelink:developfrom
LuxoftSDL:feature/SDL-0262-New-vehicle-data-SeatOccupancy
Jan 13, 2021
Merged

[SDL-0262] New vehicle data SeatOccupancy#351
renonick87 merged 2 commits intosmartdevicelink:developfrom
LuxoftSDL:feature/SDL-0262-New-vehicle-data-SeatOccupancy

Conversation

@ymalovanyi
Copy link
Copy Markdown
Contributor

@ymalovanyi ymalovanyi commented Nov 19, 2020 •
edited
Loading

Fixes #336

Risk

This PR makes minor API changes.

Testing Plan

  • I have verified that I have not introduced new warnings in this PR
  • I have run the unit tests with this PR
  • I have tested this PR against Core and verified behavior

Unit Tests

Added unit tests cover [SDL 0262] New vehicle data SeatOccupancy changes

Core Tests

  • VehicleDataType enum contains additional value "VEHICLEDATA_SEATOCCUPANCY"
  • each of the properties in the struct SeatStatus can be received from HMI
  • each of the properties in the struct SeatOccupancy can be received from HMI
  • SubscribeVehicleData, UnsubscribeVehicleData, GetVehicleData request messages can be sent with seatOccupancy parameter defined as boolean value
  • SubscribeVehicleData, UnsubscribeVehicleData response messages contain seatOccupancy parameter defined as VehicleDataResult struct
  • GetVehicleData , OnVehicleData response messages contain seatOccupancy parameter defined as SeatOccupancy struct

Core version / branch / commit hash / module tested against: smartdevicelink/sdl_core#3585
HMI name / version / branch / commit hash / module tested against: smartdevicelink/sdl_hmi#467

Summary

Applied [SDL 0262] New vehicle data SeatOccupancy changes

Changelog

Breaking Changes
  • N/A
Enhancements
  • New structs SeatStatus, SeatOccupancy
  • Added required getters/setters in related RPC classes
Bug Fixes
  • N/A

Tasks Remaining:

  • N/A

CLA

@ymalovanyi
Copy link
Copy Markdown
Contributor Author

ymalovanyi commented Nov 19, 2020

@santhanamk the PR is ready for Ford review.

santhanamk
santhanamk reviewed Nov 20, 2020
View reviewed changes
Copy link
Copy Markdown

@santhanamk santhanamk left a comment

Choose a reason for hiding this comment

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

Hi @ymalovanyi I reviewed the PR. Code looks good.

I have a few comments. Please look through it, and see if any changes need to be made.

tests/Test.js Outdated
};

const GENERAL_SEAT_OCCUPANCY = Test.GENERAL_SEAT_OCCUPANCY = new SeatOccupancy()
.setSeatsOccupied([Test.GENERAL_SEAT_STATUS])
Copy link
Copy Markdown

@santhanamk santhanamk Nov 20, 2020

Choose a reason for hiding this comment

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

Given that seatsOccupied and seatsBelted are arrays in the 'SeatsOccupancy.js' struct, does the GENERAL_SEAT_STATUS on line 431 need to be instantiated differently?

For example on line 128 imageTypeSupported is an array and is set up with the GENERAL_FILETYPE_LIST using the line GENERAL_IMAGEFIELD.setImageTypeSupported(GENERAL_FILETYPE_LIST);.

Copy link
Copy Markdown
Contributor Author

@ymalovanyi ymalovanyi Nov 23, 2020

Choose a reason for hiding this comment

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

Hi @santhanamk
I've created GENERAL_SEAT_STATUS_LIST and JSON_SEATSTATUS_LIST arrays to make code look cleaner.

tests/node/rpc/messages/GetVehicleDataResponseTests.js
Validator.assertEquals(Test.GENERAL_BOOLEAN, testHandsOffSteering);
Validator.assertEquals([Test.GENERAL_WINDOW_STATUS], testWindowStatus);
Validator.assertEquals(this.gearStatus, testGearStatus);
Validator.assertEquals(Test.GENERAL_SEAT_OCCUPANCY, testSeatOccupancy);
Copy link
Copy Markdown

@santhanamk santhanamk Nov 20, 2020

Choose a reason for hiding this comment

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

On line 80 are brackets ( [] ) needed around Test.GENERAL_SEAT_OCCUPANCY similar to line 76 for Test.GENERAL_WINDOW_STATUS?

Copy link
Copy Markdown
Contributor Author

@ymalovanyi ymalovanyi Nov 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

No, because seatOccupancy is not an array (SeatOccupancy), compared to windowStatus which is indeed an array (WindowStatus[])

tests/node/rpc/messages/OnVehicleDataTests.js
Validator.assertEquals(this.stabilityControlsStatus, testStabilityControlsStatus);
Validator.assertEquals([Test.GENERAL_WINDOW_STATUS], testWindowStatus);
Validator.assertEquals(this.gearStatus, testGearStatus);
Validator.assertEquals(Test.GENERAL_SEAT_OCCUPANCY, testSeatOccupancy);
Copy link
Copy Markdown

@santhanamk santhanamk Nov 20, 2020

Choose a reason for hiding this comment

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

On line 78 are brackets ( [] ) needed around Test.GENERAL_SEAT_OCCUPANCY similar to line 76 for Test.GENERAL_WINDOW_STATUS?

Copy link
Copy Markdown
Contributor Author

@ymalovanyi ymalovanyi Nov 23, 2020

Choose a reason for hiding this comment

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

Also not needed, same as in GetVehicleDataResponseTests.js

santhanamk
santhanamk approved these changes Dec 1, 2020
View reviewed changes
Copy link
Copy Markdown

@santhanamk santhanamk left a comment

Choose a reason for hiding this comment

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

Hi @ymalovanyi . The changes you have made from the code review good. I will approve this PR. Thanks.

@ymalovanyi ymalovanyi marked this pull request as ready for review December 3, 2020 10:07
@ymalovanyi
Copy link
Copy Markdown
Contributor Author

ymalovanyi commented Dec 3, 2020

@santhanamk, thank you for the approval! @crokita, Ford approved this PR, please review.

@jordynmackool
Copy link
Copy Markdown

jordynmackool commented Dec 10, 2020 •
edited
Loading

Hi @ymalovanyi and @santhanamk, since the core PR for this feature is not complete, it seems that this PR would have not been tested fully. To ensure the review process goes smoothly:

  1. Please avoid removing sections from the PR template as all the information requested is needed.
  2. Please provide the Core branch/PR that this was tested against.

Once this PR is fully tested, tag me and Livio will review. Thank you!

@vladmu
Copy link
Copy Markdown
Contributor

vladmu commented Dec 18, 2020

@jordynmackool @crokita the description was fixed to include all sections from the template and the code was tested against the Core and HMI (corresponded links also included in the description). Please review.

renonick87
renonick87 approved these changes Jan 13, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@renonick87 renonick87 left a comment

Choose a reason for hiding this comment

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

Successfully tested against core and sdl_hmi. Everything here looks good. Approved.

@renonick87 renonick87 merged commit 51b40ab into smartdevicelink:develop Jan 13, 2021
@vladmu
Copy link
Copy Markdown
Contributor

vladmu commented Jan 14, 2021

Hi @renonick87, after merging, we have some conflicted vehicle data PRs. Could you define which will be reviewed next, then we could prepare it and fix conflicts before your review?

@crokita
Copy link
Copy Markdown
Contributor

crokita commented Jan 14, 2021

Hello @vladmu. I will be reviewing this PR: #302
That will be the next one tested.

I also noticed that my unit tests started failing because the associated rpc_spec PR for this feature wasn't merged in develop yet. Please check that the associated rpc_spec PR is merged in before merging the JS one!

@vladmu
Copy link
Copy Markdown
Contributor

vladmu commented Jan 14, 2021

@crokita thank you for your reply, I'll update #302 shortly. Regarding merging, it is a good point, definitely, the related rpc_spec should be merged first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@renonick87 renonick87 renonick87 approved these changes

+1 more reviewer

@santhanamk santhanamk santhanamk approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[SDL 0262] New vehicle data SeatOccupancy

6 participants

@ymalovanyi @jordynmackool @vladmu @crokita @renonick87 @santhanamk