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

Add screen mirroring in Infotainment.vspec #737

Merged
merged 7 commits into from
May 8, 2024
Merged

Conversation

spshin3
Copy link
Contributor

@spshin3 spshin3 commented Apr 12, 2024

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.

SmartphoneScreenMirroring.Source:
datatype: string
type: actuator
allowed: ['USB', 'BLUETOOTH', 'WIFI']
Copy link
Collaborator

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:

Suggested change
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/

Copy link
Collaborator

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 WIFImay be removed later)

I suggest we discuss it at a meeting

Copy link
Collaborator

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.

Copy link
Contributor Author

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?

Copy link
Contributor Author

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.

Copy link
Collaborator

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.

SmartphoneScreenMirroring.Resolution:
datatype: string
type: sensor
allowed: ['RES_720P', 'RES_1080P', 'RES_4K']
Copy link
Collaborator

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?

Copy link
Contributor Author

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.

Copy link
Collaborator

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.

@erikbosch
Copy link
Collaborator

MoM

  • WI-FI/WIFI
  • PP: No major preferences, we could align with old `WIFI
  • Stefan: Does not matter
  • Erik: Please review/discuss, continued discussion next meeting

@spshin3 spshin3 force-pushed the master branch 2 times, most recently from 52ab4a3 to 3db0f54 Compare May 3, 2024 04:58
spshin3 and others added 6 commits May 3, 2024 15:36
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]>
@spshin3
Copy link
Contributor Author

spshin3 commented May 3, 2024

I resolved the DCO error before the meeting.
Thanks for your summary.

MoM

  • WI-FI/WIFI
  • PP: No major preferences, we could align with old `WIFI
  • Stefan: Does not matter
  • Erik: Please review/discuss, continued discussion next meeting

@erikbosch
Copy link
Collaborator

Trying to summarize my view on this PR.

  • At last meeting we agreed to use WIFI rather than WI-FI to avoid changing name of other already existing signals using WIFI
  • For the other signal, maybe we should clarify that the signal concerns currently used resolution to avoid ambiguity

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

@spshin3
Copy link
Contributor Author

spshin3 commented May 7, 2024

MoM

  • WI-FI/WIFI
  • PP: No major preferences, we could align with old `WIFI
  • Stefan: Does not matter
  • Erik: Please review/discuss, continued discussion next meeting

Thanks for your clarification.

In my personal opinion, resolving the ambiguity surrounding 'resolution' is challenging in our current signal.
This is because, we don't have a specific signal for display-related and resolutions with in our spec.

I think for this issue, we need to discuss adding the display-related signal before.
Once this is added, we can clarify what 'resolution' means in the context of screen mirroring.

Therefore, my suggestion for this PR is removing the resolution signal for this time.
And I will prepare another PR for the display related signal for the infotainment system.

Thanks.

@erikbosch
Copy link
Collaborator

MoM:

  • OK to merge if resolution signal removed
  • Richard Fernandes - Resolution can be complex due to DRM, we from AOSP could possibly contribute

Signed-off-by: Sungpil Shin <[email protected]>
@spshin3
Copy link
Contributor Author

spshin3 commented May 7, 2024

MoM:

  • OK to merge if resolution signal removed
  • Richard Fernandes - Resolution can be complex due to DRM, we from AOSP could possibly contribute

Thanks for your feedback and the decision.
I just removed the resolution in my PR.

@erikbosch erikbosch merged commit 2f45377 into COVESA:master May 8, 2024
4 checks passed
erikbosch pushed a commit to boschglobal/vehicle_signal_specification that referenced this pull request May 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants