- BT HID message parsing framework and fixes for Keyboards in boot mode.

PATCH In review Matthew Grant 2 participants 5 messages Dec 14, 2004 – Dec 17, 2004

Subsystems (2)

Bluetooth drivers Bluetooth

lore

Share & tools

lore

Matthew Grant

author

2004-12-14 20:30 UTC #0 lore raw

PATCH 1/1 - BT HID message parsing framework and fixes for Keyboards in boot mode.

Dear Marcel,

I have reworked the patches as you have requested. They are attached to this message.

This is the BT HID message parsing framework that I wrote, based on the Bluetooth HID specs. The kernel versions the patches are against are mentioned in their names. The one against 2.6.10-rc3-bk6 is the one for the standard BT HID code in the stock kernel, then other contains fixes for report mode processing where the BT device is stuck in Boot mode due to being first of all configured by the computer BIOS.

The devices I tested this with are Apple BT keyboard, and Logitech Mx900 mouse. the changes are all conservative, and should work with all devices.

I have left the debug work I did as is in the patches as I need it to debug the work I am doing. It fixes various compile problems with the debug code, as well as adding output of data that I found useful in diagnosing my keyboard problems. I am planning to continue my work on the BT HID as I am able, and hope to do phase 2 (SET_ and GET_) HID state machines over the Christmas holidays. The Debug stuff has to stay there. Basically if you want me to significant work in the BT HID area, I need a free hand to do things. I already have to spend quite a bit of time making a patch for the standard kernel, as well as against the 2.6.x-mhx patches which I do my development against. We need to discuss how we are going to work this so that it does not take too much of your time.

I am hoping to get a Web site up shortly with these patches and further work, as well as a version control system for a BT HID project!

Best Regards,

Matthew Grant

Signed-off-by Matthew Grant <[email protected]>

Implements changes in core BT HID for incoming packet processing, plus start of processing for incoming messages in report mode. Also sets a device stuck in boot mode to report mode when device first connects.

net/bluetooth/hidp/core.c +232 ~~-17~~

@@ -46,7 +46,7 @@
 
 #include "hidp.h"
 
-#ifndef CONFIG_BT_HIDP_DEBUG
+#ifndef CONFIG_BT_HIDP_DEBUG_BT
 #undef  BT_DBG
 #define BT_DBG(D...)
 #endif

@@ -138,8 +138,8 @@
 	struct sk_buff *skb;
 	unsigned char newleds;
 
-	BT_DBG("session %p hid %p data %p size %d", session, device, data, size);
-
+	BT_DBG("");
+	 
 	if (type != EV_LED)
 		return -1;

@@ -159,7 +159,7 @@
 		return -ENOMEM;
 	}
 
-	*skb_put(skb, 1) = 0xa2;
+	*skb_put(skb, 1) = HIDP_TRANS_DATA | HIDP_DATA_RTYPE_OUPUT;
 	*skb_put(skb, 1) = 0x01;
 	*skb_put(skb, 1) = newleds;

@@ -263,7 +263,35 @@
 		del_timer(&session->timer);
 }
 
-static inline void hidp_send_message(struct hidp_session *session, unsigned char hdr)
+static inline int hidp_send_ctrl_message(struct hidp_session *session,
+			unsigned char hdr,
+			unsigned char *data, int size)
+{
+	struct sk_buff *skb;
+
+	BT_DBG("session %p data %p size %d", session, data, size);
+
+	if (!(skb = alloc_skb(size + 1, GFP_ATOMIC))) {
+		BT_ERR("Can't allocate memory for new frame");
+		return -ENOMEM;
+	}
+
+	*skb_put(skb, 1) = hdr;
+	if (size > 0)
+		memcpy(skb_put(skb, size), data, size);
+
+	skb_queue_tail(&session->ctrl_transmit, skb);
+
+	hidp_schedule(session);
+
+	return 0;
+}
+
+/* Send a 1 byte control message.
+ * For calling outside HID kthread process 
+ */
+static inline void hidp_send_ctrl_byte(struct hidp_session *session,
+		 			unsigned char hdr)
 {
 	struct sk_buff *skb;

@@ -281,29 +309,205 @@
 	hidp_schedule(session);
 }
 
-static inline int hidp_recv_frame(struct hidp_session *session, struct sk_buff *skb)
+/* Send a 1 byte control message.
+ * For calling inside HID kthread process 
+ */
+static inline void hidp_send_ctrl_reply(struct hidp_session *session,
+		 			unsigned char hdr)
+{
+	struct sk_buff *skb;
+
+	BT_DBG("session %p", session);
+
+	if (!(skb = alloc_skb(1, GFP_ATOMIC))) {
+		BT_ERR("Can't allocate memory for message");
+		return;
+	}
+
+	*skb_put(skb, 1) = hdr;
+
+	skb_queue_tail(&session->ctrl_transmit, skb);
+}
+
+static inline void hidp_process_handshake(struct hidp_session *session, __u8 param)
+{
+	switch (param) {
+	case HIDP_HSHK_SUCCESSFUL:
+		/* Call into SET_ GET_ handlers here */
+		break;
+	case HIDP_HSHK_NOT_READY:
+	case HIDP_HSHK_ERR_INVALID_REPORT_ID:
+	case HIDP_HSHK_ERR_UNSUPPORTED_REQUEST:
+	case HIDP_HSHK_ERR_INVALID_PARAMETER:
+		/* Call into SET_ GET_ handlers here */
+		break;
+	case HIDP_HSHK_ERR_UNKNOWN:
+		BT_INFO("HANDSHAKE parameter ERR_UNKNOWN seen.");
+		break;
+	case HIDP_HSHK_ERR_FATAL:
+		/* Device requests a reboot, as this is the only way this error
+ 		 * can be recovered.
+		 */
+		hidp_send_ctrl_reply(session,
+			HIDP_TRANS_HID_CONTROL | HIDP_CTRL_SOFT_RESET);
+		break;
+	default:
+		hidp_send_ctrl_reply(session,
+			HIDP_TRANS_HANDSHAKE | HIDP_HSHK_ERR_INVALID_PARAMETER);
+		break;
+	}
+
+}
+
+static inline void hidp_process_hid_control (struct hidp_session *session, __u8 param)
+{
+	switch (param) {
+	case HIDP_CTRL_NOP:
+		break;
+	case HIDP_CTRL_VIRTUAL_CABLE_UNPLUG:
+		/* Flush the transmit queues */
+		skb_queue_purge(&session->ctrl_transmit);
+		skb_queue_purge(&session->intr_transmit);
+
+		/* Kill session thread */
+		atomic_inc(&session->terminate);
+
+		/* Do some funky HCI stuff here to delete pairing on dongle? */
+		break;
+	case HIDP_CTRL_HARD_RESET:
+	case HIDP_CTRL_SOFT_RESET:
+	case HIDP_CTRL_SUSPEND:
+	case HIDP_CTRL_EXIT_SUSPEND:
+		/* We have to parse these and return no error */
+		break;
+	default:
+		hidp_send_ctrl_reply(session,
+			HIDP_TRANS_HANDSHAKE | HIDP_HSHK_ERR_INVALID_PARAMETER);
+		break;
+	}
+}
+
+static inline int hidp_process_data(struct hidp_session *session, struct sk_buff *skb, __u8 param)
+{
+	int result = 0;
+
+	BT_INFO("DATA packet on control channel, GET_, SET_ not implemented");
+
+	switch (param) {
+	case HIDP_DATA_RTYPE_INPUT:
+		hidp_set_timer(session);
+
+		if (session->input)
+			hidp_input_report(session, skb);
+
+		if (session->hid) {
+			result = hid_recv_report(session->hid, 
+					HID_INPUT_REPORT, 
+					skb->data, 
+					skb->len);
+			switch (result) {
+			case -EPROTOTYPE:
+				hidp_send_ctrl_reply(session,
+                        		HIDP_TRANS_SET_PROTOCOL | HIDP_PROTO_REPORT);
+				break;
+			case -EBADF:
+				hidp_send_ctrl_reply(session,
+                        		HIDP_TRANS_HANDSHAKE | HIDP_HSHK_ERR_INVALID_REPORT_ID);
+				break;
+			default:
+				break;
+			}
+		}
+		break;
+	case HIDP_DATA_RTYPE_OTHER:
+	case HIDP_DATA_RTYPE_OUPUT:
+	case HIDP_DATA_RTYPE_FEATURE:
+		BT_DBG("Unimplemented DATA parameter 0x%01x", param);
+		break;
+	default:
+		BT_DBG("Invalid DATA parameter 0x%01x", param);
+		hidp_send_ctrl_reply(session,
+			HIDP_TRANS_HANDSHAKE | HIDP_HSHK_ERR_INVALID_PARAMETER);
+	}
+	
+	return 0;
+}
+
+
+static inline int hidp_recv_ctrl_frame(struct hidp_session *session, struct sk_buff *skb)
 {
 	__u8 hdr;
+	__u8 type;
+	__u8 param;
+	int err = 0;
 
 	BT_DBG("session %p skb %p len %d", session, skb, skb->len);
 
 	hdr = skb->data[0];
 	skb_pull(skb, 1);
 
-	if (hdr == 0xa1) {
-		hidp_set_timer(session);
+	type = hdr & HIDP_THDR_TRANS_MASK;
+	param = hdr & HIDP_THDR_PARAM_MASK;
 
+	switch (type) {
+	case HIDP_TRANS_HANDSHAKE:
+		hidp_process_handshake(session, param);
+		break;
+	case HIDP_TRANS_HID_CONTROL:
+		hidp_process_hid_control(session, param);
+		break;
+	case HIDP_TRANS_DATA:
+		err = hidp_process_data(session, skb, param);
+		break;
+	default:
+		BT_DBG("Unsupported protocol header 0x%02x", hdr);
+		hidp_send_ctrl_reply(session,
+			HIDP_TRANS_HANDSHAKE | HIDP_HSHK_ERR_UNSUPPORTED_REQUEST);
+		break;
+	}
+		
+	kfree_skb(skb);
+	return err;
+}
+
+/* This needs to be separate as there are only DATA and DATC packets on the 
+ * interrupt channel. iData recieved, and no result given, so we swallow errors
+ * and throw out invalid stuff. MAG
+ */ 
+static inline void hidp_recv_intr_frame(struct hidp_session *session, struct sk_buff *skb)
+{
+	__u8 hdr;
+	int err = 0;
+
+	BT_DBG("session %p skb %p len %d", session, skb, skb->len);
+
+	hdr = skb->data[0];
+	skb_pull(skb, 1);
+
+	if (hdr == (HIDP_TRANS_DATA | HIDP_DATA_RTYPE_INPUT)) {
+		hidp_set_timer(session);
 		if (session->input)
 			hidp_input_report(session, skb);
 
 		if (session->hid)
-			hid_recv_report(session->hid, HID_INPUT_REPORT, skb->data, skb->len);
-	} else {
-		BT_DBG("Unsupported protocol header 0x%02x", hdr);
+			err = hid_recv_report(session->hid, 
+					HID_INPUT_REPORT, 
+					skb->data, 
+					skb->len);
+			switch (err) {
+			case -EPROTOTYPE:
+				hidp_send_ctrl_reply(session,
+                        		HIDP_TRANS_SET_PROTOCOL | HIDP_PROTO_REPORT);
+				break;
+			default:
+				break;
+			}
 	}
-
+	else {
+		BT_INFO("Unsupported protocol header 0x%02x", hdr);
+	}
+		
 	kfree_skb(skb);
-	return 0;
 }
 
 static int hidp_send_frame(struct socket *sock, unsigned char *data, int len)

@@ -389,12 +593,12 @@
 
 		while ((skb = skb_dequeue(&ctrl_sk->sk_receive_queue))) {
 			skb_orphan(skb);
-			hidp_recv_frame(session, skb);
+			hidp_recv_ctrl_frame(session, skb);
 		}
 
 		while ((skb = skb_dequeue(&intr_sk->sk_receive_queue))) {
 			skb_orphan(skb);
-			hidp_recv_frame(session, skb);
+			hidp_recv_intr_frame(session, skb);
 		}
 
 		hidp_process_transmit(session);

@@ -608,13 +812,23 @@
 		goto unlink;
 
 	if (session->input) {
-		hidp_send_message(session, 0x70);
+		hidp_send_ctrl_byte(session, 
+			HIDP_TRANS_SET_PROTOCOL | HIDP_PROTO_BOOT);
 		session->flags |= (1 << HIDP_BOOT_PROTOCOL_MODE);
 
 		session->leds = 0xff;
 		hidp_input_event(session->input, EV_LED, 0, 0);
 	}
 
+	/* Make sure device IS in report mode
+         * Some devices start off in boot protocol, and do not respond correctly 
+	 * to a reset command.
+         */
+	if (session->hid) {
+		hidp_send_ctrl_byte(session,
+                        HIDP_TRANS_SET_PROTOCOL | HIDP_PROTO_REPORT);
+	}
+
 	up_write(&hidp_session_sem);
 	return 0;

@@ -654,7 +868,8 @@
 	session = __hidp_get_session(&req->bdaddr);
 	if (session) {
 		if (req->flags & (1 << HIDP_VIRTUAL_CABLE_UNPLUG)) {
-			hidp_send_message(session, 0x15);
+			hidp_send_ctrl_byte(session, 
+				HIDP_TRANS_HID_CONTROL | HIDP_CTRL_VIRTUAL_CABLE_UNPLUG);
 		} else {
 			/* Flush the transmit queues */
 			skb_queue_purge(&session->ctrl_transmit);

net/bluetooth/hidp/hid.c +26 -7

@@ -858,11 +858,11 @@
 
 	if (!size) {
 		dbg("empty report");
-		return -1;
+		return -EINVAL;
 	}
 
-#ifdef DEBUG_DATA
-	printk(KERN_DEBUG __FILE__ ": report (size %u) (%snumbered)\n", len, report_enum->numbered ? "" : "un");
+#ifdef CONFIG_BT_HIDP_DEBUG_DATA
+	printk(KERN_DEBUG __FILE__ ": report (size %u) (%snumbered)\n", size, report_enum->numbered ? "" : "un");
 #endif
 
 	n = 0;				/* Normally report number is 0 */

@@ -871,7 +871,7 @@
 		size--;
 	}
 
-#ifdef DEBUG_DATA
+#ifdef CONFIG_BT_HIDP_DEBUG_DATA
 	{
 		int i;
 		printk(KERN_DEBUG __FILE__ ": report %d (size %u) = ", n, size);

@@ -883,14 +883,33 @@
 
 	if (!(report = report_enum->report_id_hash[n])) {
 		dbg("undefined report_id %d received", n);
-		return -1;
+		return -EBADF;
 	}
 
 	rsize = ((report->size - 1) >> 3) + 1;
 
+#ifdef CONFIG_BT_HIDP_DEBUG_DATA
+	dbg("report %d, expected %d bits, %d maxfield", report->id, report->size, report->maxfield);
+	for (n = 0; n < report->maxfield; n++)
+		dbg("report:field %d:%d, count %d , offset %d, size(bits) %d",
+			report->id, n,
+			report->field[n]->report_count,
+			report->field[n]->report_offset,
+			report->field[n]->report_size);
+#endif
+
 	if (size < rsize) {
-		dbg("report %d is too short, (%d < %d)", report->id, size, rsize);
-		return -1;
+		dbg("report %d is too short, (%d < %d) %d bits expected, %d maxfield", report->id, size, rsize, report->size, report->maxfield);
+		if (size == 8) {
+			/* 
+			 * FIXME - need to check if device is a keyboard!!
+			 */
+			dbg("tell upper layer wrong protocol - switch to report"); 
+			return -EPROTOTYPE;
+		}
+		else {
+			return -EMSGSIZE;
+		}
 	}
 
 	for (n = 0; n < report->maxfield; n++)

net/bluetooth/hidp/hid.h +1 -1

@@ -26,7 +26,7 @@
 #ifndef __HID_H
 #define __HID_H
 
-#ifdef DEBUG
+#ifdef CONFIG_BT_HIDP_DEBUG_REPORT
 #define dbg(format, arg...) printk(KERN_DEBUG "%s: " format "\n" , __FILE__ , ## arg)
 #else
 #define dbg(format, arg...) do {} while (0)

net/bluetooth/hidp/hidp.h +65 -0

71 lines not on this page · raw message

net/bluetooth/hidp/sock.c +1 -1

@@ -40,7 +40,7 @@
 
 #include "hidp.h"
 
-#ifndef CONFIG_BT_HIDP_DEBUG
+#ifndef CONFIG_BT_HIDP_DEBUG_BT
 #undef  BT_DBG
 #define BT_DBG(D...)
 #endif

Signed-off-by Matthew Grant <[email protected]>

Updates to BT HID for phase 1 of implementation of BT HID spec.

Introduces changes to incoming packet processing, and parsing structure for recieved messages.

net/bluetooth/hidp/core.c +192 ~~-16~~

273 lines not on this page · raw message

net/bluetooth/hidp/hidp.h +65 -0

71 lines not on this page · raw message

net/bluetooth/hidp/sock.c +1 -1

diff -uNr linux-2.6.10-rc3-bk6/net/bluetooth/hidp/sock.c linux-2.6.10-rc3-bk6-magbthid/net/bluetooth/hidp/sock.c
@@ -40,7 +40,7 @@
 
 #include "hidp.h"
 
-#ifndef CONFIG_BT_HIDP_DEBUG
+#ifndef CONFIG_BT_HIDP_DEBUG_BT
 #undef  BT_DBG
 #define BT_DBG(D...)
 #endif

Marcel Holtmann

maintainer

2004-12-15 12:05 UTC #1 lore raw

Hi Matthew,

Quoted 14 lines

I have reworked the patches as you have requested. They are attached to this message.
This is the BT HID message parsing framework that I wrote, based on the Bluetooth HID specs. The kernel versions the patches are against are mentioned in their names. The one against 2.6.10-rc3-bk6 is the one for the standard BT HID code in the stock kernel, then other contains fixes for report mode processing where the BT device is stuck in Boot mode due to being first of all configured by the computer BIOS.
The devices I tested this with are Apple BT keyboard, and Logitech Mx900 mouse. the changes are all conservative, and should work with all devices.

Quoted 12 lines

I have left the debug work I did as is in the patches as I need it to debug the work I am doing. It fixes various compile problems with the debug code, as well as adding output of data that I found useful in diagnosing my keyboard problems. I am planning to continue my work on the BT HID as I am able, and hope to do phase 2 (SET_ and GET_) HID state machines over the Christmas holidays. The Debug stuff has to stay there. Basically if you want me to significant work in the BT HID area, I need a free hand to do things. I already have to spend quite a bit of time making a patch for the standard kernel, as well as against the 2.6.x-mhx patches which I do my development against. We need to discuss how we are going to work this so that it does not take too much of your time.

I only looked at the -rc3-bk6 patch, because as I said, this is what we gonna do first. I don't wanna see any debug specific changes in there. I will remove them anyway and this takes me only longer to merge this patch into my tree. The rest of this patch looks fine and there is only one coding style issues that I might not mentioned last time. In a "switch" construct we normally add an empty line between "break" and the next "case".

And another thing. What is the difference between ...ctrl_message() and ...ctrl_byte()? We don't need that. If data is NULL then both is the same.

Quoted 2 lines

I am hoping to get a Web site up shortly with these patches and further work, as well as a version control system for a BT HID project!

I don't see any advantage of it, but if it helps you, do it. I only accept plain patches with a description like you sent.

Regards

Marcel

-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users. Discover which products truly live up to the hype. Start reading now.
http://productguide.itmanagersjournal.com/
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel

Matthew Grant

author

2004-12-15 19:32 UTC #2 lore raw

Hi Marcel,

How are you? I hope that you are coping with the upcoming Silly season (Christmas, high pressure commercialism etc). I am getting sick of the same TV ads repeated every 10 minutes if not hourly... (I am a person :-) ).

Please read, there is a lot in hear that you should look at and think carefully about.

Quoted 4 lines · Marcel Holtmann

I only looked at the -rc3-bk6 patch, because as I said, this is what we gonna do first. I don't wanna see any debug specific changes in there. I will remove them anyway and this takes me only longer to merge this patch into my tree.

You have to appreciate this given that I will bw writing 500-1000 lines more of code for net/bluetooth/hidp/core.c (maybe a seperate file) for the control channel state machine:

o Your debug code in the hidp directory DOES NOT COMPILE!!! I need working debug for the amount of work I am doing. Please read my patch and fix it!

o The code I am writing needs debug code so I can write it and get it working.

o Extra debug code is needed in other places in the hidp directory to support any general user debugging of the control channel state machine code.

Quoted 8 lines

The rest of this patch looks fine and there is only
one coding style issues that I might not mentioned last time. In a "switch" construct we normally add an empty line between "break" and the next "case".
And another thing. What is the difference between ...ctrl_message() and ...ctrl_byte()? We don't need that. If data is NULL then both is the same.

...ctrl_message() is needed latter in the state machine work as some control messages will be over 1 byte in size. It will be used in phase 2.

You may also notice the 2 separate functions for queueing the sending of control bytes (..ctrl_byte() and ...ctrl_reply()), one from within the hidp_session() kernel process, and one for use from the ioctl(s). Here is my reasoning for it. I take it that you do not want to put extra schedule() calls in the middle of the hidp_session() while loop, between the processing of individual received control packets, before even processing the interrupt (high priority, basically incoming HID events) and the transmission queue. Even if this inserted schedule() call is conditional, 2 extra context switches for the hidp_session() thread get introduced before received keystrokes/HID events are processed, and the transmit queue gets looked at. The BT HID spec says that the interrupt channel should be processed at higher priority (and even before?) the control channel. Both ..ctrl_byte() and ...ctrl_reply() are needed according to this.

Now about us working together, from your replies and comments about my patches it has been obvious to me that you have not had much time to look them over and think about things. You comments on coding style are appreciated, and I myself don't like being sloppy, and I know that the netfilter people are way too sloppy for my own liking to. But you are holding on too tightly to the control of the BT HID code. I need the flexibility and autonomy of being the delegated lead developer there so that my development can be speed along.

Could you please freeze the BT HID code in the kernel for 3 months (apart from brown-paper bag stuff), and let me subsume the HID report code in your mh* patches, and strip the net/bluetooth/hidp code from your patches, and use my stuff instead so that I can efficiently get the work done on improving it rapidly. If changes are needed to the in kernel stuff, please feed them my way and I will integrate them as quickly as I can. This should not be too hard, as it looks like there has not been much activity in the BT HID area in the last few months apart from my work.

I really want to work together with you on this, as cooperation will get far more done in the bluetooth area of the kernel given the small user base. If the above arrangement is not suitable to you, please come up with one that we do not have major code clashes when merging each others code.

If you have doubts as to my ability, I am a professional router programmer who got fired because I insisted on making my work bug-free, robust, and high quality, with experience in OSPF, router device driveers and IPX code. My early CV reads like that of Alan Cox, and I have been using Linux since 1993, with a bit work accepted into the 2.2.x kernel for ethernet bridging, 2.2.x security patches, and 2.2.x/2.4.x Sangoma driver fixes. I will send you my resume if you want to look it over (it is a little out of date).

My patience on this is starting to wear out. If we can't work something out soon, I am going to go ahead anyhow, and the merits of the work by itself will warrant its inclusion in a few months time.

Looking forward to hearing back from you soon,

Best Regards,

Matthew Grant

Marcel Holtmann

maintainer

2004-12-16 10:01 UTC #3 lore raw

Hi Matthew,

Quoted 8 lines

Quoted 4 lines
I only looked at the -rc3-bk6 patch, because as I said, this is what we gonna do first. I don't wanna see any debug specific changes in there. I will remove them anyway and this takes me only longer to merge this patch into my tree.
You have to appreciate this given that I will bw writing 500-1000 lines more of code for net/bluetooth/hidp/core.c (maybe a seperate file) for the control channel state machine:

no need for an extra file. 500-1000 lines are not a lot of code.

Quoted 3 lines

o Your debug code in the hidp directory DOES NOT COMPILE!!! I need working debug for the amount of work I am doing. Please read my patch and fix it!

If my debug code doesn't compile then this is a bug. Why didn't you send in a patch for this? Such a fix could already be merged mainline.

Quoted 2 lines

o The code I am writing needs debug code so I can write it and get it working.

Fine with me, but before you submit it for inclusion remove this debug code.

Quoted 3 lines

o Extra debug code is needed in other places in the hidp directory to support any general user debugging of the control channel state machine code.

As I said before, I will only allow simple debug code in the Bluetooth code that is off by default. I don't think that you can convince to add deeper levels of debugging in the code. No extra debug code will make it easy to merge your code back mainline. Otherwise we will be stuck here and I have to remove everything by myself.

Quoted 5 lines

Quoted 5 lines
The rest of this patch looks fine and there is only
one coding style issues that I might not mentioned last time. In a "switch" construct we normally add an empty line between "break" and the next "case".

Quoted 7 lines

Quoted 3 lines
And another thing. What is the difference between ...ctrl_message() and ...ctrl_byte()? We don't need that. If data is NULL then both is the same.
...ctrl_message() is needed latter in the state machine work as some control messages will be over 1 byte in size. It will be used in phase 2.

You don't get my point. You need only one of these functions to do the job. What you did is code duplication:

	#define ...ctrl_byte() ...ctrl_message(a, b, NULL, 0)

Quoted 6 lines

You may also notice the 2 separate functions for queueing the sending of control bytes (..ctrl_byte() and ...ctrl_reply()), one from within the hidp_session() kernel process, and one for use from the ioctl(s). Here is my reasoning for it. I take it that you do not want to put extra schedule() calls in the middle of the hidp_session() while loop, between the processing of individual received control packets, before even

Quoted 8 lines

processing the interrupt (high priority, basically incoming HID events) and the transmission queue. Even if this inserted schedule() call is conditional, 2 extra context switches for the hidp_session() thread get introduced before received keystrokes/HID events are processed, and the transmit queue gets looked at. The BT HID spec says that the interrupt channel should be processed at higher priority (and even before?) the control channel. Both ..ctrl_byte() and ...ctrl_reply() are needed according to this.

I haven't said anything about these two. I will look at it when I applied the patch to me tree and started testing.

Quoted 8 lines

Now about us working together, from your replies and comments about my patches it has been obvious to me that you have not had much time to look them over and think about things. You comments on coding style are appreciated, and I myself don't like being sloppy, and I know that the netfilter people are way too sloppy for my own liking to. But you are holding on too tightly to the control of the BT HID code. I need the flexibility and autonomy of being the delegated lead developer there so that my development can be speed along.

The coding style is important, because it makes it a lot easier to merge your changes back mainline. I simply won't spent any time fixing coding style mistakes of other people.

And actually I looked in detail at your patches, but as I said only the patch against the -rc3-bk6. We do this step by step and this is the one that comes first. I will merge it back mainline after I tested it with my devices. However this will be after 2.6.10 is out.

There is no tight control on the BT HID code, but every patch goes through my hands. I acknowlege it or reject it with a reason and an advice on how to fix it. The BT HID is a little bit special, because we don't have the generic HID parser at the moment. I am working on a final Bitkeeper tree for mainline inclusion. This must be done this way, because we must keep the revision history. If this is done and 2.6.10 is finally out everything will be very easy.

Quoted 9 lines

Could you please freeze the BT HID code in the kernel for 3 months (apart from brown-paper bag stuff), and let me subsume the HID report code in your mh* patches, and strip the net/bluetooth/hidp code from your patches, and use my stuff instead so that I can efficiently get the work done on improving it rapidly. If changes are needed to the in kernel stuff, please feed them my way and I will integrate them as quickly as I can. This should not be too hard, as it looks like there has not been much activity in the BT HID area in the last few months apart from my work.

The generic HID parser is the way to go.

Quoted 5 lines

I really want to work together with you on this, as cooperation will get far more done in the bluetooth area of the kernel given the small user base. If the above arrangement is not suitable to you, please come up with one that we do not have major code clashes when merging each others code.

Since you don't use Bitkeeper, there can't be any easy working together until I got the generic HID parser in the right shape. I am not willing to loose the revision history of the current USB HID code. It documents many reasons why decissions are made and why it is done this way. It is way too important.

Quoted 8 lines

If you have doubts as to my ability, I am a professional router programmer who got fired because I insisted on making my work bug-free, robust, and high quality, with experience in OSPF, router device driveers and IPX code. My early CV reads like that of Alan Cox, and I have been using Linux since 1993, with a bit work accepted into the 2.2.x kernel for ethernet bridging, 2.2.x security patches, and 2.2.x/2.4.x Sangoma driver fixes. I will send you my resume if you want to look it over (it is a little out of date).

I have absolutly no doubts on your ability and actually I never read any CVs, because I judge on the code people write.

Quoted 3 lines

My patience on this is starting to wear out. If we can't work something out soon, I am going to go ahead anyhow, and the merits of the work by itself will warrant its inclusion in a few months time.

I am sorry for that and I really appreciate your help. At the moment we are in the final phase of 2.6.10 and so hoping that we get any stuff into it is ridiculous (besides fixing my debug code bug maybe). So what we can do is to get your patch against -rc3-bk6 ready for inclusion and I will merge it mainline as soon as 2.6.10 is out.

I can start with the merge tree of the generic HID parser tomorrow, but actually there is one patch from the input subsystem tree that hasn't been merged back mainline so far. It changes a lot of things and I haven't looked at it in detail at the moment.

If these two things happened I will remove my current report mode support from the -mh patch and we will have a common base. Then you can send in changes for the HID parser and/or for the HIDP code.

Regards

Marcel

Matthew Grant

author

2004-12-17 09:20 UTC #4 lore raw

Hi Marcel,

Thank you for this message. It is good to know all the reasons, and have an explanation for what is going on. Having the full picture like this averts a lot of the frustration and makes it all understandable.

I do not have much time to respond fully now, but I will tomorrow.

It all looks really good on my first read.

Best Regards,

Matthew Grant

Have a good weekend!

Quoted 170 lines · Marcel Holtmann

Hi Matthew,
Quoted 8 lines
Quoted 4 lines
I only looked at the -rc3-bk6 patch, because as I said, this is what we gonna do first. I don't wanna see any debug specific changes in there. I will remove them anyway and this takes me only longer to merge this patch into my tree.
You have to appreciate this given that I will bw writing 500-1000 lines more of code for net/bluetooth/hidp/core.c (maybe a seperate file) for the control channel state machine:
no need for an extra file. 500-1000 lines are not a lot of code.
Quoted 3 lines
o Your debug code in the hidp directory DOES NOT COMPILE!!! I need working debug for the amount of work I am doing. Please read my patch and fix it!
If my debug code doesn't compile then this is a bug. Why didn't you send in a patch for this? Such a fix could already be merged mainline.
Quoted 2 lines
o The code I am writing needs debug code so I can write it and get it working.
Fine with me, but before you submit it for inclusion remove this debug code.
Quoted 3 lines
o Extra debug code is needed in other places in the hidp directory to support any general user debugging of the control channel state machine code.
As I said before, I will only allow simple debug code in the Bluetooth code that is off by default. I don't think that you can convince to add deeper levels of debugging in the code. No extra debug code will make it easy to merge your code back mainline. Otherwise we will be stuck here and I have to remove everything by myself.
Quoted 5 lines
Quoted 5 lines
The rest of this patch looks fine and there is only
one coding style issues that I might not mentioned last time. In a "switch" construct we normally add an empty line between "break" and the next "case".
Quoted 7 lines
Quoted 3 lines
And another thing. What is the difference between ...ctrl_message() and ...ctrl_byte()? We don't need that. If data is NULL then both is the same.
...ctrl_message() is needed latter in the state machine work as some control messages will be over 1 byte in size. It will be used in phase 2.
You don't get my point. You need only one of these functions to do the job. What you did is code duplication:
	#define ...ctrl_byte() ...ctrl_message(a, b, NULL, 0)
Quoted 6 lines
You may also notice the 2 separate functions for queueing the sending of control bytes (..ctrl_byte() and ...ctrl_reply()), one from within the hidp_session() kernel process, and one for use from the ioctl(s). Here is my reasoning for it. I take it that you do not want to put extra schedule() calls in the middle of the hidp_session() while loop, between the processing of individual received control packets, before even
Quoted 8 lines
processing the interrupt (high priority, basically incoming HID events) and the transmission queue. Even if this inserted schedule() call is conditional, 2 extra context switches for the hidp_session() thread get introduced before received keystrokes/HID events are processed, and the transmit queue gets looked at. The BT HID spec says that the interrupt channel should be processed at higher priority (and even before?) the control channel. Both ..ctrl_byte() and ...ctrl_reply() are needed according to this.
I haven't said anything about these two. I will look at it when I applied the patch to me tree and started testing.
Quoted 8 lines
Now about us working together, from your replies and comments about my patches it has been obvious to me that you have not had much time to look them over and think about things. You comments on coding style are appreciated, and I myself don't like being sloppy, and I know that the netfilter people are way too sloppy for my own liking to. But you are holding on too tightly to the control of the BT HID code. I need the flexibility and autonomy of being the delegated lead developer there so that my development can be speed along.
The coding style is important, because it makes it a lot easier to merge your changes back mainline. I simply won't spent any time fixing coding style mistakes of other people.
And actually I looked in detail at your patches, but as I said only the patch against the -rc3-bk6. We do this step by step and this is the one that comes first. I will merge it back mainline after I tested it with my devices. However this will be after 2.6.10 is out.
There is no tight control on the BT HID code, but every patch goes through my hands. I acknowlege it or reject it with a reason and an advice on how to fix it. The BT HID is a little bit special, because we don't have the generic HID parser at the moment. I am working on a final Bitkeeper tree for mainline inclusion. This must be done this way, because we must keep the revision history. If this is done and 2.6.10 is finally out everything will be very easy.
Quoted 9 lines
Could you please freeze the BT HID code in the kernel for 3 months (apart from brown-paper bag stuff), and let me subsume the HID report code in your mh* patches, and strip the net/bluetooth/hidp code from your patches, and use my stuff instead so that I can efficiently get the work done on improving it rapidly. If changes are needed to the in kernel stuff, please feed them my way and I will integrate them as quickly as I can. This should not be too hard, as it looks like there has not been much activity in the BT HID area in the last few months apart from my work.
The generic HID parser is the way to go.
Quoted 5 lines
I really want to work together with you on this, as cooperation will get far more done in the bluetooth area of the kernel given the small user base. If the above arrangement is not suitable to you, please come up with one that we do not have major code clashes when merging each others code.
Since you don't use Bitkeeper, there can't be any easy working together until I got the generic HID parser in the right shape. I am not willing to loose the revision history of the current USB HID code. It documents many reasons why decissions are made and why it is done this way. It is way too important.
Quoted 8 lines
If you have doubts as to my ability, I am a professional router programmer who got fired because I insisted on making my work bug-free, robust, and high quality, with experience in OSPF, router device driveers and IPX code. My early CV reads like that of Alan Cox, and I have been using Linux since 1993, with a bit work accepted into the 2.2.x kernel for ethernet bridging, 2.2.x security patches, and 2.2.x/2.4.x Sangoma driver fixes. I will send you my resume if you want to look it over (it is a little out of date).
I have absolutly no doubts on your ability and actually I never read any CVs, because I judge on the code people write.
Quoted 3 lines
My patience on this is starting to wear out. If we can't work something out soon, I am going to go ahead anyhow, and the merits of the work by itself will warrant its inclusion in a few months time.
I am sorry for that and I really appreciate your help. At the moment we are in the final phase of 2.6.10 and so hoping that we get any stuff into it is ridiculous (besides fixing my debug code bug maybe). So what we can do is to get your patch against -rc3-bk6 ready for inclusion and I will merge it mainline as soon as 2.6.10 is out.
I can start with the merge tree of the generic HID parser tomorrow, but actually there is one patch from the input subsystem tree that hasn't been merged back mainline so far. It changes a lot of things and I haven't looked at it in detail at the moment.
If these two things happened I will remove my current report mode support from the -mh patch and we will have a common base. Then you can send in changes for the HID parser and/or for the HIDP code.
Regards
Marcel
-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users. Discover which products truly live up to the hype. Start reading now.
http://productguide.itmanagersjournal.com/
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel