Re: [PATCH 1/4] gisi: fix crash bug in g_isi_remove_subscription
Hi Mika, 2010/11/10 Mika Liljeberg mika.liljeb...@nokia.com: --- gisi/client.c | 7 +-- 1 files changed, 5 insertions(+), 2 deletions(-) Patch has been applied. Thanks. Cheers, Aki ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
Re: [PATCH 2/4] gisi: return NULL instead of asserting
Hi Mika, 2010/11/10 Mika Liljeberg mika.liljeb...@nokia.com: --- gisi/pep.c | 6 -- 1 files changed, 4 insertions(+), 2 deletions(-) Patch has been applied. Thanks. Cheers, Aki ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
Re: [PATCH 3/4] isimodem: revector GPRS context driver
Hi Mika, 2010/11/10 Mika Liljeberg mika.liljeb...@nokia.com: --- drivers/isimodem/gprs-context.c | 280 +-- 1 files changed, 121 insertions(+), 159 deletions(-) Patch has been applied. Thanks! Cheers, Aki ___ 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: [PATCH 4/4] isigen: make number of PDP contexts configurable
Hi Mika, 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. if it can be probed from the ISI modem that would be perfect. 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. That is sort of a dream world. AT command based modems where you need a TTY/DLC for every active GPRS context are not cheap. It is not a crazy waste of resources, but it is some kind of waste. Regards Marcel ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
[PATCH 1/1] isigen: create four gprs contexts
---
plugins/isigen.c | 19 ++-
1 files changed, 14 insertions(+), 5 deletions(-)
diff --git a/plugins/isigen.c b/plugins/isigen.c
index 838d060..3ea7110 100644
--- a/plugins/isigen.c
+++ b/plugins/isigen.c
@@ -58,6 +58,8 @@
#include drivers/isimodem/mtc.h
#include drivers/isimodem/debug.h
+#define ISI_DEFAULT_PDPS 4 /* Number of supported PDP contexts */
+
struct isi_data {
struct ofono_modem *modem;
char const *ifname;
@@ -407,6 +409,7 @@ static void isigen_post_online(struct ofono_modem *modem)
struct isi_data *isi = ofono_modem_get_data(modem);
struct ofono_gprs *gprs;
struct ofono_gprs_context *gc;
+ int i;
DBG((%p) with %s, modem, isi-ifname);
@@ -420,13 +423,19 @@ static void isigen_post_online(struct ofono_modem *modem)
ofono_call_barring_create(isi-modem, 0, isimodem, isi-idx);
ofono_call_meter_create(isi-modem, 0, isimodem, isi-idx);
ofono_radio_settings_create(isi-modem, 0, isimodem, isi-idx);
- gprs = ofono_gprs_create(isi-modem, 0, isimodem, isi-idx);
- gc = ofono_gprs_context_create(isi-modem, 0, isimodem, isi-idx);
- if (gprs gc)
+ gprs = ofono_gprs_create(isi-modem, 0, isimodem, isi-idx);
+ if (!gprs)
+ return;
+ for (i = 0; i ISI_DEFAULT_PDPS; i++) {
+ gc = ofono_gprs_context_create(isi-modem, 0,
+ isimodem, isi-idx);
+ if (!gc) {
+ DBG(Failed to add context %d, i);
+ break;
+ }
ofono_gprs_add_context(gprs, gc);
- else
- DBG(Failed to add context);
+ }
}
static int isigen_enable(struct ofono_modem *modem)
--
1.7.0.4
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
Re: isigen: enabled multiple PDP contexts
Hi Mika, Here's the patch to enable multiple PDP contexts in isigen. Turns out that probing the context count is not feasible, so we just default to four. 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. Regards Marcel ___ 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
[PATCH] monitor-ofono: monitor DisconnectReason
From: Pekka Pessi pekka.pe...@nokia.com
---
test/monitor-ofono | 11 +++
1 files changed, 11 insertions(+), 0 deletions(-)
diff --git a/test/monitor-ofono b/test/monitor-ofono
index 2f49f76..8570c34 100755
--- a/test/monitor-ofono
+++ b/test/monitor-ofono
@@ -75,6 +75,10 @@ def ussd(msg, member, path, interface):
iface = interface[interface.rfind(.) + 1:]
print {%s} [%s] %s %s % (iface, path, member, str(msg))
+def value(value, member, path, interface):
+ iface = interface[interface.rfind(.) + 1:]
+ print {%s} [%s] %s %s % (iface, path, member, str(value))
+
if __name__ == '__main__':
dbus.mainloop.glib.DBusGMainLoop(set_as_default=True)
@@ -150,6 +154,13 @@ if __name__ == '__main__':
path_keyword=path,
interface_keyword=interface)
+ bus.add_signal_receiver(value,
+ bus_name=org.ofono,
+ signal_name = DisconnectReason,
+ member_keyword=member,
+ path_keyword=path,
+ interface_keyword=interface)
+
for member in [IncomingBroadcast, EmergencyBroadcast,
IncomingMessage, ImmediateMessage]:
bus.add_signal_receiver(message,
--
1.7.1
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
[PATCH] test/answer-calls: answer waiting calls, too
From: Pekka Pessi pekka.pe...@nokia.com
---
test/answer-calls | 20 ++--
1 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/test/answer-calls b/test/answer-calls
index 0deb832..1218c66 100755
--- a/test/answer-calls
+++ b/test/answer-calls
@@ -4,8 +4,11 @@ import dbus
bus = dbus.SystemBus()
-manager = dbus.Interface(bus.get_object('org.ofono', '/'),
- 'org.ofono.Manager')
+def oface(path, name):
+ obj = bus.get_object('org.ofono', path)
+ return dbus.Interface(obj, name)
+
+manager = oface('/', 'org.ofono.Manager')
modems = manager.GetModems()
@@ -15,8 +18,7 @@ for path, properties in modems:
if org.ofono.VoiceCallManager not in properties[Interfaces]:
continue
- mgr = dbus.Interface(bus.get_object('org.ofono', path),
- 'org.ofono.VoiceCallManager')
+ mgr = oface(path, 'org.ofono.VoiceCallManager')
calls = mgr.GetCalls()
@@ -24,10 +26,8 @@ for path, properties in modems:
state = properties[State]
print [ %s ] %s % (path, state)
- if state != incoming:
- continue
-
- call = dbus.Interface(bus.get_object('org.ofono', path),
- 'org.ofono.VoiceCall')
+ if state == incoming:
+ oface(path, 'org.ofono.VoiceCall').Answer()
+ elif state == waiting:
+ mgr.HoldAndAnswer()
- call.Answer()
--
1.7.1
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
Re: [PATCH 1/1] isigen: create four gprs contexts
Hi Mika,
On 11/11/2010 04:00 AM, Mika Liljeberg wrote:
---
plugins/isigen.c | 19 ++-
1 files changed, 14 insertions(+), 5 deletions(-)
diff --git a/plugins/isigen.c b/plugins/isigen.c
index 838d060..3ea7110 100644
--- a/plugins/isigen.c
+++ b/plugins/isigen.c
@@ -58,6 +58,8 @@
#include drivers/isimodem/mtc.h
#include drivers/isimodem/debug.h
+#define ISI_DEFAULT_PDPS 4 /* Number of supported PDP contexts */
+
struct isi_data {
struct ofono_modem *modem;
char const *ifname;
@@ -407,6 +409,7 @@ static void isigen_post_online(struct ofono_modem *modem)
struct isi_data *isi = ofono_modem_get_data(modem);
struct ofono_gprs *gprs;
struct ofono_gprs_context *gc;
+ int i;
DBG((%p) with %s, modem, isi-ifname);
@@ -420,13 +423,19 @@ static void isigen_post_online(struct ofono_modem
*modem)
ofono_call_barring_create(isi-modem, 0, isimodem, isi-idx);
ofono_call_meter_create(isi-modem, 0, isimodem, isi-idx);
ofono_radio_settings_create(isi-modem, 0, isimodem, isi-idx);
- gprs = ofono_gprs_create(isi-modem, 0, isimodem, isi-idx);
- gc = ofono_gprs_context_create(isi-modem, 0, isimodem, isi-idx);
- if (gprs gc)
+ gprs = ofono_gprs_create(isi-modem, 0, isimodem, isi-idx);
+ if (!gprs)
+ return;
Tiny nitpick, but please follow the coding style. Specifically item M1.
+ for (i = 0; i ISI_DEFAULT_PDPS; i++) {
+ gc = ofono_gprs_context_create(isi-modem, 0,
+ isimodem, isi-idx);
+ if (!gc) {
+ DBG(Failed to add context %d, i);
+ break;
+ }
And again, item M1 here
ofono_gprs_add_context(gprs, gc);
- else
- DBG(Failed to add context);
+ }
}
static int isigen_enable(struct ofono_modem *modem)
Regards,
-Denis
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
[PATCH] voicecall: Add EmergencyCall property.
---
doc/voicecall-api.txt |4
src/voicecall.c | 34 +-
2 files changed, 37 insertions(+), 1 deletions(-)
diff --git a/doc/voicecall-api.txt b/doc/voicecall-api.txt
index f0ba316..60c692c 100644
--- a/doc/voicecall-api.txt
+++ b/doc/voicecall-api.txt
@@ -125,3 +125,7 @@ Properties string LineIdentification [readonly]
Icon identifier to be used instead of or together
with the text information.
+
+ boolean EmergencyCall [readonly]
+
+ Contains the indication if the voice call is emergency
call or not.
diff --git a/src/voicecall.c b/src/voicecall.c
index bd64432..e39f4b7 100644
--- a/src/voicecall.c
+++ b/src/voicecall.c
@@ -315,6 +315,20 @@ static void tone_request_finish(struct ofono_voicecall *vc,
g_free(entry);
}
+static gboolean voicecall_isemergency(struct voicecall *v)
+{
+ struct ofono_call *call = v-call;
+ const char *lineid_str;
+
+ lineid_str = phone_number_to_string(call-phone_number);
+
+ if (g_slist_find_custom(v-vc-en_list, lineid_str,
+ (GCompareFunc) strcmp))
+ return TRUE;
+ else
+ return FALSE;
+}
+
static void append_voicecall_properties(struct voicecall *v,
DBusMessageIter *dict)
{
@@ -322,7 +336,8 @@ static void append_voicecall_properties(struct voicecall *v,
const char *status;
const char *callerid;
const char *timestr;
- ofono_bool_t mpty;
+ ofono_bool_t mpt;
+ dbus_bool_t emergency_call;
status = call_status_to_string(call-status);
callerid = phone_number_to_string(call-phone_number);
@@ -358,6 +373,14 @@ static void append_voicecall_properties(struct voicecall
*v,
if (v-icon_id)
ofono_dbus_dict_append(dict, Icon, DBUS_TYPE_BYTE,
v-icon_id);
+
+ if (voicecall_isemergency(v))
+ emergency_call = TRUE;
+ else
+ emergency_call = FALSE;
+
+ ofono_dbus_dict_append(dict, EmergencyCall, DBUS_TYPE_BOOLEAN,
+ emergency_call);
}
static DBusMessage *voicecall_get_properties(DBusConnection *conn,
@@ -722,6 +745,15 @@ static void voicecall_set_call_lineid(struct voicecall *v,
OFONO_VOICECALL_INTERFACE,
LineIdentification,
DBUS_TYPE_STRING, lineid_str);
+
+ if (voicecall_isemergency(v)) {
+ dbus_bool_t emergency_call = TRUE;
+ ofono_dbus_signal_property_changed(conn, path,
+ OFONO_VOICECALL_INTERFACE,
+ EmergencyCall,
+ DBUS_TYPE_BOOLEAN,
+ emergency_call);
+ }
}
static gboolean voicecall_dbus_register(struct voicecall *v)
--
1.7.0.4
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
Re: [PATCH] monitor-ofono: monitor DisconnectReason
Hi Pekka, On 11/11/2010 06:54 AM, pekka.pe...@nokia.com wrote: From: Pekka Pessi pekka.pe...@nokia.com --- test/monitor-ofono | 11 +++ 1 files changed, 11 insertions(+), 0 deletions(-) This patch has been applied, thanks. Regards, -Denis ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
Re: [PATCH] test/answer-calls: answer waiting calls, too
Hi Pekka,
On 11/11/2010 06:54 AM, pekka.pe...@nokia.com wrote:
From: Pekka Pessi pekka.pe...@nokia.com
---
test/answer-calls | 20 ++--
1 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/test/answer-calls b/test/answer-calls
index 0deb832..1218c66 100755
--- a/test/answer-calls
+++ b/test/answer-calls
@@ -4,8 +4,11 @@ import dbus
bus = dbus.SystemBus()
-manager = dbus.Interface(bus.get_object('org.ofono', '/'),
- 'org.ofono.Manager')
+def oface(path, name):
+ obj = bus.get_object('org.ofono', path)
+ return dbus.Interface(obj, name)
+
+manager = oface('/', 'org.ofono.Manager')
I'd really like to keep things consistent even inside the test
directory. Right now we have about two or three distinct styles of
python, and this change isn't helping ;)
modems = manager.GetModems()
@@ -15,8 +18,7 @@ for path, properties in modems:
if org.ofono.VoiceCallManager not in properties[Interfaces]:
continue
- mgr = dbus.Interface(bus.get_object('org.ofono', path),
- 'org.ofono.VoiceCallManager')
+ mgr = oface(path, 'org.ofono.VoiceCallManager')
calls = mgr.GetCalls()
@@ -24,10 +26,8 @@ for path, properties in modems:
state = properties[State]
print [ %s ] %s % (path, state)
- if state != incoming:
- continue
-
- call = dbus.Interface(bus.get_object('org.ofono', path),
- 'org.ofono.VoiceCall')
+ if state == incoming:
+ oface(path, 'org.ofono.VoiceCall').Answer()
+ elif state == waiting:
+ mgr.HoldAndAnswer()
Actually I'd prefer a separate script for this.
- call.Answer()
Regards,
-Denis
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
Re: [PATCH] voicecall: Add EmergencyCall property.
Hi John, doc/voicecall-api.txt |4 src/voicecall.c | 34 +- 2 files changed, 37 insertions(+), 1 deletions(-) diff --git a/doc/voicecall-api.txt b/doc/voicecall-api.txt index f0ba316..60c692c 100644 --- a/doc/voicecall-api.txt +++ b/doc/voicecall-api.txt @@ -125,3 +125,7 @@ Propertiesstring LineIdentification [readonly] Icon identifier to be used instead of or together with the text information. + + boolean EmergencyCall [readonly] + + Contains the indication if the voice call is emergency call or not. do we really wanna call this EmergencyCall instead of just Emergency? In theory the call is already in the interface name. Regards Marcel ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: [PATCH] voicecall: Add EmergencyCall property.
Hi Marcel, Will change the property name to Emergency and resend the patch. Regards, John -Original Message- From: ofono-boun...@ofono.org [mailto:ofono-boun...@ofono.org] On Behalf Of Marcel Holtmann Sent: 11 November 2010 17:15 To: ofono@ofono.org Subject: Re: [PATCH] voicecall: Add EmergencyCall property. Hi John, doc/voicecall-api.txt |4 src/voicecall.c | 34 +- 2 files changed, 37 insertions(+), 1 deletions(-) diff --git a/doc/voicecall-api.txt b/doc/voicecall-api.txt index f0ba316..60c692c 100644 --- a/doc/voicecall-api.txt +++ b/doc/voicecall-api.txt @@ -125,3 +125,7 @@ Propertiesstring LineIdentification [readonly] Icon identifier to be used instead of or together with the text information. + + boolean EmergencyCall [readonly] + + Contains the indication if the voice call is emergency call or not. do we really wanna call this EmergencyCall instead of just Emergency? In theory the call is already in the interface name. 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 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: [PATCH] voicecall: Add EmergencyCall property.
Hi John,
+static gboolean voicecall_isemergency(struct voicecall *v)
Please use voicecall_is_emergency
+{
+ struct ofono_call *call = v-call;
+ const char *lineid_str;
+
+ lineid_str = phone_number_to_string(call-phone_number);
+
+ if (g_slist_find_custom(v-vc-en_list, lineid_str,
+ (GCompareFunc) strcmp))
As a general rule, the only function casting we allow is for GFunc use
of g_free. I really prefer that you write a separate function here.
+ return TRUE;
+ else
Please drop the else, it isn't actually required and makes the code
slightly cleaner.
+ return FALSE;
+}
+
static void append_voicecall_properties(struct voicecall *v,
DBusMessageIter *dict)
{
@@ -322,7 +336,8 @@ static void append_voicecall_properties(struct voicecall
*v,
const char *status;
const char *callerid;
const char *timestr;
- ofono_bool_t mpty;
+ ofono_bool_t mpt;
Is there a reason you're changing this variable name?
+ dbus_bool_t emergency_call;
status = call_status_to_string(call-status);
callerid = phone_number_to_string(call-phone_number);
@@ -358,6 +373,14 @@ static void append_voicecall_properties(struct voicecall
*v,
if (v-icon_id)
ofono_dbus_dict_append(dict, Icon, DBUS_TYPE_BYTE,
v-icon_id);
+
+ if (voicecall_isemergency(v))
+ emergency_call = TRUE;
+ else
+ emergency_call = FALSE;
+
+ ofono_dbus_dict_append(dict, EmergencyCall, DBUS_TYPE_BOOLEAN,
+ emergency_call);
}
static DBusMessage *voicecall_get_properties(DBusConnection *conn,
@@ -722,6 +745,15 @@ static void voicecall_set_call_lineid(struct voicecall
*v,
OFONO_VOICECALL_INTERFACE,
LineIdentification,
DBUS_TYPE_STRING, lineid_str);
+
+ if (voicecall_isemergency(v)) {
+ dbus_bool_t emergency_call = TRUE;
In general we prefer an empty line after every variable declaration block.
+ ofono_dbus_signal_property_changed(conn, path,
+ OFONO_VOICECALL_INTERFACE,
+ EmergencyCall,
+ DBUS_TYPE_BOOLEAN,
+ emergency_call);
+ }
}
static gboolean voicecall_dbus_register(struct voicecall *v)
Regards,
-Denis
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
Re: [PATCH 4/4] isigen: make number of PDP contexts configurable
On 11/11/2010 02:16 AM, mika.liljeb...@nokia.com wrote: 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. The gprs context structure is about the size of 5 pointers, so there's not much to be saved here. If you're worried about space usage of the isi specific data, you can always allocate it during the activation stage. 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. Hah, I know some people who vehemently disagree ;) Of course I'm not one of them... Strictly speaking what you say is possible even with today's architecture, however almost no AT modem we have supports more than about 3-4 contexts. So doing this to save 300-400 bytes / context is simply not worth it at this point. Regards, -Denis ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
isigen: create four gprs contexts
Newlines added. Br, MikaL [PATCH 1/1] isigen: create four gprs contexts plugins/isigen.c | 19 +++ 1 files changed, 15 insertions(+), 4 deletions(-) ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
[PATCH 1/1] isigen: create four gprs contexts
---
plugins/isigen.c | 19 +++
1 files changed, 15 insertions(+), 4 deletions(-)
diff --git a/plugins/isigen.c b/plugins/isigen.c
index 838d060..fad4e20 100644
--- a/plugins/isigen.c
+++ b/plugins/isigen.c
@@ -58,6 +58,8 @@
#include drivers/isimodem/mtc.h
#include drivers/isimodem/debug.h
+#define ISI_DEFAULT_PDPS 4 /* Number of supported PDP contexts */
+
struct isi_data {
struct ofono_modem *modem;
char const *ifname;
@@ -407,6 +409,7 @@ static void isigen_post_online(struct ofono_modem *modem)
struct isi_data *isi = ofono_modem_get_data(modem);
struct ofono_gprs *gprs;
struct ofono_gprs_context *gc;
+ int i;
DBG((%p) with %s, modem, isi-ifname);
@@ -420,13 +423,21 @@ static void isigen_post_online(struct ofono_modem *modem)
ofono_call_barring_create(isi-modem, 0, isimodem, isi-idx);
ofono_call_meter_create(isi-modem, 0, isimodem, isi-idx);
ofono_radio_settings_create(isi-modem, 0, isimodem, isi-idx);
+
gprs = ofono_gprs_create(isi-modem, 0, isimodem, isi-idx);
- gc = ofono_gprs_context_create(isi-modem, 0, isimodem, isi-idx);
+ if (!gprs)
+ return;
+
+ for (i = 0; i ISI_DEFAULT_PDPS; i++) {
+ gc = ofono_gprs_context_create(isi-modem, 0,
+ isimodem, isi-idx);
+ if (!gc) {
+ DBG(Failed to add context %d, i);
+ break;
+ }
- if (gprs gc)
ofono_gprs_add_context(gprs, gc);
- else
- DBG(Failed to add context);
+ }
}
static int isigen_enable(struct ofono_modem *modem)
--
1.7.0.4
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
Re: [RfC] SIM file watch support.
Hi Andrew, On 11/05/2010 07:13 AM, Andrzej Zaborowski wrote: In order to support the Refresh STK command we need to implement at least two types of reset: UICC reset (manufacturer specific) and NAA application reset. Other types can fall back to application reset which can be implemented by removing all atoms and reinitialising them. When we get a Refresh with File Change Notification we should first check if we're even interested in the file. We check if the file is being watched by any atom and whether the atom can deal with the change dynamically. If not, we fall back to NAA application reset. The proposed api lets the users of sim_fs_read register to notifications of file change by adding a watch. In the simplest case they can add a null watch for a file ID that they care about (callback address is NULL), to indicate that the file is important to us, but we haven't implemented a way to deal with the contents change dynamically, or we can not. (Maybe all files being read should automatically become watched, but some files might be read only on some user action, in that case we don't need to watch them) stk.c will check if any of the file IDs supplied in the command have any watches on them. If there are only watches with non-NULL callback, it'll call all of them and not reset application. If there are any NULL notifiers, then stk.c will fall back to full application reset. If any of the files were cached, they need to be re-read. So the last paragraph is confusing me a bit. What is meant by a full application reset? Do you mean simulating a sim removal and re-insertion as per a UICC reset? --- src/simfs.h | 12 src/sim.c | 21 - 2 files changed, 32 insertions(+), 1 deletions(-) diff --git a/src/simfs.h b/src/simfs.h index ef962db..679955a 100644 --- a/src/simfs.h +++ b/src/simfs.h @@ -25,6 +25,8 @@ typedef void (*sim_fs_read_info_cb_t)(int ok, unsigned char file_status, int total_length, int record_length, void *userdata); +typedef void (*sim_fs_ef_notify_t)(int id, void *userdata); + struct sim_fs *sim_fs_new(struct ofono_sim *sim, const struct ofono_sim_driver *driver); @@ -48,3 +50,13 @@ char *sim_fs_get_cached_image(struct sim_fs *fs, int id); void sim_fs_cache_image(struct sim_fs *fs, const char *image, int id); void sim_fs_free(struct sim_fs *fs); + +int sim_fs_add_ef_watch(struct sim_fs *fs, int id, + sim_fs_ef_notify_t *notify, void *userdata); + +int sim_fs_remove_ef_watch(struct sim_fs *fs, int id, + sim_fs_ef_notify_t *notify, void *userdata); + +ofono_bool_t sim_fs_has_empty_watches(struct sim_fs *fs, int id); Shouldn't we be adding the watch functionality to the STK atom? I think logically it belongs there. Otherwise the API seems fine to me. + +int sim_fs_notify_file_change(struct sim_fs *fs, int id); This actually sounds fine. Do you think this function needs to also peek inside the currently running queue and terminate the reading of files that are changed? Regards, -Denis ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: [PATCH v4 1/2] stemodem: Add RTNL functionality managing CAIF Network Interfaces.
Hi Marcel.
what happens for network triggered deactivation. Some networks here
disconnect the GPRS context used MMS. Has some funny behavior that need
to be taken into account.
Good question! When code reading with this in mind I realize that
we missed to mark the conn_info-interface as available by setting
cid = 0. This is bug, thank you for leading me to it.
FYI this is how CAIF and AT works together:
In order to have payload over CAIF there need to be
both a CAIF channel connected and a context activated with AT*EPPSD.
When Channel-ID configured for CAIF IP Interface and the Channel-Id given
in AT*EPPSD matches, the modem side will connect the CAIF Channel to
the Network Signaling Stack in the modem.
With this new patch CAIF channels are created statically from probe.
Activation and deactivation is controlled by AT*EPPSD, or notified by
+CGEV which will result in a CGACT status query.
If the network disconnects GPRS we should receive a
+CGEV: NW DEACT, subsequent CGACT status query.
Here the connection (and CAIF Network Interface) should be
marked as unused by setting cid to zero.
+#define NLMSG_TAIL(nmsg) \
+ ((struct rtattr *) (((void *) (nmsg)) + NLMSG_ALIGN((nmsg)-
nlmsg_len)))
We have no global define this? You wanted to look into that. What was
the outcome of it?
I did mention this before - I have looked around, but he did not find
any relevant macro for this unfortunately.
+struct iplink_req {
+ struct nlmsghdr n;
+ struct ifinfomsg i;
+ char pad[1024];
What is this huge pad for? It looks totally fishy to me.
The pad is there to hold the remaining of the rtnl attributes.
I'm working on restructuring this so that I can separate the
buffer holding the rtnl_message and the iplink_req.
I just have to make it work...
+static guint rtnl_watch;
Get rid of the double spaces for rtnl_watch.
+static GIOChannel *channel;
If we follow your convention here and not to overload variables this
might be better named rtnl_channel.
And yes, I am fine with keeping it since there is no way to connect a
netlink socket properly to just use send(). You will require to use
sendto for everything.
OK, Done.
+static void store_newlink_param(struct iplink_req *req, unsigned
short type,
+ int index, unsigned flags,
+ unsigned change, struct ifinfomsg *msg,
+ int bytes)
+{
+ const char *ifname = NULL;
+
+ get_ifname(msg, bytes, ifname);
+ strncpy(req-ifname, ifname,
+ sizeof(req-ifname));
+ req-ifname[sizeof(req-ifname)-1] = '\0';
+ req-ifindex = index;
+}
So I think that store_newlink_... and get_ifname can be nicely combined
into one helper function. And using extract_parameters() function name
is a bit better.
And please only add parameters to that function that you really need in
there. And the iplink_req should be last parameter.
As one general is to have input parameters first and then the output
parameters last. So read this something like extract values from this
structure to this structure.
Yes, I agree. I have squashed the two functions together and cleaned up the
parameters.
This is much nicer, thank you.
+static int send_rtnl_req(struct iplink_req *req)
+{
+ struct sockaddr_nl addr;
+ int sk = g_io_channel_unix_get_fd(channel);
+
+ memset(addr, 0, sizeof(addr));
+ addr.nl_family = AF_NETLINK;
+
+ return sendto(sk, req, req-n.nlmsg_len, 0,
+ (struct sockaddr *) addr, sizeof(addr));
+}
I don't think this should be a separate function. You use it only twice
and having it close to the code using it would be better.
OK, good I have squashed these to functions as well.
+static struct iplink_req *find_request(guint32 seq)
+{
+ GSList *list;
+
+ for (list = pending_requests; list; list = list-next) {
+ struct iplink_req *req = list-data;
+
+ if (req-n.nlmsg_seq == seq)
+ return req;
+ }
+
+ return NULL;
+}
I would move this function up in this file at the top. It should go
close to the variable declaration for pending_request.
OK, done.
+static void rtnl_client_response(struct iplink_req *req, int result)
+{
+
+ if (req-callback req-n.nlmsg_type == RTM_NEWLINK)
+ req-callback(result, req-user_data,
+ req-ifname, req-ifindex);
+
+ pending_requests = g_slist_remove(pending_requests, req);
+ g_free(req);
+}
I don't like this split out. You already checked the nlmsg_type and
here
you keep checking it again just to reuse some code. This looks like
waste to me.
I have squashed this into parse_rtnl_message, as you suggested.
I'm not really convinced this was any cleaner though..?
I end up with duplicating the code in the two if statements.
+static void parse_rtnl_message(void *buf, size_t len)
Re: [PATCH 1/1] isigen: create four gprs contexts
Hi Mika, On 11/11/2010 09:50 AM, Mika Liljeberg wrote: --- plugins/isigen.c | 19 +++ 1 files changed, 15 insertions(+), 4 deletions(-) Patch has been applied, thanks. Regards, -Denis ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
Re: PPP disconnected via Huawei modem
Hi Elton, On 11/11/2010 01:14 AM, Chen Yuwei wrote: Hi, I use MeeGo NB 10/26 release to browse some website via Huawei modem, The PPP connection is disconnected when recieve a PPP event (RXJ-). Then the PPP can not be connected anymore. Why? What condition does ofono recieve this event? We won't really know unless we have a detailed protocol trace. Have you tried running gatchat/gsmdial to reproduce this problem? That application has the ability to dump a detailed trace of the ppp packets. See if you can reproduce the issue with gsmdial and include the ppp dump file. Regards, -Denis ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: [PATCH] voicecall: Add EmergencyCall property.
Hi John, please do not top post on this mailing list. I am really serious with the fact that I am going to ignore people from now on that do that. Will change the property name to Emergency and resend the patch. This was a question open for discussion. What would be the appropriate property name? Regards Marcel ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: [PATCH] voicecall: Add EmergencyCall property.
Hi Denis,
+static gboolean voicecall_isemergency(struct voicecall *v)
Please use voicecall_is_emergency
Will change
+{
+ struct ofono_call *call = v-call;
+ const char *lineid_str;
+
+ lineid_str = phone_number_to_string(call-phone_number);
+
+ if (g_slist_find_custom(v-vc-en_list, lineid_str,
+ (GCompareFunc) strcmp))
As a general rule, the only function casting we allow is for GFunc use of
g_free. I really prefer that you write a separate function here.
I had two options, to write a function comparing the string, or to use strcmp.
A search in ofono source showed me 2 instances where strcmp is used and I
followed it here too.
I can write a function to compare the phone number, and will make a patch.
+ return TRUE;
+ else
Please drop the else, it isn't actually required and makes the code slightly
cleaner.
Agreed, will make the change.
+ return FALSE;
+}
+
static void append_voicecall_properties(struct voicecall *v,
DBusMessageIter *dict)
{
@@ -322,7 +336,8 @@ static void append_voicecall_properties(struct voicecall
*v,
const char *status;
const char *callerid;
const char *timestr;
- ofono_bool_t mpty;
+ ofono_bool_t mpt;
Is there a reason you're changing this variable name?
Will recity this, came in as typo mistake.
+ dbus_bool_t emergency_call;
status = call_status_to_string(call-status);
callerid = phone_number_to_string(call-phone_number);
@@ -358,6 +373,14 @@ static void append_voicecall_properties(struct voicecall
*v,
if (v-icon_id)
ofono_dbus_dict_append(dict, Icon, DBUS_TYPE_BYTE,
v-icon_id);
+
+ if (voicecall_isemergency(v))
+ emergency_call = TRUE;
+ else
+ emergency_call = FALSE;
+
+ ofono_dbus_dict_append(dict, EmergencyCall, DBUS_TYPE_BOOLEAN,
+ emergency_call);
}
static DBusMessage *voicecall_get_properties(DBusConnection *conn, @@
-722,6 +745,15 @@ static void voicecall_set_call_lineid(struct voicecall *v,
OFONO_VOICECALL_INTERFACE,
LineIdentification,
DBUS_TYPE_STRING, lineid_str);
+
+ if (voicecall_isemergency(v)) {
+ dbus_bool_t emergency_call = TRUE;
In general we prefer an empty line after every variable declaration block.
Sure will change
+ ofono_dbus_signal_property_changed(conn, path,
+ OFONO_VOICECALL_INTERFACE,
+ EmergencyCall,
+ DBUS_TYPE_BOOLEAN,
+ emergency_call);
+ }
}
static gboolean voicecall_dbus_register(struct voicecall *v)
Regards,
John
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
RE: [PATCH v4 1/2] stemodem: Add RTNL functionality managing CAIF Network Interfaces.
Hi Sjur,
what happens for network triggered deactivation. Some networks here
disconnect the GPRS context used MMS. Has some funny behavior that need
to be taken into account.
Good question! When code reading with this in mind I realize that
we missed to mark the conn_info-interface as available by setting
cid = 0. This is bug, thank you for leading me to it.
fall into a similar trap with GPRS support for IFX.
FYI this is how CAIF and AT works together:
In order to have payload over CAIF there need to be
both a CAIF channel connected and a context activated with AT*EPPSD.
When Channel-ID configured for CAIF IP Interface and the Channel-Id given
in AT*EPPSD matches, the modem side will connect the CAIF Channel to
the Network Signaling Stack in the modem.
With this new patch CAIF channels are created statically from probe.
Activation and deactivation is controlled by AT*EPPSD, or notified by
+CGEV which will result in a CGACT status query.
If the network disconnects GPRS we should receive a
+CGEV: NW DEACT, subsequent CGACT status query.
Here the connection (and CAIF Network Interface) should be
marked as unused by setting cid to zero.
Sounds good to me.
Does anything cleans up the channel when oFono unexpectedly crashes?
+#define NLMSG_TAIL(nmsg) \
+ ((struct rtattr *) (((void *) (nmsg)) + NLMSG_ALIGN((nmsg)-
nlmsg_len)))
We have no global define this? You wanted to look into that. What was
the outcome of it?
I did mention this before - I have looked around, but he did not find
any relevant macro for this unfortunately.
I might just overread your response. Just wanted to make sure.
+struct iplink_req {
+ struct nlmsghdr n;
+ struct ifinfomsg i;
+ char pad[1024];
What is this huge pad for? It looks totally fishy to me.
The pad is there to hold the remaining of the rtnl attributes.
I'm working on restructuring this so that I can separate the
buffer holding the rtnl_message and the iplink_req.
I just have to make it work...
I think you can just use a stack buffer when you actually send the
message. This is damn ugly and from a security point it actually scares
me.
Please only keep the seqnum in here. Since that is the only thing you
need to find that pending request. Just get rid of everything that you
don't need for the callback.
+static void rtnl_client_response(struct iplink_req *req, int result)
+{
+
+ if (req-callback req-n.nlmsg_type == RTM_NEWLINK)
+ req-callback(result, req-user_data,
+ req-ifname, req-ifindex);
+
+ pending_requests = g_slist_remove(pending_requests, req);
+ g_free(req);
+}
I don't like this split out. You already checked the nlmsg_type and
here
you keep checking it again just to reuse some code. This looks like
waste to me.
I have squashed this into parse_rtnl_message, as you suggested.
I'm not really convinced this was any cleaner though..?
I end up with duplicating the code in the two if statements.
We will see. I might have been wrong, but in my mind it just locked
fine. I will take a second look when you send the updated patch.
An you can have more than one RTNL message inside this buffer. You need
to be able to handle this. Otherwise you might loose responses. The
case
of just calling return when you received one of the two messages you
care about is not really acceptable.
OK, done. I'll keep looping.
+ while (len 0) {
+ struct nlmsghdr *hdr = buf;
+
+ if (!NLMSG_OK(hdr, len))
+ break;
+
+ if (hdr-nlmsg_type == RTM_NEWLINK) {
+ req = g_slist_nth_data(pending_requests, 0);
+ if (req == NULL)
+ break;
How does this work? You just pick the first pending request and don't
really compare the sequence number. Who guarantees the order of these
events. I don't think we should be doing this.
I think that the kernel implementation of rtnl will guarantee the
requests to be processed in order. rtnetlink_rcv in ..net/core/rtnetlink.c
takes the rtnl_lock before processing the netlink messages.
I believe this will guarantee that the NEWLINK responses will be received
in the same order as they are sent.
it might be keeping the order, but I wouldn't count on it. And just
using your find request function would make this code simpler.
+ msg = (struct ifinfomsg *) NLMSG_DATA(hdr);
+ store_newlink_param(req, msg-ifi_type,
+ msg-ifi_index, msg-ifi_flags,
+ msg-ifi_change, msg,
+ IFA_PAYLOAD(hdr));
+ rtnl_client_response(req, 0);
+ return;
+
+ } else if (hdr-nlmsg_type == NLMSG_ERROR) {
So I would prefer if you use a switch statement here. It
