Headset plugin, compatibility Plantronics W720

[Lukas Schreiner]

Hi David,

tested your changes. If i click on button, call is hang up, LED turns
off, reinitalize and goes green again (= sound connection between
docking station and headset is established again).
If i put the headset on the docking station afterwards, connection is
closed, LED keeps off. So there is no flickering anymore with your
changes.

But still the issue, that the connection is established again after
pressing button.

New redacted log file: https://gist.github.com/monofox/9a171e5d9b43dc8a46beeedb3f7598ba
Its b0020, right? After call / media stream ends, b0020 is 1, changes to
0, then its getting again 1 (keeps) and after i put the headset on the docking
station, it going and keeping 0.

Best regards,
Lukas

On Wed, May 20, 2020 at 05:03:00PM +0100, David Woodhouse wrote:
> 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
>