chrippa / ds4drv

A Sony DualShock 4 userspace driver for Linux
MIT License
1.05k stars 213 forks source link

Bluetooth hidraw leds #79

Closed poconbhui closed 8 years ago

poconbhui commented 8 years ago

There was nothing stopping the LEDs from working with Bluetooth on hidraw, there was just a return statement in the hidraw bluetooth write_report method. The only problem I had was the LEDs didn't change back to a neutral colour on disconnecting.

Coupled with the other pull request I made for graceful thread closing with device disconnection, this resets the LEDs to a default colour on closing ds4drv.

Ape commented 8 years ago

Thanks! Let's merge #78 first.

Ape commented 8 years ago

Remove the note in README.md.

Note: Unfortunately due to a kernel bug it is currently not possible to use any LED functionality when using bluetooth devices in this mode.

Ape commented 8 years ago

Also, did you test if this works in USB mode?

poconbhui commented 8 years ago

It seems to work with usb.

The initial hidraw connection colour is different. I'm not great at matching colours, so working out the actual values for the LEDs is beyond me. I couldn't find any mention of the controllers reporting LED colours, which would be pretty handy here.

poconbhui commented 8 years ago

I've rebased this onto master. Should be good to squash and push once we've worked out the details.

Ape commented 8 years ago

What happens if we try to write the report with an old kernel that doesn't support it? The TODO comment tells us to check whether the kernel supports it and not just blindly write it.

poconbhui commented 8 years ago

No idea... It will probably just not work. I'm not actually sure if the kernel matters all that much since we're writing raw hid data.

Ape commented 8 years ago

Ok, let's merge this. I tested this with older distributions and it seems that either hidraw doesn't work at all, or this patch works just fine. So there is no need to check for kernel version or anything. There might be kernel versions that do not work, but they should be rare and this probably won't cause any issues anyway.

I'll some code comments.

poconbhui commented 8 years ago

Do you think adding an option to disable sending reports to controllers in case it is a breaking feature might be an option? We might want to add some try/except wrappers around some of the write routines to catch os errors like that without crashing anyway.

Ape commented 8 years ago

Hmm, maybe. Is there some use case where somebody would like to disable sending the reports even with a working kernel? Or is it just to provide a workaround for broken kernels?

If it's only a workaround for an issue that might not even exist, then I don't think it's worth the added complexity. We should try to do as much testing as possible before making the next stable release (0.6.0). Maybe a beta release first.

poconbhui commented 8 years ago

Probably not, and you're probably right.

Ape commented 8 years ago

Ok, please squash this if you think it ready for merging.

poconbhui commented 8 years ago

Ok, squashed.

Ape commented 8 years ago

Thanks!