Closed Bug 1049240 Opened 5 years ago Closed 5 years ago

Integrate MTP into the AutoMounter

Categories

(Firefox OS Graveyard :: General, defect)

x86_64
Linux
defect
Not set

Tracking

(feature-b2g:2.1)

RESOLVED FIXED
2.1 S3 (29aug)
feature-b2g 2.1

People

(Reporter: dhylands, Assigned: dhylands)

References

Details

(Whiteboard: [ft:devices])

Attachments

(2 files, 2 obsolete files)

The AutoMounter currently control USB Mass Storage, and should also be responsible for configuring the usb gadget to start mtp as well as determining which volumes should be shared via MTP.
Attached patch WIP: MTP/AutoMounter integration (obsolete) — Splinter Review
This is the MTP/AutoMounter integration so far.

I put a hack in AutoMounter.h where I swapped the definitions of AUTOMOUNT_ENABLE_UMS and AUTOMOUNTER_ENABLE_MTP so that I can turn it on/off using the Settings App.

Next piece of work is to make adding/removing storages happen when volumes become mounted/not-mounted.
Attachment #8471727 - Attachment description: mtp-automounter.patch → WIP: MTP/AutoMounter integration
feature-b2g: --- → 2.1
Whiteboard: [ft:devices]
Duplicate of this bug: 1043264
Comment on attachment 8471727 [details] [diff] [review]
WIP: MTP/AutoMounter integration

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

Hi Dave,
Based on this patch, I cannot change the mode to MTP via setting APP.

How to reproduce:
Boot up device -> enable UMS from setting APP.

::: dom/system/gonk/AutoMounter.cpp
@@ +657,5 @@
> +  // CONFIGURED -> DISCONNECTED -> CONNECTED -> CONFIGURED
> +  //
> +  // Since IsUsbCablePluggedIn returns state == CONFIGURED, it will look
> +  // like a cable pull and replugin.
> +

From log, the state stay on "UMS_CONFIGURED".
The following switch don't take care about "UMS_CONFIGURED" state.
Every time we got here, we just do nothing.
Sorry about that. I was at a point where things were compiling, but apparently not working. I've been debugging everything and I expect I'll be able to update the patch later today with a working version.
This version seems to be working properly.

And with the hack (Pt2) you can now control individual storage areas and whether they're shared via MTP or not.

Currently all storage areas are advertised as removable.
Attachment #8471727 - Attachment is obsolete: true
Attachment #8473521 - Flags: review?(echou)
This hack basically swaps the UMS/MTP modes so that we can test MTP using the existing UI.
Comment on attachment 8473521 [details] [diff] [review]
Pt 1 - MTP/AutoMounter integration v2

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

Overall looks good to me, r=me with nits addressed and question answered. Please see my comments below:

::: dom/system/gonk/AutoMounter.cpp
@@ +659,3 @@
>    bool  mtpEnabled = false;
> +  bool  usbCablePluggedIn = false;
> +  bool usbConfigured = false;

The variable |usbConfigured| is only used in the if-block starting from #681. Please move the declaration into that if-block.

@@ +687,5 @@
> +      DBG("UpdateState: USB functions: '%s'", functionsStr);
> +
> +      umsAvail = (access(ICS_SYS_UMS_DIRECTORY, F_OK) == 0);
> +      if (umsAvail) {
> +        umsConfigured = usbConfigured && strstr(functionsStr, "mass_storage") != nullptr;

Should we use USB_FUNC_UMS instead of "mass_storage"?

@@ +696,5 @@
> +      }
> +
> +      mtpAvail = (access(ICS_SYS_MTP_DIRECTORY, F_OK) == 0);
> +      if (mtpAvail) {
> +        mtpConfigured = usbConfigured && strstr(functionsStr, "mtp") != nullptr;

Ditto. You may want to use USB_FUNC_MTP instead of "mtp"

@@ +732,5 @@
> +          SetState(STATE_UMS_CONFIGURED);
> +        } else {
> +          // We do this whether or not UMS is enabled. With UMS, it's the
> +          // sharing of the volume which is significant. What is important
> +          // is that we don't leave it in MTP mode is MTP isn't enabled.

There should be a typo somewhere ... maybe you wanted to say "when MTP isn't enabled"?

@@ +793,5 @@
> +    // there is a bug in the state machine and that b2g is now consuming 100%
> +    // of the CPU, so you won't even see your logcats. By limiting the number
> +    // of times we loop at least the phone will boot up and we'll get to see
> +    // the state transitions in logcat.
> +  } while (mState != prevState && ++loopCount < 10);

