Linux bluetooth development
 help / color / mirror / Atom feed
* Re: [PATCH] Fix signal strength for HFP in maemo6 telephony due to changed API in libcsnet.
From: Hedberg Johan (Nokia-D/Helsinki) @ 2010-07-09 16:39 UTC (permalink / raw)
  To: Dmitriy Paliy; +Cc: linux-bluetooth
In-Reply-To: <1278686657-6997-1-git-send-email-dmitriy.paliy@nokia.com>

Hi Dmitriy,

Congrats on your first patch submission! However, there are a few things
that need fixing:

On Fri, Jul 09, 2010, Dmitriy Paliy wrote:
> Signed-off-by: Dmitriy Paliy <dmitriy.paliy@nokia.com>

We don't use Signed-off-by in user space BlueZ, so please remove it.
Also make sure that your commit message lines don't exceed 74
characters. When you do "git log" on a 80-character wide terminal the
message should be propertly viewable. In this case the summary line
could just be "Fix signal strength for HFP in maemo6 telephony driver"
and then in the body of the commit message (which is encouraged to have
for any non-trivial commits) you can have a proper explanation of how
the csd API has changed. In general, being too verbose in the commit
message is less bad than not being verbose enough.

> -	.signals_bar = 0,
> +	.signal_bars = 0,	/* Init as 0 meaning inactive mode. In modem power off state it can */
> +				/* be -1, but we treat all values as 0s regardless inactive or power off. */

No over 79 character lines in the code please. In this case you can just
put the comment above instead of on the same line.

> -	{ "signal",	"0-5",	0,	TRUE },
> +	{ "signal",	"0-5",	0,	TRUE },			/* signal strength in terms of bars */

Same here.

> +	if (signal_strength_bars < 0) {
> +		DBG("signal strength bars smaller than expected: %d", signal_strength_bars);
> +		signal_strength_bars = 0;
> +	} else if (signal_strength_bars > 5) {
> +		DBG("signal strength bars greater than expected: %d", signal_strength_bars);
> +		signal_strength_bars = 5;

The DBG lines look like they're over 79 too.

Johan

^ permalink raw reply

* [PATCH] Fix signal strength for HFP in maemo6 telephony due to changed API in libcsnet.
From: Dmitriy Paliy @ 2010-07-09 14:44 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Dmitriy Paliy

Signed-off-by: Dmitriy Paliy <dmitriy.paliy@nokia.com>
---
 audio/telephony-maemo6.c |   50 +++++++++++++++++++++------------------------
 1 files changed, 23 insertions(+), 27 deletions(-)

diff --git a/audio/telephony-maemo6.c b/audio/telephony-maemo6.c
index 31704c3..da3c19a 100644
--- a/audio/telephony-maemo6.c
+++ b/audio/telephony-maemo6.c
@@ -134,11 +134,12 @@ struct csd_call {
 static struct {
 	char *operator_name;
 	uint8_t status;
-	int32_t signals_bar;
+	int32_t signal_bars;
 } net = {
 	.operator_name = NULL,
 	.status = NETWORK_REG_STATUS_UNKOWN,
-	.signals_bar = 0,
+	.signal_bars = 0,	/* Init as 0 meaning inactive mode. In modem power off state it can */
+				/* be -1, but we treat all values as 0s regardless inactive or power off. */
 };
 
 static int get_property(const char *iface, const char *prop);
@@ -181,7 +182,7 @@ static guint create_request_timer = 0;
 static struct indicator maemo_indicators[] =
 {
 	{ "battchg",	"0-5",	5,	TRUE },
-	{ "signal",	"0-5",	0,	TRUE },
+	{ "signal",	"0-5",	0,	TRUE },			/* signal strength in terms of bars */
 	{ "service",	"0,1",	0,	TRUE },
 	{ "call",	"0,1",	0,	TRUE },
 	{ "callsetup",	"0-3",	0,	TRUE },
@@ -1227,43 +1228,38 @@ static void handle_registration_changed(DBusMessage *msg)
 	update_registration_status(status);
 }
 
-static void update_signal_strength(int32_t signals_bar)
+static void update_signal_strength(int32_t signal_strength_bars)
 {
-	int signal;
-
-	if (signals_bar < 0)
-		signals_bar = 0;
-	else if (signals_bar > 100) {
-		DBG("signals_bar greater than expected: %u", signals_bar);
-		signals_bar = 100;
+	if (signal_strength_bars < 0) {
+		DBG("signal strength bars smaller than expected: %d", signal_strength_bars);
+		signal_strength_bars = 0;
+	} else if (signal_strength_bars > 5) {
+		DBG("signal strength bars greater than expected: %d", signal_strength_bars);
+		signal_strength_bars = 5;
 	}
 
-	if (net.signals_bar == signals_bar)
+	if (net.signal_bars == signal_strength_bars)
 		return;
 
-	/* A simple conversion from 0-100 to 0-5 (used by HFP) */
-	signal = (signals_bar + 20) / 21;
-
-	telephony_update_indicator(maemo_indicators, "signal", signal);
+	telephony_update_indicator(maemo_indicators, "signal", signal_strength_bars);
 
-	net.signals_bar = signals_bar;
+	net.signal_bars = signal_strength_bars;
 
-	DBG("telephony-maemo6: signal strength updated: %u/100, %d/5", signals_bar, signal);
+	DBG("telephony-maemo6: signal strength bars updated: %d/5", signal_strength_bars);
 }
 
-static void handle_signal_strength_changed(DBusMessage *msg)
+static void handle_signal_bars_changed(DBusMessage *msg)
 {
-	int32_t signals_bar, rssi_in_dbm;
+	int32_t signal_bars;
 
 	if (!dbus_message_get_args(msg, NULL,
-					DBUS_TYPE_INT32, &signals_bar,
-					DBUS_TYPE_INT32, &rssi_in_dbm,
+					DBUS_TYPE_INT32, &signal_bars,
 					DBUS_TYPE_INVALID)) {
-		error("Unexpected parameters in SignalStrengthChanged");
+		error("Unexpected parameters in SignalBarsChanged");
 		return;
 	}
 
-	update_signal_strength(signals_bar);
+	update_signal_strength(signal_bars);
 }
 
 static gboolean iter_get_basic_args(DBusMessageIter *iter,
@@ -1530,7 +1526,7 @@ static void get_property_reply(DBusPendingCall *call, void *user_data)
 		dbus_message_iter_get_basic(&sub, &name);
 		update_operator_name(name);
 	} else if (g_strcmp0(prop, "SignalBars") == 0) {
-		uint32_t signal_bars;
+		int32_t signal_bars;
 
 		dbus_message_iter_get_basic(&sub, &signal_bars);
 		update_signal_strength(signal_bars);
@@ -1899,8 +1895,8 @@ static DBusHandlerResult signal_filter(DBusConnection *conn,
 				"OperatorNameChanged"))
 		handle_operator_name_changed(msg);
 	else if (dbus_message_is_signal(msg, CSD_CSNET_SIGNAL,
-				"SignalStrengthChanged"))
-		handle_signal_strength_changed(msg);
+				"SignalBarsChanged"))
+		handle_signal_bars_changed(msg);
 	else if (dbus_message_is_signal(msg, "org.freedesktop.Hal.Device",
 					"PropertyModified"))
 		handle_hal_property_modified(msg);
-- 
1.7.0.4


^ permalink raw reply related

* [PATCH] Fixed empty 'N:' parameter handling for VCARDs
From: Radoslaw Jablonski @ 2010-07-09 14:34 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Radoslaw Jablonski

Bluetooth PBAP specification expects for call history listing,
that parameter N: shall be empty when we cannot retrieve personal data
from PSE phone book.
Some devices(by example BH-903) are having problems when after N: parameter
 unnecessary characters occurs. List of dialed/incoming calls on carkit
then is useless - carkit shows only blank lines and it's impossible to
determine who made call.

In previous version unnecessary semicolons were added after N:("N:;;;;")
to represent empty name.
Now if none of the contact fields is available, then adding real empty "N:"
parameter (without semicolons).
---
 plugins/vcard.c |   35 +++++++++++++++++++++++++++++++++++
 1 files changed, 35 insertions(+), 0 deletions(-)

diff --git a/plugins/vcard.c b/plugins/vcard.c
index 5948a4a..108f6bd 100644
--- a/plugins/vcard.c
+++ b/plugins/vcard.c
@@ -133,9 +133,44 @@ static void vcard_printf_begin(GString *vcards, uint8_t format)
 		vcard_printf(vcards, "VERSION:2.1");
 }
 
