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

Revise SDL-0099 - New remote control modules #572

Conversation

joeljfischer
Copy link
Contributor

Introduction

Remove GPSLocation struct and reuse GPSData, which as of SDL-0175 supports optional parameters. Additionally, remove sRGBColor struct and reuse RGBColor from SDL-0147.

Motivation

The existing proposal adds duplicate structs.

Proposed solution

Remove the duplicate structs and reuse GPSData and RGBColor.

Potential downsides

None the author could identify.

Impact on existing code

There will be less duplicated code.

Alternatives considered

No alternatives were considered.

@joeljfischer
Copy link
Contributor Author

@theresalech all done

@yang1070
Copy link
Contributor

yang1070 commented Aug 2, 2018

This will be a breaking change as reusing the data structure will not only change the core internal implementation but also the definitions in the mobile_api.xml and hmi_api.xml. Both core and mobile proxy libraries need update to use the new data structure.

@stefanek22
Copy link

Can we align on a timing plan to update and deliver any associated code changes for both SDL Core and the Mobile SDK? This has impact on the existing deliveries for the upcoming SDL Core 5.0.0 release.

@joeljfischer
Copy link
Contributor Author

Hi @yang1070 this is not a breaking change. This removes RPCs before they are released, therefore this is not any type of an API change.

@joeljfischer
Copy link
Contributor Author

@stefanek22 For the mobile SDK, or at least the iOS SDK, the changes would be minimal (less than an hour). Core is the big question here, and hopefully @JackLivio can shed some light on how long this would take to change there?

@joeljfischer
Copy link
Contributor Author

I just noticed I named an RPC parameter RGBColorSpaceAvailable, when it should be rgbColorSpaceAvailable.

@Jack-Byrne
Copy link
Contributor

Jack-Byrne commented Aug 2, 2018

@joeljfischer We haven't reviewed the original Core implementation yet for these new RC modules. But I imagine the required changes would be small. No logic changes needed, only name changes.

@yang1070
Copy link
Contributor

yang1070 commented Aug 2, 2018

@joeljfischer I agree to reuse the existing data struct whenever it is possible, There shall be no issue regarding the GRBColor, but there is an issue regarding the GPSData data structure. According to the mobile_api.xml and hmi_api.xml in the develop branch and the update of GPS data PR smartdevicelink/sdl_core#2417, even after the change, there are still many mandatory fields in the HMI API (not available in the RC use case, the location of radio station comes from the radio broadcast signals, not the GPS device) required in the data structure. RC cannot use it unless the proposal changes all other fields to be not mandatory. And that change will be breaking change.

@joeljfischer
Copy link
Contributor Author

Should the HMI API for SDL-0175 also have been updated and it was a mistake that the Mobile API was the only part updated? We should not use the GPSLocation structure if we can manage it, the API duplication is simply too egregious.

We are already doing a breaking change to the HMI API in Core 5.0, are we not? So this would be our best opportunity to do such an update?

@yang1070
Copy link
Contributor

yang1070 commented Aug 2, 2018

@joeljfischer Yes, I agree that SDL-0175 should make the other fields optimal in GPSData in the MOBILE API.

Just checked the HMI_API.xml, everything under GPSData are optional. However, in MOBILE_API.xml most fields under GPSData are mandatory, SDL-0175 shall change them to not mandatory in order for SDL-0099 to re-use GPSData.

@yang1070
Copy link
Contributor

yang1070 commented Aug 3, 2018

@joeljfischer I have create another PR #577 for this

@joeljfischer
Copy link
Contributor Author

@yang1070 👍🏻

@joeljfischer joeljfischer deleted the revise-sdl-0099-new-remote-control-parameters-again branch August 14, 2018 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants