Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

modemmanager: add ModemManager to packages #9137

Merged
merged 3 commits into from
Oct 7, 2019
Merged

modemmanager: add ModemManager to packages #9137

merged 3 commits into from
Oct 7, 2019

Conversation

mips171
Copy link
Contributor

@mips171 mips171 commented Jun 4, 2019

Signed-off-by: Nicholas Smith nicholas.smith@telcoantennas.com.au

Maintainer: @nickberry17
Compile tested: mips, telco-electronics-x1, 18.06.2 + 18.06.4 + master; ath79, cf-e5, 18.06.2 + 18.06.4 + master
Run tested: mips, telco-electronics-x1, 18.06.2 + 18.06.4 + master, ran all basic functions; ath79, cf-e5, 18.06.2 + 18.06.4 + master, ran all basic functions

Description: ModemManager 1.10.6, libmbim 1.20.0 and libqmi 1.22.6

libs/libmbim/Makefile Outdated Show resolved Hide resolved
libs/libmbim/Makefile Outdated Show resolved Hide resolved
@BKPepe
Copy link
Member

BKPepe commented Jun 4, 2019

Are you really sure that you want to be the maintainer of those packages? I was able to track it down to this repository on Gitlab - https://gitlab.freedesktop.org/mobile-broadband/mobile-broadband-openwrt
Will you do any changes what he does there like updating packages and so on?

The author (@aleksander0m) is really active there and would be good if he is aware that you want to add his work to upstream, which is a good approach, but he should be noticed and maybe even become a maintainer. Anyway, the authorship should be kept.

I think it will be good if you can split your commit into 3 commits as you are adding 3 packages.

The README except the third point is not necessary after all.

@aleksander0m
Copy link
Contributor

aleksander0m commented Jun 5, 2019

Are you really sure that you want to be the maintainer of those packages? I was able to track it down to this repository on Gitlab - https://gitlab.freedesktop.org/mobile-broadband/mobile-broadband-openwrt
Will you do any changes what he does there like updating packages and so on?

The author (@aleksander0m) is really active there and would be good if he is aware that you want to add his work to upstream, which is a good approach, but he should be noticed and maybe even become a maintainer. Anyway, the authorship should be kept.

I would be more than happy to give maintainership of these packages I keep in https://gitlab.freedesktop.org/mobile-broadband/mobile-broadband-openwrt to anyone that is willing to maintain them under the official openwrt packages repostory.

If Nick wants to do this work, I would be extremely greateful!

@aleksander0m
Copy link
Contributor

@nickberry17 btw, please note that I updated the MM package to latest 1.10 release a couple of days ago, including the netifd proto handler using the new mmcli key-value output support.

@aleksander0m
Copy link
Contributor

@nickberry17 btw, please note that I updated the MM package to latest 1.10 release a couple of days ago, including the netifd proto handler using the new mmcli key-value output support.

And... nevermind, I see this was already included in these commits.

@dangowrt
Copy link
Member

dangowrt commented Jun 5, 2019

What are your reasons against using umbim instead of all the effort of porting freedesktop code to OpenWrt?

@aleksander0m
Copy link
Contributor

What are your reasons against using umbim instead of all the effort of porting freedesktop code to OpenWrt?

uqmi and umbim are usually more than enough to setup connectivity, but I could list several corner cases where they're not enough though (e.g. IIRC umbim cannot handle long MBIM packets that would require >1 message in the transaction).

libqmi+qmicli, libmbim+mbimcli, and ModemManager+mmcli are by themselves very useful when debugging issues further, or when you want to support also devices that would fall in those corner cases I talk about, or when you need to do things that aren't common (e.g. Sierra Wireless firmware update). In the current setup, ModemManager won't require you to know whether a device is QMI, MBIM or who-knows-what, or whether a given QMI device needs raw-ip settings or 802.3 in the network interface, or whether a specific Huawei modem is able to setup the NCM interface with NDISDUP, or whether your MBIM modem can do DHCP or not.... the settings in /etc/config/network will all look the same.

