[Bluez-devel] [PATCH] Variable arguments to Inquiry and PeriodicInquiry D-BUS methods

[Bluez-devel] [PATCH] Variable arguments to Inquiry and PeriodicInquiry D-BUS methods

[Johan Hedberg]

[-- Attachment #1: Type: text/plain, Size: 1683 bytes --]

Hi,

Here's a patch which allows Inquiry and PeriodicInquiry to be called
with two possible sets of parameters. We can't unfortunately get (at
least very easily) the same flexibility that you can get with e.g.
command line options because there are no "labels" for the D-BUS
parameters. So, the solution I went for was that either you provide only
the essential parameters (the ones that don't have a sane default
value), or then you provide all parameters.

The possible ways of calling the methods after applying this patch are:

Inquiry()
Inquiry(byte: length, byte: num_rsp, uint32: lap)
PeriodicInquiry(byte: length, uint16: min_period, uint16: max_period)
PeriodicInquiry(byte: length, uint16: min_period, uint16: max_period,
		byte: num_rsp, uint32: lap)

With dbus-test you can test this with e.g.
./dbus-test Inquiry 8 100 0x9e8b33

Then, I'd like to start some discussion about the interface for setting
default values for certain parameters. I'd suggest to use a similar
interface that we currently have for the properties where there are only
two methods ("Set" and "Get") and strings are used to identify the
parameter which should be operated on. I think it would be good to have
each module under the device path (e.g. "Controller", "PAN", etc.)
implement this interface. You would then do something like:

Object: /org/bluez/Device/hci0/Controller
Interface: org.bluez.Defaults
Method: Set("LAP", 0x9e8b00)

Object: /org/bluez/Device/hci0/Controller
Interface: org.bluez.Defaults
Method: Set("role", "slave")

Object: /org/bluez/Device/hci0/RFCOMM
Interface: org.bluez.Defaults
Method: Set("authenticate", True)

Does this sound like a sensible way to do it?

Johan


