Messages in this thread | | | Subject | Re: [Resubmit] Read battery voltage from Logitech Gaming mice | From | Pedro Vanzella <> | Date | Fri, 23 Aug 2019 11:46:50 -0400 |
| |
On 8/23/19 10:32 AM, Benjamin Tissoires wrote: > On Fri, Aug 23, 2019 at 4:22 PM Pedro Vanzella <pedro@pedrovanzella.com> wrote: >> >> Hi Benjamin, >> >> On 8/23/19 4:25 AM, Benjamin Tissoires wrote: >>> Hi Pedro, >>> >>> On Thu, Aug 22, 2019 at 10:19 PM Pedro Vanzella <pedro@pedrovanzella.com> wrote: >>>> >>>> Resumitting this after having rebased it against the latest changes. >>> >>> thanks for resubmitting. Sorry I wasn't able to provide feedback on >>> the last revision >>> >> >> No worries, I know how these things are. >> >>>> >>>> The gaming line of Logitech devices doesn't use the old hidpp20 >>>> feature for battery level reporting. Instead, they report the >>>> current voltage of the battery, in millivolts. >>>> >>>> This patch set handles this case by adding a quirk to the >>>> devices we know to have this new feature, in both wired >>>> and wireless mode. >>> >>> So the quirk is in the end a bad idea after all. I had some chats with >>> Filipe that made me realize this. >> >> I actually resubmitted by Filipe's request, since the patches weren't >> applying cleanly anymore. The idea was to apply these patches and in the >> future refactor the code to use the feature discovery routines. >> >>> Re-reading our previous exchanges also made me understood why I wasn't >>> happy with the initial submission: for every request the code was >>> checking both features 0x1000 and 0x1001 when we can remember this >>> once and for all during hidpp_initialize_battery(). >> >> Yeah I wasn't too happy about this either, but since there was nothing >> prohibiting some device to have both features enabled, I thought this >> wasn't too horrible. > > I honestly don't think we will find one device that has both features > enabled. It doesn't make sense as the "new" feature allows for fine > tuning in the software, which would be moot if you also enable the > percentage. > >> >>> >>> So I think we should remove the useless quirk in the end (bad idea >>> from me, I concede), and instead during hidpp_initialize_battery() set >>> the correct HIDPP_CAPABILITY_*. >>> Not entirely sure if we should try to call 0x1000, or 0x1001 or if we >>> should rely on the 0x0001 feature to know which feature is available, >>> but this should be implementation detail. >> >> I like the idea of calling 0x0001 once on device boot much better. I >> think we could easily just fetch the location for the features we know >> about and want to deal with once only. This also makes sure we support >> every single device that supports this feature, so that is a huge plus. >> >> In fact, I think we should _not_ call 0x0001 on battery init, but only >> call battery init _after_ we called 0x0001 and discovered either 0x1000 >> or 0x1001 (or the solar battery feature, or any other one that might >> crop up in the feature). > > ack for that > >> >>> >>>> >>>> This version of the patch set is better split, as well as adding the >>>> quirk to make sure we don't needlessly probe every device connected. >>> >>> It is for sure easy to review, but doesn't make much sense in the end. >>> I think we should squash all the patches together as you are just >>> adding one feature in the driver, and it is a little bit disturbing to >>> first add the quirk that has no use, then set up the structs when they >>> are not used, and so on, so forth. >> >> You're right. My first instinct was to send a single patch. As much as I >> tested this, I always feel like breaking the patch up post-facto will >> break a git bisect in the future and everyone will hate me :P > > as long as the patches are compiling and are not breaking, git bisect > will not be a problem. However, we might end up with the last one, > which is not very explicit in what it does as it just enables the > features implemented previously. > >> >> So we (you, me and Filipe) should probably come up with an action plan >> here. The way I see it there are two issues here: one is adding this >> feature, and the other is refactoring to use feature discovery for all >> features. There are advantages and disadvantages to doing one or another >> first and we might want to discuss that. >> >> By merging this first (probably after I resubmit it as a single squashed >> patch) we get to test it a bit better and have a usable feature sooner. >> Plenty of people have been requesting this and there is plenty of stuff >> that can be built on top of it, but only once this is actually merged I >> think. >> >> On the other hand, by first refactoring the rest of the code to use >> 0x0001 we avoid some rework on this patch. It should be minor, as most >> functions here do all the heavy lifting after the initial feature >> discovery, and are thus mostly independent from how that is done. >> >> I'm happy either way, so just let me know what you guys decide. > > I think we should merge your v3 squashed series with a slight > autodetection during battery init, like the method you used in the v1. > This would remove the quirk, but keep the straightforward commands > when addressing battery data.
Alright, I rebased against for-5.4/logitech to make sure, squashed everything and restored the detection code from v1, removing the quirk. Tested and it works on both wired and wireless on my G900.
> > Relying on 0x0001 should be done separately and can come in in a later > patch IMO (unless you plan to work on it, in which case you can send > both at once).
0x0001 is quite the task and I think Filipe already has a good plan to tackle it, so I'll leave that for him.
> > The problem I have with quirks, and that I explained to Filipe on IRC > is that this is kernel ABI. Even if there is a very low chance we have > someone using this, re-using the same drv_data bit in the future might > break someone's device. > >> >> If you guys (or anyone else reading this on the public list, really) has >> any input - naming things being notoriosly hard, I'm actually happy with >> nitpicking - I'd appreciate it. On that note, come to think of it, I'm >> not 100% sure reporting the voltage in milivolts is the standard way. I >> looked through the docs, but found no solid guideline. It was either >> that or a float, so I think I made the right call here, but still. > > I am not sure either. Adding Bastien as he has a lot more experience in upower. > > But I am under the impression that the kernel part is more "try to > deal with whatever the hardware provides, and deal with it in user > space". >
I'll submit v4 as a single patch in the next couple of minutes.
- Pedro
| |