Question: I don't think I fully understand why a do-while loop is used here. Is it because there will not be any event fired when the state changes, say from STATE_MTP_CONFIGURING to STATE_MTP_STARTED, which means we don't have any chance to call UpdateState() again after leaving the function? Also, what if mState is still equal to prevState before the whole process of operation is done? For example:

  1st loop: STATE_IDLE => mtpEnabled && usbCablePluggedIn is true => STATE_MTP_CONFIGURING
  2st loop: STATE_MTP_CONFIGURING => mtpEnabled && mtpConfigured is false => STATE_MTP_CONFIGURING => leave the loop

::: dom/system/gonk/MozMtpDatabase.cpp
@@ +252,5 @@
>  void
>  MozMtpDatabase::endSendObject(const char* aPath,
> +                              MtpObjectHandle aHandle,
> +                              MtpObjectFormat aFormat,
> +                              bool succeeded)

nit: succeeded => aSucceeded

::: dom/system/gonk/MozMtpStorage.cpp
@@ +66,5 @@
> +      // The volume is no longer accessible. We need to remove this storage
> +      // from the MTP server
> +
> +      RemoveStorage();
> +      MOZ_ASSERT(!mMtpStorage);

Suggestion: if we do need to make sure mMtpStorage has no value after RemoveStorage(), would moving the assertion to the end of RemoveStorage() be a good idea? Then we don't need to assert after RemoveStorage() are called.

::: dom/system/gonk/MozMtpStorage.h
@@ +31,5 @@
> +private:
> +  virtual ~MozMtpStorage();
> +  virtual void Notify(Volume* const& aEvent);
> +
> +  void AddStorage();

Just my suggestion and should not block the implementation -- it seems to make more sense to me to put AddStorage()/RemoveStorage() under MozMtpServer since it's a 'has-a' relation between MtpStorage and its holder in this case.

@@ +35,5 @@
> +  void AddStorage();
> +  void RemoveStorage();
> +
> +  nsRefPtr<MozMtpServer>      mMozMtpServer;
> +  ScopedDeletePtr<MtpStorage> mMtpStorage;

Question: should we use UniquePtr instead of Scoped*?
Attachment #8473521 - Flags: review?(echou) → review+
Duplicate of this bug: 1053698
(In reply to Eric Chou [:ericchou] [:echou] from comment #7)
> Comment on attachment 8473521 [details] [diff] [review]
> Pt 1 - MTP/AutoMounter integration v2
> 
> Review of attachment 8473521 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Overall looks good to me, r=me with nits addressed and question answered.
> Please see my comments below:
> 
> ::: dom/system/gonk/AutoMounter.cpp
> @@ +659,3 @@
> >    bool  mtpEnabled = false;
> > +  bool  usbCablePluggedIn = false;
> > +  bool usbConfigured = false;
> 
> The variable |usbConfigured| is only used in the if-block starting from
> #681. Please move the declaration into that if-block.

will do

> @@ +687,5 @@
> > +      DBG("UpdateState: USB functions: '%s'", functionsStr);
> > +
> > +      umsAvail = (access(ICS_SYS_UMS_DIRECTORY, F_OK) == 0);
> > +      if (umsAvail) {
> > +        umsConfigured = usbConfigured && strstr(functionsStr, "mass_storage") != nullptr;
> 
> Should we use USB_FUNC_UMS instead of "mass_storage"?

yep

> @@ +696,5 @@
> > +      }
> > +
> > +      mtpAvail = (access(ICS_SYS_MTP_DIRECTORY, F_OK) == 0);
> > +      if (mtpAvail) {
> > +        mtpConfigured = usbConfigured && strstr(functionsStr, "mtp") != nullptr;
> 
> Ditto. You may want to use USB_FUNC_MTP instead of "mtp"

yep

> @@ +732,5 @@
> > +          SetState(STATE_UMS_CONFIGURED);
> > +        } else {
> > +          // We do this whether or not UMS is enabled. With UMS, it's the
> > +          // sharing of the volume which is significant. What is important
> > +          // is that we don't leave it in MTP mode is MTP isn't enabled.
> 
> There should be a typo somewhere ... maybe you wanted to say "when MTP isn't
> enabled"?

Correct.

> @@ +793,5 @@
> > +    // there is a bug in the state machine and that b2g is now consuming 100%
> > +    // of the CPU, so you won't even see your logcats. By limiting the number
> > +    // of times we loop at least the phone will boot up and we'll get to see
> > +    // the state transitions in logcat.
> > +  } while (mState != prevState && ++loopCount < 10);
> 
> Question: I don't think I fully understand why a do-while loop is used here.
> Is it because there will not be any event fired when the state changes, say
> from STATE_MTP_CONFIGURING to STATE_MTP_STARTED, which means we don't have
> any chance to call UpdateState() again after leaving the function? Also,
> what if mState is still equal to prevState before the whole process of
> operation is done? For example:

It's really to deal with the startup conditions. If you stop and restart b2g, then the state of the usb connection doesn't change. So we need to go from STATE_INIT -> STATE_MTP_CONFIGURING -> STATE_MTP_CONFIGURED with no events to drive us.

For normal system startup where we actually change sys.usb.config then we'll get UEvents as the USB state changes.

I could remove the loop by detecting mtpConfigured and calling StartMtpServer in STATE_IDLE (and I think I'll do that).

>   1st loop: STATE_IDLE => mtpEnabled && usbCablePluggedIn is true =>
> STATE_MTP_CONFIGURING
>   2st loop: STATE_MTP_CONFIGURING => mtpEnabled && mtpConfigured is false =>
> STATE_MTP_CONFIGURING => leave the loop

This implies that the usb connection has not completed configuring. When it does finish, it will generate a UEvent, which will trigger another call to UpdateState.

> 
> ::: dom/system/gonk/MozMtpDatabase.cpp
> @@ +252,5 @@
> >  void
> >  MozMtpDatabase::endSendObject(const char* aPath,
> > +                              MtpObjectHandle aHandle,
> > +                              MtpObjectFormat aFormat,
> > +                              bool succeeded)
> 
> nit: succeeded => aSucceeded

fixed

> ::: dom/system/gonk/MozMtpStorage.cpp
> @@ +66,5 @@
> > +      // The volume is no longer accessible. We need to remove this storage
> > +      // from the MTP server
> > +
> > +      RemoveStorage();
> > +      MOZ_ASSERT(!mMtpStorage);
> 
> Suggestion: if we do need to make sure mMtpStorage has no value after
> RemoveStorage(), would moving the assertion to the end of RemoveStorage() be
> a good idea? Then we don't need to assert after RemoveStorage() are called.

I'll remove those asserts. I think that they were there due to code changes that didn't get cleaned up.

> ::: dom/system/gonk/MozMtpStorage.h
> @@ +31,5 @@
> > +private:
> > +  virtual ~MozMtpStorage();
> > +  virtual void Notify(Volume* const& aEvent);
> > +
> > +  void AddStorage();
> 
> Just my suggestion and should not block the implementation -- it seems to
> make more sense to me to put AddStorage()/RemoveStorage() under MozMtpServer
> since it's a 'has-a' relation between MtpStorage and its holder in this case.

So what I was thinking about here what that MozMtpStorage would know how to monitor the volume and it does in fact tell the MtpServer to add/remove the MtpStorage.

The issue relates to what you need to know to create an MtpStorage object. I'd basically have to move all of the information into MozMtpServer, and then it needs to create an array of objects with all of the information, which is essentially what MozMtpStorage is.

MozMtpStorage is basically monitoring the volume and tell the MTP server when the storage are is available (by calling server->addStorage) or unavailable (by calling server->removeStorage.

Perhaps MozMtpStorage::AddStorage and RemoveStorage should be renamed to be: MozMtpStorage::StorageAvailable and StorageUnavailable and then it would make more sense the way it is?

I think I'll do that.

> @@ +35,5 @@
> > +  void AddStorage();
> > +  void RemoveStorage();
> > +
> > +  nsRefPtr<MozMtpServer>      mMozMtpServer;
> > +  ScopedDeletePtr<MtpStorage> mMtpStorage;
> 
> Question: should we use UniquePtr instead of Scoped*?

I changed it, but it doesn't seem to buy you anything.

I had to change mMtpServer = new MtpServer(...) to be mMtpServer.reset(new MtpServer(...))
and instead of just passing mMtpServer, I now had to call mMtpServer.get()
This should address all of the issues that you mentioned. I think enough changed that's its worth looking at the differences.
Attachment #8473521 - Attachment is obsolete: true
Attachment #8474923 - Flags: review?(echou)
Bah - I meant to turn USE_DEBUG to 0
Comment on attachment 8474923 [details] [diff] [review]
Pt 1 - MTP/AutoMounter integration v3

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

r=me with nits picked.

Thanks!

::: dom/system/gonk/AutoMounter.cpp
@@ +727,5 @@
> +        } else {
> +          // The MTP USB layer is configuring. Wait for it to finish
> +          // before we start the MTP server.
> +          SetUsbFunction(USB_FUNC_MTP);
> +          SetState(STATE_MTP_CONFIGURING); 

nit: trailing whitespace.

@@ +786,5 @@
> +    case STATE_UMS_CONFIGURED:
> +      if (usbCablePluggedIn) {
> +        if (mtpEnabled) {
> +          // MTP was enabled. Start reconfiguring.
> +          SetState(STATE_MTP_CONFIGURING); 

nit: trailing whitespace.
Attachment #8474923 - Flags: review?(echou) → review+
Removed trailing whitespace.
Turned off debug.

https://hg.mozilla.org/integration/b2g-inbound/rev/19e59c14b0aa
Just a note, only debug xpshell tests were broken. opt was fine.
Target Milestone: 2.1 S2 (15aug) → 2.1 S3 (29aug)
(In reply to Nigel Babu [:nigelb] from comment #15)
> I've just backed this out for xpshell bustage in 
> https://hg.mozilla.org/integration/b2g-inbound/rev/6e6c307eb259
> 
> Bustage:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=46305708&tree=B2g-Inbound

The failures appear to have instead been caused by bug 1052503.
This can reland - sorry for the churn.
https://hg.mozilla.org/mozilla-central/rev/63f398919502
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Hi Dave,
there is one MTP problem with this patch landed.
We don't meet this problem before integrating MTP into the AutoMounter.

How to reproduce?
1. enable MTP first
2. unplug USB cable from PC
3. Plug USB cable into PC again

Symptom:
After I unplug USB and plug USB again, the device cannot be detected as a MTP device.
We need to disable MTP first and enable MTP again to have the MTP function work.

(LOG)
----> unplug USB cable from PC
08-22 15:22:36.120   292   981 V MtpServer: request read returned -1, errno: 5
08-22 15:22:36.120   292   981 I MozMtp  : sessionEnded:
08-22 15:22:36.120   292   981 I MozMtp  : Run: MozMtpServer finished
08-22 15:22:36.130   292   490 I AutoMounter: AutoMounter state changed from MTP_STARTED to IDLE
08-22 15:22:36.130   292   490 I AutoMounter: UpdateState: ums:A1C0E0 mtp:A1C0E1 mode:3 usb:0 tryToShare:0 state:IDLE
08-22 15:22:36.130   292   490 I AutoMounter: UpdateState: Volume external is Mounted and inserted @ /storage/sdcard1 gen 1 locked 0 sharing en-n
08-22 15:22:36.130   292   490 I AutoMounter: UpdateState: Volume sdcard is Mounted and inserted @ /storage/sdcard0 gen 2 locked 0 sharing en-n
----> plug USB cable into PC
08-22 15:22:43.020   292   490 I AutoMounter: AutoMounter state changed from IDLE to MTP_STARTED
08-22 15:22:43.020   292   490 I AutoMounter: UpdateState: ums:A1C0E0 mtp:A1C1E1 mode:3 usb:1 tryToShare:0 state:MTP_STARTED
08-22 15:22:43.020   292   490 I AutoMounter: UpdateState: Volume external is Mounted and inserted @ /storage/sdcard1 gen 1 locked 0 sharing en-n
08-22 15:22:43.020   292   490 I AutoMounter: UpdateState: Volume sdcard is Mounted and inserted @ /storage/sdcard0 gen 2 locked 0 sharing en-n
Without this patch, the behavior is different.
We will stop MtpServer after unplugging USB cable.
Then we will enable MtpServer after plugging USB cable again.

(LOG)
-----> unplug USB cable from PC
08-22 16:52:00.661   291  1240 V MtpServer: request read returned -1, errno: 5
08-22 16:52:00.661   291  1240 I MozMtp  : sessionEnded:
08-22 16:52:00.661   291  1240 I MozMtp  : Run: MozMtpServer finished
08-22 16:52:00.661   291  1240 I MozMtp  : ~MozMtpDatabase:
08-22 16:52:00.671   291   513 I AutoMounter: UpdateState: ums:10 mtp:11 mode:3 usbCablePluggedIn:0 tryToShare:0
08-22 16:52:00.671   291   513 I AutoMounter: Stopping MtpServer
----> plug USB cable into PC
08-22 16:52:13.411   291   513 I AutoMounter: UpdateState: ums:10 mtp:11 mode:3 usbCablePluggedIn:1 tryToShare:1
08-22 16:52:13.411   291   513 I AutoMounter: Starting MtpServer
08-22 16:52:13.411   291  1281 I MozMtp  : Run: MozMtpServer open done, fd: 179. Start reading.
Hi Dave,
any idea about this symptom?
I think the latest behavior is what you want.
However, I think user don't expect that he need to re-enable MTP again to get MTP work.
Flags: needinfo?(dhylands)
I think the problem should be related to MtpServer design.
From log, we knew that MtpServer is closed then our MozMtpServer is closed.
So we should new a MtpServer after re-plug in the USB cable.
Alphan - yeah there's probably an issue in how the MtpServer is being started.

I'm just finishing off my device storage integration, so I should be able to take a look at this tomorrow.
Flags: needinfo?(dhylands)
Blocks: 1059086
You need to log in before you can comment on or make changes to this bug.