+/* checks if there is at least one present contact field with personal data */
+static gboolean contact_fields_present(struct phonebook_contact * contact)
+{
+	if (contact->family && strlen(contact->family) > 0)
+		return TRUE;
+
+	if (contact->given && strlen(contact->given) > 0)
+		return TRUE;
+
+	if (contact->additional && strlen(contact->additional) > 0)
+		return TRUE;
+
+	if (contact->prefix && strlen(contact->prefix) > 0)
+		return TRUE;
+
+	if (contact->suffix && strlen(contact->suffix) > 0)
+		return TRUE;
+
+	/* none of the personal data fields is present*/
+	return FALSE;
+}
+
 static void vcard_printf_name(GString *vcards,
 					struct phonebook_contact *contact)
 {
+	if (contact_fields_present(contact) == FALSE) {
+		/*
+		 * If fields are empty, then add only 'N:' as parameter.
+		 * This is crucial for some devices ( Nokia BH-903) which
+		 * have problems with history listings - can't determine
+		 * if parameter is really empty if there are unnecessary
+		 * characters after'N:' (like 'N:;;;;').
+		 * We need to add only'N:' param - without semicolons.
+		 */
+		vcard_printf(vcards, "N:");
+		return;
+	}
+
 	vcard_printf(vcards, "N:%s;%s;%s;%s;%s", contact->family,
 				contact->given, contact->additional,
 				contact->prefix, contact->suffix);
-- 
1.6.0.4


^ permalink raw reply related

* Re: [PATCH 1/1] Bluetooth: hidp: Add support for hidraw    HIDIOCGFEATURE    and HIDIOCSFEATURE
From: Alan Ott @ 2010-07-09 14:06 UTC (permalink / raw)
  To: Marcel Holtmann
  Cc: David S Miller, Jiri Kosina, Michael Poole, Bastien Nocera,
	Eric Dumazet, linux-bluetooth, linux-kernel, netdev
In-Reply-To: <1278680790.10421.106.camel@localhost.localdomain>

On 07/09/2010 09:06 AM, Marcel Holtmann wrote:
>>>>> I looked at this and I am bit worried that this should not be done in
>>>>> this detail in the HIDP driver. Essentially HIDP is a pure transport
>>>>> driver. It should not handle all these details. Can we make this a bit
>>>>> easier for the transport drivers to support such features?
>>>>>
>>>>>            
>>>> I put these changes (most notably the addition of hidp_get_raw_report())
>>>> in hidp because that's where the parallel function
>>>> hidp_output_raw_report() was already located. I figured the input should
>>>> go with the output. That said, if there's a better place for both of
>>>> them (input and output) to go, let me know where you think it should be,
>>>> and I'll get them moved into the proper spot.
>>>>
>>>> I'm not sure what you mean about HIDP being a pure transport driver.
>>>>
>>>>          
>>> what is usb-hid.ko doing here? I would expect a bunch of code
>>> duplication with minor difference between USB and Bluetooth.
>>>        
>> usbhid doesn't have a lot of code for hidraw. Two functions are involved:
>>       usbhid_output_raw_report()
>>           - calls usb_control_msg() with Get_Report
>>       usbhid_get_raw_report()
>>           - calls usb_control_msg() with Set_Report
>>               OR
>>           - calls usb_interrupt_msg() on the Ouput pipe.
>>
>> This is of course easier than bluetooth because usb_control_msg() is
>> synchronous, even when requesting reports, mostly because of the nature
>> of USB, where the request and response are part of the same transfer.
>>
>> For Bluetooth, it's a bit more complicated since the kernel treats it
>> more like a networking interface (and indeed it is). My understanding is
>> that to make a synchronous transfer in bluetooth, one must:
>>       - send the request packet
>>       - block (wait_event_*())
>>       - when the response is received in the input handler, wake_up_*().
>>
>> There's not really any code duplication, mostly because initiating
>> synchronous USB transfers (input and output) is easy (because of the
>> usb_*_msg() functions), while making synchronous Bluetooth transfers
>> must be done manually. If there's a nice, convenient, synchronous
>> function in Bluetooth similar to usb_control_msg() that I've missed,
>> then let me know, as it would simplify this whole thing.
>>      
> there is not and I don't think we ever get one. My question here was
> more in the direction why HID core is doing these synchronously in the
> first place. Especially since USB can do everything async as well.
>
> Regards
>
> Marcel
>    

Hi Marcel,

I'm open to suggestions. The way I see it is from a user space 
perspective. With Get_Feature being on an ioctl(), I don't see any clean 
way to do it other than synchronously. Other operating systems (I can 
say for sure Windows, Mac OS X, and FreeBSD) handle Get/Set Feature the 
same way (synchronously) from user space.

You seem to be proposing an asynchronous interface. What would that look 
like from user space?

Alan.

^ permalink raw reply

* Re: Proposed API for HDP
From: Elvis Pfützenreuter @ 2010-07-09 14:04 UTC (permalink / raw)
  To: José Antonio Santos Cadenas; +Cc: linux-bluetooth
In-Reply-To: <201007091549.42621.santoscadenas@gmail.com>

Hi,

On Jul 9, 2010, at 10:49 AM, José Antonio Santos Cadenas wrote:

> I wrote a new API based on the changes suggested in this thread.
> 
> Regards.
> 
> Jose.
> 
> 
> BlueZ D-Bus Health API description
> **********************************
> 
> 	Santiago Carot-Nemesio <sancane@gmail.com>
> 	José Antonio Santos-Cadenas <santoscadenas@gmail.com>
> 	Elvis Pfützenreuter <epx@signove.com>
> 
> Health Device Profile hierarchy
> ===============================
> 
> Service		org.bluez
> Interface	org.bluez.HealthManager
> Object path	[variable prefix]/{hci0,hci1,...}
> 
> Methods:
> 
> 	path	RegisterApplication(object path, dict config)
> 
> 		Returns the path of the new registered application. The path
> 		parameter is the path of the object with the callbacks to
> 		notify events (see org.bluez.HealthAgent at the end of this
> 		document)
> 		This petition starts an mcap instance on every adapter and also
> 		register a proper record in the SDP if is needed.
> 
> 		Dict is defined as bellow:
> 		{
> 		  "end_points" : [{ (optional)
> 			"role" : ("source" or "sink"), (mandatory)
> 			"specs" :[{ (mandatory)
> 				"data_type" : uint16, (mandatory)
> 				"description" : string, (optional)
> 			}]
> 		  }]
> 		}
> 
> 		Application will be closed by the call or implicitly when the
> 		programs leaves the bus.
> 
> 		Possible errors: org.bluez.Error.InvalidArguments
> 
> 	void	UnregisterApplication(path application)
> 
> 		Closes the HDP application identified by the object path. Also
> 		application will be closed if the process that started it leaves
> 		the bus. If there is a SDP record associated to this application
> 		it will also be removed.
> 
> 		Possible errors: org.bluez.Error.InvalidArguments
> 				org.bluez.Error.NotFound
> 
> 	void UpdateServices()
> 
> 		This method searches for HDP applications on the all remote
> 		devices and notifies them to the appropriate agents.
> 
> --------------------------------------------------------------------------------
> 
> Service		org.bluez
> Interface	org.bluez.HealthService
> Object path	[variable prefix]/{hci0,hci1,...}/dev_XX_XX_XX_XX_XX_XX/hdp_YYYY
> 
> Methods:
> 
> 	array GetProperties()
> 
> 		Gets the information of the remote application published on its
> 		SDP record. The returned data format is as follows:
> 
> 		{
> 			"end_points": [
> 				"mdepid": uint8,
> 				"role"  : "source" or "sink" ,
> 				"specs" : [{
> 					"dtype"       : uint16,
> 					"description" : string, (optional)
> 					}]
> 				]
> 		}
> 
> 	boolean Echo(array{byte})
> 
> 		Sends an echo petition to the remote service. Returns True if
> 		response matches with the buffer sent. If some error is detected
> 		False value is returned and the associated MCL is closed.
> 
> 	path OpenDataChannel(byte mdepid, string conf)
> 
> 		Creates a new data channel with the indicated config to the
> 		remote MCAP Data End Point (MDEP).
> 		The configuration should indicate the channel quality of
> 		service using one of this values "reliable", "streaming", "any".
> 
> 		Returns the data channel path.
> 
> 		Possible errors: org.bluez.Error.InvalidArguments
> 				org.bluez.Error.HealthError
> 
> 	void ReconnectDataChannel(path data_channel)
> 
> 		Reconnects a previously created data channel indicated by its
> 		path.
> 
> 		Possible errors: org.bluez.Error.InvalidArguments
> 				org.bluez.Error.HealthError
> 				org.bluez.Error.NotFound
> 
> 	void DeleteDataChannel(path data_channel)
> 
> 		Deletes a data channel so it will not be available.
> 
> 		Possible errors: org.bluez.Error.InvalidArguments
> 				org.bluez.Error.NotFound
> 				org.bluez.Error.HealthError
> 
> 	void DeleteAllDataChannels()
> 
> 		Deletes all data channels so they will not be available for
> 		future use.
> 
> 		Possible errors: org.bluez.Error.HealthError
> 
> HealthAgent hierarchy
> =====================
> 
> (this object is implemented by the HDP user in order to receive notifications)
> 
> Service		unique name
> Interface	org.bluez.HealthAgent
> Object path	freely definable
> 
> Methods:
> 
> 	void ServiceDiscovered(object path)
> 
> 		This method is called when a device containing an HDP
> 		application is paired or when the method Update of the
> 		HealthManager is called and new HealthServices are discovered.
> 		The object path is the HealthService path. The method will be
> 		called for each HealthService.
> 
> (Not shure if this should be added)
> 	void ServiceRemoved(object path)
> 
> 		This method is called if during an Update some HealthServices
> 		have disappeared. The method is called once for each removed
> 		HealthService.
> 
> 	void DataChannelCreated(object path, path data_channel, string conf,
> 							fd file_descriptor)
> 
> 		This method is called when a new data channel is created.
> 
> 		The path contains the object path of the HealthService that
> 		created the connection, the data_channel is the path that
> 		identifies the data channel, conf is the quality of service of
> 		the data channel ("reliable" or "streaming") and file_descriptor
> 		the file descriptor for reading and writing data.
> 
> 	void DataChannelReconnected(object path, path data_channel, string conf,
> 							fd file_descriptor))
> 
> 		This method is called when a closed data channel is reconnected
> 		by the remote device.
> 
> 		The path contains the object path of the HealthService that
> 		reconnected the channel. data_channel is the path that
> 		identifies the data channel, conf is the quality of service of
> 		the data channel ("reliable" or "streaming") and file_descriptor
> 		is the file descriptor for reading and writing data.

