-
Notifications
You must be signed in to change notification settings - Fork 171
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 screen mirroring in Infotainment.vspec #737
Conversation
SmartphoneScreenMirroring.Source: | ||
datatype: string | ||
type: actuator | ||
allowed: ['USB', 'BLUETOOTH', 'WIFI'] |
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.
Consider using the following instead:
allowed: ['USB', 'BLUETOOTH', 'WIFI'] | |
allowed: ['USB', 'BLUETOOTH', 'WI-FI'] |
Although many folks tend to write "Wifi" or "WiFi", the correct spelling is "Wi-Fi": https://www.wi-fi.org/
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.
I have no strong opinion but the literals used here correspond to what we have in https://github.com/COVESA/vehicle_signal_specification/blob/master/spec/Cabin/Infotainment.vspec#L258
SmartphoneProjection.Source:
datatype: string
type: actuator
allowed: ['USB', 'BLUETOOTH', 'WIFI']
description: Connectivity source selected for projection.
So if we are to change to WI-FI
we should also consider changing Vehicle.Cabin.Infotainment.SmartphoneProjection.Source
(and then comes the question on how to handle backward compatibility, just adding WI-FI
and keep WIFI
for now, but state that WIFI
may be removed later)
I suggest we discuss it at a meeting
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.
My point was mainly that if we wish the catalog to be a standard, we should ensure we use and refer to other standards. Here, the Wi-Fi alliance is using "Wi-Fi" in all its documentation. Even their domain is www.wi-fi.com.
Most people write it as Wifi, so "WIFI" should be fine. If we already have "WFI" elsewhere, I am not sure it is worth it to change to other uses.
My nitpick fits in more in the context of my interest in developing a signal catalog submission review process that looks at these kinds of checks as well as other questions we may wish to ask for each submission.
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.
Thanks for your comment.
Personally, I don't have a strong opinion on this matter either. I also agree that following the global standards is important.
However, I'm little bit confused about whether it's appropriate to use "WI-FI" for data naming. When I see the android document, "WIFI" is used as a data type name, and "Wi-Fi" is used for descriptive text in the document. (https://source.android.com/docs/core/ota/modular-system/wifi)
Typically, hyphens ('-') are not used in data type names in VSS. Therefore, how about we keep using "WIFI" as it is?
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.
I think changing "WIFI" to "WI-FI" is not too difficult since we only need to update the naming in SmartphoneProjection. More important thing is making the right decision regarding the correct naming between WIFI and WI-FI.
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.
At the VSS meeting earlier this week we agreed that keeping WIFI
is the best option for now, as we then do not need to change the other existing signal that use WIFI
.
spec/Cabin/Infotainment.vspec
Outdated
SmartphoneScreenMirroring.Resolution: | ||
datatype: string | ||
type: sensor | ||
allowed: ['RES_720P', 'RES_1080P', 'RES_4K'] |
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.
Not clear if and how this is useful. If useful, there is possibly much more information that may be useful (screen size, etc.).
Pardon my lack of knowledge, but isn't resolution a factor of what the display supports as well as what the application wishes to use or requires? If so, how do we address it here?
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.
In my opinion, screen size is not an appropriate parameter for screen mirroring configuration. This is because even within the same screen size, smartphones can support various resolutions based on user preferences. The android base smartphone also supports the configuration of your resolution in 'configuration->display->screen resolution'.
The resolution setting is actually related to the display graphics processing load of the infotainment system, and typically, it can be adjusted to suit the user's preferences.
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.
Would it be a possible way forward to clarify in the description that this signal concerns the resolution currently used, not maximum allowed resolution, possibly by changing to something like:
"Display resolution currently used for the mirrored content"
This off course does not prevent that more signals are added later if needed, like attributes for screen size, supported resolutions and so on, but that could be added later if/when needed.
MoM
|
52ab4a3
to
3db0f54
Compare
Signed-off-by: Sungpil Shin <[email protected]>
Signed-off-by: Sungpil Shin <[email protected]>
Reflect erik's comment. Signed-off-by: Sungpil Shin <[email protected]>
This resolve the DCO error. Signed-off-by: Sungpil Shin <[email protected]>
This reverts commit 210301f. Signed-off-by: Sungpil Shin <[email protected]>
Signed-off-by: Sungpil Shin <[email protected]>
I resolved the DCO error before the meeting.
|
Trying to summarize my view on this PR.
At next meeting I suggest that we agree on remaining needed changes (if any) , so that we can merge it as soon as they have been fixed |
Thanks for your clarification. In my personal opinion, resolving the ambiguity surrounding 'resolution' is challenging in our current signal. I think for this issue, we need to discuss adding the display-related signal before. Therefore, my suggestion for this PR is removing the resolution signal for this time. Thanks. |
MoM:
|
Signed-off-by: Sungpil Shin <[email protected]>
Thanks for your feedback and the decision. |
Signed-off-by: Sungpil Shin <[email protected]>
This PR is rework of PR #699 .
The type of SmartphoneScreenMirroring.Resolution is changed to 'sensor'.
The differences of smartphone screen mirroring with smartphone projection as follow:
Screen Mirroring, such as through AirPlay, mirrors all activities of a device’s screen, allowing every detail to be displayed on a secondary screen. On the other hand, existing Projection specifically targets in-vehicle use, selectively providing access to certain applications in smartphone like navigation and music players.
Specifically, Screen Mirroring enables the casting of applications not typically offered by Projection, such as 'YouTube', to a vehicle's display. Therefore, Screen Mirroring and Projection are parallel technologies, each serving distinct use cases without overlapping functionalities.