-
Notifications
You must be signed in to change notification settings - Fork 164
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 MapScaleMode to Navigation branch #622
Conversation
wonsuk73
commented
Jun 19, 2023
- Need to add MapScaleMode to support the map scale mode switching function to scale fixed mode or auto scale mode in navigation
Signed-off-by: Wonsuk Lee <wonsuk.lee@etri.re.kr>
Signed-off-by: Wonsuk Lee <wonsuk.lee@etri.re.kr>
spec/Cabin/Infotainment.vspec
Outdated
Navigation.MapScaleMode: | ||
datatype: string | ||
type: actuator | ||
allowed: ['SCALE_FIXED_MODE', 'AUTO_SCALE_MODE'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we expect more alternatives in the future? if not an alternative solution could be a boolean signal like IsAutoScaleMode
(and if false then fixed mode is used). But this approach work for me as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@erikbosch Thanks for valuable comment! I think IsAutoScaleMode is enough. I am going to revise the PR based on the consensus.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@erikbosch Could you review updated one? Any comment on this?
Signed-off-by: Wonsuk Lee <wonsuk.lee@etri.re.kr>
The name of the signal and accompanying description are somewhat lacking. Previously the signal name indicated it was for scaling of maps, but now we don't have such an indication. I'd suggest renaming the signal to Navigation.IsMapScaleModeAuto or even Navigation.Map.IsScaleModeAuto if we foresee other signals for navigational maps coming in the future and therefore would benefit by creating a branch here. Regardless, please elaborate the description in the same manner e.g. Used to select between auto-scaling mode and manual-scaling mode of navigation map. A minor nit: remove the two instances of "it is" from the comment i.e. If true, then..., If false, then... |
@nickrbb Thanks for valuable comments! I agree to make Navigation.Map branch to cover other map signals. I revised as below. Is it okay to you?
|
spec/Cabin/Infotainment.vspec
Outdated
@@ -120,6 +120,13 @@ Navigation.GuidanceVoice: | |||
description: Navigation guidance state that was selected. | |||
comment: ETC indicates a voice alternative not covered by the explicitly listed alternatives. | |||
|
|||
Navigation.IsAutoScaleMode: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if we add term "used" into the leaf name? like IsAutoScaleModeUsed
Meeting notes: Please review/discuss |
@wonsuk73 Signal name looks better, but please also update the description to something more informative and meaningful. |
@nickrbb Thanks for feedback! Could you review updated one?
|
Signed-off-by: Wonsuk Lee <wonsuk.lee@etri.re.kr>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good
OK with me now, thank you. |
Meeting notes: Ok to merge |