I think that only one method for receiving both channel and creation and reconnection events is better. An additional boolean parameter like "new" or "reconnection" should suffice.

And we need to know which MDEP ID the remote side connected to. Otherwise, an acceptor would never know which endpoint the remote device actually connected to. (The initiator knows because it passed the MDEP ID in OpenDataChannel()).

How will the application detect a simple disconnection (which does not delete the data channel?) It is easily detectable by file descriptor closure, but this must be explicitly mentioned in API, so nobody keeps looking for a "disconnected" method.

Finally, the initiator call is Open...() and the method here is ...Created(). I guess both should use the same nomenclature.

> 
> 	void DataChannelRemoved(object path, path data_channel)
> 
> 		This method is called when a data channel is deleted.
> 
> 		After this call the data channel path will not be valid and can
> 		be reused for future creation of data channels. The path is the
> 		path of the HealthService that removes the data channel.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


^ permalink raw reply

* Re: Proposed API for HDP
From: José Antonio Santos Cadenas @ 2010-07-09 13:49 UTC (permalink / raw)
  To: linux-bluetooth
In-Reply-To: <201007081912.31407.santoscadenas@gmail.com>

I wrote a new API based on the changes suggested in this thread.

Regards.

Jose.


BlueZ D-Bus Health API description
**********************************

	Santiago Carot-Nemesio <sancane@gmail.com>
	José Antonio Santos-Cadenas <santoscadenas@gmail.com>
	Elvis Pfützenreuter <epx@signove.com>

Health Device Profile hierarchy
===============================

Service		org.bluez
Interface	org.bluez.HealthManager
Object path	[variable prefix]/{hci0,hci1,...}

Methods:

	path	RegisterApplication(object path, dict config)

		Returns the path of the new registered application. The path
		parameter is the path of the object with the callbacks to
		notify events (see org.bluez.HealthAgent at the end of this
		document)
		This petition starts an mcap instance on every adapter and also
		register a proper record in the SDP if is needed.

		Dict is defined as bellow:
		{
		  "end_points" : [{ (optional)
			"role" : ("source" or "sink"), (mandatory)
			"specs" :[{ (mandatory)
				"data_type" : uint16, (mandatory)
				"description" : string, (optional)
			}]
		  }]
		}

		Application will be closed by the call or implicitly when the
		programs leaves the bus.

		Possible errors: org.bluez.Error.InvalidArguments

	void	UnregisterApplication(path application)

		Closes the HDP application identified by the object path. Also
		application will be closed if the process that started it leaves
		the bus. If there is a SDP record associated to this application
		it will also be removed.

		Possible errors: org.bluez.Error.InvalidArguments
				org.bluez.Error.NotFound

	void UpdateServices()

		This method searches for HDP applications on the all remote
		devices and notifies them to the appropriate agents.

--------------------------------------------------------------------------------

Service		org.bluez
Interface	org.bluez.HealthService
Object path	[variable prefix]/{hci0,hci1,...}/dev_XX_XX_XX_XX_XX_XX/hdp_YYYY

Methods:

	array GetProperties()

		Gets the information of the remote application published on its
		SDP record. The returned data format is as follows:

		{
			"end_points": [
				"mdepid": uint8,
				"role"  : "source" or "sink" ,
				"specs" : [{
					"dtype"       : uint16,
					"description" : string, (optional)
					}]
				]
		}

	boolean Echo(array{byte})

		Sends an echo petition to the remote service. Returns True if
		response matches with the buffer sent. If some error is detected
		False value is returned and the associated MCL is closed.

	path OpenDataChannel(byte mdepid, string conf)

		Creates a new data channel with the indicated config to the
		remote MCAP Data End Point (MDEP).
		The configuration should indicate the channel quality of
		service using one of this values "reliable", "streaming", "any".

		Returns the data channel path.

		Possible errors: org.bluez.Error.InvalidArguments
				org.bluez.Error.HealthError

	void ReconnectDataChannel(path data_channel)

		Reconnects a previously created data channel indicated by its
		path.

		Possible errors: org.bluez.Error.InvalidArguments
				org.bluez.Error.HealthError
				org.bluez.Error.NotFound

	void DeleteDataChannel(path data_channel)

		Deletes a data channel so it will not be available.

		Possible errors: org.bluez.Error.InvalidArguments
				org.bluez.Error.NotFound
				org.bluez.Error.HealthError

	void DeleteAllDataChannels()

		Deletes all data channels so they will not be available for
		future use.

		Possible errors: org.bluez.Error.HealthError

HealthAgent hierarchy
=====================

(this object is implemented by the HDP user in order to receive notifications)

Service		unique name
Interface	org.bluez.HealthAgent
Object path	freely definable

Methods:

	void ServiceDiscovered(object path)

		This method is called when a device containing an HDP
		application is paired or when the method Update of the
		HealthManager is called and new HealthServices are discovered.
		The object path is the HealthService path. The method will be
		called for each HealthService.

(Not shure if this should be added)
	void ServiceRemoved(object path)

		This method is called if during an Update some HealthServices
		have disappeared. The method is called once for each removed
		HealthService.

	void DataChannelCreated(object path, path data_channel, string conf,
							fd file_descriptor)

		This method is called when a new data channel is created.

		The path contains the object path of the HealthService that
		created the connection, the data_channel is the path that
		identifies the data channel, conf is the quality of service of
		the data channel ("reliable" or "streaming") and file_descriptor
		the file descriptor for reading and writing data.

	void DataChannelReconnected(object path, path data_channel, string conf,
							fd file_descriptor))

		This method is called when a closed data channel is reconnected
		by the remote device.

		The path contains the object path of the HealthService that
		reconnected the channel. data_channel is the path that
		identifies the data channel, conf is the quality of service of
		the data channel ("reliable" or "streaming") and file_descriptor
		is the file descriptor for reading and writing data.

	void DataChannelRemoved(object path, path data_channel)

		This method is called when a data channel is deleted.

		After this call the data channel path will not be valid and can
		be reused for future creation of data channels. The path is the
		path of the HealthService that removes the data channel.

^ permalink raw reply

* Re: [PATCH] Added support for bluetooth controller shipped with iMac i5
From: Marcel Holtmann @ 2010-07-09 13:21 UTC (permalink / raw)
  To: Cyril Lacoux; +Cc: linux-bluetooth, Cyril Lacoux
In-Reply-To: <1278671689-9444-1-git-send-email-clacoux@ifeelgood.org>

Hi Cyril,

> Device class is ff(vend.) instead of e0(wlcon).
> 
> Output from command `usb-devices`:
> T:  Bus=01 Lev=03 Prnt=03 Port=00 Cnt=01 Dev#=  6 Spd=12  MxCh= 0
> D:  Ver= 2.00 Cls=ff(vend.) Sub=01 Prot=01 MxPS=64 #Cfgs=  1
> P:  Vendor=05ac ProdID=8215 Rev=01.82
> S:  Manufacturer=Apple Inc.
> S:  Product=Bluetooth USB Host Controller
> S:  SerialNumber=7C6D62936607
> C:  #Ifs= 4 Cfg#= 1 Atr=e0 MxPwr=0mA
> I:  If#= 0 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=01 Prot=01 Driver=btusb
> I:  If#= 1 Alt= 0 #EPs= 2 Cls=e0(wlcon) Sub=01 Prot=01 Driver=btusb
> I:  If#= 2 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=ff Driver=(none)
> I:  If#= 3 Alt= 0 #EPs= 0 Cls=fe(app. ) Sub=01 Prot=00 Driver=(none)

you need to add a Signed-off-by: line here.

> ---
>  drivers/bluetooth/btusb.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index 5d9cc53..8eb6be5 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -75,6 +75,9 @@ static struct usb_device_id btusb_table[] = {
>  	/* Canyon CN-BTU1 with HID interfaces */
>  	{ USB_DEVICE(0x0c10, 0x0000) },
>  
> +	/* Apple iMac11,1 */
> +	{ USB_DEVICE(0x05ac, 0x8215) },
> +

And since this a more likely device since all the other ones that are
not even sold anymore, just put it after the generic entry and before
the AVM one.

Regards

Marcel



^ permalink raw reply

* Re: [PATCH v4 1/3] Implements hci_reassembly to reassemble Rx packets
From: Marcel Holtmann @ 2010-07-09 13:16 UTC (permalink / raw)
  To: suraj; +Cc: linux-bluetooth, Luis.Rodriguez, Jothikumar.Mothilal
In-Reply-To: <1278672569.4048.15.camel@atheros013-desktop>

Hi Suraj,

you do need to fix your email or at least add a From: line. Otherwise
when I apply the patch this gets screwed up.

> Implements feature to reassemble HCI frames received from an input stream.
> 
> Signed-off-by: Suraj Sumangala <suraj@atheros.com>
> ---
>  net/bluetooth/hci_core.c |  115 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 115 insertions(+), 0 deletions(-)
> 
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 5e83f8e..3eb4c1b 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -1030,6 +1030,121 @@ int hci_recv_frame(struct sk_buff *skb)
>  }
>  EXPORT_SYMBOL(hci_recv_frame);
>  
> +static int hci_reassembly(struct hci_dev *hdev, int type, void *data,
> +			  int count, struct sk_buff **skb_ptr, int *remain)

Why are we not not just return remain and err combined. Negative's are
errors positives are remains.