[-- Attachment #2: inq-params.patch --]
[-- Type: text/plain, Size: 7397 bytes --]

Index: hcid/dbus-test
===================================================================
RCS file: /cvsroot/bluez/utils/hcid/dbus-test,v
retrieving revision 1.3
diff -u -r1.3 dbus-test
--- hcid/dbus-test	6 Nov 2005 21:24:05 -0000	1.3
+++ hcid/dbus-test	8 Nov 2005 13:05:29 -0000
@@ -182,12 +182,12 @@
 
         # Device.Controller methods
         elif self.cmd == 'Inquiry':
-            if len(self.cmd_args) != 2:
-                length, maxrsp = (10, 100)
-            else:
-                length, maxrsp = self.cmd_args
             try:
-                self.ctl.Inquiry(dbus.Byte(length), dbus.Byte(maxrsp))
+                if len(self.cmd_args) != 3:
+                    self.ctl.Inquiry()
+                else:
+                    length, maxrsp, lap = self.cmd_args
+                    self.ctl.Inquiry(dbus.Byte(length), dbus.Byte(maxrsp), dbus.UInt32(long(lap, 0)))
             except dbus.DBusException, e:
                 print 'Sending %s failed: %s' % (self.cmd, e)
                 sys.exit(1)
@@ -216,13 +216,18 @@
             self.exit_events.append('RemoteName')
 
         elif self.cmd == 'PeriodicInquiry':
-            if len(self.cmd_args) != 3:
-                length, min, max = (6, 20, 60)
-            else:
-                length, min, max = self.cmd_args
-            self.listen = True
             try:
-                self.ctl.PeriodicInquiry(dbus.Byte(length), dbus.UInt16(min), dbus.UInt16(max))
+                if len(self.cmd_args) < 3:
+                    length, min, max = (6, 20, 60)
+                    self.ctl.PeriodicInquiry(dbus.Byte(length), dbus.UInt16(min), dbus.UInt16(max))
+                elif len(self.cmd_args) == 3:
+                    length, min, max = self.cmd_args
+                    self.ctl.PeriodicInquiry(dbus.Byte(length), dbus.UInt16(min), dbus.UInt16(max))
+                else:
+                    length, min, max, nrsp, lap = self.cmd_args
+                    self.ctl.PeriodicInquiry(dbus.Byte(length), dbus.UInt16(min), dbus.UInt16(max),
+                            dbus.Byte(nrsp), dbus.UInt32(long(lap, 0)))
+                self.listen = True
             except dbus.DBusException, e:
                 print 'Sending %s failed: %s' % (self.cmd, e)
                 sys.exit(1)
Index: hcid/dbus.c
===================================================================
RCS file: /cvsroot/bluez/utils/hcid/dbus.c,v
retrieving revision 1.51
diff -u -r1.51 dbus.c
--- hcid/dbus.c	6 Nov 2005 21:37:17 -0000	1.51
+++ hcid/dbus.c	8 Nov 2005 13:05:30 -0000
@@ -356,9 +356,11 @@
 
 static const struct service_data device_hci_services[] = {
 	{ HCI_PERIODIC_INQ,		handle_periodic_inq_req,	HCI_PERIODIC_INQ_SIGNATURE		},
+	{ HCI_PERIODIC_INQ,		handle_periodic_inq_req,	HCI_PERIODIC_INQ_EXT_SIGNATURE		},
 	{ HCI_CANCEL_PERIODIC_INQ,	handle_cancel_periodic_inq_req,	HCI_CANCEL_PERIODIC_INQ_SIGNATURE	},
 	{ HCI_ROLE_SWITCH,		handle_role_switch_req,		HCI_ROLE_SWITCH_SIGNATURE		},
 	{ HCI_INQ,			handle_inq_req,			HCI_INQ_SIGNATURE			},
+	{ HCI_INQ,			handle_inq_req,			HCI_INQ_EXT_SIGNATURE			},
 	{ HCI_CANCEL_INQ,		handle_cancel_inq_req,		HCI_CANCEL_INQ_SIGNATURE		},
 	{ HCI_REMOTE_NAME,		handle_remote_name_req,		HCI_REMOTE_NAME_SIGNATURE		},
 	{ HCI_CONNECTIONS,		handle_display_conn_req,	HCI_CONNECTIONS_SIGNATURE		},
@@ -1312,9 +1314,10 @@
 	periodic_inquiry_cp inq_param;
 	DBusMessage *reply = NULL;
 	struct hci_dbus_data *dbus_data = data;
-	uint8_t length;
+	uint8_t length, num_rsp = 0;
 	uint16_t max_period;
 	uint16_t min_period;
+	uint32_t lap = 0x9e8b33;
 	int dd = -1;
 
 	dd = hci_open_dev(dbus_data->dev_id);
@@ -1324,29 +1327,38 @@
 		goto failed;
 	}
 
-	dbus_message_get_args(msg, NULL,
-			DBUS_TYPE_BYTE, &length,
-			DBUS_TYPE_UINT16, &min_period,
-			DBUS_TYPE_UINT16, &max_period,
-			DBUS_TYPE_INVALID);
-
-	syslog(LOG_DEBUG, "%02X %04X %04X", length, min_period, max_period);
-
-	if (length >= min_period || min_period >= max_period) {
+	if (dbus_message_has_signature(msg, HCI_PERIODIC_INQ_EXT_SIGNATURE))
+		dbus_message_get_args(msg, NULL,
+				DBUS_TYPE_BYTE, &length,
+				DBUS_TYPE_UINT16, &min_period,
+				DBUS_TYPE_UINT16, &max_period,
+				DBUS_TYPE_BYTE, &num_rsp,
+				DBUS_TYPE_UINT32, &lap,
+				DBUS_TYPE_INVALID);
+	else
+		dbus_message_get_args(msg, NULL,
+				DBUS_TYPE_BYTE, &length,
+				DBUS_TYPE_UINT16, &min_period,
+				DBUS_TYPE_UINT16, &max_period,
+				DBUS_TYPE_INVALID);
+
+	/* Check for valid parameters */
+	if (length >= min_period || min_period >= max_period
+			|| length < 0x01 || length > 0x30
+			|| lap < 0x9e8b00 || lap > 0x9e8b3f) {
 		reply = bluez_new_failure_msg(msg, BLUEZ_EDBUS_WRONG_PARAM);
 		goto failed;
 	}
 
-	inq_param.num_rsp = 100;
+	inq_param.num_rsp = num_rsp;
 	inq_param.length  = length;
 
 	inq_param.max_period = htobs(max_period);
 	inq_param.min_period = htobs(min_period);
 
-	/* General/Unlimited Inquiry Access Code (GIAC) */
-	inq_param.lap[0] = 0x33;
-	inq_param.lap[1] = 0x8b;
-	inq_param.lap[2] = 0x9e;
+	inq_param.lap[0] = (lap & 0xff);
+	inq_param.lap[1] = (lap >> 8) & 0xff;
+	inq_param.lap[2] = (lap >> 16) & 0xff;
 
 	inq_mode.mode = 1; //INQUIRY_WITH_RSSI;
 
@@ -1409,16 +1421,21 @@
 	struct hci_request rq;
 	struct hci_dbus_data *dbus_data = data;
 	int dd = -1;
-	uint8_t length, num_rsp;
+	uint8_t length = 8, num_rsp = 0;
+	uint32_t lap = 0x9e8b33;
 
-	dbus_message_get_args(msg, NULL,
-					DBUS_TYPE_BYTE, &length,
-					DBUS_TYPE_BYTE, &num_rsp,
-					DBUS_TYPE_INVALID);
-
-	if (length < 0x01 || length > 0x30) {
-		reply = bluez_new_failure_msg(msg, BLUEZ_EDBUS_WRONG_PARAM);
-		goto failed;
+	if (dbus_message_has_signature(msg, HCI_INQ_EXT_SIGNATURE)) {
+		dbus_message_get_args(msg, NULL,
+				DBUS_TYPE_BYTE, &length,
+				DBUS_TYPE_BYTE, &num_rsp,
+				DBUS_TYPE_UINT32, &lap,
+				DBUS_TYPE_INVALID);
+
+		if (length < 0x01 || length > 0x30
+				|| lap < 0x9e8b00 || lap > 0x9e8b3f) {
+			reply = bluez_new_failure_msg(msg, BLUEZ_EDBUS_WRONG_PARAM);
+			goto failed;
+		}
 	}
 
 	dd = hci_open_dev(dbus_data->dev_id);
@@ -1429,9 +1446,9 @@
 	}
 
 	memset(&cp, 0, sizeof(cp));
-	cp.lap[0]  = 0x33;
-	cp.lap[1]  = 0x8b;
-	cp.lap[2]  = 0x9e;
+	cp.lap[0]  = (lap & 0xff);
+	cp.lap[1]  = (lap >> 8) & 0xff;
+	cp.lap[2]  = (lap >> 16) & 0xff;
 	cp.length  = length;
 	cp.num_rsp = num_rsp;
 
Index: hcid/dbus.h
===================================================================
RCS file: /cvsroot/bluez/utils/hcid/dbus.h,v
retrieving revision 1.15
diff -u -r1.15 dbus.h
--- hcid/dbus.h	6 Nov 2005 21:24:05 -0000	1.15
+++ hcid/dbus.h	8 Nov 2005 13:05:31 -0000
@@ -164,6 +164,12 @@
 #define DEV_GET_PROPERTY_SIGNATURE		DBUS_TYPE_STRING_AS_STRING \
 						__END_SIG__
 
+#define HCI_PERIODIC_INQ_EXT_SIGNATURE			DBUS_TYPE_BYTE_AS_STRING \
+							DBUS_TYPE_UINT16_AS_STRING \
+							DBUS_TYPE_UINT16_AS_STRING \
+							DBUS_TYPE_BYTE_AS_STRING \
+							DBUS_TYPE_UINT32_AS_STRING \
+							__END_SIG__
 
 #define HCI_PERIODIC_INQ_SIGNATURE			DBUS_TYPE_BYTE_AS_STRING \
 							DBUS_TYPE_UINT16_AS_STRING \
@@ -172,8 +178,11 @@
 
 #define HCI_CANCEL_PERIODIC_INQ_SIGNATURE		__END_SIG__
 
-#define HCI_INQ_SIGNATURE				DBUS_TYPE_BYTE_AS_STRING \
+#define HCI_INQ_SIGNATURE				__END_SIG__
+
+#define HCI_INQ_EXT_SIGNATURE				DBUS_TYPE_BYTE_AS_STRING \
 							DBUS_TYPE_BYTE_AS_STRING \
+							DBUS_TYPE_UINT32_AS_STRING \
 							__END_SIG__
 
 #define HCI_CANCEL_INQ_SIGNATURE			__END_SIG__

Re: [Bluez-devel] [PATCH] Variable arguments to Inquiry and PeriodicInquiry D-BUS methods

[Marcel Holtmann]

Hi Johan,

> Here's a patch which allows Inquiry and PeriodicInquiry to be called
> with two possible sets of parameters. We can't unfortunately get (at
> least very easily) the same flexibility that you can get with e.g.
> command line options because there are no "labels" for the D-BUS
> parameters. So, the solution I went for was that either you provide only
> the essential parameters (the ones that don't have a sane default
> value), or then you provide all parameters.
> 
> The possible ways of calling the methods after applying this patch are:
> 
> Inquiry()
> Inquiry(byte: length, byte: num_rsp, uint32: lap)
> PeriodicInquiry(byte: length, uint16: min_period, uint16: max_period)
> PeriodicInquiry(byte: length, uint16: min_period, uint16: max_period,
> 		byte: num_rsp, uint32: lap)

please swap the lap and num_rsp (call it num_responses btw.) and make
the num_responses also optional. It is more usual that you will specify
a different lap instead of num_responses. The reason for this is,
because the num_responses value is not really useful anymore. All chips
in inquiry mode 1 or 2 tend to send results multiple times and this
makes the num_responses value useless. It is only good for devices where
the memory storage for inquiry results is limited. Maybe we shouldn't
use it at all.

Is it not better to call it access_code instead of lap?

The length parameter should also be in seconds and not the slot timing
we get from the baseband.

> Then, I'd like to start some discussion about the interface for setting
> default values for certain parameters. I'd suggest to use a similar
> interface that we currently have for the properties where there are only
> two methods ("Set" and "Get") and strings are used to identify the
> parameter which should be operated on. I think it would be good to have
> each module under the device path (e.g. "Controller", "PAN", etc.)
> implement this interface. You would then do something like:
> 
> Object: /org/bluez/Device/hci0/Controller
> Interface: org.bluez.Defaults
> Method: Set("LAP", 0x9e8b00)
> 
> Object: /org/bluez/Device/hci0/Controller
> Interface: org.bluez.Defaults
> Method: Set("role", "slave")
> 
> Object: /org/bluez/Device/hci0/RFCOMM
> Interface: org.bluez.Defaults
> Method: Set("authenticate", True)
> 
> Does this sound like a sensible way to do it?

I am not unhappy with it at the moment.

Regards

Marcel




-------------------------------------------------------
SF.Net email is sponsored by:
Tame your development challenges with Apache's Geronimo App Server. Download
it for free - -and be entered to win a 42" plasma tv or your very own
Sony(tm)PSP.  Click here to play: http://sourceforge.net/geronimo.php
_______________________________________________
Bluez-devel mailing list
Bluez-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/bluez-devel

Re: [Bluez-devel] [PATCH] Variable arguments to Inquiry and PeriodicInquiry D-BUS methods

[Johan Hedberg]

[-- Attachment #1: Type: text/plain, Size: 1948 bytes --]

Hi Marcel,

Attached is a new patch with some of your proposed changes (the ones I
agree with ;)

On Tue, Nov 08, 2005, Marcel Holtmann wrote:
> > The possible ways of calling the methods after applying this patch are:
> > 
> > Inquiry()
> > Inquiry(byte: length, byte: num_rsp, uint32: lap)
> > PeriodicInquiry(byte: length, uint16: min_period, uint16: max_period)
> > PeriodicInquiry(byte: length, uint16: min_period, uint16: max_period,
> > 		byte: num_rsp, uint32: lap)
> 
> please swap the lap and num_rsp (call it num_responses btw.) and make
> the num_responses also optional. It is more usual that you will specify
> a different lap instead of num_responses. The reason for this is,
> because the num_responses value is not really useful anymore. All chips
> in inquiry mode 1 or 2 tend to send results multiple times and this
> makes the num_responses value useless. It is only good for devices where
> the memory storage for inquiry results is limited. Maybe we shouldn't
> use it at all.

I removed the num_responses parameter alltogether. If it's needed we can
always allow changing it through the default settings interface in the
future.

> Is it not better to call it access_code instead of lap?

Maybe. I called it lap since that's what your structs in hci.h and the
HCI spec calls it. Note that the terms we use in the code don't have to
the same what we put in the (yet to be created) public documentation of
the interface.

> The length parameter should also be in seconds and not the slot timing
> we get from the baseband.

Well, then it would always have to be rounded to the nearest 1.28 sec
multiple, so I'm not so sure how useful it would be (and it might be
even confusing for the user when the inquiry doesn't last the exact
amount of seconds that was given). I've left it unchanged for now. I can
send a new patch later specifically for it. If we do change it, should
it be an integer or a floating point number?

Johan

[-- Attachment #2: inq-params2.patch --]
[-- Type: text/plain, Size: 7358 bytes --]

Index: hcid/dbus-test
===================================================================
RCS file: /cvsroot/bluez/utils/hcid/dbus-test,v
retrieving revision 1.3
diff -u -r1.3 dbus-test
--- hcid/dbus-test	6 Nov 2005 21:24:05 -0000	1.3
+++ hcid/dbus-test	8 Nov 2005 14:24:16 -0000
@@ -182,12 +182,12 @@
 
         # Device.Controller methods
         elif self.cmd == 'Inquiry':
-            if len(self.cmd_args) != 2:
-                length, maxrsp = (10, 100)
-            else:
-                length, maxrsp = self.cmd_args
             try:
-                self.ctl.Inquiry(dbus.Byte(length), dbus.Byte(maxrsp))
+                if len(self.cmd_args) != 2:
+                    self.ctl.Inquiry()
+                else:
+                    length, lap = self.cmd_args
+                    self.ctl.Inquiry(dbus.Byte(length), dbus.UInt32(long(lap, 0)))
             except dbus.DBusException, e:
                 print 'Sending %s failed: %s' % (self.cmd, e)
                 sys.exit(1)
@@ -216,13 +216,18 @@
             self.exit_events.append('RemoteName')
 
         elif self.cmd == 'PeriodicInquiry':
-            if len(self.cmd_args) != 3:
-                length, min, max = (6, 20, 60)
-            else:
-                length, min, max = self.cmd_args
-            self.listen = True
             try:
-                self.ctl.PeriodicInquiry(dbus.Byte(length), dbus.UInt16(min), dbus.UInt16(max))
+                if len(self.cmd_args) < 3:
+                    length, min, max = (6, 20, 60)
+                    self.ctl.PeriodicInquiry(dbus.Byte(length), dbus.UInt16(min), dbus.UInt16(max))
+                elif len(self.cmd_args) == 3:
+                    length, min, max = self.cmd_args
+                    self.ctl.PeriodicInquiry(dbus.Byte(length), dbus.UInt16(min), dbus.UInt16(max))
+                else:
+                    length, min, max, lap = self.cmd_args
+                    self.ctl.PeriodicInquiry(dbus.Byte(length), dbus.UInt16(min), dbus.UInt16(max),
+                            dbus.UInt32(long(lap, 0)))
+                self.listen = True
             except dbus.DBusException, e:
                 print 'Sending %s failed: %s' % (self.cmd, e)
                 sys.exit(1)
Index: hcid/dbus.c
===================================================================
RCS file: /cvsroot/bluez/utils/hcid/dbus.c,v
retrieving revision 1.51
diff -u -r1.51 dbus.c
--- hcid/dbus.c	6 Nov 2005 21:37:17 -0000	1.51
+++ hcid/dbus.c	8 Nov 2005 14:24:17 -0000
@@ -356,9 +356,11 @@
 
 static const struct service_data device_hci_services[] = {
 	{ HCI_PERIODIC_INQ,		handle_periodic_inq_req,	HCI_PERIODIC_INQ_SIGNATURE		},
+	{ HCI_PERIODIC_INQ,		handle_periodic_inq_req,	HCI_PERIODIC_INQ_EXT_SIGNATURE		},
 	{ HCI_CANCEL_PERIODIC_INQ,	handle_cancel_periodic_inq_req,	HCI_CANCEL_PERIODIC_INQ_SIGNATURE	},
 	{ HCI_ROLE_SWITCH,		handle_role_switch_req,		HCI_ROLE_SWITCH_SIGNATURE		},
 	{ HCI_INQ,			handle_inq_req,			HCI_INQ_SIGNATURE			},
+	{ HCI_INQ,			handle_inq_req,			HCI_INQ_EXT_SIGNATURE			},
 	{ HCI_CANCEL_INQ,		handle_cancel_inq_req,		HCI_CANCEL_INQ_SIGNATURE		},
 	{ HCI_REMOTE_NAME,		handle_remote_name_req,		HCI_REMOTE_NAME_SIGNATURE		},
 	{ HCI_CONNECTIONS,		handle_display_conn_req,	HCI_CONNECTIONS_SIGNATURE		},
@@ -1312,9 +1314,10 @@
 	periodic_inquiry_cp inq_param;
 	DBusMessage *reply = NULL;
 	struct hci_dbus_data *dbus_data = data;
-	uint8_t length;
+	uint8_t length, num_responses = 0;
 	uint16_t max_period;
 	uint16_t min_period;
+	uint32_t lap = 0x9e8b33;
 	int dd = -1;
 
 	dd = hci_open_dev(dbus_data->dev_id);
@@ -1324,29 +1327,37 @@
 		goto failed;
 	}
 
-	dbus_message_get_args(msg, NULL,
-			DBUS_TYPE_BYTE, &length,
-			DBUS_TYPE_UINT16, &min_period,
-			DBUS_TYPE_UINT16, &max_period,
-			DBUS_TYPE_INVALID);
-
-	syslog(LOG_DEBUG, "%02X %04X %04X", length, min_period, max_period);
-
-	if (length >= min_period || min_period >= max_period) {
+	if (dbus_message_has_signature(msg, HCI_PERIODIC_INQ_EXT_SIGNATURE))
+		dbus_message_get_args(msg, NULL,
+				DBUS_TYPE_BYTE, &length,
+				DBUS_TYPE_UINT16, &min_period,
+				DBUS_TYPE_UINT16, &max_period,
+				DBUS_TYPE_UINT32, &lap,
+				DBUS_TYPE_INVALID);
+	else
+		dbus_message_get_args(msg, NULL,
+				DBUS_TYPE_BYTE, &length,
+				DBUS_TYPE_UINT16, &min_period,
+				DBUS_TYPE_UINT16, &max_period,
+				DBUS_TYPE_INVALID);
+
+	/* Check for valid parameters */
+	if (length >= min_period || min_period >= max_period
+			|| length < 0x01 || length > 0x30
+			|| lap < 0x9e8b00 || lap > 0x9e8b3f) {
 		reply = bluez_new_failure_msg(msg, BLUEZ_EDBUS_WRONG_PARAM);
 		goto failed;
 	}
 
-	inq_param.num_rsp = 100;
+	inq_param.num_rsp = num_responses;
 	inq_param.length  = length;
 
 	inq_param.max_period = htobs(max_period);
 	inq_param.min_period = htobs(min_period);
 
-	/* General/Unlimited Inquiry Access Code (GIAC) */
-	inq_param.lap[0] = 0x33;
-	inq_param.lap[1] = 0x8b;
-	inq_param.lap[2] = 0x9e;
+	inq_param.lap[0] = (lap & 0xff);
+	inq_param.lap[1] = (lap >> 8) & 0xff;
+	inq_param.lap[2] = (lap >> 16) & 0xff;
 
 	inq_mode.mode = 1; //INQUIRY_WITH_RSSI;
 
@@ -1409,16 +1420,20 @@
 	struct hci_request rq;
 	struct hci_dbus_data *dbus_data = data;
 	int dd = -1;
-	uint8_t length, num_rsp;
+	uint8_t length = 8, num_responses = 0;
+	uint32_t lap = 0x9e8b33;
 
-	dbus_message_get_args(msg, NULL,
-					DBUS_TYPE_BYTE, &length,
-					DBUS_TYPE_BYTE, &num_rsp,
-					DBUS_TYPE_INVALID);
-
-	if (length < 0x01 || length > 0x30) {
-		reply = bluez_new_failure_msg(msg, BLUEZ_EDBUS_WRONG_PARAM);
-		goto failed;
+	if (dbus_message_has_signature(msg, HCI_INQ_EXT_SIGNATURE)) {
+		dbus_message_get_args(msg, NULL,
+				DBUS_TYPE_BYTE, &length,
+				DBUS_TYPE_UINT32, &lap,
+				DBUS_TYPE_INVALID);
+
+		if (length < 0x01 || length > 0x30
+				|| lap < 0x9e8b00 || lap > 0x9e8b3f) {
+			reply = bluez_new_failure_msg(msg, BLUEZ_EDBUS_WRONG_PARAM);
+			goto failed;
+		}
 	}
 
 	dd = hci_open_dev(dbus_data->dev_id);
@@ -1429,11 +1444,11 @@
 	}
 
 	memset(&cp, 0, sizeof(cp));
-	cp.lap[0]  = 0x33;
-	cp.lap[1]  = 0x8b;
-	cp.lap[2]  = 0x9e;
+	cp.lap[0]  = (lap & 0xff);
+	cp.lap[1]  = (lap >> 8) & 0xff;
+	cp.lap[2]  = (lap >> 16) & 0xff;
 	cp.length  = length;
-	cp.num_rsp = num_rsp;
+	cp.num_rsp = num_responses;
 
 	memset(&rq, 0, sizeof(rq));
 	rq.ogf    = OGF_LINK_CTL;
Index: hcid/dbus.h
===================================================================
RCS file: /cvsroot/bluez/utils/hcid/dbus.h,v
retrieving revision 1.15
diff -u -r1.15 dbus.h
--- hcid/dbus.h	6 Nov 2005 21:24:05 -0000	1.15
+++ hcid/dbus.h	8 Nov 2005 14:24:18 -0000
@@ -164,6 +164,11 @@
 #define DEV_GET_PROPERTY_SIGNATURE		DBUS_TYPE_STRING_AS_STRING \
 						__END_SIG__
 
+#define HCI_PERIODIC_INQ_EXT_SIGNATURE			DBUS_TYPE_BYTE_AS_STRING \
+							DBUS_TYPE_UINT16_AS_STRING \
+							DBUS_TYPE_UINT16_AS_STRING \
+							DBUS_TYPE_UINT32_AS_STRING \
+							__END_SIG__
 
 #define HCI_PERIODIC_INQ_SIGNATURE			DBUS_TYPE_BYTE_AS_STRING \
 							DBUS_TYPE_UINT16_AS_STRING \
@@ -172,8 +177,10 @@
 
 #define HCI_CANCEL_PERIODIC_INQ_SIGNATURE		__END_SIG__
 
-#define HCI_INQ_SIGNATURE				DBUS_TYPE_BYTE_AS_STRING \
-							DBUS_TYPE_BYTE_AS_STRING \
+#define HCI_INQ_SIGNATURE				__END_SIG__
+
+#define HCI_INQ_EXT_SIGNATURE				DBUS_TYPE_BYTE_AS_STRING \
+							DBUS_TYPE_UINT32_AS_STRING \
 							__END_SIG__
 
 #define HCI_CANCEL_INQ_SIGNATURE			__END_SIG__

Re: [Bluez-devel] [PATCH] Variable arguments to Inquiry and PeriodicInquiry D-BUS methods

[Claudio Takahasi]

Hi,

The interface for get/set the default values is fine!

Some suggestions/comments...
1. It's better use the same method name used by device configuration.
Currently, device configuration uses SetProperty and GetProperty, it
should be better rename to Set and Get. Do you agree?

2. Does it make sense add an extra argument(device class) in order to
be notified only by device class that the client wants? Maybe this
enhancement should be applied when become possible retrieve the
Inquiry cache using the new userspace/kernel communication interface.

3. Periodic inquiry is a concurrent task. Currently there isn't
verification for PeriodicInquiry requested by more than one client. In
this case how it should work?

Regards,
Claudio.

On 11/8/05, Johan Hedberg <johan.hedberg@nokia.com> wrote:
> Hi Marcel,
>
> Attached is a new patch with some of your proposed changes (the ones I
> agree with ;)
>
> On Tue, Nov 08, 2005, Marcel Holtmann wrote:
> > > The possible ways of calling the methods after applying this patch ar=
e:
> > >
> > > Inquiry()
> > > Inquiry(byte: length, byte: num_rsp, uint32: lap)
> > > PeriodicInquiry(byte: length, uint16: min_period, uint16: max_period)
> > > PeriodicInquiry(byte: length, uint16: min_period, uint16: max_period,
> > >             byte: num_rsp, uint32: lap)
> >
> > please swap the lap and num_rsp (call it num_responses btw.) and make
> > the num_responses also optional. It is more usual that you will specify
> > a different lap instead of num_responses. The reason for this is,
> > because the num_responses value is not really useful anymore. All chips
> > in inquiry mode 1 or 2 tend to send results multiple times and this
> > makes the num_responses value useless. It is only good for devices wher=
e
> > the memory storage for inquiry results is limited. Maybe we shouldn't
> > use it at all.
>
> I removed the num_responses parameter alltogether. If it's needed we can
> always allow changing it through the default settings interface in the
> future.
>
> > Is it not better to call it access_code instead of lap?
>
> Maybe. I called it lap since that's what your structs in hci.h and the
> HCI spec calls it. Note that the terms we use in the code don't have to
> the same what we put in the (yet to be created) public documentation of
> the interface.
>
> > The length parameter should also be in seconds and not the slot timing
> > we get from the baseband.
>
> Well, then it would always have to be rounded to the nearest 1.28 sec
> multiple, so I'm not so sure how useful it would be (and it might be
> even confusing for the user when the inquiry doesn't last the exact
> amount of seconds that was given). I've left it unchanged for now. I can
> send a new patch later specifically for it. If we do change it, should
> it be an integer or a floating point number?
>
> Johan
>
>
>


--
---------------------------------------------------------
Claudio Takahasi
Instituto Nokia de Tecnologia - INdT


-------------------------------------------------------
SF.Net email is sponsored by:
Tame your development challenges with Apache's Geronimo App Server. Download
it for free - -and be entered to win a 42" plasma tv or your very own
Sony(tm)PSP.  Click here to play: http://sourceforge.net/geronimo.php
_______________________________________________
Bluez-devel mailing list
Bluez-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/bluez-devel

Re: [Bluez-devel] [PATCH] Variable arguments to Inquiry and PeriodicInquiry D-BUS methods

[Marcel Holtmann]

Hi Johan,

> Attached is a new patch with some of your proposed changes (the ones I
> agree with ;)

the patch is in the CVS now (slightly modified).

> > > The possible ways of calling the methods after applying this patch are:
> > > 
> > > Inquiry()
> > > Inquiry(byte: length, byte: num_rsp, uint32: lap)
> > > PeriodicInquiry(byte: length, uint16: min_period, uint16: max_period)
> > > PeriodicInquiry(byte: length, uint16: min_period, uint16: max_period,
> > > 		byte: num_rsp, uint32: lap)
> > 
> > please swap the lap and num_rsp (call it num_responses btw.) and make
> > the num_responses also optional. It is more usual that you will specify
> > a different lap instead of num_responses. The reason for this is,
> > because the num_responses value is not really useful anymore. All chips
> > in inquiry mode 1 or 2 tend to send results multiple times and this
> > makes the num_responses value useless. It is only good for devices where
> > the memory storage for inquiry results is limited. Maybe we shouldn't
> > use it at all.
> 
> I removed the num_responses parameter alltogether. If it's needed we can
> always allow changing it through the default settings interface in the
> future.

and I renamed it back to num_rsp, because it is only internally.

> > Is it not better to call it access_code instead of lap?
> 
> Maybe. I called it lap since that's what your structs in hci.h and the
> HCI spec calls it. Note that the terms we use in the code don't have to
> the same what we put in the (yet to be created) public documentation of
> the interface.

Inside the code this doesn't matter. However don't be so picky on the
structs defined in hci.h. Some of them are over 4 years old and back
then sometimes we had no idea what we were doing.

> > The length parameter should also be in seconds and not the slot timing
> > we get from the baseband.
> 
> Well, then it would always have to be rounded to the nearest 1.28 sec
> multiple, so I'm not so sure how useful it would be (and it might be
> even confusing for the user when the inquiry doesn't last the exact
> amount of seconds that was given). I've left it unchanged for now. I can
> send a new patch later specifically for it. If we do change it, should
> it be an integer or a floating point number?

I don't see it as a big problem, because we will round to next 1.28 sec
to make it at least "length" number of seconds. In most cases this will
be some microseconds longer, but that doesn't really matter. The API is
event driven and so yes, they can't start another inquiry after exactly
"length" number seconds, but they periodic inquiry should used for this
kind of tasks anyhow.

Regards

Marcel




-------------------------------------------------------
SF.Net email is sponsored by:
Tame your development challenges with Apache's Geronimo App Server. Download
it for free - -and be entered to win a 42" plasma tv or your very own
Sony(tm)PSP.  Click here to play: http://sourceforge.net/geronimo.php
_______________________________________________
Bluez-devel mailing list
Bluez-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/bluez-devel

Re: [Bluez-devel] [PATCH] Variable arguments to Inquiry and PeriodicInquiry D-BUS methods

[Marcel Holtmann]

Hi Claudio,

> 1. It's better use the same method name used by device configuration.
> Currently, device configuration uses SetProperty and GetProperty, it
> should be better rename to Set and Get. Do you agree?

is HAL using something similar? If yes, then we should adapt their way.

> 2. Does it make sense add an extra argument(device class) in order to
> be notified only by device class that the client wants? Maybe this
> enhancement should be applied when become possible retrieve the
> Inquiry cache using the new userspace/kernel communication interface.

We really need this lifetime monitoring. This shouldn't be an extra
parameter, but a general filter setting for inquiry would be a good
enhancement. And we can do it all in userspace.

> 3. Periodic inquiry is a concurrent task. Currently there isn't
> verification for PeriodicInquiry requested by more than one client. In
> this case how it should work?

It should fail with somekind in progress error indication. However the
kernel should set the INQUIRY flag is an inquiry is already in progress.
I know that this work for normal inquiries, but I never checked that for
the periodic inquiry.

Regards

Marcel




-------------------------------------------------------
SF.Net email is sponsored by:
Tame your development challenges with Apache's Geronimo App Server. Download
it for free - -and be entered to win a 42" plasma tv or your very own
Sony(tm)PSP.  Click here to play: http://sourceforge.net/geronimo.php
_______________________________________________
Bluez-devel mailing list
Bluez-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/bluez-devel

Re: [Bluez-devel] [PATCH] Variable arguments to Inquiry and PeriodicInquiry D-BUS methods

[Claudio Takahasi]

Hi,

On 11/8/05, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Claudio,
>
> > 1. It's better use the same method name used by device configuration.
> > Currently, device configuration uses SetProperty and GetProperty, it
> > should be better rename to Set and Get. Do you agree?
>
> is HAL using something similar? If yes, then we should adapt their way.
[Claudio Takahasi]
HAL implements 5 different methods to set device properties.
void SetProperty(string key, any value)
void SetPropertyString(string key, string value)
void SetPropertyInteger(string key, int32 value)
void SetPropertyBoolean(string key, bool value)
void SetPropertyDouble(string key, double value)

We don't need use this approach because we are extracting/comparing
the message signature before call the function responsible to execute
the task. Our design of having the same method name with differents
signatures is fine! Regarding the name, it's better use the same
method name "SetProperty" or only "Set" for both(device configuration
and Controller).

>
> > 2. Does it make sense add an extra argument(device class) in order to
> > be notified only by device class that the client wants? Maybe this
> > enhancement should be applied when become possible retrieve the
> > Inquiry cache using the new userspace/kernel communication interface.
>
> We really need this lifetime monitoring. This shouldn't be an extra
> parameter, but a general filter setting for inquiry would be a good
> enhancement. And we can do it all in userspace.
>
> > 3. Periodic inquiry is a concurrent task. Currently there isn't
> > verification for PeriodicInquiry requested by more than one client. In
> > this case how it should work?
>
> It should fail with somekind in progress error indication. However the
> kernel should set the INQUIRY flag is an inquiry is already in progress.
> I know that this work for normal inquiries, but I never checked that for
> the periodic inquiry.
[Claudio Takahasi]
ok. we need check how it is working now and try implement a policy for
this case.

>
> Regards
>
> Marcel
>
>
>
>
> -------------------------------------------------------
> SF.Net email is sponsored by:
> Tame your development challenges with Apache's Geronimo App Server. Downl=
oad
> it for free - -and be entered to win a 42" plasma tv or your very own
> Sony(tm)PSP.  Click here to play: http://sourceforge.net/geronimo.php
> _______________________________________________
> Bluez-devel mailing list
> Bluez-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/bluez-devel
>


--
---------------------------------------------------------
Claudio Takahasi
Instituto Nokia de Tecnologia - INdT


-------------------------------------------------------
SF.Net email is sponsored by:
Tame your development challenges with Apache's Geronimo App Server. Download
it for free - -and be entered to win a 42" plasma tv or your very own
Sony(tm)PSP.  Click here to play: http://sourceforge.net/geronimo.php
_______________________________________________
Bluez-devel mailing list
Bluez-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/bluez-devel

Re: [Bluez-devel] [PATCH] Variable arguments to Inquiry and PeriodicInquiry D-BUS methods

[Johan Hedberg]

On Tue, Nov 08, 2005, Claudio Takahasi wrote:
> HAL implements 5 different methods to set device properties.
> void SetProperty(string key, any value)
> void SetPropertyString(string key, string value)
> void SetPropertyInteger(string key, int32 value)
> void SetPropertyBoolean(string key, bool value)
> void SetPropertyDouble(string key, double value)
> 
> We don't need use this approach because we are extracting/comparing
> the message signature before call the function responsible to execute
> the task. Our design of having the same method name with differents
> signatures is fine! Regarding the name, it's better use the same
> method name "SetProperty" or only "Set" for both(device configuration
> and Controller).

I think it's a matter of what the interface name is. If the interface is
"org.bluez.Device", then "SetProperty" describes the method better than
"Set". If the interface is "org.bluez.Defaults" then just "Set" would
probably be enough.

Johan


-------------------------------------------------------
SF.Net email is sponsored by:
Tame your development challenges with Apache's Geronimo App Server. Download
it for free - -and be entered to win a 42" plasma tv or your very own
Sony(tm)PSP.  Click here to play: http://sourceforge.net/geronimo.php
_______________________________________________
Bluez-devel mailing list
Bluez-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/bluez-devel