[PATCHv5] plugin: Add ste modem initd integration
From: Sjur Brændeland sjur.brandel...@stericsson.com
This patch introduces auto discovery of ST-Ericsson modems.
ST-Ericsson modems (M57XX, M7XX, M74XX) are managed by a
Modem Init Daemon responsible for start, power cycles,
flashing etc. This STE plugin monitors the modem state exposed
from the Modem Init Damon's Dbus API. When the modem is in state
on the STE modem is created and registered. Muliple modem
instances, and reset handling is supported.
---
Hi Marcel.
+
+builtin_modules += stemgr
+builtin_sources += plugins/stemgr.c
+
This is a minor nitpick, but I think this is better placed between ste
and caif plugin details. And not on top of stemodem.
Sure, I've moved this entry.
+#define MGR_SERVICE com.stericsson.modeminit
+#define MGR_MGR_INTERFACEMGR_SERVICE .Manager
Really? MGR_MGR and not MGR_MANAGER ;)
Oops, s/mid/mgr didn't do it right this time ;-)
Let's call it MGR_INTERFACE instead.
+struct ste_modem {
+ struct ofono_modem *modem;
+ char *path;
You don't have to necessarily do it this way, but since you are using
the path memory also as index for your hash table it might be good to
put it first in your struct.
Ok, I've put path first in struct.
+static guint mgr_api_watch;
It is more like a service watch or daemon watch.
Yes, I renamed this to modem_daemon_watch.
+static guint mgr_state_watch;
And this is property changed watch, right?
Yes, renamed this to property_changed_watch
What is this exactly for? How many do you expect to have pending?
I have removed this one. get_modems is *only* called
when the Modem Init Daemon is first connected.
+static inline gboolean is_ready(const char *action)
+{
+ return g_strcmp0(action, ready) == 0;
+}
...
This is nasty. Why not convert the state into an enum once and be done
with it.
Nasty? Anyway I've changed this to a ste_operation enum,
and I'll give you that - it does make the state machine a more readable.
I find this all a bit complicated. Can you quickly describe the logic
here in plain words.
Sure, I've written some prose describing the state machine.
I've also done this function like a standard FSM implementation:
switch(state) {
case STATE_A:
switch (operation) {
case OPERATION_1:
...
+static void get_modems(const char *path)
+{
This path parameter is rather pointless. It is always /.
Agree.
+ if (get_modems_call_pending)
+ return;
I still haven't figured out what you are trying to protect here.
+static gboolean property_changed(DBusConnection *connection,
+ DBusMessage *message, void *user_data)
+{
...
+ stemodem = g_hash_table_lookup(modem_list,
+ dbus_message_get_path(message));
+
+ if (stemodem == NULL) {
+ get_modems(/);
+ return TRUE;
+ }
So here we go. So you are expecting a property changed signal before
GetModems finished and therefor you just call it again. This is rather
pointless since you get the properties with the return of the GetModems
call and these should be the actual ones.
So if the modem path is not in your hash-table, then just ignore the
property changed signal.
Yes, agree. This was a left over from previous API. I have removed this
completely. get_modems() is only called when Modem Init Daemon API
becomes available. If modems ever pop up dynamically I will add a new
signal for this in the DBus API as suggested.
+static void stemgr_exit()
+{
The only thing you missed here is to remove the property changed watch
in case it was registered (aka the init daemon started).
Good catch thanks, I'v removed this watch.
Thank you for reviewing this code, hopefully we're close to completion now ;-)
Regards,
Sjur
Makefile.am |3 +
plugins/stemgr.c | 385 ++
2 files changed, 388 insertions(+), 0 deletions(-)
create mode 100644 plugins/stemgr.c
diff --git a/Makefile.am b/Makefile.am
index 8a8555d..1e4f9df 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -292,6 +292,9 @@ builtin_sources += plugins/ifx.c
builtin_modules += ste
builtin_sources += plugins/ste.c
+builtin_modules += stemgr
+builtin_sources += plugins/stemgr.c
+
builtin_modules += caif
builtin_sources += plugins/caif.c
endif
diff --git a/plugins/stemgr.c b/plugins/stemgr.c
new file mode 100644
index 000..0e2745c
--- /dev/null
+++ b/plugins/stemgr.c
@@ -0,0 +1,385 @@
+/*
+ *
+ * oFono - Open Source Telephony
+ *
+ * Copyright (C) 2011 ST-Ericsson AB.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU
[PATCH] Fix compile warning at MeeGo The gcc version is MeeGo 4.5.1
From: blutolan bluto...@blutolan-desktop.(none)
---
src/call-forwarding.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/call-forwarding.c b/src/call-forwarding.c
index 512f223..36ba4f1 100644
--- a/src/call-forwarding.c
+++ b/src/call-forwarding.c
@@ -246,7 +246,7 @@ static gboolean is_cfu_enabled(struct ofono_call_forwarding
*cf,
static void sim_set_cf_indicator(struct ofono_call_forwarding *cf)
{
gboolean cfu_voice;
- struct ofono_call_forwarding_condition *cond;
+ struct ofono_call_forwarding_condition *cond = NULL;
cfu_voice = is_cfu_enabled(cf, cond);
--
1.7.2.2
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
Re: [PATCH] Fix compile warning at MeeGo The gcc version is MeeGo 4.5.1
Hi Martin. On Thu, Jan 6, 2011 at 10:46 AM, martin...@intel.com wrote: From: blutolan bluto...@blutolan-desktop.(none) Your git-config is wrong, you need to set name and email right in your git-config before committing. Regards, Sjur ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: [PATCH] Fix compile warning at MeeGo The gcc version is MeeGo 4.5.1
Using the old version of gcc 4.4.2 I did not meet the issue, but using 4.5.1,
we have warning.
-Original Message-
From: Xu, Martin
Sent: Thursday, January 06, 2011 5:46 PM
To: ofono@ofono.org
Cc: Xu, Martin; blutolan
Subject: [PATCH] Fix compile warning at MeeGo The gcc version is MeeGo 4.5.1
From: blutolan bluto...@blutolan-desktop.(none)
---
src/call-forwarding.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/call-forwarding.c b/src/call-forwarding.c
index 512f223..36ba4f1 100644
--- a/src/call-forwarding.c
+++ b/src/call-forwarding.c
@@ -246,7 +246,7 @@ static gboolean is_cfu_enabled(struct
ofono_call_forwarding *cf,
static void sim_set_cf_indicator(struct ofono_call_forwarding *cf)
{
gboolean cfu_voice;
- struct ofono_call_forwarding_condition *cond;
+ struct ofono_call_forwarding_condition *cond = NULL;
cfu_voice = is_cfu_enabled(cf, cond);
--
1.7.2.2
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
RE: [PATCH] Fix compile warning at MeeGo The gcc version is MeeGo 4.5.1
-Original Message- From: ofono-boun...@ofono.org [mailto:ofono-boun...@ofono.org] On Behalf Of Sjur Br?ndeland Sent: Thursday, January 06, 2011 5:49 PM To: ofono@ofono.org Cc: blutolan (none) Subject: Re: [PATCH] Fix compile warning at MeeGo The gcc version is MeeGo 4.5.1 Hi Martin. On Thu, Jan 6, 2011 at 10:46 AM, martin...@intel.com wrote: From: blutolan bluto...@blutolan-desktop.(none) Thanks, just reinstall the system, and missing that. :-) ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
[PATCH] Fix compile warning at MeeGo The gcc version is MeeGo 4.5.1
From: Martin Xu martin...@intel.com
---
src/call-forwarding.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/call-forwarding.c b/src/call-forwarding.c
index 512f223..36ba4f1 100644
--- a/src/call-forwarding.c
+++ b/src/call-forwarding.c
@@ -246,7 +246,7 @@ static gboolean is_cfu_enabled(struct ofono_call_forwarding
*cf,
static void sim_set_cf_indicator(struct ofono_call_forwarding *cf)
{
gboolean cfu_voice;
- struct ofono_call_forwarding_condition *cond;
+ struct ofono_call_forwarding_condition *cond = NULL;
cfu_voice = is_cfu_enabled(cf, cond);
--
1.7.2.2
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
Re: [RFC PATCH] gprs: add function to handle activated context
Hi Redouane,
...
+static void gprs_cid_take(struct ofono_gprs *gprs, unsigned int id)
+{
+ idmap_take(gprs-cid_map, id);
+}
+
How do we assure that this CID is not already in use?
I think the M7400 is going to use CID=0 and M700 uses CID=1.
Perhaps we should make sure CID=0 is a legal value and reserve CID 0
and 1 at start up.
There is probably other ways of handling this as well, but I think any
other solution
would end up with race conditions, as context could be initiated from
two sources.
Regards,
Sjur
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
RE: [PATCH 3/5 v2] Cell-info: Atom for obtaining ECID info of cells
Hi Antti,
-Original Message-
Subject: [PATCH 3/5 v2] Cell-info: Atom for obtaining ECID info of cells
+static int append_utra_neigh_cell_data(DBusMessageIter *iter,
+ struct cell_measured_results *cmr)
+{
+
+ if (cmr-rscp != OFONO_CI_FIELD_RSCP_UNDEFINED)
+ ofono_dbus_dict_append(iter,
+ CPICH-RSCP,
+ DBUS_TYPE_INT16,
+ cmr-rscp);
+
The type for RSCP should be DBUS_TYPE_BYTE
Regards,
Wei, Jun
jun@intel.com
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
RE: [RFC PATCH] gprs: add function to handle activated context
Hi Marcel, Behavior of the function : List all the context and try to find correct APN we have to be careful to only match against APN==APN and Username== + Password==. The username and password option is pretty much pointless, but it is present and we do support it. So we need to make sure the APN matching sensible. Or does the network provide us with additional information about the username + password as well? Network doesn't provide login/password information, the UE shall provide it if needed. I'm not aware of any case where we can have network initiated connection with an APN usually used with authentication. If the connection requires a login/password then it's UE initiated procedure. If you think it's safer to check against Login/Password anyway, it's OK for me. If such context is not found create a new one with Internet type. I don't really agree that we should create a context if no match is found. I think the better sensible strategy is to hold this information internally and only map it to a context once the user or provisioning creates it. Also oFono will create at least one Internet context automatically. And in the future that might be auto-provisioned. Another option would be to introduce the concept of temporary context where the lifetime is mandated by the network. I agree with Denis, I think for now we can assume that it's either Internet or IMS (well-known APN). Later if we see operators doing something else we ll adapt. So I propose to keep this behavior for now, until we see a real need for something else. Regards, Redouane. - Intel Corporation SAS (French simplified joint stock company) Registered headquarters: Les Montalets- 2, rue de Paris, 92196 Meudon Cedex, France Registration Number: 302 456 199 R.C.S. NANTERRE Capital: 4,572,000 Euros This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies. ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
Re: [PATCH] Fix compile warning at MeeGo The gcc version is MeeGo 4.5.1
On Thu, Jan 6, 2011 at 7:54 AM, Xu, Martin martin...@intel.com wrote: Using the old version of gcc 4.4.2 I did not meet the issue, but using 4.5.1, we have warning. Humn, gcc 4.5.2 here and I don't get the warning. IMHO, this warning doesn't make sense. Lucas De Marchi ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: [RFC PATCH] gprs: add function to handle activated context
Hi Sjur,
+static void gprs_cid_take(struct ofono_gprs *gprs, unsigned int id)
+{
+ idmap_take(gprs-cid_map, id);
+}
+
How do we assure that this CID is not already in use?
I think the M7400 is going to use CID=0 and M700 uses CID=1.
Perhaps we should make sure CID=0 is a legal value and reserve CID 0
and 1 at start up.
From my understanding the ID in the core is an internal ID for oFono core, even
if today we usually have a mapping 1 to 1 between oFono core ID and modems CID.
Denis, Marcel could you confirm ?
In this case the CID parameter in ofono_gprs_context_activated should be out
parameter and renamed as ID.
---
void ofono_gprs_context_activated(struct ofono_gprs_context *gc,
int *id, const char *apn,
const char *interface, ofono_bool_t static_ip,
const char *address, const char *netmask,
const char *gw, const char **dns)
---
Also if we look to ofono_gprs_context_deactivated function the parameter used
is ID and not CID, maybe it'll be beter if we rename cid in
ofono_gprs_primary_context structure to avoid confusion :
struct ofono_gprs_primary_context {
unsigned int id;
int direction;
char apn[OFONO_GPRS_MAX_APN_LENGTH + 1];
char username[OFONO_GPRS_MAX_USERNAME_LENGTH + 1];
char password[OFONO_GPRS_MAX_PASSWORD_LENGTH + 1];
enum ofono_gprs_proto proto;
};
If we try to do something else oFono core will be modem dependent and it's
really not a good idea.
Regards,
-
Intel Corporation SAS (French simplified joint stock company)
Registered headquarters: Les Montalets- 2, rue de Paris,
92196 Meudon Cedex, France
Registration Number: 302 456 199 R.C.S. NANTERRE
Capital: 4,572,000 Euros
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
Re: [RFC PATCH] gprs: add function to handle activated context
Hi Sjur,
On 01/06/2011 06:02 AM, Sjur Brændeland wrote:
Hi Redouane,
...
+static void gprs_cid_take(struct ofono_gprs *gprs, unsigned int id)
+{
+ idmap_take(gprs-cid_map, id);
+}
+
How do we assure that this CID is not already in use?
I think the M7400 is going to use CID=0 and M700 uses CID=1.
Perhaps we should make sure CID=0 is a legal value and reserve CID 0
and 1 at start up.
You already know how I feel about CID 0, I think you're trying to make
it into something special when it really isn't.
There is probably other ways of handling this as well, but I think any
other solution
would end up with race conditions, as context could be initiated from
two sources.
The spec is essentially broken in this area. Barring a complete
redesign of the spec (e.g. making the host stack activate the default
context) reserving a particular range for such default contexts seems
fine to me.
Regards,
-Denis
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
Re: [RFC PATCH] gprs: add function to handle activated context
Hi Redouane,
On 01/06/2011 08:59 AM, Soum, RedouaneX wrote:
Hi Sjur,
+static void gprs_cid_take(struct ofono_gprs *gprs, unsigned int id)
+{
+ idmap_take(gprs-cid_map, id);
+}
+
How do we assure that this CID is not already in use?
I think the M7400 is going to use CID=0 and M700 uses CID=1.
Perhaps we should make sure CID=0 is a legal value and reserve CID 0
and 1 at start up.
From my understanding the ID in the core is an internal ID for oFono core,
even if today we usually have a mapping 1 to 1 between oFono core ID and
modems CID.
Denis, Marcel could you confirm ?
Actually the intent was and still is to map it 1:1 to the modem cid to
make life easier for the drivers. Since we follow 27.007 this makes
sense. The modem driver can still do its own internal id mapping /
management, e.g. isi drivers do this AFAIK. For AT modems I'd like to
avoid doing so...
oFono can be told the range of cids to use. There's nothing precluding
the modem from giving oFono cids that are not in this range when a
context is auto-activated.
However, this gets into a real gray area whether you can have multiple
(network|auto)-activated contexts and whether the cid picked for all of
them is guaranteed to be outside this range.
Regards,
-Denis
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
Re: [PATCH v3 2/7] cdma-sms: Add CDMA SMS Support
Hi Rajesh, On 01/05/2011 11:17 AM, ext rajesh.naga...@elektrobit.com wrote: Hi Lei, +void ofono_cdma_sms_deliver_notify(struct ofono_cdma_sms *cdma_sms, + unsigned char *pdu, int tpdu_len); As there is no SC number in CDMA, the whole PDU is the actual TPDU as well. So either we should call pdu/pdu_len or tpdu/tpdu_len. Having pdu/tpdu_len might be mistaken. Will change to tpdu/tpdu_len per CDMA SMS spec for transport layer. BR, Rajesh ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono Regards, Lei ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
Re: [RFC PATCH] gprs: add function to handle activated context
Hi,
From my understanding the ID in the core is an internal ID for oFono core,
even if today we usually have a mapping 1 to 1 between oFono core ID and
modems CID.
Denis, Marcel could you confirm ?
Negative, to my understanding the CID is allocated in src/gprs.c and used
when activating/deactivating PDP contexts (PDN Connections):
snip
static gboolean assign_context(struct pri_context *ctx)
{
...
ctx-context.cid = gprs_cid_alloc(ctx-gprs);
snip
static void ...gprs_activate_primary(struct ofono_gprs_context *gc,
const struct ofono_gprs_primary_context *ctx,
ofono_gprs_context_up_cb_t cb, void *data)
{
gcd-active_context = ctx-cid;
If we try to do something else oFono core will be modem dependent and it's
really not a good idea.
It has been talk about standardizing the use of CID=0 for initial
PDN.If this comes
true this is no longer modem vendor dependent.
Regards,
Sjur
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
Re: [RFC PATCH] gprs: add function to handle activated context
Hi Sjur,
+static void gprs_cid_take(struct ofono_gprs *gprs, unsigned int id)
+{
+ idmap_take(gprs-cid_map, id);
+}
+
How do we assure that this CID is not already in use?
I think the M7400 is going to use CID=0 and M700 uses CID=1.
Perhaps we should make sure CID=0 is a legal value and reserve CID 0
and 1 at start up.
I thought that we came to the conclusion that CID=0 is pretty much a
really bad idea. So I would prefer that your modem is actually not using
CID=0 ever.
Regards
Marcel
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
Re: [PATCH v3 7/7] cdmaphonesim: Add CDMA SMS Support
Hi Denis, On 01/05/2011 10:05 AM, ext Denis Kenzior wrote: If we have both phonesim and cdmaphonesim plugins active, we need to make sure they don't interfere with each other. Perhaps adding a Type=cdma and having cdmaphonesim look for that Type tag and ignore the rest would be a good idea? I am a bit lost about the Type attribute. cdmaphonesim is currently defined as a separate plugin and by the time cdmaphonesim's parse_config() is invoked, it already knows that it is cdma. Thus, I don't see why needing another flag indicating this. When you say ignore the rest, could you pls be more specific about what to ignore and what's the intention of doing that? Sorry, still learning... :-) So correct me if I'm wrong, but there's a race between cdmaphonesim and phonesim plugins: if phonesim starts first it reads phonesim.conf and creates both phonesim and cdmaphonesim modems. When cdmaphonesim starts, it tries to create the modems again, but fails. Similar situation vice-versa. This is why I'm suggesting the Type attribute. Maybe I'm blind and this race does not happen? Now, I understand your point. I checked the log, in my setup, cdmaphonsim will run first and it will create cdmaphonsim plugin. Next, phonesim will run and try to create cdmaphonsim plugin with wrong type: phonesim but failed. I.e., we are lucky to have cdmaphonsim run first. So, the way to fix this is to add a Type=cdma flag under [cdmaphonsim] and let cdmaphonsim.c only create modem if type is set to cdma. In addition to this, we also need to modify phonesim.c to ignore any modem group that has type set to cdma. Could you pls clarify whether above solution will work? Regards, -Denis Regards, Lei ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
Re: [PATCH] Fix compile warning at MeeGo The gcc version is MeeGo 4.5.1
Hi Martin,
src/call-forwarding.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/call-forwarding.c b/src/call-forwarding.c
index 512f223..36ba4f1 100644
--- a/src/call-forwarding.c
+++ b/src/call-forwarding.c
@@ -246,7 +246,7 @@ static gboolean is_cfu_enabled(struct
ofono_call_forwarding *cf,
static void sim_set_cf_indicator(struct ofono_call_forwarding *cf)
{
gboolean cfu_voice;
- struct ofono_call_forwarding_condition *cond;
+ struct ofono_call_forwarding_condition *cond = NULL;
cfu_voice = is_cfu_enabled(cf, cond);
I really hate trying to fix compiler warnings like this.
/*
* For now we only support Voice, although Fax all Data
* basic services are applicable as well.
*/
for (; l; l = l-next) {
cond = l-data;
if (cond-cls BEARER_CLASS_VOICE)
continue;
if (out)
*out = cond;
return TRUE;
}
return FALSE;
So this is clearly a false positive. The only why this would fail is
when l-data is NULL, but even then cond is initialized properly.
Regards
Marcel
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
Re: [RFC PATCH] gprs: add function to handle activated context
Hi Sjur, I thought that we came to the conclusion that CID=0 is pretty much a really bad idea. So I would prefer that your modem is actually not using CID=0 ever. Yea, I know you or Denis said so, but I'm not sure we understood your concern... I really thought it became pretty much clear during the discussion that introducing a CID=0 that is just special on LTE is a pretty much bad idea. As long as CID=0 concept does not exist in GSM/UMTS, you should not make LTE a special case. It will not fit and just makes things utterly complex for the telephony stack to figure out when contexts are moved from LTE to GSM/UMTS. If CID=0 is not valid for GSM/UTMS then you are in big trouble to make this work properly. And if it is valid, then there is no difference between it being CID=0 or any other CID. So please step away from the CID=0 idea and just give the next free CID to the network activated context. And per specification the CID start with 1. Regards Marcel ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
[PATCH] stk: Remove unwanted error check
Possible return values of __ofono_voicecall_tone_send
are -ENOSYS, -ENOENT, -ENOMEM and -EINVAL.
---
src/stk.c |5 -
1 files changed, 0 insertions(+), 5 deletions(-)
diff --git a/src/stk.c b/src/stk.c
index bec46ea..932e49a 100644
--- a/src/stk.c
+++ b/src/stk.c
@@ -2170,11 +2170,6 @@ static gboolean handle_command_send_dtmf(const struct
stk_command *cmd,
err = __ofono_voicecall_tone_send(vc, dtmf, dtmf_sent_cb, stk);
- if (err == -EBUSY) {
- rsp-result.type = STK_RESULT_TYPE_TERMINAL_BUSY;
- return TRUE;
- }
-
if (err == -ENOSYS) {
rsp-result.type = STK_RESULT_TYPE_NOT_CAPABLE;
return TRUE;
--
1.7.0.4
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
Re: [RFC PATCH] gprs: add function to handle activated context
Hi Sjur, On 01/06/2011 11:45 AM, Sjur Brændeland wrote: Hi Marcel and Denis. I thought that we came to the conclusion that CID=0 is pretty much a really bad idea. So I would prefer that your modem is actually not using CID=0 ever. Yea, I know you or Denis said so, but I'm not sure we understood your concern... The bottom line is that the network can ask for a context to be initiated at any time. Furthermore, any number of such contexts can be activated. You can't really treat LTE specially and CID=0 really doesn't help you anyway. e.g.: crlf+CGEV: NW PDN ACT 1crlf ... crlf+CGEV: NW PDN ACT 2crlf You need to allocate these cids properly. However, solving the cid allocation problem in the general case is basically impossible. The spec is just not setup to do so in its current form. e.g. --- context1.SetActive(True); idmap_alloc(cid_map) - cid == 1 [+CGDCONT=1,...] crlf+CGEV: ME PDN ACT 1crlf Bad things happen. The best you can do is allocate these cids from a different range than oFono. e.g.: AT+CGDCONT=? +CGDCONT: (0-25) ofono_gprs_set_cid_range(gprs, 12, 25) This way oFono will use the upper range for stack initiated contexts and you guys have the lower range to play in. Regards, -Denis ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
Re: [PATCH] stk: Remove unwanted error check
Hi Jeevaka, Possible return values of __ofono_voicecall_tone_send are -ENOSYS, -ENOENT, -ENOMEM and -EINVAL. --- src/stk.c |5 - 1 files changed, 0 insertions(+), 5 deletions(-) patch has been applied. Thanks. Regards Marcel ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
[PATCH 0/3] Make Message state feature independent of the stack
This set of patches enable the org.ofono.Message interface code to be independent of the protocol stack. This will enable both GSM and CDMA SMS stack to reuse the interface code. The changes have been tested with phonesim using send-sms script. Faiyaz Baxamusa (3): doc: Add message state API include: Introduce message message: Code independent of protocol stack Makefile.am |6 +- doc/cdma-message-api.txt |5 + doc/message-api.txt | 24 + doc/message-state-api.txt | 27 + include/message.h | 67 src/message.c | 257 src/ofono.h |2 + src/sms.c | 258 +++-- 8 files changed, 399 insertions(+), 247 deletions(-) create mode 100644 doc/message-state-api.txt create mode 100644 include/message.h create mode 100644 src/message.c ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
[PATCH 1/3] doc: Add message state API
---
Makefile.am |2 +-
doc/cdma-message-api.txt |5 +
doc/message-api.txt | 24 +---
doc/message-state-api.txt | 27 +++
4 files changed, 34 insertions(+), 24 deletions(-)
create mode 100644 doc/message-state-api.txt
diff --git a/Makefile.am b/Makefile.am
index 09dc9ad..14f53ef 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -369,7 +369,7 @@ doc_files = doc/overview.txt doc/ofono-paper.txt
doc/release-faq.txt \
doc/phonebook-api.txt doc/radio-settings-api.txt \
doc/sim-api.txt doc/stk-api.txt \
doc/audio-settings-api.txt doc/text-telephony-api.txt \
- doc/calypso-modem.txt
+ doc/calypso-modem.txt doc/message-state-api.txt
test_scripts = test/backtrace \
diff --git a/doc/cdma-message-api.txt b/doc/cdma-message-api.txt
index 8e6b9ea..c3d03f8 100644
--- a/doc/cdma-message-api.txt
+++ b/doc/cdma-message-api.txt
@@ -110,3 +110,8 @@ Properties boolean UseDeliveryAcknowledgement
The call back number for the user. If the number is
empty, then the optional field is not included
in the encoded PDU.
+
+Message hierarchy
+===
+
+Please refer to message-state-api.txt
diff --git a/doc/message-api.txt b/doc/message-api.txt
index f7ab22a..e6c8522 100644
--- a/doc/message-api.txt
+++ b/doc/message-api.txt
@@ -90,26 +90,4 @@ Properties string ServiceCenterAddress
Message hierarchy
===
-Serviceorg.ofono
-Interface org.ofono.Message
-Object path[variable prefix]/{modem0,modem1,...}/{message_01,...}
-
-Methodsdict GetProperties()
-
- Returns properties for the message object. See
- the properties section for available properties.
-
- Possible Errors: [service].Error.InvalidArguments
-
-SignalsPropertyChanged(string name, variant value)
-
- This signal indicates a changed value of the given
- property.
-
-Properties string State
-
- Contains the state of the message object. Possible
- values are:
- pending,
- sent,
- failed
+Please refer to message-state-api.txt
diff --git a/doc/message-state-api.txt b/doc/message-state-api.txt
new file mode 100644
index 000..edd0cbf
--- /dev/null
+++ b/doc/message-state-api.txt
@@ -0,0 +1,27 @@
+Message hierarchy
+===
+
+Serviceorg.ofono
+Interface org.ofono.Message
+Object path[variable prefix]/{modem0,modem1,...}/{message_01,...}
+
+Methodsdict GetProperties()
+
+ Returns properties for the message object. See
+ the properties section for available properties.
+
+ Possible Errors: [service].Error.InvalidArguments
+
+SignalsPropertyChanged(string name, variant value)
+
+ This signal indicates a changed value of the given
+ property.
+
+Properties string State
+
+ Contains the state of the message object. Possible
+ values are:
+ pending,
+ sent,
+ failed
+ invalid
--
1.7.0.4
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
[PATCH 2/3] include: Introduce message
---
Makefile.am |2 +-
include/message.h | 67 +
2 files changed, 68 insertions(+), 1 deletions(-)
create mode 100644 include/message.h
diff --git a/Makefile.am b/Makefile.am
index 14f53ef..8b19eef 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -15,7 +15,7 @@ include_HEADERS = include/log.h include/plugin.h
include/history.h \
include/radio-settings.h include/stk.h \
include/audio-settings.h include/nettime.h \
include/ctm.h include/cdma-voicecall.h \
- include/cdma-sms.h
+ include/cdma-sms.h include/message.h
nodist_include_HEADERS = include/version.h
diff --git a/include/message.h b/include/message.h
new file mode 100644
index 000..7c60390
--- /dev/null
+++ b/include/message.h
@@ -0,0 +1,67 @@
+/*
+ *
+ * oFono - Open Source Telephony
+ *
+ * Copyright (C) 2008-2011 Intel Corporation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
+ *
+ */
+
+#ifndef __OFONO_MESSAGE_H
+#define __OFONO_MESSAGE_H
+
+#ifdef __cplusplus
+extern C {
+#endif
+
+#include ofono/types.h
+
+enum message_state {
+ MESSAGE_STATE_PENDING,
+ MESSAGE_STATE_SENT,
+ MESSAGE_STATE_FAILED
+};
+
+struct message;
+
+struct message *ofono_message_create(const struct ofono_uuid *uuid,
+ void *data);
+
+gboolean ofono_message_dbus_register(const char *atompath, struct message *m);
+void ofono_message_dbus_unregister(const char *atompath, struct message *m);
+
+const struct ofono_uuid *ofono_message_get_uuid(const struct message *m);
+
+void ofono_message_set_state(const char *atompath, struct message *m,
+ enum message_state new_state);
+
+void ofono_message_append_properties(struct message *m, DBusMessageIter *dict);
+
+void ofono_emit_message_added(const char *atompath, const char *interface,
+ struct message *m);
+
+void ofono_emit_message_removed(const char *atompath, const char *interface,
+ struct message *m);
+
+void *ofono_message_get_data(struct message *m);
+
+const char *ofono_message_path_from_uuid(const char *atompath,
+ const struct ofono_uuid *uuid);
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* __OFONO_MESSAGE_H */
--
1.7.0.4
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
[PATCH 3/3] message: Code independent of protocol stack
---
Makefile.am |2 +-
src/message.c | 257
src/ofono.h |2 +
src/sms.c | 258
4 files changed, 297 insertions(+), 222 deletions(-)
create mode 100644 src/message.c
diff --git a/Makefile.am b/Makefile.am
index 8b19eef..756e6b7 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -329,7 +329,7 @@ src_ofonod_SOURCES = $(gdbus_sources) $(builtin_sources)
src/ofono.ver \
src/nettime.c src/stkagent.c src/stkagent.h \
src/simfs.c src/simfs.h src/audio-settings.c \
src/smsagent.c src/smsagent.h src/ctm.c \
- src/cdma-voicecall.c
+ src/cdma-voicecall.c src/message.c
src_ofonod_LDADD = $(builtin_libadd) @GLIB_LIBS@ @DBUS_LIBS@ @CAPNG_LIBS@ -ldl
diff --git a/src/message.c b/src/message.c
new file mode 100644
index 000..9e880f6
--- /dev/null
+++ b/src/message.c
@@ -0,0 +1,257 @@
+/*
+ *
+ * oFono - Open Source Telephony
+ *
+ * Copyright (C) 2008-2011 Intel Corporation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include config.h
+#endif
+
+#include string.h
+#include gdbus.h
+#include stdio.h
+
+#include ofono/types.h
+
+#include ofono.h
+#include common.h
+
+struct message {
+ struct ofono_uuid uuid;
+ enum message_state state;
+ void *data;
+};
+
+static const char *message_state_to_string(enum message_state s)
+{
+ switch (s) {
+ case MESSAGE_STATE_PENDING:
+ return pending;
+ case MESSAGE_STATE_SENT:
+ return sent;
+ case MESSAGE_STATE_FAILED:
+ return failed;
+ }
+
+ return invalid;
+}
+
+static DBusMessage *message_get_properties(DBusConnection *conn,
+ DBusMessage *msg, void *data)
+{
+ struct message *m = data;
+ DBusMessage *reply;
+ DBusMessageIter iter;
+ DBusMessageIter dict;
+
+ reply = dbus_message_new_method_return(msg);
+ if (reply == NULL)
+ return NULL;
+
+ dbus_message_iter_init_append(reply, iter);
+
+ dbus_message_iter_open_container(iter, DBUS_TYPE_ARRAY,
+ OFONO_PROPERTIES_ARRAY_SIGNATURE,
+ dict);
+ ofono_message_append_properties(m, dict);
+ dbus_message_iter_close_container(iter, dict);
+
+ return reply;
+}
+
+static GDBusMethodTable message_methods[] = {
+ { GetProperties, ,a{sv}, message_get_properties },
+ { }
+};
+
+static GDBusSignalTable message_signals[] = {
+ { PropertyChanged,sv },
+ { }
+};
+
+struct message *ofono_message_create(const struct ofono_uuid *uuid,
+ void *data)
+{
+ struct message *v;
+
+ if (uuid == NULL)
+ return NULL;
+
+ v = g_try_new0(struct message, 1);
+ if (v == NULL)
+ return NULL;
+
+ memcpy(v-uuid, uuid, sizeof(*uuid));
+
+ v-data = data;
+
+ return v;
+}
+
+static void message_destroy(gpointer userdata)
+{
+ struct message *m = userdata;
+
+ if (m == NULL)
+ return;
+
+ m-data = NULL;
+ g_free(m);
+}
+
+gboolean ofono_message_dbus_register(const char *atompath, struct message *m)
+{
+ DBusConnection *conn = ofono_dbus_get_connection();
+ const char *path;
+
+ if ((atompath == NULL) || (m == NULL))
+ return FALSE;
+
+ path = ofono_message_path_from_uuid(atompath, m-uuid);
+
+ if (!g_dbus_register_interface(conn, path, OFONO_MESSAGE_INTERFACE,
+ message_methods, message_signals,
+ NULL, m, message_destroy)) {
+ ofono_error(Could not register Message %s, path);
+ message_destroy(m);
+
+ return FALSE;
+ }
+
+ return TRUE;
+}
+
+void ofono_message_dbus_unregister(const char *atompath, struct message *m)
+{
+ DBusConnection *conn = ofono_dbus_get_connection();
+ const char *path;
+
+ if ((atompath == NULL) || (m == NULL))
+ return;
+
+
[PATCH 0/2] Add additional information for mandatory general result
Hi, As per the ETSI TS 102 223 specification section 8.12.2, For the general result terminal currently unable to process command, it is mandatory for the terminal to provide additional information. Defined values for additional information can be found in the same section. Coding '00' shall be used by the terminal if none of the defined values apply. Following set of patches, introduce a macro for adding the additional information and changes done to existing code to make use of added macro. Regards, Jeevaka Jeevaka Badrappan (2): stk: add additional info for terminal busy result stk: make use of ADD_ERROR_RESULT macro src/stk.c | 114 - 1 files changed, 67 insertions(+), 47 deletions(-) ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
[PATCH 1/2] stk: add additional info for terminal busy result
As per the ETSI TS 102 223 specification 8.12.2, it is
mandatory to provide additional information for the
general result ME currently unable to process command.
---
src/stk.c | 45 +
1 files changed, 37 insertions(+), 8 deletions(-)
diff --git a/src/stk.c b/src/stk.c
index bec46ea..a00190d 100644
--- a/src/stk.c
+++ b/src/stk.c
@@ -99,6 +99,11 @@ struct extern_req {
static void envelope_queue_run(struct ofono_stk *stk);
static void timers_update(struct ofono_stk *stk);
+#define ADD_ERROR_RESULT(result, error, addn_info) \
+ result.type = error;\
+ result.additional_len = sizeof(addn_info); \
+ result.additional = addn_info; \
+
static int stk_respond(struct ofono_stk *stk, struct stk_response *rsp,
ofono_stk_generic_cb_t cb)
{
@@ -857,7 +862,10 @@ static gboolean handle_command_send_sms(const struct
stk_command *cmd,
msg_list.next = NULL;
if (__ofono_sms_txq_submit(sms, msg_list, 0, uuid, NULL, NULL) 0) {
- rsp-result.type = STK_RESULT_TYPE_TERMINAL_BUSY;
+ unsigned char no_cause_result[] = { 0x00 };
+
+ ADD_ERROR_RESULT(rsp-result, STK_RESULT_TYPE_TERMINAL_BUSY,
+ no_cause_result);
return TRUE;
}
@@ -1183,9 +1191,12 @@ static gboolean handle_command_select_item(const struct
stk_command *cmd,
request_selection_cb, stk,
request_selection_destroy,
stk-timeout * 1000) 0) {
+ unsigned char no_cause_result[] = { 0x00 };
+
request_selection_destroy(stk);
- rsp-result.type = STK_RESULT_TYPE_TERMINAL_BUSY;
+ ADD_ERROR_RESULT(rsp-result, STK_RESULT_TYPE_TERMINAL_BUSY,
+ no_cause_result);
return TRUE;
}
@@ -1301,7 +1312,10 @@ static gboolean handle_command_display_text(const struct
stk_command *cmd,
/* We most likely got an out of memory error, tell SIM to retry */
if (err 0) {
- rsp-result.type = STK_RESULT_TYPE_TERMINAL_BUSY;
+ unsigned char no_cause_result[] = { 0x00 };
+
+ ADD_ERROR_RESULT(rsp-result, STK_RESULT_TYPE_TERMINAL_BUSY,
+ no_cause_result);
return TRUE;
}
@@ -1475,11 +1489,14 @@ static gboolean handle_command_get_inkey(const struct
stk_command *cmd,
g_free(text);
if (err 0) {
+ unsigned char no_cause_result[] = { 0x00 };
+
/*
* We most likely got an out of memory error, tell SIM
* to retry
*/
- rsp-result.type = STK_RESULT_TYPE_TERMINAL_BUSY;
+ ADD_ERROR_RESULT(rsp-result, STK_RESULT_TYPE_TERMINAL_BUSY,
+ no_cause_result);
return TRUE;
}
@@ -1564,11 +1581,14 @@ static gboolean handle_command_get_input(const struct
stk_command *cmd,
g_free(text);
if (err 0) {
+ unsigned char no_cause_result[] = { 0x00 };
+
/*
* We most likely got an out of memory error, tell SIM
* to retry
*/
- rsp-result.type = STK_RESULT_TYPE_TERMINAL_BUSY;
+ ADD_ERROR_RESULT(rsp-result, STK_RESULT_TYPE_TERMINAL_BUSY,
+ no_cause_result);
return TRUE;
}
@@ -1772,11 +1792,14 @@ static gboolean handle_command_set_up_call(const struct
stk_command *cmd,
g_free(alpha_id);
if (err 0) {
+ unsigned char no_cause_result[] = { 0x00 };
+
/*
* We most likely got an out of memory error, tell SIM
* to retry
*/
- rsp-result.type = STK_RESULT_TYPE_TERMINAL_BUSY;
+ ADD_ERROR_RESULT(rsp-result, STK_RESULT_TYPE_TERMINAL_BUSY,
+ no_cause_result);
return TRUE;
}
@@ -2193,11 +2216,14 @@ static gboolean handle_command_send_dtmf(const struct
stk_command *cmd,
}
if (err 0) {
+ unsigned char no_cause_result[] = { 0x00 };
+
/*
* We most likely got an out of memory error, tell SIM
* to retry
*/
- rsp-result.type = STK_RESULT_TYPE_TERMINAL_BUSY;
+ ADD_ERROR_RESULT(rsp-result, STK_RESULT_TYPE_TERMINAL_BUSY,
+ no_cause_result);
return TRUE;
}
@@ -2329,11 +2355,14 @@ static gboolean handle_command_play_tone(const struct
stk_command *cmd,
Re: [PATCH v2, 3/7] cdma-sms: Add CDMA SMS Support
Hi Lei, So it looks to me like this function takes arbitrary bit fields from a stream. Is this a 'peculiarity' of the address field or will this be used elsewhere? Yes, your interpretation of the function is correct. Actually, cross CDMA SMS spec (and also other CDMA protocol specs), a lot of the message (parameter and etc) structures are defined just as 'peculiar' as Address field. For example: User Data (section 4.5.2 of C.S0015-B), Call Back Number (can be considered same as address). Essentially, CDMA SMS specs defines structures as bit streams and a field can start at any offset within the bit stream. Even worse, depending on the values of the previous fields, a field can start at different locations within a bit stream for the different instances of the same type of the message. Yes, some really smart engineers must have designed this spec ;) This looks like a really inefficient function to call for every field, so it might makes things easier if the offsets were simply hardcoded... The main reason I do this way is that I feel there will be more chances to make mistake if the 'shift', 'bit-wise and', bit-wise or' operation is carried out when decoding each field separately. It will be cleaner to have this generic function and decoding of each field will just care about size of the field and start offset of the field into the stream. I'm unconvinced. The first thing I question is why you bother decoding up to 32 bits at once in a loop. The max size of the field I've seen so far is 16 bits and even that is not common. Decoding as 8 bits and bitwise ORing in the appropriate place might be way simpler (and faster). Efficiency-wise, I see worst case a few extra machine instructions will be used in comparing to 'manually' decoding at each place. Efficiency wise this is actually pretty bad compared to a tight loop. Especially for things like user-data which is just 8 bits but bit-shifted a random amount. We're talking about potentially 250+ functions calls per sms segment... Since the function is fairly large, not sure if it can even be inlined successfully. We are targeting embedded devices here, right? ;) Can you try substituting a macro / inline function that can extrat a max of 8 bits from a bitstream instead? I believe such an operation would be easily inlined, would not require branching and still keep most of the advantages you list out above... Explicit casts should be avoided Will fix. Could you pls also educate me the rationales behind this rule? Two reasons: It looks ugly; and When I'm reviewing code, every time there's an explicit cast my alarm bells go off. It could mean that one: a. Didn't think through one's data structures / code (e.g. const / non-const, enum vs int, etc). The compiler's job is to point out such possibilities in order for them to be fixed appropriately. Often casting is just an attempt to hide these warning signs. b. is doing something really evil without fully thinking it through. See point a. c. Doing something really evil for a good reason, but it is so tricky it needs special attention. This last one should be pretty rare ;) Have you looked at how STK pdus are decoded in stkutil.c, parse_dataobj() in particular. Looking at the basic code structure so far, that design pattern could be (or not) a really nice fit here and save you some kLoC in the future. Looked at it. I am assuming that the design pattern you are referring to is that we can create an array of structure where decoding function for a parameter record can be stored thus, this part of the code will be reduced to something like: parsing the type and invoke the decoding function (dataobj_handler as in stkutil.c) as in the array of the function handlers. I don't see this approach and the approach I described above are really not much differences since the logics below are nothing but a big switch-case state which will be required even with function handler array approach anyway. The only simplification I can see below is to somehow to put set_bitmap() into one place. This will reduce ~30 LOC. There's more to it than that. You also get automatic checking of the presence of the mandatory fields which you do not do right now and will repeat at least twice. You also don't need to code the same exact loop multiple times. That reminds me, it might be useful to have a proper iterator for subparameter data. E.g. similar to simple_tlv_iter. It should make things way easier to read for the uninitiated. Regards, -Denis ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: [PATCH v3] ifx: Adding modem selftest for Infineon modem
plugins/ifx.c | 76
+---
1 files changed, 66 insertions(+), 10 deletions(-)
diff --git a/plugins/ifx.c b/plugins/ifx.c
index c0a69c2..e0eb982 100644
--- a/plugins/ifx.c
+++ b/plugins/ifx.c
@@ -71,6 +71,8 @@
#define GPRS3_DLC 4
#define AUX_DLC 5
+#define IFX_SELF_TESTS_TIMEOUT 10
+
I asked this 3 times now, where is this magic value of 10 seconds coming
from. What is the average expected execution time of each test?
Typical response times are much less than 0.5 sec. 10 seconds is considered
much longer than any normal response time.
+static struct {
+ char *test_desc;
+ char *at_cmd;
+} const mst[] = {
+ { AT Command Test, ATE0 +CMEE=1 }, /* set echo error reporting */
I really don't like to put ATE0 into this. It is not a selftest or test
command.
It acts as a selftest command because if the modem fails to come out of reset
and/or start the AT command parser you will never get an OK back on it. In the
error reporting we would like to be able to distinguish between the AT command
parser not working at all and any subsequent test command failing.
+ { RTC GTI Test, a...@rtc:rtc_gti_test_verify_32khz() },
+ { Device Version Test, a...@vers:device_version_id() },
+ { NULL, NULL }
+};
And I like to still see an answer why we have to trigger selftests here.
To make sure the modem is properly functioning.
Can we just not have one AT command to check the modem health and be
done with it?
The IFX modem we are targetting implements this with two AT commands.
Another question that I did ask is to see some sample results from these
test cases in failure and success case.
Robertino?
Do we actually care about test
description here at all or can we just drop it?
If you prefer cryptic messages then the AT command itself can act as the
description.
Is that preferred?
This is rather pointless. We are not starting a timeout for every single
command. The mux_timeout handling was designed for spawning the overall
setup process and not just one command.
The design documentation wasn't clear on that point. The name mux_timeout
suggested that the timeout only applied to the mux, maybe it should be renamed
to
setup_timeout to better describe the design intention?
Regardless, having a timeout for each command is conceptually cleaner as you
will
know for sure which command is responsible for exceeding the timeout.
Does oFono require the more ambiguous solution with a single timeout?
I am fine with using the established mutliplexer timeout handling here,
but essentially this is not a SELF_TESTS timeout. This is overall setup
timeout. So can we please get the naming right here. I really dislike
code where function names and constants are not named properly. This
causes major confusion later on.
In the patch it is the timeout for the first self test command.
I understand you are thinking about a potential future patch in which it would
be
used as the overall setup timeout, I agree that in that case the name should be
changed accordingly as well, yes.
Cheers,
Waldo
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
RE: [PATCH v3] ifx: Adding modem selftest for Infineon modem
Hi, The IFX modem we are targetting implements this with two AT commands. Another question that I did ask is to see some sample results from these test cases in failure and success case. Robertino? I think you meant except form the log, such as this: ofonod[1342]: Using multiplexer line discipline 29 ofonod[1342]: Master: ATE0 +CMEE=1\r ofonod[1342]: Master: ATE0 +CMEE=1\r ofonod[1342]: Master: \r\nOK\r\n ofonod[1342]: Master: a...@rtc:rtc_gti_test_verify_32khz()\r ofonod[1342]: Master: \r\n ofonod[1342]: Master: Verification passed\r\nOK\r\n ofonod[1342]: Master: a...@vers:device_version_id()\r ofonod[1342]: Master: \r\n ofonod[1342]: Master: \r\nCHIP ID = XG626 ES2\r\nFLASH TYPE = NumonyxM18 NOR\r\nFLASH ID = 0x898881\r\nSmartiUE2 = 37664\r\nRF PMU = 10548\r\nPA PMU = 40244\r\nRF ASM = 0\r\nFEM PA = 21812\r\nOK\r\n Simulated Self test Failure ofonod[1260]: Using multiplexer line discipline 29 ofonod[1260]: Master: ATE0 +CMEE=1\r ofonod[1260]: Master: ATE0 +CMEE=1\r ofonod[1260]: Master: \r\nOK\r\n ofonod[1260]: Master: a...@rtc:rtc_gti_test_verify_32khz()\r ofonod[1260]: Master: \r\n ofonod[1260]: Master: Verification passed\r\nOK\r\n ofonod[1260]: Master: a...@vers:device_version_id\r ofonod[1260]: Master: \r\nERROR\r\n ofonod[1260]: Modem Device Version Test: FAIL In the patch it is the timeout for the first self test command. I understand you are thinking about a potential future patch in which it would be used as the overall setup timeout, I agree that in that case the name should be changed accordingly as well, yes. Do you think we should use overall setup timeout? Thanks, -- r. ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: [PATCH] Fix compile warning at MeeGo The gcc version is MeeGo 4.5.1
-Original Message-
From: ofono-boun...@ofono.org [mailto:ofono-boun...@ofono.org] On Behalf
Of Marcel Holtmann
Sent: Friday, January 07, 2011 1:45 AM
To: ofono@ofono.org
Subject: Re: [PATCH] Fix compile warning at MeeGo The gcc version is MeeGo
4.5.1
Hi Martin,
src/call-forwarding.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/call-forwarding.c b/src/call-forwarding.c
index 512f223..36ba4f1 100644
--- a/src/call-forwarding.c
+++ b/src/call-forwarding.c
@@ -246,7 +246,7 @@ static gboolean is_cfu_enabled(struct
ofono_call_forwarding *cf,
static void sim_set_cf_indicator(struct ofono_call_forwarding *cf)
{
gboolean cfu_voice;
- struct ofono_call_forwarding_condition *cond;
+ struct ofono_call_forwarding_condition *cond = NULL;
cfu_voice = is_cfu_enabled(cf, cond);
I really hate trying to fix compiler warnings like this.
/*
* For now we only support Voice, although Fax all Data
* basic services are applicable as well.
*/
for (; l; l = l-next) {
cond = l-data;
if (cond-cls BEARER_CLASS_VOICE)
continue;
if (out)
*out = cond;
return TRUE;
}
return FALSE;
So this is clearly a false positive. The only why this would fail is
This is not a false positive, it is quite possible not to reach the for(;;)
branch, and uninitialized the *cond.
I have added
*out = NULL;
Out of the branch, and found that the warning gone.
So here I think gcc is right, and we need the patch.
when l-data is NULL, but even then cond is initialized properly.
Regards
Marcel
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
RE: [PATCH] Fix compile warning at MeeGo The gcc version is MeeGo 4.5.1
Hi Martin,
src/call-forwarding.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/call-forwarding.c b/src/call-forwarding.c
index 512f223..36ba4f1 100644
--- a/src/call-forwarding.c
+++ b/src/call-forwarding.c
@@ -246,7 +246,7 @@ static gboolean is_cfu_enabled(struct
ofono_call_forwarding *cf,
static void sim_set_cf_indicator(struct ofono_call_forwarding *cf)
{
gboolean cfu_voice;
- struct ofono_call_forwarding_condition *cond;
+ struct ofono_call_forwarding_condition *cond = NULL;
cfu_voice = is_cfu_enabled(cf, cond);
I really hate trying to fix compiler warnings like this.
/*
* For now we only support Voice, although Fax all Data
* basic services are applicable as well.
*/
for (; l; l = l-next) {
cond = l-data;
if (cond-cls BEARER_CLASS_VOICE)
continue;
if (out)
*out = cond;
return TRUE;
}
return FALSE;
So this is clearly a false positive. The only why this would fail is
This is not a false positive, it is quite possible not to reach the for(;;)
branch, and uninitialized the *cond.
I have added
*out = NULL;
Out of the branch, and found that the warning gone.
So here I think gcc is right, and we need the patch.
it is a false positive.
The only caller that uses the cond value is sim_set_cf_indicator. And it
only uses conf if the return value is TRUE. So where can this go wrong?
Regards
Marcel
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
RE: [PATCH] Fix compile warning at MeeGo The gcc version is MeeGo 4.5.1
Hi Marcel: it is a false positive. The only caller that uses the cond value is sim_set_cf_indicator. And it only uses conf if the return value is TRUE. So where can this go wrong? You are right. I will ask MeeGo tool chain guy to resolve it. Marcel ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