> +{
> +	int len = 0;
> +	int header_len = 0;
> +	struct sk_buff *skb = *skb_ptr;
> +	struct { struct bt_skb_cb cb_info; int expect; } *scb;
> +
> +	*remain = count;
> +
> +	if (type < HCI_ACLDATA_PKT || type > HCI_EVENT_PKT)
> +		return -EILSEQ;
> +
> +	if (!skb) {
> +
> +		switch (type) {

Remove this empty line here.

> +		case HCI_ACLDATA_PKT:
> +			len = HCI_MAX_FRAME_SIZE;
> +			header_len = HCI_ACL_HDR_SIZE;
> +		break;

The break needs to be on the same line as the code.

> +		case HCI_EVENT_PKT:
> +			len = HCI_MAX_EVENT_SIZE;
> +			header_len = HCI_EVENT_HDR_SIZE;
> +		break;
> +		case HCI_SCODATA_PKT:
> +			len = HCI_MAX_SCO_SIZE;
> +			header_len = HCI_SCO_HDR_SIZE;
> +		break;
> +		}
> +
> +		skb = bt_skb_alloc(len, GFP_ATOMIC);
> +
> +		if (!skb)

Remove the empty line here.

Do we always need GFP_ATOMIC here. I would prefer that we make that an
option of the function caller.

> +			return -ENOMEM;
> +
> +		scb = (void *) skb->cb;
> +		scb->expect = header_len;
> +		scb->cb_info.pkt_type = (__u8)type;

It is "(__u8) type" when casting.

> +
> +		skb->dev = (void *) hdev;
> +		*skb_ptr = skb;
> +
> +	}
> +
> +	while (count) {
> +
> +		scb = (void *) skb->cb;

Remove empty line here.

> +		len = min(scb->expect, count);
> +
> +		memcpy(skb_put(skb, len), data, len);
> +
> +		count -= len;
> +		data += len;
> +		scb->expect -= len;
> +		*remain = count;
> +
> +		switch (type) {
> +		case HCI_EVENT_PKT:
> +			if (skb->len == HCI_EVENT_HDR_SIZE) {
> +				struct hci_event_hdr *h = hci_event_hdr(skb);
> +				scb->expect = h->plen;
> +
> +				if (skb_tailroom(skb) < scb->expect) {
> +					kfree_skb(skb);
> +					*skb_ptr = NULL;
> +
> +					return -ENOMEM;

Remove this empty line.

> +				}
> +			}
> +			break;
> +
> +		case HCI_ACLDATA_PKT:
> +			if (skb->len  == HCI_ACL_HDR_SIZE) {
> +				struct hci_acl_hdr *h = hci_acl_hdr(skb);
> +				scb->expect = __le16_to_cpu(h->dlen);
> +
> +				if (skb_tailroom(skb) < scb->expect) {
> +					kfree_skb(skb);
> +					*skb_ptr = NULL;
> +
> +					return -ENOMEM;

Same here.

> +				}
> +			}
> +			break;
> +
> +		case HCI_SCODATA_PKT:
> +			if (skb->len == HCI_SCO_HDR_SIZE) {
> +				struct hci_sco_hdr *h = hci_sco_hdr(skb);
> +				scb->expect = h->dlen;
> +
> +				if (skb_tailroom(skb) < scb->expect) {
> +					kfree_skb(skb);
> +					*skb_ptr = NULL;
> +
> +					return -ENOMEM;

And here.

> +				}
> +			}
> +			break;
> +		}
> +
> +		if (scb->expect == 0) {
> +
> +			/* Complete frame */

Remove the empty line here.

> +			bt_cb(skb)->pkt_type = type;
> +			hci_recv_frame(skb);
> +
> +			*skb_ptr = NULL;
> +
> +			return 0;

And here.

> +		}
> +
> +	}

What is this empty line for. Please remove.

> +
> +	return 0;
> +}
>  /* Receive packet type fragment */

And here we need an extra empty line.

>  #define __reassembly(hdev, type)  ((hdev)->reassembly[(type) - 2])

Regards

Marcel



^ permalink raw reply

* Re: [PATCH 1/1] Bluetooth: hidp: Add support for hidraw   HIDIOCGFEATURE   and HIDIOCSFEATURE
From: Marcel Holtmann @ 2010-07-09 13:06 UTC (permalink / raw)
  To: Alan Ott
  Cc: David S Miller, Jiri Kosina, Michael Poole, Bastien Nocera,
	Eric Dumazet, linux-bluetooth, linux-kernel, netdev
In-Reply-To: <4C371DE8.9020002@signal11.us>

Hi Alan,

> >>> I looked at this and I am bit worried that this should not be done in
> >>> this detail in the HIDP driver. Essentially HIDP is a pure transport
> >>> driver. It should not handle all these details. Can we make this a bit
> >>> easier for the transport drivers to support such features?
> >>>        
> >> I put these changes (most notably the addition of hidp_get_raw_report())
> >> in hidp because that's where the parallel function
> >> hidp_output_raw_report() was already located. I figured the input should
> >> go with the output. That said, if there's a better place for both of
> >> them (input and output) to go, let me know where you think it should be,
> >> and I'll get them moved into the proper spot.
> >>
> >> I'm not sure what you mean about HIDP being a pure transport driver.
> >>      
> > what is usb-hid.ko doing here? I would expect a bunch of code
> > duplication with minor difference between USB and Bluetooth.
>
> usbhid doesn't have a lot of code for hidraw. Two functions are involved:
>      usbhid_output_raw_report()
>          - calls usb_control_msg() with Get_Report
>      usbhid_get_raw_report()
>          - calls usb_control_msg() with Set_Report
>              OR
>          - calls usb_interrupt_msg() on the Ouput pipe.
> 
> This is of course easier than bluetooth because usb_control_msg() is 
> synchronous, even when requesting reports, mostly because of the nature 
> of USB, where the request and response are part of the same transfer.
> 
> For Bluetooth, it's a bit more complicated since the kernel treats it 
> more like a networking interface (and indeed it is). My understanding is 
> that to make a synchronous transfer in bluetooth, one must:
>      - send the request packet
>      - block (wait_event_*())
>      - when the response is received in the input handler, wake_up_*().
> 
> There's not really any code duplication, mostly because initiating 
> synchronous USB transfers (input and output) is easy (because of the 
> usb_*_msg() functions), while making synchronous Bluetooth transfers 
> must be done manually. If there's a nice, convenient, synchronous 
> function in Bluetooth similar to usb_control_msg() that I've missed, 
> then let me know, as it would simplify this whole thing.

there is not and I don't think we ever get one. My question here was
more in the direction why HID core is doing these synchronously in the
first place. Especially since USB can do everything async as well.

Regards

Marcel



^ permalink raw reply

* Re: [PATCH 1/1] Bluetooth: hidp: Add support for hidraw   HIDIOCGFEATURE   and HIDIOCSFEATURE
From: Alan Ott @ 2010-07-09 13:02 UTC (permalink / raw)
  To: Marcel Holtmann
  Cc: David S Miller, Jiri Kosina, Michael Poole, Bastien Nocera,
	Eric Dumazet, linux-bluetooth, linux-kernel, netdev
In-Reply-To: <1278662497.10421.94.camel@localhost.localdomain>

On 07/09/2010 04:01 AM, Marcel Holtmann wrote:
> Hi Alan,
>
>    
>>> I looked at this and I am bit worried that this should not be done in
>>> this detail in the HIDP driver. Essentially HIDP is a pure transport
>>> driver. It should not handle all these details. Can we make this a bit
>>> easier for the transport drivers to support such features?
>>>        
>> I put these changes (most notably the addition of hidp_get_raw_report())
>> in hidp because that's where the parallel function
>> hidp_output_raw_report() was already located. I figured the input should
>> go with the output. That said, if there's a better place for both of
>> them (input and output) to go, let me know where you think it should be,
>> and I'll get them moved into the proper spot.
>>
>> I'm not sure what you mean about HIDP being a pure transport driver.
>>      
> what is usb-hid.ko doing here? I would expect a bunch of code
> duplication with minor difference between USB and Bluetooth.
>
> Regards
>
> Marcel
>    

Hi Marcel,

usbhid doesn't have a lot of code for hidraw. Two functions are involved:
     usbhid_output_raw_report()
         - calls usb_control_msg() with Get_Report
     usbhid_get_raw_report()
         - calls usb_control_msg() with Set_Report
             OR
         - calls usb_interrupt_msg() on the Ouput pipe.

This is of course easier than bluetooth because usb_control_msg() is 
synchronous, even when requesting reports, mostly because of the nature 
of USB, where the request and response are part of the same transfer.

For Bluetooth, it's a bit more complicated since the kernel treats it 
more like a networking interface (and indeed it is). My understanding is 
that to make a synchronous transfer in bluetooth, one must:
     - send the request packet
     - block (wait_event_*())
     - when the response is received in the input handler, wake_up_*().

There's not really any code duplication, mostly because initiating 
synchronous USB transfers (input and output) is easy (because of the 
usb_*_msg() functions), while making synchronous Bluetooth transfers 
must be done manually. If there's a nice, convenient, synchronous 
function in Bluetooth similar to usb_control_msg() that I've missed, 
then let me know, as it would simplify this whole thing.

See the Set/Get Feature patch, including USB support, here:
     http://lkml.org/lkml/2010/6/9/222

Alan.

^ permalink raw reply

* Re: [PATCH] Fixed empty 'N:' parameter handling for VCARDs
From: Marcel Holtmann @ 2010-07-09 13:00 UTC (permalink / raw)
  To: Radoslaw Jablonski; +Cc: linux-bluetooth
In-Reply-To: <1278670762-10119-1-git-send-email-ext-jablonski.radoslaw@nokia.com>