@sfx2000
Copy link

sfx2000 commented Jun 6, 2019

Note - to build MM, should add the following ubuntu packages for the host...

intltool
libtool
gtk-doc-tools
libglib2.0-dev
libgudev-1.0-dev

Anyways - really happy to see traction on this - I've been working with it on ath79 and ar7xxx/ar9xxx with GL-iNet AR-150, and it's good for a 64/16 target on MIPS24Kc on Master with uBlox CATM1 (Sara R4 family)

libs/libqmi/Makefile Outdated Show resolved Hide resolved
libs/libqmi/Makefile Outdated Show resolved Hide resolved
net/modemmanager/LICENSE Outdated Show resolved Hide resolved
net/modemmanager/Makefile Outdated Show resolved Hide resolved
$(INSTALL_BIN) ./files/modemmanager.proto $(1)/lib/netifd/proto/modemmanager.sh
endef

$(eval $(call RequireCommand,xsltproc, \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will definitely fail on the buildbots. Please create Host packages if needed or add PKG_BUILD_DEPENDS:=package/host

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nickberry17 why this is marked as resolved when it isn't?

@neheb
Copy link
Contributor

neheb commented Jun 6, 2019

My comments apply to multiple packages.

I see you're also adding multiple shell scripts. Could be good to run through shellcheck.

@aleksander0m
Copy link
Contributor

Note - to build MM, should add the following ubuntu packages for the host...

intltool

intltool is not required since MM 1.10, only a new enough gettext is needed.

@mips171
Copy link
Contributor Author

mips171 commented Jun 9, 2019

Note - to build MM, should add the following ubuntu packages for the host...

intltool
libtool
gtk-doc-tools
libglib2.0-dev
libgudev-1.0-dev

Anyways - really happy to see traction on this - I've been working with it on ath79 and ar7xxx/ar9xxx with GL-iNet AR-150, and it's good for a 64/16 target on MIPS24Kc on Master with uBlox CATM1 (Sara R4 family)

Is this the right way to specify these? @neheb

  HOST_BUILD_DEPENDS:= \
	+libtool/host \
	+gtk-doc-tools/host \
	+libglib2.0-dev/host \
	+libgudev-1.0-dev/host

@neheb
Copy link
Contributor

neheb commented Jun 9, 2019

No. Only the first one will work. The others need to be added if indeed necessary.

libs/libqmi/Makefile Outdated Show resolved Hide resolved
net/modemmanager/Makefile Outdated Show resolved Hide resolved
@BKPepe
Copy link
Member

BKPepe commented Jun 15, 2019

@nickberry17 Can you look why the DCO check failed for you?

@mips171
Copy link
Contributor Author

mips171 commented Jun 15, 2019

@nickberry17 Can you look why the DCO check failed for you?

Fixed

libs/libmbim/Makefile Outdated Show resolved Hide resolved
libs/libmbim/Makefile Show resolved Hide resolved
libs/libqmi/Makefile Show resolved Hide resolved
net/modemmanager/Makefile Show resolved Hide resolved
net/modemmanager/Makefile Outdated Show resolved Hide resolved
net/modemmanager/files/modemmanager.common Outdated Show resolved Hide resolved
net/modemmanager/files/modemmanager.common Outdated Show resolved Hide resolved
net/modemmanager/files/modemmanager.common Outdated Show resolved Hide resolved
net/modemmanager/files/modemmanager.proto Show resolved Hide resolved
net/modemmanager/files/modemmanager.proto Show resolved Hide resolved
Copy link
Member

@mhei mhei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also prefer to have 3 commits in this PR: one for each single package, i.e. libmbim, libqmi and modemmanager.

libs/libmbim/Makefile Outdated Show resolved Hide resolved
libs/libqmi/Makefile Outdated Show resolved Hide resolved
libs/libqmi/Makefile Outdated Show resolved Hide resolved
libs/libmbim/Makefile Outdated Show resolved Hide resolved
libs/libqmi/Makefile Outdated Show resolved Hide resolved
net/modemmanager/files/modemmanager.proto Outdated Show resolved Hide resolved
net/modemmanager/files/25-modemmanager-net Outdated Show resolved Hide resolved
net/modemmanager/files/25-modemmanager-net Show resolved Hide resolved
net/modemmanager/files/25-modemmanager-tty Outdated Show resolved Hide resolved
net/modemmanager/README Outdated Show resolved Hide resolved
libs/libqmi/Makefile Outdated Show resolved Hide resolved
libs/libmbim/Makefile Outdated Show resolved Hide resolved
libs/libqmi/Makefile Outdated Show resolved Hide resolved
@neheb
Copy link
Contributor

neheb commented Sep 10, 2019

Comment was deleted so let me just clarify the LICENSE[_FILES] request. The libs come with their own utilities which are GPL licensed, not LGPL. Those utilities are not packaged, making it somewhat of a moot point but that's the reasoning.

@diizzyy
Copy link
Contributor

diizzyy commented Sep 10, 2019

LGTM otherwise

net/modemmanager/Makefile Outdated Show resolved Hide resolved
libs/libqmi/Makefile Outdated Show resolved Hide resolved
libs/libqmi/Makefile Outdated Show resolved Hide resolved
@neheb
Copy link
Contributor

neheb commented Sep 27, 2019

I don't see much else. This looks ready.

Nicholas Smith added 3 commits September 27, 2019 11:26
Signed-off-by: Nicholas Smith <nicholas.smith@telcoantennas.com.au>
Signed-off-by: Nicholas Smith <nicholas.smith@telcoantennas.com.au>
Signed-off-by: Nicholas Smith <nicholas.smith@telcoantennas.com.au>
@mips171
Copy link
Contributor Author

mips171 commented Oct 7, 2019

I don't see much else. This looks ready.

LGTM

@mhei mhei merged commit d3012ec into openwrt:master Oct 7, 2019
@BKPepe
Copy link
Member

BKPepe commented Oct 8, 2019

You or somebody else should add host dependencies to build requirements.

Checking 'xsltproc'... failed.
Checking 'intltoolize'... failed.

modemmanager: modemmanager requires xsltproc installed on the host-system.
modemmanager: modemmanager requires intltool installed on the host-system.

@aleksander0m
Copy link
Contributor

You or somebody else should add host dependencies to build requirements.

Checking 'xsltproc'... failed.
Checking 'intltoolize'... failed.

modemmanager: modemmanager requires xsltproc installed on the host-system.
modemmanager: modemmanager requires intltool installed on the host-system.

intltool is actually not required at all (since it was ported to use gettext only), the check in the packaging should be removed.

@mips171
Copy link
Contributor Author

mips171 commented Oct 9, 2019

Thanks, I had overlooked that. From what I can tell, xsltproc and gettext are already listed as build requirements for the systems that need them, so I think removing the check for intltool should suffice; but can I create an entry for ModemManager in the documentation? Is there no way to specify these host dependencies in the makefile so that they are automatically resolved?

@neheb
Copy link
Contributor

neheb commented Oct 10, 2019

Why is xsltproc needed? man pages are not being installed for OpenWrt.

@aleksander0m
Copy link
Contributor

Why is xsltproc needed? man pages are not being installed for OpenWrt.

xsltproc is used to generate the ModemManager-names.h header file (part of the daemon API) from all available introspection files. This header includes all DBus object path prefixes, interfaces, methods, properties and so on.

@bobafetthotmail
Copy link
Contributor

@nickberry17 with the default config, most people that have performant modems (using QMI or MBIM) would need to compile their own modemmanager package because by default that support is disabled https://github.com/openwrt/packages/blob/master/net/modemmanager/Config.in

That's very inconvenient imho.

I think you should either enable that by default or find a way to split that into packages that users can install without compiling.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants