1000581 - [Flame][Build][RIL] include DSDS ril system properties to Flame

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Tony Chung [:tchung]]

+++ This bug was initially created as a clone of Bug #994463 +++

Separating out https://bugzilla.mozilla.org/show_bug.cgi?id=994463#c12 into a new bug here.

(In reply to Alexandre LISSY :gerard-majax from comment #11)
> Not sure if it's the bug I experienced, but building and flashing for Flame,
> I got only one SIM to be recognized.
DSDS properties for Moz RIL are not added in flame. Please add following properties in release image of flame.
[ro.moz.ril.0.network_types]: [gsm,wcdma]
[ro.moz.ril.1.network_types]: [gsm]
[ro.moz.ril.numclients]: [2] 

cc'ing mwu for a solution.  Please have DSDS configured as part of the mozilla flashing process.

Comment 1

[viral [:viralwang]]

I think I can do that. It's an easy patch.

Comment 2

[Vicamo Yang [:vicamo][:vyang]]

The original title looks like we should have some kind of mechanism to detect DSDS change on Gecko launched, but it's not the case.  These properties are read-only and built, installed in root image, so theny can't be modified.  It's not about flashing Gecko or not.

Comment 3

[viral [:viralwang]]

Attachment: Configure DSDS for flame

Hi Michael,

Could you please review it first? I will send PR after bug 994577 land
Thanks.

Comment 4

[bhavana bajaj [:bajaj]]

Not blocking 1.4 technically but I agree we need this, hence giving an exception here to request approval-gaia-v1.4 once this is ready to land on 1.4

Comment 5

[viral [:viralwang]]

Attachment: Configure DSDS for flame

since bug 994577 already land, send PR for DSDS config

Comment 6

[Michael Wu [:mwu]]

Hmm, the PR looks fine, but I'm not sure it goes far enough to *really* fix the problem in every scenario. Ideally, DSDS would work with or without a full flash. I think there would be two things we'd have to do to make this work:

1. Check persist.radio.multisim.config and see if it's set to dsds, if ro.moz.ril.numclients isn't available. This appears to be the property that's *really* used to configure dsds.
2. Try to fallback to rild* if rilproxy* isn't found. It looks like the rilproxy1 symlink isn't setup by default on the Flame.

Comment 7

[Jason Smith [:jsmith]]

Not a QA blocker - this isn't a regression.

Comment 8

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Hi viral, I'm not sure if you saw comment 6.  Could you take a look please?

Comment 9

[viral [:viralwang]]

I saw comment 6 but I have no idea about this, I thought I only need to add the properties for this bug.

Hi Ken,
Any suggestion for comment 6?
In my understanding, I should add properties for DSDS base on https://bugzilla.mozilla.org/show_bug.cgi?id=994463#c12, is that all we need to do for DSDS now?

Comment 10

[viral [:viralwang]]

(In reply to viral [:viralwang][5/14 - 5/20 business trip] from comment #9)
> I saw comment 6 but I have no idea about this, I thought I only need to add
> the properties for this bug.
> 
> Hi Ken,
> Any suggestion for comment 6?
> In my understanding, I should add properties for DSDS base on
> https://bugzilla.mozilla.org/show_bug.cgi?id=994463#c12, is that all we need
> to do for DSDS now?

add ni in case of any missing :)

Comment 11

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to viral [:viralwang][5/14 - 5/20 business trip] from comment #10)
> (In reply to viral [:viralwang][5/14 - 5/20 business trip] from comment #9)
> > I saw comment 6 but I have no idea about this, I thought I only need to add
> > the properties for this bug.
> > 
> > Hi Ken,
> > Any suggestion for comment 6?
> > In my understanding, I should add properties for DSDS base on
> > https://bugzilla.mozilla.org/show_bug.cgi?id=994463#c12, is that all we need
> > to do for DSDS now?
> 
> add ni in case of any missing :)

We also need "ro.moz.ril.subscription_control" and have it set to true.

Comment 12

[Ken Chang[:kenkai]]

(In reply to viral [:viralwang][5/14 - 5/20 business trip] from comment #9)
> In my understanding, I should add properties for DSDS base on
> https://bugzilla.mozilla.org/show_bug.cgi?id=994463#c12, is that all we need
> to do for DSDS now?
Because you want to create a system imange, I think you need to check https://bugzilla.mozilla.org/show_bug.cgi?id=994463#c29.
 
And email is unreliable....:-(. Edgar had emailed to told us that he wanted to add ro.moz.ril.subscription_control for bug 1000581 before. T2M have added this in their image. You also need to add this as well.

Comment 13

[viral [:viralwang]]

Thanks Ken.

I can update the properties send the PR again.

Comment 14

[viral [:viralwang]]

Attachment: Configure DSDS for flame

Hi Michael,

I think we can update properties for DSDS first.
Any suggestion?

Comment 15

[Michael Wu [:mwu]]

Comment on attachment 8426144 [details] [review]
Configure DSDS for flame

FWIW, I will generally stamp any property changes to match what's in the vendor image. However, I think we should try to do the right thing without new properties in order to make things just work on devices other than the Flame. That's not always possible, but I think we could've avoided adding ro.moz.ril.numclients, for example.

Comment 16

[viral [:viralwang]]

(In reply to Michael Wu [:mwu] from comment #15)
> Comment on attachment 8426144 [details] [review]
> Configure DSDS for flame
> 
> FWIW, I will generally stamp any property changes to match what's in the
> vendor image. However, I think we should try to do the right thing without
> new properties in order to make things just work on devices other than the
> Flame. That's not always possible, but I think we could've avoided adding
> ro.moz.ril.numclients, for example.

Hi Ken,

Any feedback for mwu's comment?
I can add those properities but not sure the details of implementation.
Thanks :)

Comment 17

[Ken Chang[:kenkai]]

(In reply to Michael Wu [:mwu] from comment #15)
> Comment on attachment 8426144 [details] [review]
> Configure DSDS for flame
> 
> FWIW, I will generally stamp any property changes to match what's in the
> vendor image. However, I think we should try to do the right thing without
> new properties in order to make things just work on devices other than the
> Flame. That's not always possible, but I think we could've avoided adding
> ro.moz.ril.numclients, for example.

We always face some platform/HW related issues. For example, we don't know how many
SIMs a mobile device supports, we need a way to get this information for RIL. And
SPRD's DSDS platform is a little different from QC's, which causes SIM initial procedure
different. We also need a way to know the platform information to run different SIM
initial procedure. It seems inevitable problems for us to face different platform/HW.
Currently, we use *property* to fix these kind of problems.
But according to what you said, it seems not a good way. May I know your suggestion
for supporting different HW platform? Using compiler option? Or...

Comment 18

[viral [:viralwang]]

Comment on attachment 8426144 [details] [review]
Configure DSDS for flame

Hi Michael,

Update the PR to add property persist.radio.multisim.config=dsds

Comment 19

[viral [:viralwang]]

Attachment: remove rilproxy for flame

Hi Michael,

If we use our own rilproxy, socket can not connect successfully (looks like permission problem)
I'm not sure if this is the best way to make ril function works.

Comment 20

[:gerard-majax]

(In reply to viral [:viralwang] from comment #19)
> Created attachment 8427580 [details] [review]
> remove rilproxy for flame
> 
> Hi Michael,
> 
> If we use our own rilproxy, socket can not connect successfully (looks like
> permission problem)
> I'm not sure if this is the best way to make ril function works.

rilproxy works fine on my custom built flame images.

Comment 21

[viral [:viralwang]]

(In reply to Alexandre LISSY :gerard-majax from comment #20)
> (In reply to viral [:viralwang] from comment #19)
> > Created attachment 8427580 [details] [review]
> > remove rilproxy for flame
> > 
> > Hi Michael,
> > 
> > If we use our own rilproxy, socket can not connect successfully (looks like
> > permission problem)
> > I'm not sure if this is the best way to make ril function works.
> 
> rilproxy works fine on my custom built flame images.
Hi Alexandre,

your sim 1 can work even use our own rilproxy?
It can not work in my side :(

(test environment is with Attachment #8426144 [details])
use our rilproxy => sim 1 not work, sim 2 works
srwx------ root     radio             1970-01-04 04:05 rilproxy
lrwxrwxrwx root     root              1970-01-04 04:05 rilproxy1 -> /dev/socket/rild1

remove rilproxy in our side => sim 1 and 2 work
lrwxrwxrwx root     root              1970-01-04 03:46 rilproxy -> /dev/socket/rild
lrwxrwxrwx root     root              1970-01-04 03:46 rilproxy1 -> /dev/socket/rild1

Comment 22

[:gerard-majax]

(In reply to viral [:viralwang] from comment #21)
> (In reply to Alexandre LISSY :gerard-majax from comment #20)
> > (In reply to viral [:viralwang] from comment #19)
> > > Created attachment 8427580 [details] [review]
> > > remove rilproxy for flame
> > > 
> > > Hi Michael,
> > > 
> > > If we use our own rilproxy, socket can not connect successfully (looks like
> > > permission problem)
> > > I'm not sure if this is the best way to make ril function works.
> > 
> > rilproxy works fine on my custom built flame images.
> Hi Alexandre,
> 
> your sim 1 can work even use our own rilproxy?
> It can not work in my side :(
> 
> (test environment is with Attachment #8426144 [details])
> use our rilproxy => sim 1 not work, sim 2 works
> srwx------ root     radio             1970-01-04 04:05 rilproxy
> lrwxrwxrwx root     root              1970-01-04 04:05 rilproxy1 ->
> /dev/socket/rild1
> 
> remove rilproxy in our side => sim 1 and 2 work
> lrwxrwxrwx root     root              1970-01-04 03:46 rilproxy ->
> /dev/socket/rild
> lrwxrwxrwx root     root              1970-01-04 03:46 rilproxy1 ->
> /dev/socket/rild1

Or you're right and I don't have any rilproxy running :D. Sorry, I should have checked.

Comment 23

[viral [:viralwang]]

update reviewer in commit

Comment 24

[Ryan VanderMeulen [:RyanVM]]

device-flame/master: https://github.com/mozilla-b2g/device-flame/commit/81eab0894a773fc0274b64432f1f49da95d63b7d

b2g-manifest/master: https://github.com/mozilla-b2g/b2g-manifest/commit/9683b1bb7c496677ac6448cae498f0f34652ea3e

Comment 25

[Ryan VanderMeulen [:RyanVM]]

AFAICT, only the b2g-manifest change requires explicit v1.4 uplift. Please needinfo me if I got that wrong :)

v1.4: https://github.com/mozilla-b2g/b2g-manifest/commit/be91807138deb48af10f835e09fb1ed2ce82f3bf