smartdevicelink/sdl_evolution

[Accepted with Revisions] SDL 0269 - New vehicle data ClimateData

Closed this issue · 10 comments

theresalech commented

Hello SDL community,

The review of "SDL 0269 - New vehicle data ClimateData" begins now and runs through December 17, 2019. The proposal is available here:

https://github.com/smartdevicelink/sdl_evolution/blob/master/proposals/0269-New-vehicle-data-ClimateData.md

Reviews are an important part of the SDL evolution process. All reviews should be sent to the associated Github issue at:

#891

What goes into a review?

The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of SDL. When writing your review, here are some questions you might want to answer in your review:

  • Is the problem being addressed significant enough to warrant a change to SDL?
  • Does this proposal fit well with the feel and direction of SDL?
  • If you have used competitors with a similar feature, how do you feel that this proposal compares to those?
  • How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
    Please state explicitly whether you believe that the proposal should be accepted into SDL.

More information about the SDL evolution process is available at

https://github.com/smartdevicelink/sdl_evolution/blob/master/process.md

Thank you,
Theresa Lech

Program Manager - Livio
theresa@livio.io

joeljfischer commented

I'm okay with the general approach. Just a couple notes:

  1. I think the atmospheric pressure range should be extended to minvalue="0" and maxValue="2000".

  2. The deprecated values need history tags.

atiwari9 commented

@joeljfischer

  1. ok #ToUpdate
  2. ok #ToUpdate
joeygrover commented

If we're continuing to do these vehicle data refactors, why don't we use Temperature struct instead of float in the new struct?

I'm also a little hesitant on the name ClimateData because ClimateControlData already exists in the RPC spec and they have very little to do with each other. Not sure if there is a better name we could give here.

atiwari9 commented

@joeygrover

If we're continuing to do these vehicle data refactors, why don't we use Temperature struct instead of float in the new struct?

Numering this as point 3.
ClimateData is itself a struct, so this keeps the option of expansion open for future. Temperature is divided to sub types externalTemperature and cabinTemperature itself, which do not need sub-bifurcation. Hope this clarifies.

I'm also a little hesitant on the name ClimateData because ClimateControlData already exists in the RPC spec and they have very little to do with each other. Not sure if there is a better name we could give here.

Numering this as point 4.
At the risk of sounding biased, i believe ClimateData and ClimateControlData won't be confused by someone familiar with Vehicle Data and Remote Control. The keyword being control for structs/enums related to Remote Control. I am still open to any suggestions you may have on an alternate name which could represent all the sub-vehicle data related to climate.

joeygrover commented

3. That is not what I'm asking. You use float in ClimateData for externalTemperature and cabinTemperature params, but since this is a new struct and new params you should use Temperature because those parameters are... temperatures.

4. Part of my fear here is that autocomplete in IDEs tend to ignore context, so it will be easy to accidentally use one in place of the other. Looking at them side by side is fine, but the risk of incorrect usage here is high and the burden it adds to developers to understand which ones are which is increased. I'm just asking the author to try and think a little deeper about the issue and how something as simple as a name change for a parameter can reduce the developer's burden.

atiwari9 commented

  1. I misunderstood, yes it does make sense to use the existing Temperature struct #ToUpdate

  2. As i said, i am open to suggestions on the name. After all we just trying to make improvements for feature content and of course we should make every attempt to make it easier to use for developers and users alike. One alternate name which i can think of, and is possibly more fitting is microClimateData or microClimateInformation.

theresalech commented

The Steering Committee voted to accept this proposal with the following revisions:

  • Atmospheric pressure range should be extended to minvalue="0" and maxValue="2000".
  • Add history tags to the deprecated values.
  • Use existing Temperaturestruct in externalTemperature and cabinTemperature parameters
theresalech commented

@atiwari9 Please advise when a new PR has been entered to update the proposal to reflect the agreed upon revisions. I'll then merge the PR so the proposal is up to date, and enter issues in the respective repositories for implementation. Thanks!

jordynmackool commented

The proposal has been updated to reflect the accepted revisions, and implementation issues have been entered:

rpc_spec
sdl_hmi
core
iOS
manticore
server
Java Suite