Re: [PATCH v2 0/9] rc: Add IR encode based wakeup filtering
Hi Mauro, On Wednesday 23 July 2014 16:39:36 Mauro Carvalho Chehab wrote: Em Fri, 14 Mar 2014 23:04:10 + James Hogan ja...@albanarts.com escreveu: A recent discussion about proposed interfaces for setting up the hardware wakeup filter lead to the conclusion that it could help to have the generic capability to encode and modulate scancodes into raw IR events so that drivers for hardware with a low level wake filter (on the level of pulse/space durations) can still easily implement the higher level scancode interface that is proposed. I posted an RFC patchset showing how this could work, and Antti Seppälä posted additional patches to support rc5-sz and nuvoton-cir. This patchset improves the original RFC patches and combines updates Antti's patches. I'm happy these patches are a good start at tackling the problem, as long as Antti is happy with them and they work for him of course. Future work could include: - Encoders for more protocols. - Carrier signal events (no use unless a driver makes use of it). Patch 1 adds the new encode API. Patches 2-3 adds some modulation helpers. Patches 4-6 adds some raw encode implementations. Patch 7 adds some rc-core support for encode based wakeup filtering. Patch 8 adds debug loopback of encoded scancode when filter set. Patch 9 (untested) adds encode based wakeup filtering to nuvoton-cir. Changes in v2: Any news about this patch series? There are some comments about them, so I'll be tagging it as changes requested at patchwork, waiting for a v3 (or is it already there in the middle of the 49 patches from David?). This patch series seems to have been forgotten. I do have a few changes on top of v2 to address the review comments, so as you say I should probably rebase and do a v3 at some point. Cheers James -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/9] rc: Add IR encode based wakeup filtering
Em Fri, 14 Mar 2014 23:04:10 + James Hogan ja...@albanarts.com escreveu: A recent discussion about proposed interfaces for setting up the hardware wakeup filter lead to the conclusion that it could help to have the generic capability to encode and modulate scancodes into raw IR events so that drivers for hardware with a low level wake filter (on the level of pulse/space durations) can still easily implement the higher level scancode interface that is proposed. I posted an RFC patchset showing how this could work, and Antti Seppälä posted additional patches to support rc5-sz and nuvoton-cir. This patchset improves the original RFC patches and combines updates Antti's patches. I'm happy these patches are a good start at tackling the problem, as long as Antti is happy with them and they work for him of course. Future work could include: - Encoders for more protocols. - Carrier signal events (no use unless a driver makes use of it). Patch 1 adds the new encode API. Patches 2-3 adds some modulation helpers. Patches 4-6 adds some raw encode implementations. Patch 7 adds some rc-core support for encode based wakeup filtering. Patch 8 adds debug loopback of encoded scancode when filter set. Patch 9 (untested) adds encode based wakeup filtering to nuvoton-cir. Changes in v2: Any news about this patch series? There are some comments about them, so I'll be tagging it as changes requested at patchwork, waiting for a v3 (or is it already there in the middle of the 49 patches from David?). Regards, Mauro -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/9] rc: Add IR encode based wakeup filtering
On Mon, Mar 17, 2014 at 10:34:25PM +, James Hogan wrote: It's ambiguous the other way too (which is probably a strong point against having actual protocol bits for each NEC variant, since they only differ in how the scancode is constructed). E.g. the Tivo keymap is 32-bit NEC, but has extended NEC scancodes where the bytes of the command are complements (i.e. the extended NEC command checksum passes). This makes it hard to filter on at the scancode level (the drivers will probably get it right for the hardware filters, but the software filter will likely get it wrong in those corner cases since it knows nothing of NEC). There's multiple ways the NEC scancode formats could be improved (incompatibly!) to reduce the problems, but none are perfect. E.g. one possibility is to scrap the NEC and extended NEC scancodes and just use 32-bit NEC scancodes format throughout: YES! All the knowledge of original NEC (16 bit), extended NEC, etc that have multiplied over both drivers and in various parts of rc-core is a big mistake IMHO. The only sane way of handling NEC is to always treat it as a 32 bit scancode (and only in cases where e.g. the hardware returns or expects a 16 bit value should the scancode be converted to/from the canonical 32 bit format). There is absolutely no advantages in trying to parse or understand the NEC format unless it absolutely cannot be avoided. I haven't had the time to really review your patches in depth, but whatever you do, please try to keep any knowledge of NEC 16/24/32 bit distinction out of any functionality you add. I had a suggested patch before which would also make the keymap handling 32-bit centric...essentially by redefining the set/get keymap ioctls a bit (with backwards compatibility that guesses if the scancode is 16/24/32 bit based). It's been on my todo list for a long time to dust it off...(yeah...I know)... 0x[16-bit-address][16-bit-command] which encodes scancodes for extended NEC like this: 0x[16-bit-address][~8-bit-command][8-bit-command] and normal NEC like this: 0x[~8-bit-address][8-bit-address][~8-bit-command][8-bit-command] Thanks James -- David Härdeman -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/9] rc: Add IR encode based wakeup filtering
Hi James, On 17 March 2014 00:41, James Hogan ja...@albanarts.com wrote: Yeh I'm in two minds about this now. It's actually a little awkward since some of the protocols have multiple variants (i.e. rc-5 = RC5+RC5X), but an encoded message is only ever a single variant, so technically if you're going to draw the line for wakeup protocols it should probably be at one enabled variant, which isn't always convenient or necessary. I'd very much prefer to have the selector as it currently is - protocol groups instead of variants which would keep it consistent with decoding protocol selection. Note, ATM even disallowing +proto and -proto we would already have to guess which variant is desired from the scancode data, which in the case of NEC scancodes is a bit horrid since NEC scancodes are ambiguous. This actually means it's driver specific whether a filter mask of 0x filters out NEC32/NEC-X messages (scancode/encode driver probably will since it needs to pick a variant, but software fallback won't). How common is it that NEC codes are really ambiguous? Or that a wrong variant is selected for encoding? A quick look suggests that the length of the scancode will be good enough way to determine which variant is used for NEC, RC-5(X) and RC-6(A). If the variant is really needed selecting it might be done in some other sysfs file. But I'd not implement it yet if we can manage without such logic. -Antti -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/9] rc: Add IR encode based wakeup filtering
On Monday 17 March 2014 19:01:51 Antti Seppälä wrote: On 17 March 2014 00:41, James Hogan ja...@albanarts.com wrote: Yeh I'm in two minds about this now. It's actually a little awkward since some of the protocols have multiple variants (i.e. rc-5 = RC5+RC5X), but an encoded message is only ever a single variant, so technically if you're going to draw the line for wakeup protocols it should probably be at one enabled variant, which isn't always convenient or necessary. I'd very much prefer to have the selector as it currently is - protocol groups instead of variants which would keep it consistent with decoding protocol selection. Yeh, I'll submit a patch to fix wakeup-protocols to disallow multiple groups of protocols from being enabled at the same time. Note, ATM even disallowing +proto and -proto we would already have to guess which variant is desired from the scancode data, which in the case of NEC scancodes is a bit horrid since NEC scancodes are ambiguous. This actually means it's driver specific whether a filter mask of 0x filters out NEC32/NEC-X messages (scancode/encode driver probably will since it needs to pick a variant, but software fallback won't). How common is it that NEC codes are really ambiguous? Or that a wrong variant is selected for encoding? A quick look suggests that the length of the scancode will be good enough way to determine which variant is used for NEC, RC-5(X) and RC-6(A). When I tried filtering for my TV remote it didn't work. It turned out to be because the extended nec scancode has the address bytes in the wrong order so that the bits are discontinuous compared to the raw data. The remote uses extended NEC but has zero in the lower byte of the address, which unfortunately goes in bits 23:16 of the scancode above the other byte of the address, so it looks as if it's using normal NEC (16bit scancodes). This is why I ended up making img-ir use the mask too in the decision. It's ambiguous the other way too (which is probably a strong point against having actual protocol bits for each NEC variant, since they only differ in how the scancode is constructed). E.g. the Tivo keymap is 32-bit NEC, but has extended NEC scancodes where the bytes of the command are complements (i.e. the extended NEC command checksum passes). This makes it hard to filter on at the scancode level (the drivers will probably get it right for the hardware filters, but the software filter will likely get it wrong in those corner cases since it knows nothing of NEC). There's multiple ways the NEC scancode formats could be improved (incompatibly!) to reduce the problems, but none are perfect. E.g. one possibility is to scrap the NEC and extended NEC scancodes and just use 32-bit NEC scancodes format throughout: 0x[16-bit-address][16-bit-command] which encodes scancodes for extended NEC like this: 0x[16-bit-address][~8-bit-command][8-bit-command] and normal NEC like this: 0x[~8-bit-address][8-bit-address][~8-bit-command][8-bit-command] Thanks James signature.asc Description: This is a digitally signed message part.
Re: [PATCH v2 0/9] rc: Add IR encode based wakeup filtering
On 15 March 2014 01:04, James Hogan ja...@albanarts.com wrote: A recent discussion about proposed interfaces for setting up the hardware wakeup filter lead to the conclusion that it could help to have the generic capability to encode and modulate scancodes into raw IR events so that drivers for hardware with a low level wake filter (on the level of pulse/space durations) can still easily implement the higher level scancode interface that is proposed. I posted an RFC patchset showing how this could work, and Antti Seppälä posted additional patches to support rc5-sz and nuvoton-cir. This patchset improves the original RFC patches and combines updates Antti's patches. I'm happy these patches are a good start at tackling the problem, as long as Antti is happy with them and they work for him of course. Future work could include: - Encoders for more protocols. - Carrier signal events (no use unless a driver makes use of it). Patch 1 adds the new encode API. Patches 2-3 adds some modulation helpers. Patches 4-6 adds some raw encode implementations. Patch 7 adds some rc-core support for encode based wakeup filtering. Patch 8 adds debug loopback of encoded scancode when filter set. Patch 9 (untested) adds encode based wakeup filtering to nuvoton-cir. Hi James. This is looking very good. I've reviewed the series and have only minor comments to some of the patches which I'll post individually shortly. I've also tested the nuvoton with actual hardware with rc-5-sz and nec encoders and both generate wakeup samples correctly and can wake the system. While doing my tests I also noticed that there is a small bug in the wakeup_protocols handling where one can enable multiple protocols with the + -notation. E.g. echo +nec +rc-5 /sys/class/rc/rc0/wakeup_protocols shouldn't probably succeed. -Antti -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 0/9] rc: Add IR encode based wakeup filtering
On Sunday 16 March 2014 10:22:02 Antti Seppälä wrote: On 15 March 2014 01:04, James Hogan ja...@albanarts.com wrote: A recent discussion about proposed interfaces for setting up the hardware wakeup filter lead to the conclusion that it could help to have the generic capability to encode and modulate scancodes into raw IR events so that drivers for hardware with a low level wake filter (on the level of pulse/space durations) can still easily implement the higher level scancode interface that is proposed. I posted an RFC patchset showing how this could work, and Antti Seppälä posted additional patches to support rc5-sz and nuvoton-cir. This patchset improves the original RFC patches and combines updates Antti's patches. I'm happy these patches are a good start at tackling the problem, as long as Antti is happy with them and they work for him of course. Future work could include: - Encoders for more protocols. - Carrier signal events (no use unless a driver makes use of it). Patch 1 adds the new encode API. Patches 2-3 adds some modulation helpers. Patches 4-6 adds some raw encode implementations. Patch 7 adds some rc-core support for encode based wakeup filtering. Patch 8 adds debug loopback of encoded scancode when filter set. Patch 9 (untested) adds encode based wakeup filtering to nuvoton-cir. Hi James. This is looking very good. I've reviewed the series and have only minor comments to some of the patches which I'll post individually shortly. I've also tested the nuvoton with actual hardware with rc-5-sz and nec encoders and both generate wakeup samples correctly and can wake the system. Thanks for reviewing and testing! While doing my tests I also noticed that there is a small bug in the wakeup_protocols handling where one can enable multiple protocols with the + -notation. E.g. echo +nec +rc-5 /sys/class/rc/rc0/wakeup_protocols shouldn't probably succeed. Yeh I'm in two minds about this now. It's actually a little awkward since some of the protocols have multiple variants (i.e. rc-5 = RC5+RC5X), but an encoded message is only ever a single variant, so technically if you're going to draw the line for wakeup protocols it should probably be at one enabled variant, which isn't always convenient or necessary. Note, ATM even disallowing +proto and -proto we would already have to guess which variant is desired from the scancode data, which in the case of NEC scancodes is a bit horrid since NEC scancodes are ambiguous. This actually means it's driver specific whether a filter mask of 0x filters out NEC32/NEC-X messages (scancode/encode driver probably will since it needs to pick a variant, but software fallback won't). Ideally there'd now be a way to specify the protocol variants exactly as well as whole protocols groups through this sysfs interface (and probably NEC should have protocol bits for each variant too, which is tricky ATM since keymaps can use scancodes of multiple variants and it's hard to guarantee which variant was intended for each key mapping by reading it). Adding proto_names entries for each variant is easy enough, though I'm not sure what you'd call the one for RC_BIT_RC5 (since rc-5 is taken to mean both RC5 and RC5X). We could then check that the enabled wakeup protocols fit entirely within one of the proto_names entry's proto mask if we think it's helpful (which would allow you to specify e.g. sony12, or sony (sony 12,15,20), or sony -sony12 (sony 15,20), but not +sony +nec). Cheers James signature.asc Description: This is a digitally signed message part.
[PATCH v2 0/9] rc: Add IR encode based wakeup filtering
A recent discussion about proposed interfaces for setting up the hardware wakeup filter lead to the conclusion that it could help to have the generic capability to encode and modulate scancodes into raw IR events so that drivers for hardware with a low level wake filter (on the level of pulse/space durations) can still easily implement the higher level scancode interface that is proposed. I posted an RFC patchset showing how this could work, and Antti Seppälä posted additional patches to support rc5-sz and nuvoton-cir. This patchset improves the original RFC patches and combines updates Antti's patches. I'm happy these patches are a good start at tackling the problem, as long as Antti is happy with them and they work for him of course. Future work could include: - Encoders for more protocols. - Carrier signal events (no use unless a driver makes use of it). Patch 1 adds the new encode API. Patches 2-3 adds some modulation helpers. Patches 4-6 adds some raw encode implementations. Patch 7 adds some rc-core support for encode based wakeup filtering. Patch 8 adds debug loopback of encoded scancode when filter set. Patch 9 (untested) adds encode based wakeup filtering to nuvoton-cir. Changes in v2: Patchset: - Alter encode API to return -ENOBUFS when there isn't enough buffer space. When this occurs all buffer contents must have been written with the partial encoding of the scancode. This is to allow drivers such as nuvoton-cir to provide a shorter buffer and still get a useful partial encoding for the wakeup pattern. - Added RC-5 RC-5X encoder. - Add encode_wakeup support which keeps allowed wakeup protocols in sync with registered encoders. Pulse-distance modulation helper: - Update ir_raw_gen_pd() with a kerneldoc comment and individual buffer full checks rather than a single one at the beginning, in order to support -ENOBUFS properly. - Update ir_raw_gen_pulse_space() to check the number of free slots and fill as many as possible before returning -ENOBUFS. - Fix brace placement for timings struct. Manchester encoding helper: - Add kerneldoc comment. - Add individual buffer full checks, in order to support -ENOBUFS properly. - Make i unsigned to theoretically support all 32bits of data. - Increment *ev at end so caller can calculate correct number of events (during the loop *ev points to the last written event to allow it to be extended in length). - Make start/leader pulse optional, continuing from (*ev)[-1] if disabled. This helps support rc-5x which has a space in the middle of the bits. rc5-sz encoder: - Turn ir_rc5_sz_encode() comment into kerneldoc and update to reflect new API. - Be more flexible around accepted scancode masks, as long as all the important bits are set (0x2fff) and none of the unimportant bits are set in the data. Also mask off the unimportant bits before passing to ir_raw_gen_manchester(). - Explicitly enable leader bit in Manchester modulation timings. rc-loopback: - Move img-ir-raw test code to rc-loopback. - Set encode_wakeup so that the set of allowed wakeup protocols matches the set of raw IR encoders. nuvoton-cir: - Change reference to rc_dev::enabled_protocols to enabled_protocols[type] since it has been converted to an array. - Fix IR encoding buffer loop condition to be i ret rather than i = ret. The return value of ir_raw_encode_scancode is the number of events rather than the last event. - Set encode_wakeup so that the set of allowed wakeup protocols matches the set of raw IR encoders. Cc: Mauro Carvalho Chehab m.che...@samsung.com Cc: Antti Seppälä a.sepp...@gmail.com Cc: David Härdeman da...@hardeman.nu Cc: Jarod Wilson ja...@redhat.com Cc: Wei Yongjun yongjun_...@trendmicro.com.cn Cc: Hans Verkuil hans.verk...@cisco.com Antti Seppälä (3): rc: ir-raw: Add Manchester encoder (phase encoder) helper rc: ir-rc5-sz-decoder: Add ir encoding support rc: nuvoton-cir: Add support for writing wakeup samples via sysfs filter callback James Hogan (6): rc: ir-raw: Add scancode encoder callback rc: ir-raw: Add pulse-distance modulation helper rc: ir-nec-decoder: Add encode capability rc: ir-rc5-decoder: Add encode capability rc: rc-core: Add support for encode_wakeup drivers rc: rc-loopback: Add loopback of filter scancodes drivers/media/rc/ir-nec-decoder.c| 93 + drivers/media/rc/ir-raw.c| 191 +++ drivers/media/rc/ir-rc5-decoder.c| 103 +++ drivers/media/rc/ir-rc5-sz-decoder.c | 45 + drivers/media/rc/nuvoton-cir.c | 123 ++ drivers/media/rc/nuvoton-cir.h | 1 + drivers/media/rc/rc-core-priv.h | 85 drivers/media/rc/rc-loopback.c | 39 +++ drivers/media/rc/rc-main.c | 11 +- include/media/rc-core.h | 7 ++ 10 files changed, 695 insertions(+), 3 deletions(-) -- 1.8.3.2
