Re: [PATCHv5] plugin: Add ste modem initd integration
Hi Marcel.
+static void get_modems()
+{
so these are declared as get_modems(void) btw. The compiler should have
warned you about this.
I don't get a compiler warning for this one, not even with -Wall
-Wextra -Wpedantic on GCC 4.4.3.
But a new rule in the coding-style should perhaps be added, e.g:
M15: Use void if function has no parameters
A function with no parameters must use void in the parameter list.
Example:
1)
void foo(void)
{
}
2)
void foo() // Wrong
{
}
...
+static void mgr_disconnect(DBusConnection *connection, void *user_data)
+{
+ g_hash_table_remove_all(modem_list);
+ g_dbus_remove_watch(connection, property_changed_watch);
+}
+
+static void stemgr_exit()
+{
+ g_hash_table_destroy(modem_list);
+ g_dbus_remove_watch(connection, modem_daemon_watch);
+ g_dbus_remove_watch(connection, property_changed_watch);
This is not right. You need to remove the property_changed_watch in the
disconnect callback. Otherwise if your daemon restarts twice you
actually have two watches.
It's already present in mgr_disconnect, did you miss that?
In the previous review 2011/1/5 you said:
+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).
Anyway, I think the watch handing in the latest patch is correct,
we need to do remove_watch for property_changed_watch
in both stemgr_exit and in stemgr_disconnect.
Regards,
Sjur
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
Re: [PATCHv5] plugin: Add ste modem initd integration
Hi Sjur,
so these are declared as get_modems(void) btw. The compiler should have
warned you about this.
I don't get a compiler warning for this one, not even with -Wall
-Wextra -Wpedantic on GCC 4.4.3.
But a new rule in the coding-style should perhaps be added, e.g:
M15: Use void if function has no parameters
A function with no parameters must use void in the parameter list.
Example:
1)
void foo(void)
{
}
2)
void foo()// Wrong
{
}
please go ahead and send a patch for this. I am happily adding it.
...
+static void mgr_disconnect(DBusConnection *connection, void *user_data)
+{
+ g_hash_table_remove_all(modem_list);
+ g_dbus_remove_watch(connection, property_changed_watch);
+}
+
+static void stemgr_exit()
+{
+ g_hash_table_destroy(modem_list);
+ g_dbus_remove_watch(connection, modem_daemon_watch);
+ g_dbus_remove_watch(connection, property_changed_watch);
This is not right. You need to remove the property_changed_watch in the
disconnect callback. Otherwise if your daemon restarts twice you
actually have two watches.
It's already present in mgr_disconnect, did you miss that?
I did miss that. So you are correct, but you should add
property_changed_watch = 0
to mgr_disconnect.
And then do this in stemgr_exit
if (property_changed_watch 0)
g_dbus_remove_watch(...
In the previous review 2011/1/5 you said:
+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).
Anyway, I think the watch handing in the latest patch is correct,
we need to do remove_watch for property_changed_watch
in both stemgr_exit and in stemgr_disconnect.
I got lost a little bit, but you still need to make sure to not remove a
watch on plugin exit in case modem init manager not running as I
mentioned above.
Regards
Marcel
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
Re: [PATCHv5] plugin: Add ste modem initd integration
Hi Marcel,
On 01/11/2011 03:52 PM, Marcel Holtmann wrote:
Hi Sjur,
so these are declared as get_modems(void) btw. The compiler should have
warned you about this.
I don't get a compiler warning for this one, not even with -Wall
-Wextra -Wpedantic on GCC 4.4.3.
But a new rule in the coding-style should perhaps be added, e.g:
M15: Use void if function has no parameters
A function with no parameters must use void in the parameter list.
Example:
1)
void foo(void)
{
}
2)
void foo() // Wrong
{
}
please go ahead and send a patch for this. I am happily adding it.
Sorry I'm not following why this is a good idea?
Regards,
-Denis
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
Re: [PATCHv5] plugin: Add ste modem initd integration
Hi Denis,
so these are declared as get_modems(void) btw. The compiler should have
warned you about this.
I don't get a compiler warning for this one, not even with -Wall
-Wextra -Wpedantic on GCC 4.4.3.
But a new rule in the coding-style should perhaps be added, e.g:
M15: Use void if function has no parameters
A function with no parameters must use void in the parameter list.
Example:
1)
void foo(void)
{
}
2)
void foo() // Wrong
{
}
please go ahead and send a patch for this. I am happily adding it.
Sorry I'm not following why this is a good idea?
we have always declared functions with no parameters as (void) and not
as (). At least I have.
Regards
Marcel
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
[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
