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

[Accepted] SDL 0299 - Add a Mechanism to Avoid Deadlock #999

Closed
theresalech opened this issue Apr 22, 2020 · 14 comments
Closed

[Accepted] SDL 0299 - Add a Mechanism to Avoid Deadlock #999

theresalech opened this issue Apr 22, 2020 · 14 comments

Comments

@theresalech
Copy link
Contributor

theresalech commented Apr 22, 2020

Hello SDL community,

The review of the revised proposal "SDL 0299 - Add a Mechanism to Avoid Deadlock" begins now and runs through June 23, 2020. The review of the original proposal took place April 21 - May 12 2020. The proposal is available here:

https://github.com/smartdevicelink/sdl_evolution/blob/master/proposals/0299-Avoid-Deadlock.md

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

#999

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
Copy link
Contributor

joeljfischer commented Apr 28, 2020

I don't know that this particular proposed solution needed a proposal. It's a bug in SDL Core / SDL iOS compatibility and this solution could perhaps have been treated as such.

1. Having audio streaming stop working when the app is in the background is bad from a user point of view. The user may miss their navigation turns etc. while the app is in the background. Therefore, it would be better if any head unit that can support this (i.e. that isn't currently released) does.

However, the current solution limits our options by cutting off all Core v4.5 and below head units from using audio streaming in the background even if the head unit is capable of it. For example, we tested with Ford's Sync Gen 3 platform (and the original issue was reported by Ford here), which runs pre-v4.5 Core and is capable of playing audio while the device is in the background. This solution will break the fix for them. We need to either find a different solution or the SDLC needs to agree to break the fix for Ford.

@theresalech
Copy link
Contributor Author

During the Steering Committee meeting, the Ford team noted that revisiting the solution to be inclusive of legacy systems would be beneficial. The Project Maintainer clarified that this change would not break current integrations, but rather the previous fix that allows navigation apps to stream audio while the app is in the background would no longer work. The committee then voted to keep this proposal in review, to allow members time to review the Project Maintainer's concerns, provide their input on the proposal, and also to allow the author time to respond.

@joeljfischer
Copy link
Contributor

As much as I dislike the solution, this may need a combination of solutions, including checking manufacturer names to allow pre-v4.5 head units that have this patch to continue to work, and to prevent the deadlock on those pre-v4.5 head units to work.

I also believe that using the protocol version probably is not the best way to do this. The negotiated RPC spec version would probably be better.

@Yuki-Shoda-Nexty
Copy link
Contributor

@joeljfischer -san
We will consider a revision based on the following policies.
-Addition of manufacturer name check
-Comparison with the negotiated RPC spec version, not the protocol version

@joeljfischer
Copy link
Contributor

Any alternate ideas from members would be welcome.

@theresalech theresalech changed the title [In Review] SDL 0299 - Add a Mechanism to Avoid Deadlock [Returned for Revisions] SDL 0299 - Add a Mechanism to Avoid Deadlock May 13, 2020
@theresalech
Copy link
Contributor Author

The Steering Committee voted to return this proposal for revisions. The author will revise the proposal to use the negotiated RPC spec version instead of the protocol version, and to add a manufacturer name check to prevent the deadlock on pre-Core 4.5 head units.

@smartdevicelink smartdevicelink locked and limited conversation to collaborators May 13, 2020
@theresalech theresalech changed the title [Returned for Revisions] SDL 0299 - Add a Mechanism to Avoid Deadlock [In Review] SDL 0299 - Add a Mechanism to Avoid Deadlock Jun 17, 2020
@theresalech
Copy link
Contributor Author

The author has revised the proposal to incorporate the requested revisions, and the revised proposal is now in review until June 23, 2020.

@smartdevicelink smartdevicelink unlocked this conversation Jun 17, 2020
@kshala-ford
Copy link
Contributor

1. I don't know the issue but from the comments and the motivation this issue seems to be related to SDL Core v4.5 or older. Now the proposed solution changed from checking if protocol version is v5 or older to mobile API is v5 or older. According to SDL Core release 4.5.2 (see here) the proposed solution should be to check if the mobile API version is v4.5.2 or older. I suppose this was just a minor misunderstanding between the participants and should be valid for "accept with revisions" for the next possible vote.

2. The Ford Motor Company holds multiple car brands. Besides "Ford" the company also produces vehicles with the brand "Lincoln". The issue should not be visible on both these brands. There is also another brand "Troller" which I have to be honest I'm not sure if it has SDL support. See Ford Motor Company: Products and Services.

Previously we accepted workarounds where conditions are added to limit the workaround to a specific brand only. Instead of excluding certain "make" strings, is it possible to invert the condition and limit the workaround to a specific "make" list?

@joeljfischer
Copy link
Contributor

1.

Now the proposed solution changed from checking if protocol version is v5 or older to mobile API is v5 or older. According to SDL Core release 4.5.2 (see here) the proposed solution should be to check if the mobile API version is v4.5.2 or older.

This must have been missed in the review. All the comments above state v4.5, including Theresa's on the steering committee decision.

2.

Previously we accepted workarounds where conditions are added to limit the workaround to a specific brand only. Instead of excluding certain "make" strings, is it possible to invert the condition and limit the workaround to a specific "make" list?

I don't think reversing to a "whitelist" to get the workaround is workable, we'd have to add a list of every brand that isn't Ford / Ford-related brands and update for every new brand added. Ford is the divergent party by fixing the issue outside of open-source. We will certainly add any Ford-related brands to the list of brands that don't get the workaround for those versions.

3. Are we certain that this workaround is not needed in the Java Suite library?

4. The impact section states:

This will be a minor version change to the iOS Library.

This is not a minor version change. It's a bugfix / patch change.

@Yuki-Shoda-Nexty
Copy link
Contributor

  1. As you said, it is necessary to confirm whether it is Core v4.5.2 (Mobile API v4.5.1) or earlier. We will fix it as soon as it becomes possible.
  2. I have the same recognition as Joel's opinion.
  3. Because it is a problem particular to iOS, the Java Suite library is not affected.
  4. We will fix it as soon as it becomes possible.

@smartdevicelink smartdevicelink locked and limited conversation to collaborators Jun 23, 2020
@theresalech theresalech changed the title [In Review] SDL 0299 - Add a Mechanism to Avoid Deadlock [Returned for Revisions] SDL 0299 - Add a Mechanism to Avoid Deadlock Jun 24, 2020
@theresalech
Copy link
Contributor Author

The Steering Committee voted to return this proposal for revisions. The author will revise the Proposed solution to reference the RPC spec version of 4.5 instead of 5.0 (author to confirm this is correct and reach out to the Project Maintainer if deemed otherwise), and to update the Impact on existing code section to clarify that this would be a bugfix/patch version change instead of minor.

@theresalech theresalech changed the title [Returned for Revisions] SDL 0299 - Add a Mechanism to Avoid Deadlock [In Review] SDL 0299 - Add a Mechanism to Avoid Deadlock Jul 15, 2020
@joeljfischer
Copy link
Contributor

👍

@theresalech theresalech changed the title [In Review] SDL 0299 - Add a Mechanism to Avoid Deadlock [Accepted] SDL 0299 - Add a Mechanism to Avoid Deadlock Jul 22, 2020
@theresalech
Copy link
Contributor Author

The Steering Committee fully agreed to accept this proposal.

@theresalech
Copy link
Contributor Author

Implementation Issue: smartdevicelink/sdl_ios#1719.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants