Headset plugin, compatibility Plantronics W720

[David Woodhouse]

On Mon, 2020-05-18 at 21:06 +0200, Lukas Schreiner wrote:
> Hi David,
> 
> to be honest, i thought the attachment is forwarded. Seems not like
> that.
> 
> So patch is here: https://gist.github.com/monofox/b21485f060ec875c12a736a55f9f21fa#file-patch_plantronics_w720-patch
> I further created and redacted a debug log file based on original code:
> https://gist.github.com/monofox/b21485f060ec875c12a736a55f9f21fa#file-pidgin-headeset-w720-redacted-log
> And video for this: http://paste.fls-wiesbaden.de/fnkdsr/ (LED must turn
> off and keep off after stopping call or placing on docking station).

Thanks. So...

--- a/jabra.c
+++ b/jabra.c
@@ -437,10 +437,13 @@ static gboolean jabra_in(GIOChannel *gio, GIOCondition condition, gpointer user_
 			case Tel_Hook_Switch:
 				if (jabra.connected != ev[i].value) {
 					if (!jabra.connected) {
 						writeUsage(jabra.fd, HID_REPORT_TYPE_OUTPUT, LEDUsagePage, Led_Ring, 0);
 						writeUsage(jabra.fd, HID_REPORT_TYPE_OUTPUT, TelephonyUsagePage, Tel_Ringer, 0);
+						writeUsage(jabra.fd, HID_REPORT_TYPE_OUTPUT, LEDUsagePage, Led_Off_Hook, 0);
+					} else {
+						writeUsage(jabra.fd, HID_REPORT_TYPE_OUTPUT, LEDUsagePage, Led_Off_Hook, 1);
 					}
-					writeUsage(jabra.fd, HID_REPORT_TYPE_OUTPUT, LEDUsagePage, Led_Off_Hook, ev[i].value);
+					//writeUsage(jabra.fd, HID_REPORT_TYPE_OUTPUT, LEDUsagePage, Led_Off_Hook, ev[i].value);
 					headset_connected(NULL, ev[i].value);
 				}
 				break;

You remove the unconditional setting of the Led_Off_Hook field and...
instead do it on *both* branches of the if/else immediately above.

So moving it around is just noise; what's important is that you're
actually writing the *opposite* value to what was being written before.

See that it's !jabra.connected case where you're writing 0. And since t
any of this code only runs (jabra.connected != ev[i].value), that
clearly means that the old code writing ev[i].value would have been
writing 1.

Conversely, the one you add in the 'else' case happens when
jabra.connected == 1. You're writing 1 where the old code was writing
ev[i].value which was 0.

I think this simpler patch would have achieved much the same thing:

--- a/jabra.c
+++ b/jabra.c
@@ -440,7 +440,7 @@ static gboolean jabra_in(GIOChannel *gio, GIOCondition condition, gpointer user_
                                                writeUsage(jabra.fd, HID_REPORT_TYPE_OUTPUT, LEDUsagePage, Led_Ring, 0);
                                                writeUsage(jabra.fd, HID_REPORT_TYPE_OUTPUT, TelephonyUsagePage, Tel_Ringer, 0);
                                        }
-                                       writeUsage(jabra.fd, HID_REPORT_TYPE_OUTPUT, LEDUsagePage, Led_Off_Hook, ev[i].value);
+                                       writeUsage(jabra.fd, HID_REPORT_TYPE_OUTPUT, LEDUsagePage, Led_Off_Hook, !ev[i].value);
                                        headset_connected(NULL, ev[i].value);
                                }
                                break;


But while that makes it clearer what you've changed, I'm not sure
that's really the right thing to do. I think ev[i].value *was* the
right value to be writing; it's just that we shouldn't be doing it over
and over again. We should only do it when the connected state
*changes*. The code *looks* like we're already doing that, because of
the comparison (jabra.connected != ev[i].value)... except that we never
do set jabra.connected after processing the change, so it's seen as a
change *every* time.

Please could you try this and see if it works: 
http://git.infradead.org/users/dwmw2/pidgin-headset.git/commit/797f2cb4