9:55 p.m.
Hello
Here's a revised way of changing phonesim modem configurations at
runtime. It works by adding a dbus interface via which you can add new
modems, delete all existing modems or reset modem status to what it
was at startup. This first patch adds the dbus bits and the second one
calls the corresponding ofono functions.
This patch set is not fully polished yet but should give an idea what
we are aiming for. I'm not fully versed in phonesim's internals so I'm
not sure if I'm poking all the right bits in the dbus functions, so
please verify that with extra care.
Thanks,
---
plugins/phonesim.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 58 insertions(+)
diff --git a/plugins/phonesim.c b/plugins/phonesim.c
index 5561a05..12cd591 100644
--- a/plugins/phonesim.c
+++ b/plugins/phonesim.c
@@ -35,6 +35,7 @@
#include <glib.h>
#include <gatmux.h>
#include <gatchat.h>
+#include <gdbus.h>
#define OFONO_API_SUBJECT_TO_CHANGE
#include <ofono/plugin.h>
@@ -73,6 +74,8 @@ static const char *none_prefix[] = { NULL };
static const char *ptty_prefix[] = { "+PTTY:", NULL };
static const char *simstate_prefix[] = { "+SIMSTATE:", NULL };
static int next_iface = 0;
+static const char CONTROL_PATH[] = "/phonesim";
+static const char CONTROL_INTERFACE[] = "org.ofono.phonesim.Control";
struct phonesim_data {
GAtMux *mux;
@@ -1068,6 +1071,55 @@ done:
g_key_file_free(keyfile);
}
+static DBusMessage *control_add(DBusConnection *conn,
+ DBusMessage *msg, void *data)
+{
+ return NULL;
+}
+
+static DBusMessage *control_remove_all(DBusConnection *conn,
+ DBusMessage *msg, void *data)
+{
+ return NULL;
+}
+
+static DBusMessage *control_reset(DBusConnection *conn,
+ DBusMessage *msg, void *data)
+{
+ return NULL;
+}
+
+static const GDBusMethodTable control_methods[] = {
+ { GDBUS_METHOD("Add",
+ NULL, GDBUS_ARGS({ "properties", "sss" }),
+ control_add) },
+ { GDBUS_METHOD("RemoveAll",
+ NULL, GDBUS_ARGS({ "properties", "" }),
+ control_remove_all) },
+ { GDBUS_METHOD("Reset",
+ NULL, GDBUS_ARGS({ "properties", "" }),
+ control_reset) },
+ {}
+};
+
+static int setup_control_channel() {
+ int err = 0;
+ DBusConnection *conn = ofono_dbus_get_connection();
+ void *user_data = NULL;
+ g_dbus_register_interface(conn,
+ CONTROL_PATH, CONTROL_INTERFACE,
+ control_methods,
+ NULL,
+ NULL,
+ user_data,
+ NULL);
+ return err;
+}
+
+static void shutdown_control_channel() {
+ g_dbus_unregister_interface(ofono_dbus_get_connection(), CONTROL_PATH,
CONTROL_INTERFACE);
+}
+
static int phonesim_init(void)
{
int err;
@@ -1087,6 +1139,10 @@ static int phonesim_init(void)
else
parse_config(CONFIGDIR "/phonesim.conf");
+ err = setup_control_channel();
+ if (err < 0)
+ return err;
+
return 0;
}
@@ -1094,6 +1150,8 @@ static void phonesim_exit(void)
{
GSList *list;
+ shutdown_control_channel();
+
for (list = modem_list; list; list = list->next) {
struct ofono_modem *modem = list->data;
--
1.9.1
9:55 p.m.
New subject: [PATCH 2/2] Basic implementation of dbus methods.
From: Jussi Pakkanen <jussi.pakkanen(a)canonical.com>
---
plugins/phonesim.c | 89 ++++++++++++++++++++++++++++++++++++++++++------------
1 file changed, 70 insertions(+), 19 deletions(-)
diff --git a/plugins/phonesim.c b/plugins/phonesim.c
index 12cd591..61f15c9 100644
--- a/plugins/phonesim.c
+++ b/plugins/phonesim.c
@@ -1039,6 +1039,14 @@ static void parse_config(const char *filename)
char **modems;
int i;
+ if (filename == NULL) {
+ char *conf_override = getenv("OFONO_PHONESIM_CONFIG");
+ if (conf_override)
+ filename = conf_override;
+ else
+ filename = CONFIGDIR "/phonesim.conf";
+ }
+
DBG("filename %s", filename);
keyfile = g_key_file_new();
@@ -1071,33 +1079,87 @@ done:
g_key_file_free(keyfile);
}
+static void release_modems() {
+ GSList *list;
+
+ for (list = modem_list; list; list = list->next) {
+ struct ofono_modem *modem = list->data;
+
+ ofono_modem_remove(modem);
+ }
+
+ g_slist_free(modem_list);
+ modem_list = NULL;
+}
+
static DBusMessage *control_add(DBusConnection *conn,
DBusMessage *msg, void *data)
{
- return NULL;
+ const char *driver = "phonesim";
+ struct ofono_modem *modem;
+ DBusMessageIter iter;
+ char *name;
+ char *address;
+ char *port;
+
+ if (!dbus_message_iter_init(msg, &iter))
+ return __ofono_error_invalid_args(msg);
+
+ if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_STRING)
+ return __ofono_error_invalid_args(msg);
+
+ dbus_message_iter_get_basic(&iter, &name);
+ dbus_message_iter_next(&iter);
+
+ if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_STRING)
+ return __ofono_error_invalid_args(msg);
+
+ dbus_message_iter_get_basic(&iter, &address);
+ dbus_message_iter_next(&iter);
+
+ if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_STRING)
+ return __ofono_error_invalid_args(msg);
+
+ dbus_message_iter_get_basic(&iter, &port);
+
+
+ modem = ofono_modem_create(name, driver);
+ if (modem == NULL)
+ return NULL;
+
+ ofono_modem_set_string(modem, "Address", address);
+ ofono_modem_set_integer(modem, "Port", atoi(port));
+ modem_list = g_slist_prepend(modem_list, modem);
+
+ return dbus_message_new_method_return(msg);
}
static DBusMessage *control_remove_all(DBusConnection *conn,
DBusMessage *msg, void *data)
{
- return NULL;
+ release_modems();
+
+ return dbus_message_new_method_return(msg);
}
static DBusMessage *control_reset(DBusConnection *conn,
DBusMessage *msg, void *data)
{
- return NULL;
+ release_modems();
+ parse_config(NULL);
+
+ return dbus_message_new_method_return(msg);
}
static const GDBusMethodTable control_methods[] = {
{ GDBUS_METHOD("Add",
- NULL, GDBUS_ARGS({ "properties", "sss" }),
+ NULL, GDBUS_ARGS({ "name", "s" }, {"address",
"s"}, {"port", "s"}),
control_add) },
{ GDBUS_METHOD("RemoveAll",
- NULL, GDBUS_ARGS({ "properties", "" }),
+ NULL, GDBUS_ARGS({ }),
control_remove_all) },
{ GDBUS_METHOD("Reset",
- NULL, GDBUS_ARGS({ "properties", "" }),
+ NULL, GDBUS_ARGS({ }),
control_reset) },
{}
};
@@ -1123,7 +1185,6 @@ static void shutdown_control_channel() {
static int phonesim_init(void)
{
int err;
- char *conf_override = getenv("OFONO_PHONESIM_CONFIG");
err = ofono_modem_driver_register(&phonesim_driver);
if (err < 0)
@@ -1134,10 +1195,7 @@ static int phonesim_init(void)
ofono_gprs_context_driver_register(&context_driver);
ofono_ctm_driver_register(&ctm_driver);
- if (conf_override)
- parse_config(conf_override);
- else
- parse_config(CONFIGDIR "/phonesim.conf");
+ parse_config(NULL);
err = setup_control_channel();
if (err < 0)
@@ -1148,17 +1206,10 @@ static int phonesim_init(void)
static void phonesim_exit(void)
{
- GSList *list;
-
shutdown_control_channel();
- for (list = modem_list; list; list = list->next) {
- struct ofono_modem *modem = list->data;
-
- ofono_modem_remove(modem);
- }
+ release_modems();
- g_slist_free(modem_list);
modem_list = NULL;
ofono_ctm_driver_unregister(&ctm_driver);
--
1.9.1
12:02 a.m.
Hi Jussi,
On 04/07/2014 06:55 AM, jussi.pakkanen(a)canonical.com wrote:
Hello
Here's a revised way of changing phonesim modem configurations at
runtime. It works by adding a dbus interface via which you can add new
modems, delete all existing modems or reset modem status to what it
was at startup. This first patch adds the dbus bits and the second one
calls the corresponding ofono functions.
This patch set is not fully polished yet but should give an idea what
we are aiming for. I'm not fully versed in phonesim's internals so I'm
not sure if I'm poking all the right bits in the dbus functions, so
please verify that with extra care.
Thanks,
---
plugins/phonesim.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 58 insertions(+)
As I've said before, this is overkill. It makes the phonesim plugin way
more complicated that it needs to be. A .conf parser with over-ridable
directories, a control channel with add/remove/reset? Seriously?
UI should be designed to ignore non-powered modems, and only look for
interfaces that it handles specifically. So for a properly designed UI,
none of these changes are required. If you want to handle DualSim
cleanly, then feel free to propose a mechanism / hint that makes
applications aware of the fact that two modems belong to a group.
If the above cannot be done, then please write a plugin to address your
specific use case.
Regards,
-Denis
6:34 a.m.
On 07.04.2014 17:02, Denis Kenzior wrote:
As I've said before, this is overkill. It makes the phonesim
plugin way
more complicated that it needs to be. A .conf parser with over-ridable
directories, a control channel with add/remove/reset? Seriously?
There are clear real world use cases the proposed change.
(As complete reference for the readers the mailing list, please, see the
thread "[PATCH 1/2] Allow users to specify dbus name replacement
behaviour.").
Let me summarize.
$OFONO_PHONESIM_CONFIG:
Allows tests to run unpriviledged ofono instances with the
phonesim-plugin on top of private system bus without needing write
access to /etc/ofono/phonesim.conf in order to change the number of modems.
D-Bus control channel:
Allows changing the number of modems on a single system level ofono
instance.
Both are usable with on their own and allow flexibility for testers to
choose the testing strategy based on their needs (private system bus vs.
actual system bus, multiple .conf files vs. dbus configuration on the
fly). Unit testing API bindings or testing middleware can use the
private system bus approach and system or user story testing of a
complete software stack such as a mobile phone user interface would use
the actual system bus.
Having the dbus interface also allows automated test suite to set up
number of ofono modems based on the environment specification of
individual tests.
Also not mentioned before, the dbus interface allows to test
Manager::modemAdded() and Modem::modemRemoved() logic in the unit under
test.
UI should be designed to ignore non-powered modems, and only look
for
interfaces that it handles specifically. So for a properly designed UI,
none of these changes are required. If you want to handle DualSim
cleanly, then feel free to propose a mechanism / hint that makes
applications aware of the fact that two modems belong to a group.
A system with two modems where one modem being powered off is still a
system with two modems. One modem simply being powered off does not make
the system a single-modem one.
In my opinion the cleanest way to implement Multi-SIM is to have ofono
reporting a correct number of modems.
UI should ignore non-powered modem only if the UI design for the
particular component mandates so. The UI design could also mandate that
if a modem is powered off, or has some other problem which makes it
unusable, the modem should be shown as disabled. Or something else.
My point being:
A) Having to implement testing-only code paths to components makes
testing much more complicated and potentially erroneous as the
code paths being tested differ between testing and production.
B) oFono should not impose arbitrary limits to UI/UX design.
If the above cannot be done, then please write a plugin to address
your
specific use case.
I hope these use cases show how these patches are useful, allow more
flexible testing strategies and don't change the fundamental purpose of
the phonesim-plugin.
All in less than 150 lines of code.
I would kindly ask you to reconsider.
-- Antti
7:40 a.m.
Hi Antti,
<snip>
There are clear real world use cases the proposed change.
Nope, these are not 'real world' use cases. These are constraints for a
synthetic unit test framework you are trying to create.
Both are usable with on their own and allow flexibility for testers
to
choose the testing strategy based on their needs (private system bus vs.
actual system bus, multiple .conf files vs. dbus configuration on the
fly). Unit testing API bindings or testing middleware can use the
private system bus approach and system or user story testing of a
complete software stack such as a mobile phone user interface would use
the actual system bus.
Having the dbus interface also allows automated test suite to set up
number of ofono modems based on the environment specification of
individual tests.
I don't buy any of these arguments. You have a specific case you want
to test due to the constraints of how your system is designed. Create a
plugin specific to your system; do not try to over-complicate the
phonesim plugin which is fine the way it is.
Also not mentioned before, the dbus interface allows to test
Manager::modemAdded() and Modem::modemRemoved() logic in the unit under
test.
Then make up a plugin to test that specifically if you really care.
> UI should be designed to ignore non-powered modems, and only look for
> interfaces that it handles specifically. So for a properly designed UI,
> none of these changes are required. If you want to handle DualSim
> cleanly, then feel free to propose a mechanism / hint that makes
> applications aware of the fact that two modems belong to a group.
A system with two modems where one modem being powered off is still a
system with two modems. One modem simply being powered off does not make
the system a single-modem one.
Again, that is a constraint of your particular system design. Relying
on one modem == single SIM and two modems == DualSIM is a bad idea. As
I mentioned before, introduce proper handling of Dual SIM devices and
none of these issues are relevant. Introducing features that are broken
by design will not fly.
In my opinion the cleanest way to implement Multi-SIM is to have ofono
reporting a correct number of modems.
And I disagree. If I want to plug in 100 Dual-SIM modems, I should have
the ability to do so. I do not want to rely on some magic conventions.
UI should ignore non-powered modem only if the UI design for the
particular component mandates so. The UI design could also mandate that
if a modem is powered off, or has some other problem which makes it
unusable, the modem should be shown as disabled. Or something else.
Again, that is your UI design constraint. We have plenty of situations
where oFono creates multiple modems that are not physically there (e.g.
Bluetooth HFP for example).
My point being:
A) Having to implement testing-only code paths to components makes
testing much more complicated and potentially erroneous as the
code paths being tested differ between testing and production.
You already have a testing-only code-path since you are using phonesim.
Phonesim is only barely usable as an approximation of real hardware.
Introducing a plugin specific to your system setup is not going to
change anything here.
B) oFono should not impose arbitrary limits to UI/UX design.
This has been discussed many times before. oFono is (and does) dictate
UI design to some extent.
Regards,
-Denis
10:24 p.m.
OK, thank you Denis for the feedback.
Let's consider the matter closed.
-- Antti
On 08.04.2014 00:40, Denis Kenzior wrote:
Hi Antti,
<snip>
> There are clear real world use cases the proposed change.
Nope, these are not 'real world' use cases. These are constraints for a
synthetic unit test framework you are trying to create.
> Both are usable with on their own and allow flexibility for testers to
> choose the testing strategy based on their needs (private system bus vs.
> actual system bus, multiple .conf files vs. dbus configuration on the
> fly). Unit testing API bindings or testing middleware can use the
> private system bus approach and system or user story testing of a
> complete software stack such as a mobile phone user interface would use
> the actual system bus.
>
> Having the dbus interface also allows automated test suite to set up
> number of ofono modems based on the environment specification of
> individual tests.
I don't buy any of these arguments. You have a specific case you want
to test due to the constraints of how your system is designed. Create a
plugin specific to your system; do not try to over-complicate the
phonesim plugin which is fine the way it is.
>
> Also not mentioned before, the dbus interface allows to test
> Manager::modemAdded() and Modem::modemRemoved() logic in the unit under
> test.
Then make up a plugin to test that specifically if you really care.
>
>
>> UI should be designed to ignore non-powered modems, and only look for
>> interfaces that it handles specifically. So for a properly designed UI,
>> none of these changes are required. If you want to handle DualSim
>> cleanly, then feel free to propose a mechanism / hint that makes
>> applications aware of the fact that two modems belong to a group.
>
> A system with two modems where one modem being powered off is still a
> system with two modems. One modem simply being powered off does not make
> the system a single-modem one.
Again, that is a constraint of your particular system design. Relying
on one modem == single SIM and two modems == DualSIM is a bad idea. As
I mentioned before, introduce proper handling of Dual SIM devices and
none of these issues are relevant. Introducing features that are broken
by design will not fly.
>
> In my opinion the cleanest way to implement Multi-SIM is to have ofono
> reporting a correct number of modems.
>
And I disagree. If I want to plug in 100 Dual-SIM modems, I should have
the ability to do so. I do not want to rely on some magic conventions.
> UI should ignore non-powered modem only if the UI design for the
> particular component mandates so. The UI design could also mandate that
> if a modem is powered off, or has some other problem which makes it
> unusable, the modem should be shown as disabled. Or something else.
>
Again, that is your UI design constraint. We have plenty of situations
where oFono creates multiple modems that are not physically there (e.g.
Bluetooth HFP for example).
>
> My point being:
> A) Having to implement testing-only code paths to components makes
> testing much more complicated and potentially erroneous as the
> code paths being tested differ between testing and production.
You already have a testing-only code-path since you are using phonesim.
Phonesim is only barely usable as an approximation of real hardware.
Introducing a plugin specific to your system setup is not going to
change anything here.
> B) oFono should not impose arbitrary limits to UI/UX design.
This has been discussed many times before. oFono is (and does) dictate
UI design to some extent.
Regards,
-Denis
_______________________________________________
ofono mailing list
ofono(a)ofono.org
https://lists.ofono.org/mailman/listinfo/ofono
3085
days inactive
3086
days old
5 comments
3 participants
participants (3)
-
Antti Kaijanmäki -
Denis Kenzior -
jussi.pakkanen＠canonical.com