Hi Radek,

> Bluetooth PBAP specification expects for call history listing,
> that parameter N: shall be empty when we cannot retrieve personal data
> from PSE phone book.
> Some devices(by example BH-903) are having problems when after N: parameter
>  unnecessary characters occurs. List of dialed/incoming calls on carkit
> then is useless - carkit shows only blank lines and it's impossible to
> determine who made call.
> 
> In previous version unnecessary semicolons were added after N:("N:;;;;")
> to represent empty name.
> Now if none of the contact fields is available, then adding real empty "N:"
> parameter (without semicolons).
> ---
>  plugins/vcard.c |   27 +++++++++++++++++++++++++++
>  1 files changed, 27 insertions(+), 0 deletions(-)
> 
> diff --git a/plugins/vcard.c b/plugins/vcard.c
> index 5948a4a..8c7147b 100644
> --- a/plugins/vcard.c
> +++ b/plugins/vcard.c
> @@ -123,6 +123,27 @@ static void add_slash(char *dest, const char *src, int len_max, int len)
>  	return;
>  }
>  
> +/* checks if there is at least one present contact field with personal data */
> +static gboolean contact_fields_present(struct phonebook_contact * contact)
> +{
> +	if (contact->family && strlen(contact->family))
> +		return TRUE;

I asked to use strlen(...) > 0 as test here. Please fix this.

> +
> +	if (contact->given && strlen(contact->given))
> +		return TRUE;
> +	if (contact->additional && strlen(contact->additional))
> +		return TRUE;

Why are these two tight together. Use an extra empty line.

> +	if (contact->prefix && strlen(contact->prefix))
> +		return TRUE;
> +
> +	if (contact->suffix && strlen(contact->suffix))
> +		return TRUE;
> +
> +	/* none of the personal data fields is present*/
> +	return FALSE;
> +}
> +
>  static void vcard_printf_begin(GString *vcards, uint8_t format)
>  {
>  	vcard_printf(vcards, "BEGIN:VCARD");
> @@ -136,6 +157,12 @@ static void vcard_printf_begin(GString *vcards, uint8_t format)
>  static void vcard_printf_name(GString *vcards,
>  					struct phonebook_contact *contact)
>  {
> +	if (contact_fields_present(contact) == FALSE) {
> +		/* if all fields are empty we need to have empty N: parameter */
> +		vcard_printf(vcards, "N:");

I would be good to add a bit more extra text here why we are doing this.
You should mention the broken headset and what is expect outcome.

Also please sort the contact_fields_resent() function about
vcard_printf_name to keep them close together.

Regards

Marcel



^ permalink raw reply

* Re: [PATCH v4 3/3] Implemented HCI frame reassembly for Rx from stream
From: Gustavo F. Padovan @ 2010-07-09 12:59 UTC (permalink / raw)
  To: suraj; +Cc: linux-bluetooth, marcel, Luis.Rodriguez, Jothikumar.Mothilal
In-Reply-To: <1278672821.4048.24.camel@atheros013-desktop>

Hi Suraj,

* suraj <suraj@atheros.com> [2010-07-09 16:23:41 +0530]:

> Implemented frame reassembly implementation for reassembling fragments
> received from stream.
> 
> Signed-off-by: Suraj Sumangala <suraj@atheros.com>
> ---
>  include/net/bluetooth/hci_core.h |    2 +
>  net/bluetooth/hci_core.c         |   43
> ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 45 insertions(+), 0 deletions(-)
> 
> diff --git a/include/net/bluetooth/hci_core.h
> b/include/net/bluetooth/hci_core.h
> index e42f6ed..cd89d66 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -119,6 +119,7 @@ struct hci_dev {
>  
>  	struct sk_buff		*sent_cmd;
>  	struct sk_buff		*reassembly[3];
> +	struct sk_buff		*stream_reassembly;
>  
>  	struct mutex		req_lock;
>  	wait_queue_head_t	req_wait_q;
> @@ -428,6 +429,7 @@ void hci_event_packet(struct hci_dev *hdev, struct
> sk_buff *skb);
>  
>  int hci_recv_frame(struct sk_buff *skb);
>  int hci_recv_fragment(struct hci_dev *hdev, int type, void *data, int
> count);
> +int hci_recv_stream_fragment(struct hci_dev *hdev, void *data, int
> count);
>  
>  int hci_register_sysfs(struct hci_dev *hdev);
>  void hci_unregister_sysfs(struct hci_dev *hdev);
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index db6ca71..ee75c42 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -916,6 +916,8 @@ int hci_register_dev(struct hci_dev *hdev)
>  	for (i = 0; i < 3; i++)
>  		hdev->reassembly[i] = NULL;
>  
> +	hdev->stream_reassembly = NULL;
> +
>  	init_waitqueue_head(&hdev->req_wait_q);
>  	mutex_init(&hdev->req_lock);
>  
> @@ -973,6 +975,8 @@ int hci_unregister_dev(struct hci_dev *hdev)
>  	for (i = 0; i < 3; i++)
>  		kfree_skb(hdev->reassembly[i]);
>  
> +	kfree_skb(hdev->stream_reassembly);
> +
>  	hci_notify(hdev, HCI_DEV_UNREG);
>  
>  	if (hdev->rfkill) {
> @@ -1145,6 +1149,45 @@ static int hci_reassembly(struct hci_dev *hdev,
> int type, void *data,
>  
>  	return 0;
>  }
> +
> +#define __stream_reassembly(hdev)  ((hdev)->stream_reassembly)

That's pointless. Your macro has about the same length of
hdev->stream_reassembly.

> +
> +int hci_recv_stream_fragment(struct hci_dev *hdev, void *data, int
> count)
> +{
> +	int type;
> +	int remaining = 0;
> +	int err = 0;
> +
> +	do {
> +		struct sk_buff *skb = __stream_reassembly(hdev);
> +		if (!skb) {
> +			struct { char type; } *pkt;
> +
> +			/* Start of the frame */
> +			pkt = data;
> +			type = pkt->type;
> +
> +			data++;
> +			count--;
> +		} else
> +			type = bt_cb(skb)->pkt_type;
> +
> +		err = hci_reassembly(hdev, type, data, count, &skb, &remaining);
> +
> +		if (err < 0)
> +			return err;
> +
> +		__stream_reassembly(hdev) = skb;
> +
> +		data += (count - remaining);
> +		count = remaining;
> +
> +	} while (count);
> +
> +	return err;
> +}
> +EXPORT_SYMBOL(hci_recv_stream_fragment);
> +
>  /* Receive packet type fragment */
>  #define __reassembly(hdev, type)  ((hdev)->reassembly[(type) - 2])
>  
> -- 
> 1.7.0.4
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Gustavo F. Padovan
http://padovan.org

^ permalink raw reply

* Re: Proposed API for HDP
From: José Antonio Santos Cadenas @ 2010-07-09 12:46 UTC (permalink / raw)
  To: Gustavo F. Padovan; +Cc: linux-bluetooth
In-Reply-To: <20100708191305.GC26759@vigoh>

El Thursday 08 July 2010 21:13:05 Gustavo F. Padovan escribió:
> Hi José,
> 
> * José Antonio Santos Cadenas <santoscadenas@gmail.com> [2010-07-08 20:36:00 
+0200]:
> > El Thursday 08 July 2010 19:54:30 Gustavo F. Padovan escribió:
> > > Hi José,
> > > 
> > > * José Antonio Santos Cadenas <santoscadenas@gmail.com> [2010-07-08
> > > 19:12:31 +0200]:
> > > 
> > > <snip>
> > > 
> > > > Service		org.bluez
> > > > Interface	org.bluez.HealthDeviceApplication
> > > > Object path	[variable
> > > > prefix]/{hci0,hci1,...}/dev_XX_XX_XX_XX_XX_XX/hdp_YYYY
> > > > 
> > > > Methods:
> > > > 	array GetProperties()
> > > > 	
> > > > 		Gets the information of the remote application published on 
its
> > > > 		SDP record. The returned data format is as follows:
> > > > 		
> > > > 		{
> > > > 		
> > > > 			"end_points": [
> > > > 			
> > > > 				"mdepid": uint8,
> > > > 				"role"  : "source" or "sink" ,
> > > > 				"specs" : [{
> > > > 				
> > > > 					"dtype"       : uint16,
> > > > 					"description" : string, (optional)
> > > > 					}]
> > > > 				
> > > > 				]
> > > > 		
> > > > 		}
> > > > 	
> > > > 	object Connect(path local_application_id)
> > > > 	
> > > > 		Connects the local application with the remote application.
> > > > 		
> > > > 		Only the bus client that created the local session will be 
able
> > > > 		to create connections using it.
> > > > 		
> > > > 		If the Device is already connected with an other application 
an
> > > > 		org.bluez.Error.AlreadyConnected error will be received.
> > > > 		
> > > > 		Possible errors: org.bluez.Error.InvalidArguments
> > > > 		
> > > > 				org.bluez.Error.AlreadyConnected
> > > > 				org.bluez.Error.HealthError
> > > > 	
> > > > 	void Disconnect()
> > > > 	
> > > > 		Disconnect from the remote application the state will also be
> > > > 		deleted. And no future reconnections will be possible. For
> > > > 		keeping the state the method Pause of the health link should 
be
> > > > 		used.
> > > > 		
> > > > 		Possible errors: org.bluez.Error.InvalidArguments
> > > > 		
> > > > 				org.bluez.Error.NotFound
> > > > 				org.bluez.Error.HealthError
> > > > 	
> > > > 	boolean Echo(array{byte})
> > > > 	
> > > > 		Sends an echo petition to the remote intance. Returns True if
> > > > 		response matches with the buffer sent. If some error is 
detected
> > > > 		False value is returned and the associated MCL is closed.
> > > > 	
> > > > 	path OpenDataChannel(byte mdepid, string conf)
> > > > 	
> > > > 		Creates a new data channel with the indicated config to the
> > > > 		remote MCAP Data End Point (MDEP).
> > > > 		The configuration should indicate the channel quality of
> > > > 		service using one of this values "reliable", "streaming", 
"any".
> > > > 		
> > > > 		Returns the data channel path.
> > > > 		
> > > > 		Possible errors: org.bluez.Error.InvalidArguments
> > > > 		
> > > > 				org.bluez.Error.HealthError
> > > > 	
> > > > 	void ReconnectDataChannel(path data_channel)
> > > > 	
> > > > 		Reconnects a previously created data channel indicated by its
> > > > 		path.
> > > > 		
> > > > 		Possible errors: org.bluez.Error.InvalidArguments
> > > > 		
> > > > 				org.bluez.Error.HealthError
> > > > 				org.bluez.Error.NotFound
> > > 
> > > So are we really going to expose the temporally disconnect and
> > > reconnection to the applicantion level? The application should not
> > > care if the data channel is there or not.
> > 
> > We changed our mind about this issue because there are cases were an
> > implicit reconnection will not be possible (when the remote do not
> > publish an SDP record) so if the application is concerned about this
> > issues could try to avoid this kind of failure waiting for the remote to
> > reconnect the data channel.
> 
> Is this better than simplify the API removing the Reconnection stuff?
> On average how much time takes to reconnect achieve the timeout?
> BTW, failure can occur on OpenDataChannel as well, that just happens.

I think it is better because you can deal with more situations. Imagine that 
reconnections are performed implicitly. When you try to send data over a 
closed channel, a reconnection will be attempted and fail because the remote 
does not publish an SDP record. In this case the application perceives a 
failure sending data and the channel will be ignored definitely. In the other 
hand, if reconnections are explicit you know that the failure was during the 
reconnection and it is up to the application to decide if the channel should 
be closed or just wait until the remote device reconnects it.

> 
> > > > 	int GetDataChannelFileDescriptor(path data_channel)
> > > > 	
> > > > 		Gets a file descriptor where data can be read or written.
> > > > 		
> > > > 		Possible errors: org.bluez.Error.InvalidArguments
> > > > 		
> > > > 				org.bluez.Error.NotFound
> > > > 				org.bluez.Error.HealthError
> > > 
> > > What about the fd-passing feature of D-Bus here?
> > 
> > There is an error here, the return value is not an integer is a file
> > descriptor.
> 
> No. The fd should go to the Agent automatically, once you try to
> open a data channel a callback in the Application should be registered.
> The callback is a function in the Agent API  with the fd as parameter,
> that will be called once the connection in the lower layer is done.
> See the NewConnection method from the HandsfreeAgent Interface.

^ permalink raw reply

* [PATCH v4 3/3] Implemented HCI frame reassembly for Rx from stream
From: suraj @ 2010-07-09 10:53 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: marcel, Luis.Rodriguez, Jothikumar.Mothilal
In-Reply-To: <1278672702.4048.19.camel@atheros013-desktop>

Implemented frame reassembly implementation for reassembling fragments
received from stream.

Signed-off-by: Suraj Sumangala <suraj@atheros.com>
---
 include/net/bluetooth/hci_core.h |    2 +
 net/bluetooth/hci_core.c         |   43
++++++++++++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+), 0 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h
b/include/net/bluetooth/hci_core.h
index e42f6ed..cd89d66 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -119,6 +119,7 @@ struct hci_dev {
 
 	struct sk_buff		*sent_cmd;
 	struct sk_buff		*reassembly[3];
+	struct sk_buff		*stream_reassembly;
 
 	struct mutex		req_lock;
 	wait_queue_head_t	req_wait_q;
@@ -428,6 +429,7 @@ void hci_event_packet(struct hci_dev *hdev, struct
sk_buff *skb);
 
 int hci_recv_frame(struct sk_buff *skb);
 int hci_recv_fragment(struct hci_dev *hdev, int type, void *data, int
count);
+int hci_recv_stream_fragment(struct hci_dev *hdev, void *data, int
count);
 
 int hci_register_sysfs(struct hci_dev *hdev);
 void hci_unregister_sysfs(struct hci_dev *hdev);
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index db6ca71..ee75c42 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -916,6 +916,8 @@ int hci_register_dev(struct hci_dev *hdev)
 	for (i = 0; i < 3; i++)
 		hdev->reassembly[i] = NULL;
 
+	hdev->stream_reassembly = NULL;
+
 	init_waitqueue_head(&hdev->req_wait_q);
 	mutex_init(&hdev->req_lock);
 
@@ -973,6 +975,8 @@ int hci_unregister_dev(struct hci_dev *hdev)
 	for (i = 0; i < 3; i++)
 		kfree_skb(hdev->reassembly[i]);
 
+	kfree_skb(hdev->stream_reassembly);
+
 	hci_notify(hdev, HCI_DEV_UNREG);
 
 	if (hdev->rfkill) {
@@ -1145,6 +1149,45 @@ static int hci_reassembly(struct hci_dev *hdev,
int type, void *data,
 
 	return 0;
 }
+
+#define __stream_reassembly(hdev)  ((hdev)->stream_reassembly)
+
+int hci_recv_stream_fragment(struct hci_dev *hdev, void *data, int
count)
+{
+	int type;
+	int remaining = 0;
+	int err = 0;
+
+	do {
+		struct sk_buff *skb = __stream_reassembly(hdev);
+		if (!skb) {
+			struct { char type; } *pkt;
+
+			/* Start of the frame */
+			pkt = data;
+			type = pkt->type;
+
+			data++;
+			count--;
+		} else
+			type = bt_cb(skb)->pkt_type;
+
+		err = hci_reassembly(hdev, type, data, count, &skb, &remaining);
+
+		if (err < 0)
+			return err;
+
+		__stream_reassembly(hdev) = skb;
+
+		data += (count - remaining);
+		count = remaining;
+
+	} while (count);
+
+	return err;
+}
+EXPORT_SYMBOL(hci_recv_stream_fragment);
+
 /* Receive packet type fragment */
 #define __reassembly(hdev, type)  ((hdev)->reassembly[(type) - 2])
 
-- 
1.7.0.4

^ permalink raw reply related

* [PATCH v4 2/3] Modified hci_recv_fragment() to use hci_reassembly
From: suraj @ 2010-07-09 10:51 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: marcel, Luis.Rodriguez, Jothikumar.Mothilal
In-Reply-To: <1278672569.4048.15.camel@atheros013-desktop>

modified packet based reassembly function hci_recv_fragment() to use hci_reassembly()

Signed-off-by: Suraj Sumangala <suraj@atheros.com>
---
 net/bluetooth/hci_core.c |   78 +++++++--------------------------------------
 1 files changed, 12 insertions(+), 66 deletions(-)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 3eb4c1b..db6ca71 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1150,82 +1150,28 @@ static int hci_reassembly(struct hci_dev *hdev, int type, void *data,
 
 int hci_recv_fragment(struct hci_dev *hdev, int type, void *data, int count)
 {
+	int remaining = 0;
+	int err = 0;
+
 	if (type < HCI_ACLDATA_PKT || type > HCI_EVENT_PKT)
 		return -EILSEQ;
 
-	while (count) {
+	do {
 		struct sk_buff *skb = __reassembly(hdev, type);
-		struct { int expect; } *scb;
-		int len = 0;
-
-		if (!skb) {
-			/* Start of the frame */
-
-			switch (type) {
-			case HCI_EVENT_PKT:
-				if (count >= HCI_EVENT_HDR_SIZE) {
-					struct hci_event_hdr *h = data;
-					len = HCI_EVENT_HDR_SIZE + h->plen;
-				} else
-					return -EILSEQ;
-				break;
 
-			case HCI_ACLDATA_PKT:
-				if (count >= HCI_ACL_HDR_SIZE) {
-					struct hci_acl_hdr *h = data;
-					len = HCI_ACL_HDR_SIZE + __le16_to_cpu(h->dlen);
-				} else
-					return -EILSEQ;
-				break;
+		err = hci_reassembly(hdev, type, data, count, &skb, &remaining);
 
-			case HCI_SCODATA_PKT:
-				if (count >= HCI_SCO_HDR_SIZE) {
-					struct hci_sco_hdr *h = data;
-					len = HCI_SCO_HDR_SIZE + h->dlen;
-				} else
-					return -EILSEQ;
-				break;
-			}
+		if (err < 0)
+			return err;
 
-			skb = bt_skb_alloc(len, GFP_ATOMIC);
-			if (!skb) {
-				BT_ERR("%s no memory for packet", hdev->name);
-				return -ENOMEM;
-			}
-
-			skb->dev = (void *) hdev;
-			bt_cb(skb)->pkt_type = type;
+		__reassembly(hdev, type) = skb;
 
-			__reassembly(hdev, type) = skb;
-
-			scb = (void *) skb->cb;
-			scb->expect = len;
-		} else {
-			/* Continuation */
-
-			scb = (void *) skb->cb;
-			len = scb->expect;
-		}
+		data += (count - remaining);
+		count = remaining;
 
-		len = min(len, count);
+	} while (count);
 
-		memcpy(skb_put(skb, len), data, len);
-
-		scb->expect -= len;
-
-		if (scb->expect == 0) {
-			/* Complete frame */
-
-			__reassembly(hdev, type) = NULL;
-
-			bt_cb(skb)->pkt_type = type;
-			hci_recv_frame(skb);
-		}
-
-		count -= len; data += len;
-	}
-
-	return 0;
+	return err;
 }
 EXPORT_SYMBOL(hci_recv_fragment);
 
-- 
1.7.0.4

^ permalink raw reply related

* [PATCH v4 1/3] Implements hci_reassembly to reassemble Rx packets
From: suraj @ 2010-07-09 10:49 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: marcel, Luis.Rodriguez, Jothikumar.Mothilal

Implements feature to reassemble HCI frames received from an input stream.

Signed-off-by: Suraj Sumangala <suraj@atheros.com>
---
 net/bluetooth/hci_core.c |  115 ++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 115 insertions(+), 0 deletions(-)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 5e83f8e..3eb4c1b 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1030,6 +1030,121 @@ int hci_recv_frame(struct sk_buff *skb)
 }
 EXPORT_SYMBOL(hci_recv_frame);
 
+static int hci_reassembly(struct hci_dev *hdev, int type, void *data,
+			  int count, struct sk_buff **skb_ptr, int *remain)
+{
+	int len = 0;
+	int header_len = 0;
+	struct sk_buff *skb = *skb_ptr;
+	struct { struct bt_skb_cb cb_info; int expect; } *scb;
+
+	*remain = count;
+
+	if (type < HCI_ACLDATA_PKT || type > HCI_EVENT_PKT)
+		return -EILSEQ;
+
+	if (!skb) {
+
+		switch (type) {
+		case HCI_ACLDATA_PKT:
+			len = HCI_MAX_FRAME_SIZE;
+			header_len = HCI_ACL_HDR_SIZE;
+		break;
+		case HCI_EVENT_PKT:
+			len = HCI_MAX_EVENT_SIZE;
+			header_len = HCI_EVENT_HDR_SIZE;
+		break;
+		case HCI_SCODATA_PKT:
+			len = HCI_MAX_SCO_SIZE;
+			header_len = HCI_SCO_HDR_SIZE;
+		break;
+		}
+
+		skb = bt_skb_alloc(len, GFP_ATOMIC);
+
+		if (!skb)
+			return -ENOMEM;
+
+		scb = (void *) skb->cb;
+		scb->expect = header_len;
+		scb->cb_info.pkt_type = (__u8)type;
+
+		skb->dev = (void *) hdev;
+		*skb_ptr = skb;
+
+	}
+
+	while (count) {
+
+		scb = (void *) skb->cb;
+		len = min(scb->expect, count);
+
+		memcpy(skb_put(skb, len), data, len);
+
+		count -= len;
+		data += len;
+		scb->expect -= len;
+		*remain = count;
+
+		switch (type) {
+		case HCI_EVENT_PKT:
+			if (skb->len == HCI_EVENT_HDR_SIZE) {
+				struct hci_event_hdr *h = hci_event_hdr(skb);
+				scb->expect = h->plen;
+
+				if (skb_tailroom(skb) < scb->expect) {
+					kfree_skb(skb);
+					*skb_ptr = NULL;
+
+					return -ENOMEM;
+				}
+			}
+			break;
+
+		case HCI_ACLDATA_PKT:
+			if (skb->len  == HCI_ACL_HDR_SIZE) {
+				struct hci_acl_hdr *h = hci_acl_hdr(skb);
+				scb->expect = __le16_to_cpu(h->dlen);
+
+				if (skb_tailroom(skb) < scb->expect) {
+					kfree_skb(skb);
+					*skb_ptr = NULL;
+
+					return -ENOMEM;
+				}
+			}
+			break;
+
+		case HCI_SCODATA_PKT:
+			if (skb->len == HCI_SCO_HDR_SIZE) {
+				struct hci_sco_hdr *h = hci_sco_hdr(skb);
+				scb->expect = h->dlen;
+
+				if (skb_tailroom(skb) < scb->expect) {
+					kfree_skb(skb);
+					*skb_ptr = NULL;
+
+					return -ENOMEM;
+				}
+			}
+			break;
+		}
+
+		if (scb->expect == 0) {
+
+			/* Complete frame */
+			bt_cb(skb)->pkt_type = type;
+			hci_recv_frame(skb);
+
+			*skb_ptr = NULL;
+
+			return 0;
+		}
+
+	}
+
+	return 0;
+}
 /* Receive packet type fragment */
 #define __reassembly(hdev, type)  ((hdev)->reassembly[(type) - 2])
 
-- 
1.7.0.4




^ permalink raw reply related

* [PATCH] Added support for bluetooth controller shipped with iMac i5
From: Cyril Lacoux @ 2010-07-09 10:34 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Cyril Lacoux

From: Cyril Lacoux <clacoux@easter-eggs.com>

Device class is ff(vend.) instead of e0(wlcon).

Output from command `usb-devices`:
T:  Bus=01 Lev=03 Prnt=03 Port=00 Cnt=01 Dev#=  6 Spd=12  MxCh= 0
D:  Ver= 2.00 Cls=ff(vend.) Sub=01 Prot=01 MxPS=64 #Cfgs=  1
P:  Vendor=05ac ProdID=8215 Rev=01.82
S:  Manufacturer=Apple Inc.
S:  Product=Bluetooth USB Host Controller
S:  SerialNumber=7C6D62936607
C:  #Ifs= 4 Cfg#= 1 Atr=e0 MxPwr=0mA
I:  If#= 0 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=01 Prot=01 Driver=btusb
I:  If#= 1 Alt= 0 #EPs= 2 Cls=e0(wlcon) Sub=01 Prot=01 Driver=btusb
I:  If#= 2 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=ff Driver=(none)
I:  If#= 3 Alt= 0 #EPs= 0 Cls=fe(app. ) Sub=01 Prot=00 Driver=(none)
---
 drivers/bluetooth/btusb.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 5d9cc53..8eb6be5 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -75,6 +75,9 @@ static struct usb_device_id btusb_table[] = {
 	/* Canyon CN-BTU1 with HID interfaces */
 	{ USB_DEVICE(0x0c10, 0x0000) },
 
+	/* Apple iMac11,1 */
+	{ USB_DEVICE(0x05ac, 0x8215) },
+
 	{ }	/* Terminating entry */
 };
 
-- 
1.7.1


^ permalink raw reply related

* [PATCH] Fixed empty 'N:' parameter handling for VCARDs
From: Radoslaw Jablonski @ 2010-07-09 10:19 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Radoslaw Jablonski

Bluetooth PBAP specification expects for call history listing,
that parameter N: shall be empty when we cannot retrieve personal data
from PSE phone book.
Some devices(by example BH-903) are having problems when after N: parameter
 unnecessary characters occurs. List of dialed/incoming calls on carkit
then is useless - carkit shows only blank lines and it's impossible to
determine who made call.

In previous version unnecessary semicolons were added after N:("N:;;;;")
to represent empty name.
Now if none of the contact fields is available, then adding real empty "N:"
parameter (without semicolons).
---
 plugins/vcard.c |   27 +++++++++++++++++++++++++++
 1 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/plugins/vcard.c b/plugins/vcard.c
index 5948a4a..8c7147b 100644
--- a/plugins/vcard.c
+++ b/plugins/vcard.c
@@ -123,6 +123,27 @@ static void add_slash(char *dest, const char *src, int len_max, int len)
 	return;
 }
 
+/* checks if there is at least one present contact field with personal data */
+static gboolean contact_fields_present(struct phonebook_contact * contact)
+{
+	if (contact->family && strlen(contact->family))
+		return TRUE;
+
+	if (contact->given && strlen(contact->given))
+		return TRUE;
+	if (contact->additional && strlen(contact->additional))
+		return TRUE;
+
+	if (contact->prefix && strlen(contact->prefix))
+		return TRUE;
+
+	if (contact->suffix && strlen(contact->suffix))
+		return TRUE;
+
+	/* none of the personal data fields is present*/
+	return FALSE;
+}
+
 static void vcard_printf_begin(GString *vcards, uint8_t format)
 {
 	vcard_printf(vcards, "BEGIN:VCARD");
@@ -136,6 +157,12 @@ static void vcard_printf_begin(GString *vcards, uint8_t format)
 static void vcard_printf_name(GString *vcards,
 					struct phonebook_contact *contact)
 {
+	if (contact_fields_present(contact) == FALSE) {
+		/* if all fields are empty we need to have empty N: parameter */
+		vcard_printf(vcards, "N:");
+		return;
+	}
+
 	vcard_printf(vcards, "N:%s;%s;%s;%s;%s", contact->family,
 				contact->given, contact->additional,
 				contact->prefix, contact->suffix);
-- 
1.6.0.4


^ permalink raw reply related

* [PATCH] Fixed empty 'N:' parameter handling for VCARDs
From: Radoslaw Jablonski @ 2010-07-09  9:27 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Radoslaw Jablonski

Bluetooth PBAP specification expects for call history listing, that parameter N: shall be empty when we cannot retrieve personal data from PSE phone book.
Some devices(by example BH-903) are having problems when after N: parameter unnecessary characters occurs. List of dialed/incoming calls on carkit then is useless - carkit shows only blank lines and it's impossible to determine who made call.

In previous version unnecessary semicolons were added after N: parameter ("N:;;;;") to represent empty name.
Now if none of the contact fields is available, then adding real empty "N:" parameter (without semicolons).
---
 plugins/vcard.c |   27 +++++++++++++++++++++++++++
 1 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/plugins/vcard.c b/plugins/vcard.c
index 5948a4a..8c7147b 100644
--- a/plugins/vcard.c
+++ b/plugins/vcard.c
@@ -123,6 +123,27 @@ static void add_slash(char *dest, const char *src, int len_max, int len)
 	return;
 }
 
+/* checks if there is at least one present contact field with personal data */
+static gboolean contact_fields_present(struct phonebook_contact * contact)
+{
+	if (contact->family && strlen(contact->family))
+		return TRUE;
+
+	if (contact->given && strlen(contact->given))
+		return TRUE;
+	if (contact->additional && strlen(contact->additional))
+		return TRUE;
+
+	if (contact->prefix && strlen(contact->prefix))
+		return TRUE;
+
+	if (contact->suffix && strlen(contact->suffix))
+		return TRUE;
+
+	/* none of the personal data fields is present*/
+	return FALSE;
+}
+
 static void vcard_printf_begin(GString *vcards, uint8_t format)
 {
 	vcard_printf(vcards, "BEGIN:VCARD");
@@ -136,6 +157,12 @@ static void vcard_printf_begin(GString *vcards, uint8_t format)
 static void vcard_printf_name(GString *vcards,
 					struct phonebook_contact *contact)
 {
+	if (contact_fields_present(contact) == FALSE) {
+		/* if all fields are empty we need to have empty N: parameter */
+		vcard_printf(vcards, "N:");
+		return;
+	}
+
 	vcard_printf(vcards, "N:%s;%s;%s;%s;%s", contact->family,
 				contact->given, contact->additional,
 				contact->prefix, contact->suffix);
-- 
1.6.0.4


^ permalink raw reply related

* Re: [PATCH] Fixed empty 'N:' parameter handling for VCARDs
From: Marcel Holtmann @ 2010-07-09  8:39 UTC (permalink / raw)
  To: ext-jablonski.radoslaw; +Cc: linux-bluetooth
In-Reply-To: <090FE800A758CA439B2752C082AC3DEF0378CDB08F@NOK-EUMSG-06.mgdnok.nokia.com>

Hi Radek,

> I've created patch problems with history listing with BH-903.. 
> After fix everything works as described in PBAP spec and problem no longer occurs.
> 	
> Patch is available on my gitorius master branch clone for obexd-packaging: 
> git://gitorious.org/~jradosla/maemo-bluetooth/jradoslas-obexd-packaging.git 
> It is located in:  debian/patches/006-Fixed-empty-N-parameter-handling-for-VCARDs.patch

yeah, that is not how this works. Take the patch and send it to the
mailing list for review. I am not going to extract a patch from your
packaging repos nor is anybody else.

Regards

Marcel



^ permalink raw reply

* [PATCH] Fixed empty 'N:' parameter handling for VCARDs
From: ext-jablonski.radoslaw @ 2010-07-09  8:30 UTC (permalink / raw)
  To: linux-bluetooth

Hi!
I've created patch problems with history listing with BH-903.. 
After fix everything works as described in PBAP spec and problem no longer occurs.
	
Patch is available on my gitorius master branch clone for obexd-packaging: 
git://gitorious.org/~jradosla/maemo-bluetooth/jradoslas-obexd-packaging.git 
It is located in:  debian/patches/006-Fixed-empty-N-parameter-handling-for-VCARDs.patch

BR,
Radek

^ permalink raw reply

* Re: [PATCH 1/1] Bluetooth: hidp: Add support for hidraw HIDIOCGFEATURE  and HIDIOCSFEATURE
From: Marcel Holtmann @ 2010-07-09  8:01 UTC (permalink / raw)
  To: Alan Ott
  Cc: David S Miller, Jiri Kosina, Michael Poole, Bastien Nocera,
	Eric Dumazet, linux-bluetooth, linux-kernel, netdev
In-Reply-To: <4C369CDC.9080104@signal11.us>

Hi Alan,

> > I looked at this and I am bit worried that this should not be done in
> > this detail in the HIDP driver. Essentially HIDP is a pure transport
> > driver. It should not handle all these details. Can we make this a bit
> > easier for the transport drivers to support such features?
> 
> I put these changes (most notably the addition of hidp_get_raw_report()) 
> in hidp because that's where the parallel function 
> hidp_output_raw_report() was already located. I figured the input should 
> go with the output. That said, if there's a better place for both of 
> them (input and output) to go, let me know where you think it should be, 
> and I'll get them moved into the proper spot.
> 
> I'm not sure what you mean about HIDP being a pure transport driver.

what is usb-hid.ko doing here? I would expect a bunch of code
duplication with minor difference between USB and Bluetooth.

Regards

Marcel



^ permalink raw reply

* Re: [PATCH] Adding support for bluetooth controller shipped with iMac i5
From: Marcel Holtmann @ 2010-07-09  7:59 UTC (permalink / raw)
  To: Cyril Lacoux (Yack); +Cc: linux-bluetooth
In-Reply-To: <201007090834.42674.clacoux@ifeelgood.org>

Hi Cyril,

> > please create a proper commit message and include the usb-devices output
> > in that message.
> 
> I don't understand, patch and usb-devices output were attached to my previous 
> mail. Do you want me to re-send the patch in a separate mail using the git 
> output as body ?

when you do git commit to create your fix, then please include all the
details from the USB output I asked for.

Then use git format-patch -1 to create the patch and git send-email to
submit it to the mailing list.

> > Also the HID part in the source code comment makes no sense. Why are you
> > using that one. Copy&paste from the previous one?
> 
> Controller has HID capabilities, keyboard and mouse were working (usb-hid) in 
> restricted mode until I managed to get the controller working with btusb 
> driver. That's why I copied and paste the previous one.

But the usb-devices output for this interface doesn't show HID endpoints
and thus your comment in btusb.c is wrong.

T:  Bus=01 Lev=03 Prnt=03 Port=00 Cnt=01 Dev#=  6 Spd=12  MxCh= 0
D:  Ver= 2.00 Cls=ff(vend.) Sub=01 Prot=01 MxPS=64 #Cfgs=  1
P:  Vendor=05ac ProdID=8215 Rev=01.82
S:  Manufacturer=Apple Inc.
S:  Product=Bluetooth USB Host Controller
S:  SerialNumber=7C6D62936607
C:  #Ifs= 4 Cfg#= 1 Atr=e0 MxPwr=0mA
I:  If#= 0 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=01 Prot=01 Driver=btusb
I:  If#= 1 Alt= 0 #EPs= 2 Cls=e0(wlcon) Sub=01 Prot=01 Driver=btusb
I:  If#= 2 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=ff Driver=(none)
I:  If#= 3 Alt= 0 #EPs= 0 Cls=fe(app. ) Sub=01 Prot=00 Driver=(none)

The problem you are going to fix is the Cls=ff(vend.) on line two. And
that has nothing to do with HID. That this device might have an HID
proxy as well is irrelevant.

I stated this before in some other emails about this hardware, Apple
must have been smoking something funny. Previously they got this right
and used Cls=e0(wlcon) properly.

Regards

Marcel



^ permalink raw reply

* Re: [PATCH] Adding support for bluetooth controller shipped with iMac i5
From: Cyril Lacoux (Yack) @ 2010-07-09  4:34 UTC (permalink / raw)
  To: Marcel Holtmann, linux-bluetooth
In-Reply-To: <1278623133.10421.69.camel@localhost.localdomain>

Le vendredi 09 juillet 2010 01:05:33, vous avez =C3=A9crit :

Hi,
=20
> please create a proper commit message and include the usb-devices output
> in that message.

I don't understand, patch and usb-devices output were attached to my previo=
us=20
mail. Do you want me to re-send the patch in a separate mail using the git=
=20
output as body ?

> Also the HID part in the source code comment makes no sense. Why are you
> using that one. Copy&paste from the previous one?

Controller has HID capabilities, keyboard and mouse were working (usb-hid) =
in=20
restricted mode until I managed to get the controller working with btusb=20
driver. That's why I copied and paste the previous one.

I filled a bug report[1] on Ubuntu with more explanations. I tried to rever=
t=20
it to hid mode using hid2hci, but no luck.


[1]: https://bugs.launchpad.net/ubuntu/+source/bluez/+bug/575366

Git and kernel development guidelines are really new to me, thank's to take=
=20
the time to fix this.

Regards,
=2D-=20
Cyril Lacoux -- BOFH excuse #181:

Atilla the Hub

^ permalink raw reply

* Re: [PATCH 1/1] Bluetooth: hidp: Add support for hidraw  HIDIOCGFEATURE  and HIDIOCSFEATURE
From: Alan Ott @ 2010-07-09  3:51 UTC (permalink / raw)
  To: Marcel Holtmann
  Cc: David S Miller, Jiri Kosina, Michael Poole, Bastien Nocera,
	Eric Dumazet, linux-bluetooth, linux-kernel, netdev
In-Reply-To: <1278623485.10421.73.camel@localhost.localdomain>

On 07/08/2010 05:11 PM, Marcel Holtmann wrote:
> I looked at this and I am bit worried that this should not be done in
> this detail in the HIDP driver. Essentially HIDP is a pure transport
> driver. It should not handle all these details. Can we make this a bit
> easier for the transport drivers to support such features?
>
> Regards
>
> Marcel
>    

Hi Marcel,

I put these changes (most notably the addition of hidp_get_raw_report()) 
in hidp because that's where the parallel function 
hidp_output_raw_report() was already located. I figured the input should 
go with the output. That said, if there's a better place for both of 
them (input and output) to go, let me know where you think it should be, 
and I'll get them moved into the proper spot.

I'm not sure what you mean about HIDP being a pure transport driver.

Alan.

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox