Conversation
FlorentRevest
left a comment
FlorentRevest
left a comment
There was a problem hiding this comment.
I do agree with the main principles of your changes, and ecm works surprisingly well, congratulations and thank you for your work!
However, there are a few things I would want to see changed before allowing your patches to be merged:
- Some of them are described in code comments attached to this review
- An error happens in the kernel when trying to start mtp-server. https://gist.github.com/FlorentRevest/f0350240e9d44f10bab7088cef4078ef are you aware of it?
- Every ~10 seconds USB connection is interrupted, is it a known problem?
- dev-mtp-usb.device timeouts according to https://gist.github.com/FlorentRevest/b468f9b308b1e449f5440ff1e31b41ed I haven't been able to test MTP.
- Is connman supposed to be kept installed? If you plan to remove it, have you thought about packages depending on it?
- DSME keeps track of USB State, could you check whether it's important to its inner working and if we can ignore it or replace the usage of usb-moded to your custom solution.
- lipstick also relies on usb-moded to show handy notifications when connected or disconnected. Have you thought of a solution to keep lipstick informed of this kind of events?
- asteroid-settings currently has a "USB page" relying heavily on usb-moded. If we move toward a composite device exposing almost every capabilities at once, I would want to replace the USB page with a Developer Mode page showing the IP address of the watch and maybe also a randomly generated ceres password (just like in SailfishOS). I would also want this Developer Mode to be enabled or disabled at will, allowing to turn adb and ecm off. If the overall state of your solution gets improved I may be able to help you on this task.
Keep up the good work!
| + | ||
| +target_link_libraries(mtpserver pthread) | ||
| + | ||
| +target_link_libraries(mtpserver pthread) |
There was a problem hiding this comment.
You really want this pthread badly, don't you? :)
| " | ||
|
|
||
| SRC_URI = " \ | ||
| bzr://code.launchpad.net/~phablet-team/mtp/trunk \ |
There was a problem hiding this comment.
I would rather not have asteroid's build system depend on bzr for this single recipe. I couldn't even make it work...
I'd prefer having a mtp-server_0.0.4.bb using:
git://github.com/webOS-ports/mtp-server;protocol=https
and
SRCREV="b5cdb6bbfe3fde97ba65cc270a9b874998a66d07"
| DEPENDS += "boost libhybris glog" | ||
|
|
||
| do_configure_prepend() { | ||
| echo -e "\ntarget_link_libraries(mtpserver pthread)" >> "${S}/CMakeLists.txt" |
There was a problem hiding this comment.
Considering 0001-Remove-dbus-dependency.patch, I don't think we need to insist much more.
| start_usb () { | ||
| echo "0" > $ANDROID_USB/enable | ||
|
|
||
| log_info "Starting adb mode" |
There was a problem hiding this comment.
Why indicating "adb mode" when you actually activate ffs ecm and mtp?
| @@ -0,0 +1,14 @@ | |||
| [Unit] | |||
| Description=mtp-server service | |||
| Requires=usb-setup.service dev-mtp_usb.device | |||
There was a problem hiding this comment.
Haven't you forgot "After=usb-setup.service" ?
Yep, happens on lenok too, I didn't notice that. That's probably worthy of debugging.
This is likely caused by a "feature" in libmtp combined with gvfsd automounting the device (and timeouting as you are missing /dev/mtp_usb for some reason). The proper way to fix that is getting a product ID and sending a patch to libmtp. The less-than-proper way is to steal a vid/pid which is handled correctly in libmtp.
I am not really sure about why is that the case, dory kernel has f_mtp support so udev should just spawn /dev/mtp_usb after mtp gets activated.
It's not interfering with anything so far. Which packages depend on it? I have a build without it installed (for experimenting with systemd-resolved) and everything seems to work fine (is it enough just as a build dependency?)
I am not sure. Looking through the sources, it looks like DSME might end up in an "actdead" state instead of shutting down if it does not know charger state. I don't really know if that is a problem though.
I completely missed that. I am going to add an udev rule for handling this.
I am aware of that. I wanted to get this finalized before starting to patch the interface. |
MagneFire
force-pushed
the
master
branch
2 times, most recently
from
d83c298 to
dd515ab
Compare
2 participants
This PR removes usb-moded in favor of a single-configuration USB composite device providing ADB, CDC ECM and MTP at once.