943825 - [Gaia] Mount/Unmount SD card

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Settings, defect)

Description

[[e-mail], [e-mail]]

According to the user story in Bug 921105, we have to provide notification message while a user plug/unplug-in SD card. And will need mount/unmount buttons for user action.(https://bugzilla.mozilla.org/show_bug.cgi?id=921105#c12) For the issue here, it will need mount/unmount API and events via platform.

Comment 1

[[e-mail], [e-mail]]

Note: The latest spec. is located at https://bugzilla.mozilla.org/show_bug.cgi?id=929860#c16

Comment 2

[[e-mail], [e-mail]]

Since "storageStatus" API is landed(bug971615), Gaia is able to reach all of the device status. I think we have to implement a state machine to identify needed storage behaviour in this feature.

Comment 3

[[e-mail], [e-mail]]

Hi Alan, 

I have some question about "storageStatus" API since bug 971615 is landed. Is there any initial state(not "Init") for a storage activity flow on going? Otherwise, Gaia app have to match(recognise) the machine status while each "change" event coming. I want to make sure a initial state for checking the machine status.

The other question, is it possible that the status is coming unexpectedly? I was worried something might be happened while a user operates the storage quickly. Such as, "Checking" -> "Unmounting".(Ref from: http://www.slideshare.net/wiliwe/android-storage-vold)

Comment 4

[Dave Hylands [:dhylands]]

(In reply to Ian Liu [:ianliu] from comment #3)
> Hi Alan, 
> 
> I have some question about "storageStatus" API since bug 971615 is landed.
> Is there any initial state(not "Init") for a storage activity flow on going?
> Otherwise, Gaia app have to match(recognise) the machine status while each
> "change" event coming. I want to make sure a initial state for checking the
> machine status.

Init is the initial state use by the VolumeManager while its waiting for the initial status to be reported by vold. I doubt very highly that gaia would ever see this state (it only happens extremely early in the boot process near the beginning of b2g starting up.

> The other question, is it possible that the status is coming unexpectedly? I
> was worried something might be happened while a user operates the storage
> quickly. Such as, "Checking" -> "Unmounting".(Ref from:
> http://www.slideshare.net/wiliwe/android-storage-vold)

Yes - the status can change quite unexpecteedly. For example, if the user removes the sdcard, unplugs the USB cable, etc. Normally, there will be a burst of state changes right after certain user initiated vents: enabling UMS, inserting/removing an sdcard, plugging/unplugging a USB cable. The rest of the time the state doesn't change.

The normal state transitions look like:

NoMedia -> Idle
Idle -> Checking -> Mounted
Mounted -> Idle
Idle -> Formatting
Formatting  -> Idle
Idle -> Shared
Shared -> Idle

Idle means unmounted.

When UMS is enabled and the USB cable is plugged in, then you'll see it do:

Mounted -> Idle -> Shared

and when the USB cable is unplugged then you'll see it do:

Shared -> Init -> Checking -> Mounted

Comment 5

[[e-mail], [e-mail]]

Dave, above explanation is clear for me while I'm implementing the feature. I still meet some problem to recognise what sd-card is happening. I think there are two limitation to block the identification of storage flow accurately. 

1. Async to get storage status, might get later state.

Since storageStatus() API is landed, we will use the API to get status while receive volume "change" event. This API is an async function called. It might get a later status. I was worried that Gaia is not able to reach storage status accurately.


2. Fire change event time is not sync with volume state change.

As implementation state, I try to remove sd-card from device. I only received "change" event once. Then, I called storageStatus() for reaching lasted status. It's in "Unmounting" status. I assume that Gaia is able to receive "change" event three times.

(Scenario:: remove sd-card from device)
Mounted --> Unmounting --> Idle(Unmounted) --> NoMedia

According the flow state, it's should be three "change" events for notifying Gaia. But I only got one. And got a later status "Unmounting".

========================================================
(Scenario:: plug-in sd-card to device)
NoMedia --> Pending --> Idle(Unmounted) --> Checking --> Mounted

According the flow state, it's should be four "change" events for notifying Gaia. But I still got one. In this time, i got a correct status "Mounted".

As offline discussion with Alan, looks like the event is filtered by VolumeManager. Is it possible to reach an accurate status for each volume state change?

Comment 6

[Dave Hylands [:dhylands]]

(In reply to Ian Liu [:ianliu] from comment #5)
> 1. Async to get storage status, might get later state.
> 
> Since storageStatus() API is landed, we will use the API to get status while
> receive volume "change" event. This API is an async function called. It
> might get a later status. I was worried that Gaia is not able to reach
> storage status accurately.

I think that this is the part that's missing. The "change" event is tied in with available() and I don't think we should overload it for this.

I think that we should expose a new async storage-state-changed which basically reflects the volume-state-changed message used by device storage to generate the current changed notification.

> 2. Fire change event time is not sync with volume state change.
> 
> As implementation state, I try to remove sd-card from device. I only
> received "change" event once. Then, I called storageStatus() for reaching
> lasted status. It's in "Unmounting" status. I assume that Gaia is able to
> receive "change" event three times.

That's by design. The change message indicate a change in availability (available, unavailable, shared) and these do not necessarily occur at the same time as volume state changes (they are closely related, but not exactly).

For example, when the AutoMounter decides that its ok to share a volume with the PC, it changes the availability from availabe -> shared when it issues the share command, which will occur prior to the volume actually changing status.

> As offline discussion with Alan, looks like the event is filtered by
> VolumeManager. Is it possible to reach an accurate status for each volume
> state change?

yes.

What's important is that change is tied in with available().

We need a new async event which is tied in with storageStatus()

Comment 7

[[e-mail], [e-mail]]

(In reply to Dave Hylands [:dhylands] from comment #6)
> (In reply to Ian Liu [:ianliu] from comment #5)
> > 1. Async to get storage status, might get later state.
> 
> I think that this is the part that's missing. The "change" event is tied in
> with available() and I don't think we should overload it for this.
> 
> I think that we should expose a new async storage-state-changed which
> basically reflects the volume-state-changed message used by device storage
> to generate the current changed notification.

I would like the suggestion that expose a new storage-state-changed event. That we don't need to listen or interfere with available(). Because there are many apps which is listening the change event from available(). It's not good to increase the event fired time in available().

For the async storage-state-changed which basically reflects the volume-state-changed, is it possible that we are able to get the storage status via event target immediately while a coming storage-state-changed? So that, we don't need to call storageStatus() and wait the onsuccess callback for reaching status. I'm still wondering it's an async method. That could be a later status in the burst volume-state-changed.

> 
> > 2. Fire change event time is not sync with volume state change.
> > 
> 
> That's by design. The change message indicate a change in availability
> (available, unavailable, shared) and these do not necessarily occur at the
> same time as volume state changes (they are closely related, but not
> exactly).
> 
> For example, when the AutoMounter decides that its ok to share a volume with
> the PC, it changes the availability from availabe -> shared when it issues
> the share command, which will occur prior to the volume actually changing
> status.

Yes, that is sync with my implementation experience.

> 
> > As offline discussion with Alan, looks like the event is filtered by
> > VolumeManager. Is it possible to reach an accurate status for each volume
> > state change?
> 
> yes.
> 
> What's important is that change is tied in with available().
> 
> We need a new async event which is tied in with storageStatus()

In my opinion, I think keep original design in available(). Most of the apps which is listening to storage 'change' not need volume state change event. Then, we really need a new async event which is tied in with storageStatus(). If it's possible, we could get the status in parameter of event immediately(without call storageStatus() again) while the event is coming.

Comment 8

[Dave Hylands [:dhylands]]

The volume state change notification is received here:
http://dxr.mozilla.org/mozilla-central/source/dom/devicestorage/nsDeviceStorage.cpp#3947

You could change storageStatus to not return anything and have it just trigger a storage-state-change event.

Then you no longer have an ordering problem.

It might be better to make it do both - return a status via domrequest, and also trigger a storage-state-change event. Then you can ignore what you don't need.

Comment 9

[Dave Hylands [:dhylands]]

It also occurs to me that proably we need the CanBeFormatted, CanBeMounted and CanBeShared attributes from Volume.h to be available somewhere.

I guess we don't have any shipping devices which would be affected. With the nexus 4 & 5, for example, you can't share/mount/unmount/format the sdcard.

Comment 10

[[e-mail], [e-mail]]

(In reply to Dave Hylands [:dhylands] from comment #9)
> 
> I guess we don't have any shipping devices which would be affected. With the
> nexus 4 & 5, for example, you can't share/mount/unmount/format the sdcard.

Yes. In case of internal storage. There is only providing formatting internal storage in the storage management spec. This should be okay. +cc Omega

Comment 11

[[e-mail], [e-mail]]

Attachment: pull request 16981

According to above discussion, we still need API supporting for reaching enough status accurately. This patch is working in process. Feel free to give some feedback and suggest here.

Comment 12

[Alan Huang]

(In reply to Dave Hylands [:dhylands] from comment #9)
> It also occurs to me that proably we need the CanBeFormatted, CanBeMounted
> and CanBeShared attributes from Volume.h to be available somewhere.
> 
> I guess we don't have any shipping devices which would be affected. With the
> nexus 4 & 5, for example, you can't share/mount/unmount/format the sdcard.

I think we have to deal with this case. I heard from our partner that Open C will use FUSE like nexus 4/5 :(

I found the volume "sdcard" is not listed in VolumeManager::mVolumeArray on nexus 4/5, so functions call FindVolumeByName would break. I'm looking into it to see how this happens.

Comment 13

[Alan Huang]

$ adb shell vdc volume list
200 0 Volumes listed.

ok, no volume listed. No wonder we can't find it.

Comment 14

[Alan Huang]

Hi Dave,

Both Open C and nexus 4 uses FUSE to simulate a volume in /data, and they all write the volume.cfg under /etc. One difference is that Open C still have an physical sdcard.

I have a question here, do you know any possible reason that why no volume is found (by command "volume list") on nexus 4, but we can found the one uses FUSE on Open C?

On nexus 4:
$ adb shell vdc volume list
200 0 Volumes listed.

$ adb shell cat /etc/volume.cfg
create sdcard /storage/emulated/legacy

On Open C:
$ adb shell vdc volume list
110 0 sdcard /storage/sdcard 4
110 0 extsdcard /storage/sdcard1 4
200 0 Volumes listed.

$ adb shell cat /etc/volume.cfg
create sdcard /storage/sdcard

Comment 15

[Dave Hylands [:dhylands]]

The /system/etc/volume.cfg is something we added (see bug 890144), to support adding volumes which vold doesn't report.

I'm not sure why the nexus 4 is different than the Open C. That's really a vendor setup thing.

Since the nexus 4 has no physical sdcard, it has no need for vold's services at all, whereas the Open C would still use vold for sharing the sdcard with the PC.

Currently, the AutoMounter/Volume Manager assumes that volumes reported from vold are shareable/formatable. We might need to add something to volume.cfg to indicate that the internal volume can't be shared/formatted/mounted/unmounted.

Actually, volume.cfg is parsed first, so we should just add the internal data sdcard on the Open C to volume.cfg and modify the Volume Manage to ignore volumes reported from vdc which are already registered.

Right now, this check:
http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/nsVolumeService.cpp#363
causes real volumes to take precedence over "fake" ones (the ones from volume.cfg are considered to be fake).

I think it probably makes sense to flip the logic around, and allow the fake ones to take precendence.

Comment 16

[Alan Huang]

(In reply to Dave Hylands [:dhylands] from comment #15)
> The /system/etc/volume.cfg is something we added (see bug 890144), to
> support adding volumes which vold doesn't report.
> 
> I'm not sure why the nexus 4 is different than the Open C. That's really a
> vendor setup thing.

I see. I checked with vendor yesterday, I found this indeed is really vendor setup thing. Same thing now happens on Dolphin, too.

> 
> Since the nexus 4 has no physical sdcard, it has no need for vold's services
> at all, whereas the Open C would still use vold for sharing the sdcard with
> the PC.
> 
> Currently, the AutoMounter/Volume Manager assumes that volumes reported from
> vold are shareable/formatable. We might need to add something to volume.cfg
> to indicate that the internal volume can't be
> shared/formatted/mounted/unmounted.

In this case, I think we don't need additional information. Vold won't report this (fake) volume, VolumeManager won't keep this in mVolumeArray and result in FindVolumeByName won't return this volume. I think things we need to do is set the shareable/formatable to false if we call CreateFakeVolume(). http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/nsVolumeService.cpp#378

> 
> Actually, volume.cfg is parsed first, so we should just add the internal
> data sdcard on the Open C to volume.cfg and modify the Volume Manage to
> ignore volumes reported from vdc which are already registered.
> 
> Right now, this check:
> http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/
> nsVolumeService.cpp#363
> causes real volumes to take precedence over "fake" ones (the ones from
> volume.cfg are considered to be fake).
> 
> I think it probably makes sense to flip the logic around, and allow the fake
> ones to take precendence.

I realized that the problem I met on Open C is not whether fake ones or real ones take the precedence. It is our vendor wrote a bug, assigned mount point of a vfat in volume.cfg to creat a (not that) fake volume. They should assign to the fuse one :)

Comment 17

[Dave Hylands [:dhylands]]

(In reply to Alan Huang [:ahuang] from comment #16)
> (In reply to Dave Hylands [:dhylands] from comment #15)
...snip...
> In this case, I think we don't need additional information. Vold won't
> report this (fake) volume, VolumeManager won't keep this in mVolumeArray and
> result in FindVolumeByName won't return this volume. I think things we need
> to do is set the shareable/formatable to false if we call
> CreateFakeVolume().
> http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/
> nsVolumeService.cpp#378

If by shareable/formatabble, the Volume::CanBeShared and Volume::CanBeFormatted exist in the Volume object, but not in the nsVolume object.

The nsVolume object has an isFake attribute (which is exposed in the idl).

It certainly makes sense to add canBeShared and canBeFormatted attributes (these then have obvious semantics). The implementation would be that canBeShared = !isFake;

Comment 18

[[e-mail], [e-mail]]

Per offline discussion with Alphan, Gaia still cannot get the accurate volume status(not only 'available', 'unavailable', 'shared') now. In order to implement the toast of SDCard status, Gaia have to reach each status. These status are 'NoMedia', 'Idle', 'Pending', 'Checking', 'Mounted', 'Unmounting', 'Formating', 'Shared', and 'ShareMnt'. We have to use state machine to identify what SDCard status is.

For example, 'NoMedia' -> 'Pending' -> 'Idle' -> 'Checking' -> 'Mounted', it means that a user inserts SDCard to device, and the SDCard is known with format size. In this state, we have to show a toast to figure out title: SD Card detected, body: Total size: %d.(Please reference spec. idle screen 1)

For example, 'NoMedia' -> 'Pending' -> 'Idle' -> 'Checking' -> 'Idle', it means that a user inserts SDCard to device, and the SDCard is unknown and unrecognised format for the system. In this state, we have to show a toast to figure out title: SD Card detected, body: Unknown size and format.(Please reference spec. idle screen 2)

....

We will have four idle screens in the user store. So that we need bug 1029403 implementation. The event trigger time should be accurate to reflect volume storage activity. And will expect the event coming with accurate volume state. Then, Gaia no needed to call an async API 'storageStatus()' for the current volume state. It will make some timing issue in async calls.

Comment 19

[[e-mail], [e-mail]]

Comment on attachment 8387426 [details] [review]
pull request 16981

Update the WIP for reaching user story. Then, the patch is needed to add event listener for 'storage-state-change' while bug 1029403 landed.

Comment 21

[[e-mail], [e-mail]]

Comment on attachment 8387426 [details] [review]
pull request 16981

The patch is working for user story 'Storage management'(bug 921105). And Jenny will update a new version with wording change later.

Arthur, could you please help to review my pr in part of Settings app?
Alive, could you please help to review my pr in part of System app?

I'll keep to add unit test during the reviewing process.

Comment 22

[[e-mail], [e-mail]]

(In reply to Ian Liu [:ianliu] from comment #21)
> Comment on attachment 8387426 [details] [review]
> pull request 16981
> 
For verification, please don't use flame for checking. Because it does not support hardware detect pin. In other word, the notification is not working on flame. We have to use following phones for the user story completely.

===== Reference phone with hot-plug =====
Unagi: one external storage

Helix: one internal storage, one external storage

Comment 23

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8387426 [details] [review]
pull request 16981

Thanks for the patch! It works as expected but the naming of some function may lead to readability issues. Please check my comments in github.

Comment 24

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8387426 [details] [review]
pull request 16981

I didn't read through but the new module in system app deserves a test.

Comment 25

[[e-mail], [e-mail]]

Comment on attachment 8387426 [details] [review]
pull request 16981

Alive and Arthur, I have updated the patch per comment on Github. And add unit test for new module 'StorageStatus' in system app. Could you please help to review? Thanks.

Comment 26

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8387426 [details] [review]
pull request 16981

r+ with nits:
* Rename StorageStatus to ExternalStorageMonitor if I understand it correctly.
* Please comment the know-how you replied me in the code. It deserves.
* Better if you move the get storage part to the constructor if we don't need to get storage every time.

Comment 27

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8387426 [details] [review]
pull request 16981

There are a few nits regarding selectors and it should be quick. Please check my comments in github, thanks!

Comment 28

[[e-mail], [e-mail]]

Comment on attachment 8387426 [details] [review]
pull request 16981

Alive and Arthur, thanks for your reviewing effort quickly. I have updated the pr with all of your suggestion. It's pretty clear than before.

Arthur, I will need your review again. :)

Comment 29

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8387426 [details] [review]
pull request 16981

r=me, thank you!!

Comment 30

[[e-mail], [e-mail]]

Let's wait all test passed on Gaia-try.

Comment 31

[[e-mail], [e-mail]]

Since the pr is landed, we can close the issue now.

Gaia/master:  629bc6a1230e37468d24a29b29b8b6c4a2f306b5

Comment 32

[[e-mail], [e-mail]]

Comment on attachment 8387426 [details] [review]
pull request 16981

Hi Jenny, we still need UX review in part of SD card management. Thanks.

Comment 33

[Jenny Lee]

Comment on attachment 8387426 [details] [review]
pull request 16981

Looks good aside from a few points discussed offline, thanks Ian!

Comment 34

[[e-mail], [e-mail]]

Thanks for the reviewing effort. Will give updating patch for some layout changed.