Test message - please ignore
Checking if this comes through. Sorry for the spam. MikaL ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: [PATCH 1/3] core: notify watches of already registered atoms
Hi Denis,
On 03/29/2011 02:48 AM, Mika Liljeberg wrote:
---
src/modem.c | 16 +++-
1 files changed, 15 insertions(+), 1 deletions(-)
diff --git a/src/modem.c b/src/modem.c
index 655994b..d22c718 100644
--- a/src/modem.c
+++ b/src/modem.c
@@ -271,6 +271,9 @@ unsigned int
__ofono_modem_add_atom_watch(struct ofono_modem *modem,
void *data,
ofono_destroy_func destroy)
{
struct atom_watch *watch;
+ unsigned int id;
+ GSList *l;
+ struct ofono_atom *atom;
if (notify == NULL)
return 0;
@@ -282,8 +285,19 @@ unsigned int
__ofono_modem_add_atom_watch(struct ofono_modem *modem,
watch-item.destroy = destroy;
watch-item.notify_data = data;
- return __ofono_watchlist_add_item(modem-atom_watches,
+ id = __ofono_watchlist_add_item(modem-atom_watches,
(struct
ofono_watchlist_item *)watch);
+
+ for (l = modem-atoms; l; l = l-next) {
+ atom = l-data;
+
+ if (atom-type != type || atom-unregister == NULL)
+ continue;
+
+ notify(atom,
OFONO_ATOM_WATCH_CONDITION_REGISTERED, data);
+ }
+
+ return id;
}
gboolean __ofono_modem_remove_atom_watch(struct ofono_modem *modem,
This patch is behavior changing. Whether this is a good idea
or not I'm
still undecided, though I see its merits.
However, I don't see how it is fixing anything. The watches
are called
only upon registration (which means the driver is probed and
ready). So
even if you create two netreg atoms in hopes of probing the
modem type,
only one should ever register and this code should not be needed.
I do have an actual bug, which this patch fixes. I've been investigating why,
with isiusb, the gprs atom sometimes fails to receive the network registration
status and therefore fails to attach to the GPRS service. It turns out that the
problem comes from the client side code pattern for registering atom watches.
The code assumes that there will only be a single atom of the same type
(registered or not).
Basically, gprs atom and the alternative netreg atoms are probing in parallel
and may register themselves in a random order. If gprs finishes first, it will
correctly get a call to its watch handler when one of the netreg atoms
completes its registration. However, if the order is reversed and gprs finishes
after the netreg atom, the following snippet of code tries to find the already
registered netreg atom:
netreg_atom = __ofono_modem_find_atom(modem, OFONO_ATOM_TYPE_NETREG);
if (netreg_atom __ofono_atom_get_registered(netreg_atom))
netreg_watch(netreg_atom,
OFONO_ATOM_WATCH_CONDITION_REGISTERED, gprs);
The trouble here is that __ofono_modem_find_atom() just returns the first
neterg atom it finds, regardless of whether it is registered or not. In this
case it happens to be the wrong netreg (wgmodem2.5 version, which fails to
probe), even though the other netreg atom is already present and registered.
Because of this, the watch funtion is never called.
I realize that there are other ways to fix this. The client side pattern could
be changed to use __ofono_modem_foreach_atom() to look for already registered
atoms, instead of doing the same within the __ofono_modem_add_atom_watch() as
in my patch. This would introduce quite a bit of more code. Alternatively, an
__ofono_modem_find_registered_atom() function could be introduced to look for a
registerd atom of a certain type (might be a good idea to do this in any case).
Or isiusb (and any other similar cases) could be revectored to not probe
multiple atoms of the same type in parallel. However, we already create
multiple GPRS context atoms as a matter of course, so the assumption that there
will only be a single instance is no longer valid for all atom types anyway.
IMO, it would be good to get rid of the assumption altogether.
So, I like my patch because it removes a lot of copy-paste code and does not
assume anything about how many atoms of a certain type there can be.
Regards,
MikaL
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
RE: [PATCH 0/1] isimodem: fix network registration for older modems
Hi Claudio, Thank you so much for your support and help, please let us know once you have the patch. I posted a few patches yesterday, which should improve things. is it possible to know the list of supported mobile phone as 3g modem ? We don't have a list of supported phones as such as we're not systematically tesing oFono against old Nokia phones. The isiusb plugin is primarily a development aid, allowing us to test isimodem drivers on a PC instead of on-target. There are some limitations in using the isimodem driver over USB this way. All functionality may not be available with all phones. Mostly it works fine, though. I see. Since each phone can potentially use a different way to talk to the pc (Android: rndis, Nokia: AT commands / PPP interface over USB if I understand it correctly), we are trying to understand which of these methods (hence which phone family) is preferable to be used/supported with connman / oFono. Do you have any suggestion for us? My advice is to use whatever works for you. Currently, oFono recognizes Nokia phones only via the isiusb plugin, so that is pretty much the only option. However, most Nokia phones also support AT commands over USB ACM and should be relatively easy to get working with oFono, if you're willing to write a new plugin or modify the current nokia plugin for Nokia USB modems to include support for Nokia phones. Patches are welcome. Br, MikaL ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: [PATCH 0/1] isimodem: fix network registration for older modems
Hi Claudio, I have tested the path of isi modem on nokia n95 and nokia 6760, and I report the result of my test 1) Using nokia N95 and ofono * I have attached the phone to my computer * I have used the ./online-modem * I have set apn/username/password and activate the context It works (log onlineUsingOfonoTest file attached to mail) Cool. 2) Using nokia N95 and ofono/connma * I have attached the phone to my computer * Start connman and I have checked it set OnLine the modem correctly * I have set apn/username/password and activate the context * Using connect of connman, but i receive the message org.ofono.Error.NotAttached GPRS is not attached I reports the log onlineUsingConnmanTest, can be a problem of connman ? I was able to reproduce this with N95 without using connman. There seems to be a conflict with the way atom watches are registered and the way isiusb creates and probes two variations of the network registration driver in parallel. The problem is timing dependent, which is probably why we haven't seen it here before. 1) Using nokia 6760 and ofono * I have attached the phone to my computer * I have used the ./online-modem * I have set apn/username/password and activate the context It not works, (log in nokia6760 file attached to mail) Any suggestion ? I believe the same problem also happens with 6760 and could potentially happen with any ISI modem. Now that I know what it is, I'll try to provide patches. is it possible to know the list of supported mobile phone as 3g modem ? We don't have a list of supported phones as such as we're not systematically tesing oFono against old Nokia phones. The isiusb plugin is primarily a development aid, allowing us to test isimodem drivers on a PC instead of on-target. There are some limitations in using the isimodem driver over USB this way. All functionality may not be available with all phones. Mostly it works fine, though. Regards, MikaL ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: ConnectionContext on Nokia 6760s-1
Hi Claudio, Hi, We have tried to attach an Nokia 6760s-1, for using it like 3G modem. Ofono manages correctly the mobile phone creating /isiusb0/ and /isiusb0/context1 and i think it uses the isi modem driver. Next we have activated the pdp context, but it returns the following dbus exception org.ofono.Error.NotAttached GPRS is not attached This basically means that oFono thinks the modem is not attached to the GPRS service. Did you set the modem online before trying this (e.g. by running the online-modem script from the test directory)? is it possible to use the nokia phone as 3G modem? Should be possible, although it may be that the driver needs minor modifications for 6760. Depends on the ISI interface versions of the modem firmware. If it won't work, you can set the OFONO_ISI_DEBUG and OFONO_ISI_TRACE environment variables to get more verbose logging. We'd need the full log output to check what is going wrong. Br, MikaL ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: [RESEND 3 PATCH 00/13] IPv6 Support
Hi Denis, Disabling IPv6 stateless address would be a bold move indeed, since it is declared mandatory in both IETF and 3GPP standards. Please see [RFC4294] and [3GPP 23.060]. The section Dynamic IPv6 Address Allocation in 23.060 is very clear on how IPv6 address allocation in 3GPPP networks is done. See also the section about IPv6 prefix delegation (relevant to IPv6 tethering) and applicable parts of 24.008. This still needs to be figured out. We are aware that autoconfiguration is mandatory in 3GPP. However, this seems to be going against 27.007. There is no contradiction. Let me explain what I mean. The 3GPP standards mostly talk about MS (mobile station) which comprises of both TE and MT (terminal equipment and modem terminal). As such, 3GPP specifications place requirements on both TE and MT. The 27.007 defines a TA (terminal adapter), which is one possible interface between TE and MT. However, 27.007 is not mandatory, vendor specific interfaces are also allowed. As a consequence other interfaces, such as ISI and CAIF, also exist. While stateless address autoconfiguration is mandatory for MS, a funky AT modem could potentially have an internal IPv6 stack as part of the TA function and perform address configuration against the network and then proxy any IPv6 traffic between the TE and the network. This would meet the requirements of 3GPP. The *intent* in the 3GPP standards is that TEs can behave as standard IPv6 hosts (so PCs can use a standard IPv6 stack), which means that the TE is expected to run the autoconfiguration protocols against the network in the normal case. Nothing in 27.007 specifically prevents that. The static address configuration parameters in the AT commands are optional and will simply be missing, if the modem just acts as a bitpipe between TE and the network, allowing the TE to run its IPv6 stack in the normal way. However, a funky AT modem that implements an IPv6 stack internally and exposes static address configuration parameters towards TE is certainly possible. Such a modem would presumably also block the router advertisements coming from the network, which would effectively disable stateless address autoconfiguration for the TE. This is not something you need or should do in connman. Standard IPv6 stack is sufficient. So in the end both might be required. On this I agree. Stateless address autoconfiguration is needed because it is mandatory in both IETF and 3GPP standards (sorry for the repetition). Static configuration (optional in IETF standards) may also be needed but that remains to be seen. It depends on what kind of funky AT modems are and will be out there. In the end oFono's philosophy is to always err on the side of 27.007. So far this strategy has never been (completely) wrong. 27.007 provides a nice checklist for the functionality of the modem, so in that sense you're right. In another way it also serves as a lowest common denominator for the same functionality. However, what 27.007 does not do is specify system behaviour. It's easy to jump to conclusions just by looking at what AT commands are available. Most of the commands are optional, because a lot of freedom has intentionally been left for the implementors. It is also why, IMO, the specification is so bad and we have to fight with all these quirky AT modem implementations. Remember that 3GPP standards specify the behavour between MS and the nework. I.e., they place requirements on both TE and MT. The AT interface (TA function) stands in between and the combination of TE+TA+MT must function in accordance to 3GPP standards. My point is, you *really* need to read other 3GPP specification, like 23.060 and 24.008, in order to understand what requirements 3GPP places on oFono. Reading 27.007 will not tell you that. Regards, MikaL ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: [RESEND 3 PATCH 00/13] IPv6 Support
Hi Denis,
So during OSTS Samuel, Marcel and I sat down and tried to
figure out the
IPv6 stuff. Based on this discussion and your implementation
I pushed a
series of patches implementing IPv6 and dual-stack contexts. I have
taken (and later re-worked) some of your patches so you get
credit here
as well.
Thanks for pushing the patches. I notice that these are based on my initial set
of patches rather than the later ones. A few comments, since I had some reasons
for the changes I did in later patches.
I notice that your version of the patches are not forming the IPv6 link-local
address and configuring it on the network interface. That's ok, as long as
connman takes care of it, but it does mean that Ethernet interfaces, which
always have a link-local address, will autoconfigure immediately while
point-to-point interfaces will only autoconfigure when connman sets the
link-local address on them. We talked about this with Marcel and at the time
concluded that it would make more sense to keep things consistent by having
oFono configure the link-local address on the point-to-point interfaces. I had
this implemented in my later patch sets.
A few comments on the driver API:
void ofono_gprs_context_set_ipv4_address(struct ofono_gprs_context *gc,
const char *address,
gboolean static_ip);
What's the expected behaviour if this is called with a valid IP address and
static_ip = FALSE? I think you could just drop the boolean flag and assume a
statically configured address if this method is called by the driver, otherwise
do DHCP.
void ofono_gprs_context_set_ipv4_dns_servers(struct ofono_gprs_context
*gc,
const char **dns);
void ofono_gprs_context_set_ipv6_dns_servers(struct ofono_gprs_context
*gc,
const char **dns);
I would propose to have just a single method for setting all DNS servers.
Having separate lists for IPv4 and IPv6 DNS servers made sense in my first
patch set, because a dual context could be emulated with separate IPv4 and IPv6
contexts and those DNS servers might have been behind different network
interfaces. However, now this just creates additional complexity for the
drivers. A dual context will get a list of DNS server addresses, which may
contain IPv4 addresses, IPv6 addresses or both. Now the driver has to sort them
into two separate lists for IPv4 and IPv6. Note that you can make A and
queries to any server, so there is no particular reason to separate the lists
based on address family.
void ofono_gprs_context_set_ipv6_prefix_length(struct
ofono_gprs_context *gc,
unsigned char length);
void ofono_gprs_context_set_ipv6_gateway(struct ofono_gprs_context *gc,
const char *gateway);
I'm not sure these are really needed, which is why I dropped these from
subsequent patches. This information is not received from the cellular network
as part of context activation signalling. On-link prefixes, routes and default
gateways are received as part of the standard IPv6 stateless address
autoconfiguration when the interfaces is brought up. The only reason to have
these would if a specific modem with a virtual ethernet interface deviates from
the standard address configuration practises for some reasons.
Current USB modem sticks don't seem to have IPv6 support, so I'd propose to
just drop these and add an API later if it turns out to be necessary. If USB
sticks do this propertly, they'll just proxy router advertisements and neighbor
discovery messages over the virtual ethernet interface and any additional
address configuration settings won't be needed.
If you decide to keep these, prefix length should probably default to 64 and be
always shown in the settings.
These are highly experimental and have not received much
testing (since
I don't really have any facilities to do so). So please look
and let me
know if something isn't working as intended.
I'm not able to test dual context but IPv6 seems to work with isimodem. I did
notice that the context settings allocated in assign_context() are leaked if
context activation fails. Easy enough to fix, though:
iff --git a/src/gprs.c b/src/gprs.c
index 00f6d6d..068aaf3 100644
--- a/src/gprs.c
+++ b/src/gprs.c
@@ -322,11 +322,13 @@ static gboolean assign_context(struct pri_context *ctx)
return FALSE;
}
+static void context_settings_free(struct context_settings *settings);
static void release_context(struct pri_context *ctx)
{
if (ctx == NULL || ctx-gprs == NULL || ctx-context_driver == NULL)
return;
+ context_settings_free(ctx-context_driver-settings);
gprs_cid_release(ctx-gprs, ctx-context.cid);
ctx-context.cid = 0;
RE: Setting PDP mode
Hi Denis, I would not describe it as a lack of interest. There were simply higher priority items and I wanted to discuss this with Samuel first anyway. I have reworked your IPv6 patches and the API 'slightly' and just now waiting on Marcel to cut a new release to push them. IMHO, two months with no acknowledgement whatsoever is a bit excessive no matter which way you look at it. Anyway, good to hear the patches are not disappearing into a black hole. Br, MikaL ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: [PATCH 01/18] gisi: pipe and pep for wgmodem2.5
As far as I can tell, implementing connect() on PEP sockets would be much more elegant (and symmetric). Lacking better things to do, that's something we (= you) could push upstream? MikaL ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: [PATCH 02/12] gprs: driver interface changes for IPv6
Hi Marcel, Thanks for the comments. Normally, IPv6 addresses and routes are autoconfigured using IPv6 stateless address autoconfiguration followed optionally by DHCPv6 to get additional settings. The kernel actually starts stateless address autoconfiguration immediately when the interface is configured up (which oFono does), assuming the interface already has a link-local IPv6 address (which all Ethernet interface do have, as it is calculated from the MAC address and added automatically). Point-to-point interfaces, on the other, require the link-local address to be added manually to the interface before the autoconfiguration can happen. oFono should probably add the address to be consistent with Ethernet interfaces. I am not sure we really wanna be consistent with how Ethernet works since we are not actually Ethernet. So I can see benefits for having ConnMan control the link-local in case of point-to-point, but then I also do see the benefit oFono doing it. We might have to spin of ideas and see what is the most logical one and what works best. Especially also in the context of LTE support in the future. I agree. The pragmatic way would be to just choose one option now and see how that works for us. We can revisit the issue later when we understand the requirements better. The upshot would be that IPv6 could be autoconfigured immediately without connman's intervention. IPv6 does not have private address spaces, so in theory connman does not really have to care about IPv6 addresses and routes. However, the optional DHCPv6 step would probably be controlled by connman. Configuration of IPv6 tethering would be another optional followup step controlled by connman. Yes, DHCPv6 step needs to be done by ConnMan. We do not have DHCPv6 support right now. We only plan to use it for extra settings like proxies, timeservers etc. On the other hand, if we want connman to be fully in charge, we could change oFono to always leave the network interface down and let connman do everything. Marcels input on this would be welcome as well. This is something we need to figure out. And of course this all needs to be aligned with IPv4 and IPv6. Right now we are treating this similar to how wpa_supplicant for WiFi does this. So going back to potential LTE and IMS support; if that runs IPv6 then it might make sense to just autoconfigure it without ConnMan interaction. Ok. If you agree, I'll add link-local address configuration to my patches for all IPv6 contexts for the time being. Autonomous autoconfiguration is in accordance with IPv6 philosophy anyway. Might as well see how far it takes us. We can always move to a connman controlled approach later if that turns out to be necessary. Regards, MikaL ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: [PATCH 03/12] gprs: core support for IPv6
Hi Denis,
+struct pri_context;
+
struct ofono_gprs_context {
struct ofono_gprs *gprs;
+ struct pri_context *pri;
enum ofono_gprs_context_type type;
- ofono_bool_t inuse;
I don't see how getting rid of this is not a good idea. As
soon as the
context driver is assigned to a context being activated we set it as
'inuse'. If you don't set this variable then it is possible
to pick the
same driver, even if that driver is busy but has not started
to set its
settings yet.
The pri pointer points to the owner of the context driver. It is assigned when
the context driver is allocated and cleared when the context driver is
released. Therefore, the inuse flag becomes redundant. It is enough to check if
pri != NULL to know if the context is in use.
+static gboolean assign_context(struct pri_context *ctx)
..
+static void release_context(struct pri_context *ctx)
..
For something like this I'd really like to see it in a separate patch.
You're moving the function and also modifying the
implementation. It is
much easier to review a patch set if you move a function in one patch
and then modify its behavior in another. Same with
release_context. It
is probably a good idea to put the common code in here, but modify the
behavior as a part of a different patch.
Ok, I'll split it out. I decided to do this when in the middle of making my
changes I noticed that the release code is actually copy pasted many times over.
Ideally this patch should be at least 2:
- For moving assign_context and factoring out common code into
release_context
- The rest of this patch + modifications to the two above.
Will do.
+static struct context_settings *get_settings(struct
ofono_gprs_context *gc)
+{
+ if (gc == NULL || gc-pri == NULL)
+ return NULL;
+
+ if (gc-pri-settings == NULL)
+ gc-pri-settings = g_try_new0(struct
context_settings, 1);
+
I appreciate the lazy allocation, however you get into trouble if the
driver calls one of the _set functions but still returns a failure in
the callback. Overall this seems a more complicated approach than
returning values in the callback of activate_primary.
Oops. I thought I had the failure path covered but this bit changed from the
RFC patches. The lazy allocation is a remnant from having separate dicts for
IPv4 and IPv6 settings. I'll get rid of it.
+void ofono_gprs_context_set_interface(struct
ofono_gprs_context *gc,
+ const char *interface)
+{
+ struct context_settings *settings = get_settings(gc);
+
+ if (settings == NULL || settings-interface != NULL)
+ return;
I'd let the driver call these functions as often as they want and just
take the last value.
Ok. Makes sense.
Regards,
MikaL
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
RE: [RFC 1/5] gprs: Update documentation for IPv6
Hi Sjur, + For combined IPv4v6 context (3GPP rel 8) the + same network interface may be used for both + IPv4 and IPv6. IPv4 and IPv6 may also be + offered via separate network interfaces. + I think it would be nice if we could keep only one interface here. It's a bit messy that the difference between R7 and R8 networks will be visible to the users like this. By users I assume you mean connman? So when in a R7 network the uplink traffic should be filtered into the right ipv4 and ipv6 pdp-connection automagically. I know STE will support this in the modem firmware. It's cool that STE firmware can do that. However, for modems that don't (most of them), I frankly don't see any particular reason to try to bond the bearers within the kernel. That would just bring more complexity to the kernel drivers without providing any real benefit. The Linux networking stack will handle separate interfaces just fine. The IPv4 default route can point to one interface and IPv6 routes can point to another and everything will just work. Naturally connman will need to understand about the separate interfaces in order to configure them properly but that is a minor pain compared to doing bonding separately in each modem interface driver within the kernel. Br, MikaL ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: [RFC 1/5] gprs: Update documentation for IPv6
Hi Marcel, I am not sure it is a good idea to make ConnMan do that. Why is that? And to be honest for PPP, just doing IPv4 seems acceptable since PPP is bad idea in the first place. It is a limitation I am willing to accept. Yeah, PPP is not really a requirement for us, although it would be convenient to have it for testing. As a matter of fact, I would already have trialed atmodem support for IPv6 but that was blocked because of the lack of IPV6CP support in oFono PPP implementation. Too bad. My concern is also on how we handle the Tethering cases properly. I have not yet spent enough time to think about it, but I have concerns here. Ok. Can you outline your concerns so we can talk about them? So right now I would prefer to sit ipv6 out until we have proper ipv4v6 context support in the network and the modems. I'd like to progress with this. We also have people who are keen to help out on the connman side. Just sitting and waiting for better times is not really an approach I'd prefer to take. Currently, we only discussing whether the Interface setting is needed separately in IPv4 and IPv6 settings or not. That's a minor detail as far as I am concerned. For now, allowing separate network interfaces for IPv4 and IPv6 is convenient for testing the IPv6 support on current modems. If it turns out to be hugely difficult to manage separate interface on connman side, we can always restrict the approach and drop support for older modems when rel8 modems are available. Br, MikaL ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: [PATCH 3/5 v3] nettime: DBUS and compilation configuration
Hi, On a side topic, the policy seems a bit broken to begin with. snip policy user=root allow own=org.ofono/ allow send_destination=org.ofono/ This alone already allows root access to all interfaces behind a connection named org.ofono. allow send_interface=org.ofono.SimToolkitAgent/ allow send_interface=org.ofono.PushNotificationAgent/ allow send_interface=org.ofono.SmartMessagingAgent/ So these are redundant, unless the whole thing is rewritten as: allow send_destination=org.ofono send_interface=org.ofono.SimToolkitAgent/ allow send_destination=org.ofono send_interface=org.ofono.PushNotificationAgent/ allow send_destination=org.ofono send_interface=org.ofono.SmartMessagingAgent/ /policy Br, MikaL ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: [PATCH 0/3] Attach GPRS on demand
Hi Marcel, I somehow missed your other comments on this mail. More responses below. Even if you can come up with an algorithm, testing it is very very challenging. There are plenty of differences in operator networks and it is very difficult to cover all cases. Making sure that the algorithm works requires extensive IOT and Field testing. We really don't want a case, where oFono fails to re-attach for whatever reason. We also don't want the case where oFono has not yet attempted to re-attach (e.g. on a timer) and a PDP context activation fails, even though GPRS would actually be available. I don't think such an algorithm is hard. Either we are forbidden, then we wait until we switch to a new cell. Mostly that would be just a waste of power. Some of the errors are PLMN or LA specific, so retrying at cell change would be redundant. A proper retry strategy would have to be based on the detach reason and any subsequent attach failure causes. In general, activating the RF all the time, when no-one has even requested a PDP context is simply bad idea. There are better uses for the battery power. And yes, coming up with an algorithm (whatever it is) is probably easy enough. Making sure it works with all the networks and modems out there is the hard part. Otherwise we will just retry to attach. That's also tricky. What do you do if the attach attempt fails? Retry again? How often? Use some kind of backoff? For the above reasons, retrying attach on PDP context activation makes sense as a safe-guard, regardless of whether we have a re-attach algorithm or not. We use on-demand attach in pretty much all our products (except for certain operator specific variants) precisely because it is certain to work. No funny business. If there is GPRS service, you get a connection. It is also an approach that should work with any AT modem as well. This is really not ConnMan's problem and I don't wanna make it ConnMan's problem. I am really against just pushing this problem off to someone else. No-one is proposing to. You're actually making this ConnMan's problem by monitoring the GPRS attach status. If ConnMan needs a trigger, it should be based on network registration status alone. And in ConnMan we have even less information to decide what happens when context activation fails. Unfortunately, oFono cannot hide context activation failures from ConnMan. ConnMan will just have to cope, regardless of what we do with attach. The algorithm to handle such rare error conditions smoothly becomes more complicated than making this easier. As far as I can see, the approach I'm proposing is simpler than the one you and Denis are proposing. So I agree with Denis that we need to fix this inside oFono and just try to intelligently re-attach. As I already explained to Denis, it's not really a choise between one or the other. Some opearators require autoattach, some require on-demand attach and some require a UI to change the mode. All of these should be supported. So, by all means develop a re-attach algorith. It won't be easy to maturize, though. Using context activation requests as a triggers to retry attach simply makes a lot of sense as a safe-guard, even if we're in autoattach mode. If anything, it removes one potential failure case. Br, MikaL ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: [PATCH 0/3] Attach GPRS on demand
Hi Denis, With these patches, GPRS context activation will cause oFono to automatically attempt GPRS attach if the modem is presently detached. Before I look at these patches: Have you considered fixing the attach behavior instead? I'm not quite sure what you mean here. There are many different cases, where the network can detach a UE from GPRS service. See possible detach causes in 24.008. Currently, oFono does not recover at all. Trying to figure out when GPRS is again available can get pretty complicated and the different detach causes require different handling. IMO, the simplest approach by far is to retry GPRS attach when someone actually needs a PDP context. It seems like the approach you're proposing is essentially hitting oFono over the head because it isn't doing what you want... Sorry, still confused. What do you mean? MikaL ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: [PATCH 0/3] Attach GPRS on demand
Hi Denis, I'm not quite sure what you mean here. There are many different cases, where the network can detach a UE from GPRS service. See possible detach causes in 24.008. Currently, oFono does not recover at all. Trying to figure out when GPRS is again available can get pretty complicated and the different detach causes require different handling. IMO, the simplest approach by far is to retry GPRS attach when someone actually needs a PDP context. So here's the problem, ConnMan is in charge of activating the context on Meego. ConnMan activates the context once we're attached. So how do you expect your 'on-demand' re-attach to work exactly? ConnMan really should not care whether we are attached or not. Why do you need a trigger in ConnMan, anyway? As far as I can see, a GPRS connection should only be activated if some client of ConnMan requests it. Besides, 24.008 cause codes give us plenty of hints of whether / when to re-attach, and only a few of them require 'special' handling. I still think that we should come up with some strategy to re-attach automatically when detached by the network. Even if you can come up with an algorithm, testing it is very very challenging. There are plenty of differences in operator networks and it is very difficult to cover all cases. Making sure that the algorithm works requires extensive IOT and Field testing. We really don't want a case, where oFono fails to re-attach for whatever reason. We also don't want the case where oFono has not yet attempted to re-attach (e.g. on a timer) and a PDP context activation fails, even though GPRS would actually be available. For the above reasons, retrying attach on PDP context activation makes sense as a safe-guard, regardless of whether we have a re-attach algorithm or not. We use on-demand attach in pretty much all our products (except for certain operator specific variants) precisely because it is certain to work. No funny business. If there is GPRS service, you get a connection. It is also an approach that should work with any AT modem as well. Br, MikaL ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: [PATCH 0/3] Attach GPRS on demand
Hi Denis, ConnMan really should not care whether we are attached or not. Why do you need a trigger in ConnMan, anyway? As far as I can see, a GPRS connection should only be activated if some client of ConnMan requests it. Actually it should care. We do safe-guard against attaching while roaming... I don't think so. Preventing GPRS attach and GPRS context activation during roaming can be done based on network registation status. Besides, oFono probably should be the component to enforce it, so that the feature cannot be easily bypassed. Besides, ConnMan doesn't really work this way. Maybe in the future with the session API it would be able to, but not at the moment. I'll let Marcel explain this more if he wants too.. Admittedly, I haven't really looked at ConnMan. My assumption, though, is that a PDP context is not activated by default when ConnMan starts up. I.e., someone has to request the internet connectivity. Is that correct? Activating a context by default would be very bad for power consumption and standby times, unless both the handset and the network support CPC. The decision should be up to the user. Besides, 24.008 cause codes give us plenty of hints of whether / when to re-attach, and only a few of them require 'special' handling. I still think that we should come up with some strategy to re-attach automatically when detached by the network. Even if you can come up with an algorithm, testing it is very very challenging. There are plenty of differences in operator networks and it is very difficult to cover all cases. Making sure that the algorithm works requires extensive IOT and Field testing. We really don't want a case, where oFono fails to re-attach for whatever reason. We also don't want the case where oFono has not yet attempted to re-attach (e.g. on a timer) and a PDP context activation fails, even though GPRS would actually be available. For the above reasons, retrying attach on PDP context activation makes sense as a safe-guard, regardless of whether we have a re-attach algorithm or not. We use on-demand attach in pretty much all our products (except for certain operator specific variants) precisely because it is certain to work. No funny business. If there is GPRS service, you get a connection. It is also an approach that should work with any AT modem as well. You might be right, but I'd really like to see whether the re-attach algorithm would be good enough first... But what re-attach algorithm? I sure don't have one up my sleeve... As I said, extensive IOT and field testing (with mobility) is necessary in order to see how the prospective algorithm works in practise. Even then, you can't possibly cover all the angles. In practise, you end up discovering the rest of the glitches when the first products are already out there. That is a very expensive road indeed for a feature that is a bit dubious to begin with. Why are we attaching to GPRS by default, anyway? That said, various operators tend to have different requirements on this. Some want autoattach, some want on-demand attach, and some want a UI for changing the mode. In practise we will probably need at least a variation point on this and possibly a setting to change the behaviour. I'd propose starting by adding the on-demand attach code, to make sure that GPRS at least works in all cases. MikaL ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: [PATCH 0/3] Attach GPRS on demand
Hi Marcel, so what about the cases when roaming is allowed or not? These needs to be taken into account and that is oFono's job. That is the reason why ConnMan follows the attach state. Since we don't wanna activate a context when roaming and data roaming is not allowed. Ok, I see the concern. I'd propose making GPRS availability a separate property from GPRS attach status. Availability can be decided on registration status alone (i.e., does the network advertise GPRS and does the roaming check allow GPRS). Both GPRS attach and PDP context activation can in any case fail for transient reasons, so ConnMan needs to have some logic to retry context activation. Br, MikaL ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: [PATCH 0/5] Call Counters (2nd)
Hi, Because if your battery runs out in the middle of a 4 hour conference call, your timers are not updated and become worthless. Obviously, this is a compromise between how reliable the counters are and how many wakeups and IO we can bear. I think this is not a good idea to have oFono handles this. Why can't the system daemon just shutdown all calls when the battery reaches critical limit. You will never fully run down the battery anyway. One of the system health components in the system will prevent it and then can cleanly shutdown oFono and thus all calls. The only case where the system could potentially misfunction in this area would be an emergency call. But that is a total different use case anyway. There are other ways in which the device might reset without warning: - hardware glitch triggering an involuntary reset - software glitch triggering an involuntary reset - device drops and battery connection is momentarily broken when it hits the ground - user rips out the battery in mid-use While periodically syncing call and data counters to permanent memory is by no means appealing, we have certain accuracy requirements for the stored counters. The only way those requirements can be met is by syncing the counters to storage at reasonable intervals. Whether this should be part of oFono is a matter for discussion, I guess, but at minimum we need to have the enablers to do this. MikaL ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: [PATCH 3/4] isimodem2.5: Changes to add isimodem2.5
Hi, In addition, I don't a point in a filename. So isimodem25 or isimodem2 or something like that please. I am open for proposals. Let's go for isimodem25 Quite a bit of the code is practically a copy-paste from the isimodem driver. Have you looked into the possibility of supporting both ISI interface versions in the same driver, or at least sharing some of the atom drivers? MikaL ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: [PATCH 4/4] isigen: make number of PDP contexts configurable
Hi Marcel, why to do you bother making this a configurable option? What isthe benefit here? The maximum context count is a compile time option for the ISI modem. Having this option in oFono makes it possible to optimize the APE side resource usage instead of overallocating drivers and contexts. Not a huge benefit, I guess, but the cost is not huge either. Not a big deal, though. I'll try to see if the context limit could be probed somehow. Personally I think that always enabling 4 context if the hardware supports it should be enough. If you do support more then just enable more all the time. There are no real resources used in context of ISI anyway. The AT command based modems have a different problem since for most of them we need an extra TTY/DLC and an extra GAtChat object, but ISI does not have that problem. I don't see the difference. The AT modem resources should be allocated dynamically as well, at context activation time. (Maybe that's the case already, I didn't really check.) In any case, I believe oFono has to allow things like external AT command processors and vendor specific modem tools to access the TTY's directly bypassing oFono. This means the mux channels should not be preallocated. MikaL ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: isigen: enabled multiple PDP contexts
Hi Marcel, you might wanna ask your modem firmware guys to add something like this. For me it would sounds like a good idea to know how many active GPRS contexts that specific modem supports. You wouldn't suggest that if you knew the process involved. ;) Anyway, the change would not help existing hardware. MikaL ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: [PATCH 1/1] isigen: create four gprs contexts
Tiny nitpick, but please follow the coding style. Specifically item M1. Hookay. I wish I had a script that picked these things up... *wink* ;-) MikaL ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: [RFC PATCH 2/3] voicecall: emergency call handling added
Hi, To give you a more complex example, it might well be that the gprs connection needs to be torn down when making an emergency call in 2G mode, there are such networks out there that prevents you from making an emergency call if your device is attached to a PDP context. In this given situation it comes to the question how to bring down the gprs connection. It can be done such that the gprs atom will tear down the connection after receiving the EmergencyMode notification, or another option is to have gprs connection handling functions made Have we established that this is actually still needed? I thought you guys said all the networks that have this problem have been fixed by now? AFAIK, this is still a product requirement for us. I can recheck, but I just asked last week and the answer was a very definite Yes, this is still required. MikaL ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: [PATCH 3/6] radio settings: add FastDormancy property
Hi Denis, One of the biggest principles in oFono is not to perform premature optimization. Sure there is a potential issue, but nobody knows whether it will actually manifest itself outside of malicious code (which we tell to bugger off) or what the most common manifestation pattern will be. If / once we know for sure this is a problem, then we can solve it properly. So far this approach has been working very nicely for us. There are countless occasions where taking the wait-and-see approach and gathering more information allowed us to devise a much better solution than we would have originally. So the general rule is: Do the simplest thing that is likely to work. If it doesn't work, improve it. Can't fault the approach, it's generally a good one. That said, I see this more as an API quality issue rather than an optimization issue. Anyway, I've raised the concern. Let's hope my worries are unfounded. Br, MikaL ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: [RFC PATCH 2/3] voicecall: emergency call handling added
Hi Marcel, Have we established that this is actually still needed? I thought you guys said all the networks that have this problem have been fixed by now? AFAIK, this is still a product requirement for us. I can recheck, but I just asked last week and the answer was a very definite Yes, this is still required. as Denis and I mentioned, we have been told that this is not needed anymore. So where is this still needed? Ok, I double checked the product requirements and it turns out that you're right. The requirement is no longer there for oFono based stuff. Seems we've had some wires crossed at our end, so the situation was not entirely clear. Sorry for the confusion, MikaL ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: [PATCH 3/6] radio settings: add FastDormancy property
Hi Denis, The fundamental problem here is that there is only a single CACHED flag for multiple properties, which may be modified individually. So, either you get extra signals or you get too few. I checked the CACHED flag here because otherwise the following might happen: Yes, I know. But this problem is present in every single atom. oFono does not guarantee that every attribute is signaled when the atom is initialized. So I gathered. To me this looks like a wider issue, though. InProgress errors are returned in many other contexts as well, and they are not that well documented in the API documentation. All this makes it a bit painful to write a robust oFono client. Probably this could be at least partially rectified by improving the documentation. 1. client tries to GetProperties() and gets the Operation already in progress error. 2. client waits for PropertyChanged signal to get the FastDormancy value 3. signal never comes because the default value happens to match the one returned by the driver and the signal is suppressed In general I think that for interfaces where this can happen, the likelihood is very low that it actually will in the real world. Perhaps so. The network and SIM interfaces, which are most likely to be bombared by multiple UI components, seem to be doing the right thing at least. Do note that I have had the same argument with myself off and on for the past two years. So far this was never raised as an issue. If this ever becomes a problem, we can fix it properly using an appropriate idiom. If this becomes a problem, it won't necessarily be visible to upstream. More likely this will be noticed in product maturization phase, and the fixes made to a product specific stable branches might never trickle back to upstream. I do agree that sending extra signals is bad but I think that not sending a signal is even worse. If the client cannot rely on getting a PropertyChanged signal after a busy error, all it can do is resort to polling. I.e., every client has to implement a polling pattern for GetProperties: while (GetProperties() == BUSY) sleep(FOR_A_WHILE); Having a separate CACHED flag for each value would solve this optimally. Failing that, I don't think a few extra signals is so bad. Forcing clients to poll is just ugly. Honestly, if you expect multiple applications to battle over the FastDormancy property, then it should be modeled differently. Perhaps with application registration and lifetime tracking over D-Bus, similar to how agents work. Hardly that, FastDormancy is unlikely to be a problem. I was merely curious whether there is a general design rule underneath, or if these things are decided on a case by case basis. Based on your comments and looking at the code, I guess it's more case by case. I just feel uneasy about an API that returns transient errors by design, on the (even if informed) assumption that it will probably be ok. I also dislike the fact that a generic InProgress error pretty much forces a client to just retry each operation until it succeeds. If there are problems like this, they are most likely discovered only in the product maturization phase and then it's generally too late to fix the upstream. Too late for that particular product, that is. Br, MikaL ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: [PATCH 3/6] radio settings: add FastDormancy property
Hi Denis, Patch has been applied, thanks. I did make a couple of minor tweaks afterwards. Thanks. A question regarding this particular tweak: diff --git a/src/radio-settings.c b/src/radio-settings.c index cb0a598..eb2a42d 100644 --- a/src/radio-settings.c +++ b/src/radio-settings.c @@ -126,8 +126,7 @@ static void radio_set_fast_dormancy(struct ofono_radio_settings *rs, const char *path = __ofono_atom_get_path(rs-atom); dbus_bool_t value = enable; - if ((rs-flags RADIO_SETTINGS_FLAG_CACHED) - rs-fast_dormancy == enable) + if (rs-fast_dormancy == enable) return; ofono_dbus_signal_property_changed(conn, path, The fundamental problem here is that there is only a single CACHED flag for multiple properties, which may be modified individually. So, either you get extra signals or you get too few. I checked the CACHED flag here because otherwise the following might happen: 1. client tries to GetProperties() and gets the Operation already in progress error. 2. client waits for PropertyChanged signal to get the FastDormancy value 3. signal never comes because the default value happens to match the one returned by the driver and the signal is suppressed I do agree that sending extra signals is bad but I think that not sending a signal is even worse. If the client cannot rely on getting a PropertyChanged signal after a busy error, all it can do is resort to polling. I.e., every client has to implement a polling pattern for GetProperties: while (GetProperties() == BUSY) sleep(FOR_A_WHILE); Having a separate CACHED flag for each value would solve this optimally. Failing that, I don't think a few extra signals is so bad. Forcing clients to poll is just ugly. Am I missing something? MikaL ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: [PATCH 2/6] stemodem: add default case
Hi Marcel,
That must be a new policy then, considering that stemodem
is the only one that failed compilation. Feel free to fix
this one. The first two patches in the set are unrelated to
fast dormancy anyway.
that is the whole point here. You modified the enum and the
compilation
should fail unless you add a statement for that new item.
Let me make this clear, I do want the compilation to fail here and the
STE driver is doing the right thing.
I understand that. As I said, feel free to fix. Fast dormancy patches 3-6
should apply just fine without the first two courtesy patches.
There might be other cases where this is not consistent. I
would prefer
if that never happens, but somethings things slip through even close
code review. If you know other cases, please let me know and
we fix them
as well here.
All the other drivers implementing radio settings. Here's a couple of examples:
ac63fd95 (Marcel Holtmann 2010-09-23 23:27:08 +0900 134)switch (mode) {
ac63fd95 (Marcel Holtmann 2010-09-23 23:27:08 +0900 135)case
OFONO_RADIO_ACCESS_MODE_ANY:
ac63fd95 (Marcel Holtmann 2010-09-23 23:27:08 +0900 136)value =
1;
ac63fd95 (Marcel Holtmann 2010-09-23 23:27:08 +0900 137)break;
ac63fd95 (Marcel Holtmann 2010-09-23 23:27:08 +0900 138)case
OFONO_RADIO_ACCESS_MODE_GSM:
ac63fd95 (Marcel Holtmann 2010-09-23 23:27:08 +0900 139)value =
0;
ac63fd95 (Marcel Holtmann 2010-09-23 23:27:08 +0900 140)break;
ac63fd95 (Marcel Holtmann 2010-09-23 23:27:08 +0900 141)case
OFONO_RADIO_ACCESS_MODE_UMTS:
ac63fd95 (Marcel Holtmann 2010-09-23 23:27:08 +0900 142)value =
2;
ac63fd95 (Marcel Holtmann 2010-09-23 23:27:08 +0900 143)break;
ac63fd95 (Marcel Holtmann 2010-09-23 23:27:08 +0900 144)default:
ac63fd95 (Marcel Holtmann 2010-09-23 23:27:08 +0900 145)
CALLBACK_WITH_FAILURE(cb, data);
ac63fd95 (Marcel Holtmann 2010-09-23 23:27:08 +0900 146)
g_free(cbd);
ac63fd95 (Marcel Holtmann 2010-09-23 23:27:08 +0900 147)return;
ac63fd95 (Marcel Holtmann 2010-09-23 23:27:08 +0900 148)}
30b054d0 (Marcel Holtmann 2010-06-06 15:18:57 -0700 133)switch (mode) {
30b054d0 (Marcel Holtmann 2010-06-06 15:18:57 -0700 134)case
OFONO_RADIO_ACCESS_MODE_ANY:
30b054d0 (Marcel Holtmann 2010-06-06 15:18:57 -0700 135)value =
5;
30b054d0 (Marcel Holtmann 2010-06-06 15:18:57 -0700 136)break;
30b054d0 (Marcel Holtmann 2010-06-06 15:18:57 -0700 137)case
OFONO_RADIO_ACCESS_MODE_GSM:
30b054d0 (Marcel Holtmann 2010-06-06 15:18:57 -0700 138)value =
0;
30b054d0 (Marcel Holtmann 2010-06-06 15:18:57 -0700 139)break;
30b054d0 (Marcel Holtmann 2010-06-06 15:18:57 -0700 140)case
OFONO_RADIO_ACCESS_MODE_UMTS:
30b054d0 (Marcel Holtmann 2010-06-06 15:18:57 -0700 141)value =
1;
30b054d0 (Marcel Holtmann 2010-06-06 15:18:57 -0700 142)break;
30b054d0 (Marcel Holtmann 2010-06-06 15:18:57 -0700 143)default:
30b054d0 (Marcel Holtmann 2010-06-06 15:18:57 -0700 144)
CALLBACK_WITH_FAILURE(cb, data);
30b054d0 (Marcel Holtmann 2010-06-06 15:18:57 -0700 145)
g_free(cbd);
30b054d0 (Marcel Holtmann 2010-06-06 15:18:57 -0700 146)return;
30b054d0 (Marcel Holtmann 2010-06-06 15:18:57 -0700 147)}
Sorry, couldn't resist. ;-)
Br,
MikaL
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
RE: [PATCH 2/6] stemodem: add default case
Hi Marcel, diff --git a/drivers/stemodem/radio-settings.c b/drivers/stemodem/radio-settings.c index 5b16ec0..a345975 100644 --- a/drivers/stemodem/radio-settings.c +++ b/drivers/stemodem/radio-settings.c @@ -90,7 +90,7 @@ static gboolean ofono_mode_to_ste_mode(enum ofono_radio_access_mode mode, case OFONO_RADIO_ACCESS_MODE_UMTS: *stemode = STE_RADIO_WCDMA_ONLY; return TRUE; - case OFONO_RADIO_ACCESS_MODE_LTE: + default: break; no default for enums please. I want the compiler to warn us about not handled switch statements. That must be a new policy then, considering that stemodem is the only one that failed compilation. Feel free to fix this one. The first two patches in the set are unrelated to fast dormancy anyway. Br, MikaL ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: [PATCH 1/4] radio settings: allow for more than one property
Hi Denis,
I really don't want to argue about this.
SetProperty(TechnologyPreference, ..)
- Sets RADIO_SETTINGS_MODE_CACHED
GetProperties() - queries Fast dormancy
GetProperties() - queries Fast dormancy
-
+ } else if (rs-driver-query_fast_dormancy) {
+ rs-driver-query_fast_dormancy(rs,
+
fast_dormancy_query_callback, rs);
+ dbus_message_unref(reply);
+ return NULL;
+ }
+
You never set pending in the FastDormancy case.
Good catch. That's a bug, of course. Doesn't invalidate the approach, though.
Understandable. Makes it difficult to improve on design
patterns that have already been replicated, though.
Nobody is against improving things, but please try to do it in the
appropriate forum. E.g. and RFC to the mailing list or an
IRC discussion.
Of course. This time I wasn't conciously trying to improve things, though.
Anyway, enough yakking. I'll do the rewrite sicne you're adamant.
Br,
MikaL
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
RE: [PATCH 1/6] radio settings: add FastDormancy property
Hi Marcel,
@@ -103,14 +103,60 @@ static DBusMessage
*radio_get_properties_reply(DBusMessage *msg,
OFONO_PROPERTIES_ARRAY_SIGNATURE,
dict);
- ofono_dbus_dict_append(dict, TechnologyPreference,
+ if ((int)rs-mode != -1) {
+ const char *mode =
radio_access_mode_to_string(rs-mode);
+ ofono_dbus_dict_append(dict, TechnologyPreference,
DBUS_TYPE_STRING, mode);
what is up with this (int) rs-mode cast here. That looks highly wrong
to me. The mode is an enum so please don't hack around it like this.
If mode can be invalid or not present then we need to extend this enum
with an initial value of OFONO_RADIO_ACCESS_MODE_UNKNOWN, but not hack
some cast magic into it.
Yes, it's fishy. Denis introduced the enum in commit
81bc8884b414e6c2d511789d2e183cdad55182f0 but left mode initialized as -1. I'm
not sure what's up with that but I did not want to start fixing it. I suppose
the initializer could be added to the enum, as you say, or the whole patch
could be reverted. Not my call, though.
Or you use some flags like for the cached value.
And I would propose the same for fast dormancy value. Lets
store it as a
boolean and have a flag if it is present or not.
That's what I did in the previous patch but Denis wanted a single CACHED flag.
Guys, please try to agree on this.
MikaL
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
RE: [PATCH 1/4] radio settings: allow for more than one property
Hi Denis,
Oops, will fix. For some reason my checkpatch script did
not complain about this.
You must be using a script as well. To avoid unnecessary
fuss like this, would it be possible to integrate it to the
oFono tree so everyone would check their patches using the same thing?
Not a script, but Johan's magic vim macro. I paste it here
for reference:
highlight RedundantWhitespace ctermbg=red guibg=red
match RedundantWhitespace /\s\+$\|\t\+\ze \+[^*]\| \+\ze\t/
Between checkpatch, git am and this one we catch most issues.
Thanks. Still, I'd vote for a common checkpatch script in the oFono tree. ;)
I prefer you keep the current implementation as it is
consistent with
the rest of ofono core. The way the rest of the core works
is that you
query each in sequence, then set the the CACHED flag (only one is
actually required).
We use a single cached flag to keep the logic simpler and
optimize for
the general case (e.g. UI will query the properties first before
allowing the user to set them) While this does lead to us
querying some
attributes unnecessarily in certain extremely unlikely
corner cases, I
believe it is an acceptable compromise.
This was my intention as well but I did it in a different
way, as I didn't realize there was an existing pattern for it.
My code does actually query all the properties up front but
I serialized the driver queries in a different way. I.e., I
used a separate CACHED flag for each property to keep track
of whether the value is already valid. Each driver callback
attempts to create the pending reply. The attempt to create
the reply either succeeds or triggers a new driver query for
another missing property value. This goes on until all the
properties are cached and the pending reply can be sent.
I now see that the other services use a separate state
machine to fetch the properties. That introduces more code
than my solution but I guess I can do it that way if you feel
it is easier to read.
In the beginning we tried many approaches, including ones
similar to the
path you took. We found that the current approach used in the core,
while while by no means perfect in every situation, was the best
compromise. It is quite scalable as the number of attributes
/ queries
grows and it blocks multiple (potentially malicious) applications from
DDoSing the modem when the information is not cached.
Well, my solution does exactly what you're asking, only with less code. You
seem to think that it allows parallel property queries from multiple clients.
That is not the case. See here:
static DBusMessage *radio_get_properties(DBusConnection *conn, DBusMessage *msg,
void *data)
{
struct ofono_radio_settings *rs = data;
if (rs-pending)
return __ofono_error_busy(msg);
return radio_get_properties_reply(msg, rs);
}
A busy error is returned if a previous query is in progress.
I don't mind rewriting the stuff if need be but I'd like it to be for the right
reason.
MikaL
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
RE: [PATCH 1/4] radio settings: allow for more than one property
Hi Denis,
Thanks. Still, I'd vote for a common checkpatch script in
the oFono tree. ;)
Are you volunteering to maintain such a beast and make sure
it is up to
date with the kernel upstream version?
Only if I get to decide on the coding style. Otherwise, I'm proposing you guys
should do it. Anyway, this is just a friendly suggestion in with the o sav e
everyone's time.
Yes, so let me give you a case where your approach breaks:
Set the RAT property, then run two GetProperties at the same
time. You
end up querying FastDormancy twice.
No you won't, because the second GetProperties query will be terminated right
off the bat with ofono busy error. Again, see here:
static DBusMessage *radio_get_properties(DBusConnection
*conn, DBusMessage *msg,
void *data)
{
struct ofono_radio_settings *rs = data;
if (rs-pending)
return __ofono_error_busy(msg);
return radio_get_properties_reply(msg, rs);
}
A busy error is returned if a previous query is in progress.
Only one API call can be active at one time.
So let me try to make it pretty clear. I maintain all of the
core and I
review just about every single patch. For me code readability and
consistency of implementation is very important. This is
also the case
for anyone new starting with the codebase. If I allow every
atom to do
things just a little bit differently for the sake of saving
20 lines of
code or because the original author likes his approach
better, I will go
crazy and the code will become unmaintainable.
Understandable. Makes it difficult to improve on design patterns that have
already been replicated, though.
I'd like to keep my sanity for a while longer ;) So please
modify your
patch to follow the oFono conventions.
Sure. My personal cost optimization function tells me that at this point it's
faster to rewrite than to argue further.
Br,
MikaL
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
RE: [PATCH 1/4] radio settings: allow for more than one property
Hi, Please don't mix and match tabs and spaces for indentation Oops, will fix. For some reason my checkpatch script did not complain about this. You must be using a script as well. To avoid unnecessary fuss like this, would it be possible to integrate it to the oFono tree so everyone would check their patches using the same thing? I prefer you keep the current implementation as it is consistent with the rest of ofono core. The way the rest of the core works is that you query each in sequence, then set the the CACHED flag (only one is actually required). We use a single cached flag to keep the logic simpler and optimize for the general case (e.g. UI will query the properties first before allowing the user to set them) While this does lead to us querying some attributes unnecessarily in certain extremely unlikely corner cases, I believe it is an acceptable compromise. This was my intention as well but I did it in a different way, as I didn't realize there was an existing pattern for it. My code does actually query all the properties up front but I serialized the driver queries in a different way. I.e., I used a separate CACHED flag for each property to keep track of whether the value is already valid. Each driver callback attempts to create the pending reply. The attempt to create the reply either succeeds or triggers a new driver query for another missing property value. This goes on until all the properties are cached and the pending reply can be sent. I now see that the other services use a separate state machine to fetch the properties. That introduces more code than my solution but I guess I can do it that way if you feel it is easier to read. MikaL ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: [PATCH 3/4] radio settings: document FastDormancy property
Hi, so how does the future prediction is suppose to work. Are we shipping a time machine together with oFono ;) I would say the heuristic is system dependent. Typical way would be to monitor events from different input devices. If no events arrive for a specified time the user is assumed to be inactive. The user might also have some explicit way to do this, such as locking the display and keys. MikaL ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: [PATCH 0/2] Add Suspended property to GPRS
Hi Marcel, We have something similar with MMS where the hardware or network only supports one PDP context. In that case it is even worse actually since we take the proper IP connection away and it won't come back as it was. AFAIK, there is no reliable way to detect the case where an operator only supports a single context per mobile. The second context activation fails but there is no standard error code that would indicate why. I suppose the number of contexts could be configurable somehow but determining the correct setting for a particular operator is probably a problem for the higher layers. In the end similar enough since if you wait long enough your TCP connections will time out. TCP can take several minutes to time out, though. Mobile applications will probably try to be a bit smarter if they have the means to do so. The cause code could also be exposed in the DBus interface. I wouldn't go there as exposing it over D-Bus. However telling the core a bit more details about this would be a good idea. Then it can decide to either delay the notification or send it right away. I prefer to start simple first. Making it too complicated in the beginning is a bad idea. And giving application too much information is a bad idea anyway. They should only get what they really need. And right now I don't see a need to tell them any more details, than the GPRS connection is suspended. And that means we don't know when we get it back. So deal with this situation. Agreed. Corner cases where a phone call gets hung up right away is nasty, but that happens, but then again, the user was making a phone call in the first place. So a user initiated choice we are trying to cope with. Well, the user has no control over incoming calls, aside from rejecting them or going to offline mode. Not much can be done about that, though. However suspend with flag available sounds like a bad API. Either we call this gprs_state or something similar or we have suspend/resume pairs. Just to be a bit more symmetric here. A suspend/resume pair sounds good to me. I'll do the patches. MikaL ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: [PATCH 0/2] Add Suspended property to GPRS
Hi Marcel,
From: ofono-boun...@ofono.org
[mailto:ofono-boun...@ofono.org] On Behalf Of ext Marcel Holtmann
The isimodem driver will currently suppress changes in the
property value for short-lived suspended states caused by SMS
transmission or other signalling. It would also be possible
to do the suppression in the oFono core in a generic way.
Alternatively, all the state changes and their reasons could
also be exposed to oFono clients. However, my feeling is
that the information might not be available in a sufficiently
coherent way from the different modems. Anyway, I'm open to
suggestions.
we had a brief discussion about this some time ago. And I don't think
that we can do anything than give the clients (UI and ConnMan) and
indication that the suspend has happened right now. In
general it is too
late anyway. Since making a voice call and sending a SMS are decisions
by the end user. And he/she wants to execute them right now. So it is
expected that GPRS gets suspended.
Yes, I agree, there's no sane way to provide pre-warning. A signal that
comes after the fact still has some potential uses, though:
- a hint for internet connection management to start looking for an
alternative access, in case GPRS is not back shortly
- a signal for mobile-aware applications to adjust their behavior
- UI notification for the end user
I do agree that we should not blindly tell the clients suspended if we
now that it is a short lived situation like SMS. My personal
preference
here would be that this is done inside oFono core and the
hardware just
signals suspend (if it actually supports such notifications).
So how much work would it take to do the delay in the core?
Not that hard, I guess. We could define a suspend cause code that gets
notified to the core, based on which the core would either modify the
property immediately or start a guard timer. Perhaps something along
the lines of:
enum gprs_suspend_status {
GPRS_SUSPENDED_DETACHED,
GPRS_SUSPENDED_SIGNALLING,
GPRS_SUSPENDED_CALL,
GPRS_SUSPENDED_NO_COVERAGE,
GPRS_SUSPENDED_UKNOWN_CAUSE,
GPRS_AVAILABLE,
};
src/gprs.c:
void ofono_gprs_suspend_notify(struct ofono_gprs *gprs,
enum gprs_suspend_status status)
{
...
}
The cause code could also be exposed in the DBus interface.
I know, that
the ISI modem is more sophisticated protocol, but we do have AT modems
that have a bit harder time gathering all the information.
AT modem drivers could use any subset of the above causes.
To allow opt-in implementation in the drivers, the core could
automatically change the status between GPRS_SUSPENDED_DETACHED
and GPRS_AVAILABLE based on attach/detach notifications.
I can redraft the patches to do this. I don't really have a clue
what kind of vendor extensions are available for this in AT modems,
though. Any examples would be helpful.
Br,
MikaL
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
