1043264 - [MTP] Implement needed MTP API

Status: RESOLVED DUPLICATE of bug 1049240

(Firefox OS Graveyard :: General, defect)

Description

[Alphan Chen [:alchen]]

In this bug, we need to discuss the needed MTP API from GAIA view or UX spec.
For example, we may need API for enable/disable MTP.

Comment 1

[Alphan Chen [:alchen]]

Hi Ian,
could you leave a comment about the MTP API from gaia view?

Comment 2

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

Per spec. "USB transfer protocol option" is defined via https://bugzilla.mozilla.org/show_bug.cgi?id=922927#c14, looks like we will have a setting to provide a user for setting default USB Storage mode. The options are MTP/USM. Default protocol is MTP. Will Gecko provide API for enabled/disabled MTP? And the interface will be?

Just reference USM API settings quickly. We use settings key to manage USM on/off. These keys are as following. 

"ums.enabled": false,
"ums.mode": 0,
"ums.status": 0,
"ums.volume.sdcard.enabled": true,
"ums.volume.extsdcard.enabled": false,


Will MTP API is same as USM? Or we have another better interface for MTP settings. Feel free to join the discussion.:)

Comment 3

[Dave Hylands [:dhylands]]

So I think that the fact that ums is in the setting is unfortunate.

It probably should have been "automounter.enabled", automounter.mode, etc.

ums.enabled is what the settings app changes.
ums.mode is what actually controls the automounter, and gaia/apps/system/js/storage.js is what links the 2 together.

Right now ums.mode is defined using these constants:
http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/AutoMounter.h?from=AutoMounter.h#16

I was going to suggest that chnage this to look like:
#define AUTOMOUNTER_DISABLE                 0
#define AUTOMOUNTER_ENABLE                  1 // Auto
#define AUTOMOUNTER_DISABLE_WHEN_UNPLUGGED  2
#define AUTOMOUNTER_ENABLE_MTP              3 // MTP only
#define AUTOMOUNTER_ENABLE_UMS              4 // UMS only

If we don't want "Auto", then I'd also be happy with this definition instead:

#define AUTOMOUNTER_DISABLE                 0
#define AUTOMOUNTER_ENABLE_UMS              1
#define AUTOMOUNTER_DISABLE_WHEN_UNPLUGGED  2
#define AUTOMOUNTER_ENABLE_MTP              3

so that the value 1 maintains its old meaning (just enable UMS).

I think that only the settings app and storage.js need to agree on the definition of ums.enabled. If we change the name, then we'll need to provide an update script to preserve the previous setting.

The ums.volume.VOLUMENAME.enabled will control ums or mtp, and ums.status will continue to reflect the status of the automounter.

If we do decide to rename the settings, then the AutoMounter (or perhaps storage.js) could just detect that the old settings exists, copy the values over to the new settings, and then delete the old ones.

Comment 4

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

Shell we need "Auto"? It might make some confused for what "AUTOMOUNTER" is using, or both of them? But it's impossible to mount the two protocols in the same time.

I agree to change the name if the older one is not clear enough to reflect the volume state. It also make sense to have new name since we have MTP protocol supporting. But Gaia will need more patches(work) for mapping the new name. And more tests are needed to check for existed functionality.

Otherwise, we might considerate to create MTP API in paired. Such as, "mtp.enabled", "mtp.mode", "mtp.xxx".. Gaia could reach what state of specific protocol mode is. And we might keep to use old script. In other words, looks like more duplicated.

Comment 5

[Dave Hylands [:dhylands]]

I think introducing mtp.xxx would be the worst option.

You can only enable ums or mtp (not both). Since the UI spec doesn't support Auto mode, I'm quite happy to not have to implement it.

The best names would be:

automounter.enabled
automounter.mode
automounter.status
automounter.VOLUMENAME.enabled

I think it would be pretty easy for storage.js to detect if ums.xxx is set and transfer the settings over to automounter.xxx

Comment 6

[Alphan Chen [:alchen]]

Attachment: (WIP-0812) Add a automounter.mode for enabling MTP

In this patch, I don't change the name of the related keys.
I just add a new automounter.mode in it.

ums.mode : 0 -> Disable
ums.mode : 1 -> UMS
ums.mode : 2 -> Disable when unplugged
ums.mode : 3 -> MTP


Ian can base on this patch to develop MTP related function in settings APP.

Comment 7

[Dave Hylands [:dhylands]]

Comment on attachment 8471376 [details] [diff] [review]
(WIP-0812) Add a automounter.mode for enabling MTP

Review of attachment 8471376 [details] [diff] [review]:
-----------------------------------------------------------------

So the actual code for enabling/disabling mtp is in my MTP/AutoMounter patch integration, which I just posted today in bug 1049240 (since you guys may be able to use it).

I thought this bug was defining some WebIDL stuff or something for the settings app?

::: dom/system/gonk/AutoMounter.cpp
@@ +35,5 @@
>  #include "Volume.h"
>  #include "VolumeManager.h"
>  
> +#ifdef MOZ_WIDGET_GONK
> +#include <cutils/properties.h>

The #ifdef isn't needed. All of the files in dom/system/gonk are only built for GONK

@@ +189,5 @@
>      // we'll pick it up when the VolumeManage state changes to VOLUMES_READY.
>      CheckVolumeSettings();
>  
> +    // save the original UsbConfigProp into mUsbConfigProp
> +    property_get("sys.usb.config", mUsbConfigProp, nullptr);

We definitely shouldn't save away sys.usb.config. Otherwise we'd miss changes made to mUsbConfigProp in other parts of the system (like enabling/disabling adb).

What I'm doing in bug 1049240 is that I assume persist.sys.usb.config will be setup appropriately for mass_storage.

Enabling mtp, then becomes set sys.usb.config to mtp or mtp,adb depending on whether adb is currently in sys.usb.config or not.

Turning mtp off becomes restore sys.usb.config to persist.sys.usb.config (modulo adb setting)

@@ +418,5 @@
>  
>  void
> +AutoMounter::EnableMtp()
> +{
> +  if (property_set("sys.usb.config", "mtp,adb") != 0) {

We should preserve the existing setting of adb and not forecibly overwrite it.

::: dom/system/gonk/AutoMounter.h
@@ +18,2 @@
>  #define AUTOMOUNTER_DISABLE_WHEN_UNPLUGGED  2
> +#define AUTOMOUNTER_ENABLE_MTP              3

I put a temporary hack into my WIP patch that swaps MTP and UMS so that we can use the settings app to control this.

Comment 8

[Alphan Chen [:alchen]]

Hi Dave,
is there any other work that we can do in this bug?
Or it just a duplicate bug?!
It seems that bug 1049240 will cover the work of this bug.

Comment 9

[Dave Hylands [:dhylands]]

(In reply to Alphan Chen[:Alphan] from comment #8)
> Hi Dave,
> is there any other work that we can do in this bug?
> Or it just a duplicate bug?!
> It seems that bug 1049240 will cover the work of this bug.

Quite possibly. It wasn't clear to me from the description what this bug was for.

When I talked to Eric, he mentioned something about possibly implementing an API for controlling UMS/MTP.

I figured if we weren't going to implement an API then comment 5 and/or comment 6 would have been the outcome.

Comment 10

[Eric Chou [:ericchou] [:echou]]

(In reply to Dave Hylands [:dhylands] from comment #9)
> (In reply to Alphan Chen[:Alphan] from comment #8)
> > Hi Dave,
> > is there any other work that we can do in this bug?
> > Or it just a duplicate bug?!
> > It seems that bug 1049240 will cover the work of this bug.
> 
> Quite possibly. It wasn't clear to me from the description what this bug was
> for.
> 
> When I talked to Eric, he mentioned something about possibly implementing an
> API for controlling UMS/MTP.
> 

Well, originally I talked to Ian and he tended to implement webidl instead of using mozSettings to toggle UMS/MTP. However consider the time pressure we're facing, I convinced him to just use original mechanism (mozSettings) at least for this release. This issue was filed to implement Gaia/Gecko interface of controlling MTP, which is the way to use mozSettings.

> I figured if we weren't going to implement an API then comment 5 and/or
> comment 6 would have been the outcome.

Great then. Close this as resolved duplicate of bug 1049240 per comment 8 and 9.

Thanks.

Comment 11

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

Comment on attachment 8471376 [details] [diff] [review]
(WIP-0812) Add a automounter.mode for enabling MTP

Since we will use API from bug 1049240, clean the flag here.