| Message ID | 20120702115937.623d3b41@kryten (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jul 02, 2012 at 11:59:37AM +1000, Anton Blanchard wrote: > >When using my Logitech Harmony remote I get regular dropped events >(about 1 in every 3). Adjusting the sample frequency to 6us so we >sample at a multiple of an RC-6 pulse (444us) fixes it. Sounds weird. The in-kernel RC6 decoder already has margins of around 50% for most pulse/spaces (i.e. 444us +/- 222us). Changing the sample resolution from 10 to 6 us should have little to no effect on the RC6 decoding (also, the Windows driver uses a 50us resolution IIRC). Do you have a log of a successful and unsuccesful event (the timings that is)? >Signed-off-by: Anton Blanchard <anton@samba.org> >--- > >Index: linux/drivers/media/rc/winbond-cir.c >=================================================================== >--- linux.orig/drivers/media/rc/winbond-cir.c 2012-07-01 14:54:28.993475033 +1000 >+++ linux/drivers/media/rc/winbond-cir.c 2012-07-01 14:55:50.500939910 +1000 >@@ -358,7 +358,7 @@ wbcir_irq_rx(struct wbcir_data *data, st > if (data->rxstate == WBCIR_RXSTATE_ERROR) > continue; > rawir.pulse = irdata & 0x80 ? false : true; >- rawir.duration = US_TO_NS((irdata & 0x7F) * 10); >+ rawir.duration = US_TO_NS((irdata & 0x7F) * 6); > ir_raw_event_store_with_filter(data->dev, &rawir); > } > >@@ -874,8 +874,8 @@ wbcir_init_hw(struct wbcir_data *data) > /* prescaler 1.0, tx/rx fifo lvl 16 */ > outb(0x30, data->sbase + WBCIR_REG_SP3_EXCR2); > >- /* Set baud divisor to sample every 10 us */ >- outb(0x0F, data->sbase + WBCIR_REG_SP3_BGDL); >+ /* Set baud divisor to sample every 6 us */ >+ outb(0x09, data->sbase + WBCIR_REG_SP3_BGDL); > outb(0x00, data->sbase + WBCIR_REG_SP3_BGDH); > > /* Set CEIR mode */ > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi David, > The in-kernel RC6 decoder already has margins of around 50% for most > pulse/spaces (i.e. 444us +/- 222us). Changing the sample resolution > from 10 to 6 us should have little to no effect on the RC6 decoding > (also, the Windows driver uses a 50us resolution IIRC). > > Do you have a log of a successful and unsuccesful event (the timings > that is)? I had a closer look. I dumped the RC6 debug, but I also printed the raw data in the interrupt handler: printk("%x %d %d\n", irdata, rawir.pulse, rawir.duration); A successful event begins with: 7f 1 1270000 7f 1 1270000 8 1 80000 db 0 910000 27 1 390000 b3 0 510000 26 1 380000 b0 0 480000 ir_rc6_decode: RC6 decode started at state 0 (2620us pulse) ir_rc6_decode: RC6 decode started at state 1 (910us space) ir_rc6_decode: RC6 decode started at state 2 (390us pulse) ir_rc6_decode: RC6 decode started at state 3 (510us space) ir_rc6_decode: RC6 decode started at state 2 (66us space) ir_rc6_decode: RC6 decode started at state 2 (380us pulse) 26 1 380000 db 0 910000 26 1 380000 dd 0 930000 7d 1 1250000 <--------------- dd 0 930000 25 1 370000 b4 0 520000 ir_rc6_decode: RC6 decode started at state 3 (480us space) ir_rc6_decode: RC6 decode started at state 2 (36us space) ir_rc6_decode: RC6 decode started at state 2 (380us pulse) ir_rc6_decode: RC6 decode started at state 3 (910us space) ir_rc6_decode: RC6 decode started at state 2 (466us space) ir_rc6_decode: RC6 decode started at state 3 (380us pulse) ir_rc6_decode: RC6 decode started at state 4 (0us pulse) ir_rc6_decode: RC6 decode started at state 4 (930us space) ir_rc6_decode: RC6 decode started at state 5 (1250us pulse) ir_rc6_decode: RC6 decode started at state 6 (361us pulse) ir_rc6_decode: RC6 decode started at state 7 (930us space) Now compare to an unsuccesful event, in particular the byte I have tagged in both traces: 7f 1 1270000 7f 1 1270000 2 1 20000 df 0 950000 26 1 380000 b0 0 480000 26 1 380000 b0 0 480000 26 1 380000 dc 0 920000 26 1 380000 ir_rc6_decode: RC6 decode started at state 0 (2560us pulse) ir_rc6_decode: RC6 decode started at state 1 (950us space) ir_rc6_decode: RC6 decode started at state 2 (380us pulse) ir_rc6_decode: RC6 decode started at state 3 (480us space) ir_rc6_decode: RC6 decode started at state 2 (36us space) ir_rc6_decode: RC6 decode started at state 2 (380us pulse) ir_rc6_decode: RC6 decode started at state 3 (480us space) ir_rc6_decode: RC6 decode started at state 2 (36us space) ir_rc6_decode: RC6 decode started at state 2 (380us pulse) ir_rc6_decode: RC6 decode started at state 3 (920us space) ir_rc6_decode: RC6 decode started at state 2 (476us space) dc 0 920000 ff 0 1270000 <---------------- de 0 940000 25 1 370000 b1 0 490000 26 1 380000 b0 0 480000 26 1 380000 ir_rc6_decode: RC6 decode started at state 3 (380us pulse) ir_rc6_decode: RC6 decode started at state 4 (0us pulse) ir_rc6_decode: RC6 decode started at state 4 (3130us space) ir_rc6_decode: RC6 decode failed at state 4 (3130us space) That should have been a pulse but it came out as a space. This makes me wonder if there is an issue with the run length encoding, perhaps when a pulse is the right size to just saturate it. It does seem like we set the top bit even though we should not have. If true we could choose any sample rate that avoids it. Anton -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
hi Anton, it's very debatable that is fault of the Linux driver(s) and let me elaborate why i think so: i have Logitech Harmony 890 remote and MCE USB IR receiver that is supported by 'mceusb.c' in Linux, i.e. it's different than your IR receiver, but yet the work is very unstable together with my Harmony no matter what type of pulses are used, e.g. RC5, RC6 or NEC. however, that is true only if default setting for the Harmony are set in the Logitech software for Windows that i used to initially setup the remote. so, if in "Logitech Harmony Remote Software (7.7.0)" for Windows go to: Devices -> Settings -> select 'Troubleshoot' -> click 'Next' -> select '...responds to some commands either too many times or only occasionally' -> click 'Next' then you're given the option to choose number from 0 to 5 with the following explanation by Logitech: "If your device responds too slowly, or not at all when you press a button on the remote, increase the value to 4 or 5. If your device responds too quickly, lower the value to 2, 1 or 0." there are no any details about what actually that setting is doing to the Harmony firmware and in fact i found that neither the default value, nor Logitech suggestion what value to choose in which case are good, but using value of "2" fixed all the issues with my MCE USB IR receiver in Linux - totally stable and proper work. in short i think the problem is created from Harmony firmware and the proper solution is to just play with those numbers in Logitech software until you find the setting with which you get stable work. of course, someone more interested than i'm in this could do further investigation and understand the root cause of the issue and what changes in how Harmony firmware behaves based on those settings Logitech offered in their software, but my point is that i have similar issue with RC5 pulses, Harmony 890 and MCE USB IR receiver that is supported by 'mceusb.c' or quite different setup than yours and the fix i found is from Logitech side and the fact Logitech offered such setting suggests that the proper way to fix it. best regards, konstantin On Thu, Jul 5, 2012 at 1:30 PM, Anton Blanchard <anton@samba.org> wrote: > > Hi David, > >> The in-kernel RC6 decoder already has margins of around 50% for most >> pulse/spaces (i.e. 444us +/- 222us). Changing the sample resolution >> from 10 to 6 us should have little to no effect on the RC6 decoding >> (also, the Windows driver uses a 50us resolution IIRC). >> >> Do you have a log of a successful and unsuccesful event (the timings >> that is)? > > I had a closer look. I dumped the RC6 debug, but I also printed the raw > data in the interrupt handler: > > printk("%x %d %d\n", irdata, rawir.pulse, rawir.duration); > > A successful event begins with: > > 7f 1 1270000 > 7f 1 1270000 > 8 1 80000 > db 0 910000 > 27 1 390000 > b3 0 510000 > 26 1 380000 > b0 0 480000 > ir_rc6_decode: RC6 decode started at state 0 (2620us pulse) > ir_rc6_decode: RC6 decode started at state 1 (910us space) > ir_rc6_decode: RC6 decode started at state 2 (390us pulse) > ir_rc6_decode: RC6 decode started at state 3 (510us space) > ir_rc6_decode: RC6 decode started at state 2 (66us space) > ir_rc6_decode: RC6 decode started at state 2 (380us pulse) > 26 1 380000 > db 0 910000 > 26 1 380000 > dd 0 930000 > 7d 1 1250000 <--------------- > dd 0 930000 > 25 1 370000 > b4 0 520000 > ir_rc6_decode: RC6 decode started at state 3 (480us space) > ir_rc6_decode: RC6 decode started at state 2 (36us space) > ir_rc6_decode: RC6 decode started at state 2 (380us pulse) > ir_rc6_decode: RC6 decode started at state 3 (910us space) > ir_rc6_decode: RC6 decode started at state 2 (466us space) > ir_rc6_decode: RC6 decode started at state 3 (380us pulse) > ir_rc6_decode: RC6 decode started at state 4 (0us pulse) > ir_rc6_decode: RC6 decode started at state 4 (930us space) > ir_rc6_decode: RC6 decode started at state 5 (1250us pulse) > ir_rc6_decode: RC6 decode started at state 6 (361us pulse) > ir_rc6_decode: RC6 decode started at state 7 (930us space) > > Now compare to an unsuccesful event, in particular the byte > I have tagged in both traces: > > 7f 1 1270000 > 7f 1 1270000 > 2 1 20000 > df 0 950000 > 26 1 380000 > b0 0 480000 > 26 1 380000 > b0 0 480000 > 26 1 380000 > dc 0 920000 > 26 1 380000 > ir_rc6_decode: RC6 decode started at state 0 (2560us pulse) > ir_rc6_decode: RC6 decode started at state 1 (950us space) > ir_rc6_decode: RC6 decode started at state 2 (380us pulse) > ir_rc6_decode: RC6 decode started at state 3 (480us space) > ir_rc6_decode: RC6 decode started at state 2 (36us space) > ir_rc6_decode: RC6 decode started at state 2 (380us pulse) > ir_rc6_decode: RC6 decode started at state 3 (480us space) > ir_rc6_decode: RC6 decode started at state 2 (36us space) > ir_rc6_decode: RC6 decode started at state 2 (380us pulse) > ir_rc6_decode: RC6 decode started at state 3 (920us space) > ir_rc6_decode: RC6 decode started at state 2 (476us space) > dc 0 920000 > ff 0 1270000 <---------------- > de 0 940000 > 25 1 370000 > b1 0 490000 > 26 1 380000 > b0 0 480000 > 26 1 380000 > ir_rc6_decode: RC6 decode started at state 3 (380us pulse) > ir_rc6_decode: RC6 decode started at state 4 (0us pulse) > ir_rc6_decode: RC6 decode started at state 4 (3130us space) > ir_rc6_decode: RC6 decode failed at state 4 (3130us space) > > That should have been a pulse but it came out as a space. This makes me > wonder if there is an issue with the run length encoding, perhaps when > a pulse is the right size to just saturate it. It does seem like we > set the top bit even though we should not have. > > If true we could choose any sample rate that avoids it. > > Anton > -- > To unsubscribe from this list: send the line "unsubscribe linux-media" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 5 Jul 2012 20:30:35 +1000, Anton Blanchard <anton@samba.org> wrote: > I had a closer look. I dumped the RC6 debug, but I also printed the raw > data in the interrupt handler: > > printk("%x %d %d\n", irdata, rawir.pulse, rawir.duration); > ... > That should have been a pulse but it came out as a space. This makes me > wonder if there is an issue with the run length encoding, perhaps when > a pulse is the right size to just saturate it. It does seem like we > set the top bit even though we should not have. It's quite weird to see a "short" space followed by a max space followed by a "short" space (0xdc 0xff 0xde). Almost like there's one or more (pulse) bytes missing in between. I've tested long pulses/spaces before and they've worked as expected (e.g. "max", "max", "short" events....the leading 0x7f 0x7f 0x08 sequence in your log is a good example). Now, there is a minor bug in the RLE decoding in that the duration should have 1 added to it (meaning that 0x00 or 0x80 are valid values). Just to make sure something like that isn't happening, could you correct the line in wbcir_irq_rx() which currently reads: rawir.duration = US_TO_NS((irdata & 0x7F) * 10); so that it reads rawir.duration = US_TO_NS(((irdata & 0x7F) + 1) * 10); However, I'm guessing you inserted the extra debug printk inside wbcir_irq_rx() so any 0x00 or 0x80 bytes would have been printed? Another possibility is that the printk in the interrupt handler causes overhead...could you do a debug run without the printk in the interrupt handler? Also, could you provide me with the full versions of both logs? (i.e. all the way to idle....it might help spot a missed pulse/space) Thanks, David -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
hi David, excuse me for my ignorance, but don't you think adjusting the IR receiver to universal remote control is fundamentally wrong, while the whole point of universal remote control like Logitech Harmony is to be adjusted to the IR receiver and be able to be adjusted to any IR receiver and not the other way around. so, that being said, my point is maybe the whole discussion here is just wild goose chase until those settings i mentioned in Logitech control software are not tried and there is no evidence that has already being done based on the information provided by Anton. we don't know what exactly those settings applied to Logitech Harmony firmware via Logitech control software do and it could be default pulse timings that are set trough them are just out of specification for RC6 and need to be manually refined using the Harmony firmware settings in question - once again after all universal remote control is supposed to be able to fit any IR receiver and any type of pulses and that's why provides series of different settings in order to do that - the issue seems more like misconfiguration of the universal remote control than Linux drivers problem. i'm just trying to save you time chasing not existing problems and don't mean anything else - i didn't even look at the source code you're discussing - i just have practical experience with Logitech Harmony 890 and thus i know keymaps and protocols are independently set from the proper pulse timings with Logitech control software. best regards, konstantin On Thu, Jul 5, 2012 at 5:13 PM, David Härdeman <david@hardeman.nu> wrote: > On Thu, 5 Jul 2012 20:30:35 +1000, Anton Blanchard <anton@samba.org> > wrote: >> I had a closer look. I dumped the RC6 debug, but I also printed the raw >> data in the interrupt handler: >> >> printk("%x %d %d\n", irdata, rawir.pulse, rawir.duration); >> > ... >> That should have been a pulse but it came out as a space. This makes me >> wonder if there is an issue with the run length encoding, perhaps when >> a pulse is the right size to just saturate it. It does seem like we >> set the top bit even though we should not have. > > It's quite weird to see a "short" space followed by a max space followed > by a "short" space (0xdc 0xff 0xde). Almost like there's one or more > (pulse) bytes missing in between. > > I've tested long pulses/spaces before and they've worked as expected (e.g. > "max", "max", "short" events....the leading 0x7f 0x7f 0x08 sequence in your > log is a good example). > > Now, there is a minor bug in the RLE decoding in that the duration should > have 1 added to it (meaning that 0x00 or 0x80 are valid values). > > Just to make sure something like that isn't happening, could you correct > the line in wbcir_irq_rx() which currently reads: > > rawir.duration = US_TO_NS((irdata & 0x7F) * 10); > > so that it reads > > rawir.duration = US_TO_NS(((irdata & 0x7F) + 1) * 10); > > However, I'm guessing you inserted the extra debug printk inside > wbcir_irq_rx() so any 0x00 or 0x80 bytes would have been printed? > > Another possibility is that the printk in the interrupt handler causes > overhead...could you do a debug run without the printk in the interrupt > handler? > > Also, could you provide me with the full versions of both logs? (i.e. all > the way to idle....it might help spot a missed pulse/space) > > Thanks, > David > > -- > To unsubscribe from this list: send the line "unsubscribe linux-media" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 5 Jul 2012 17:39:18 +0300, Konstantin Dimitrov <kosio.dimitrov@gmail.com> wrote: > excuse me for my ignorance, but don't you think adjusting the IR > receiver to universal remote control is fundamentally wrong, while the > whole point of universal remote control like Logitech Harmony is to be > adjusted to the IR receiver and be able to be adjusted to any IR > receiver and not the other way around. so, that being said, my point > is maybe the whole discussion here is just wild goose chase until > those settings i mentioned in Logitech control software are not tried > and there is no evidence that has already being done based on the > information provided by Anton. we don't know what exactly those > settings applied to Logitech Harmony firmware via Logitech control > software do and it could be default pulse timings that are set trough > them are just out of specification for RC6 and need to be manually > refined using the Harmony firmware settings in question - once again > after all universal remote control is supposed to be able to fit any > IR receiver and any type of pulses and that's why provides series of > different settings in order to do that - the issue seems more like > misconfiguration of the universal remote control than Linux drivers > problem. i'm just trying to save you time chasing not existing > problems and don't mean anything else - i didn't even look at the > source code you're discussing - i just have practical experience with > Logitech Harmony 890 and thus i know keymaps and protocols are > independently set from the proper pulse timings with Logitech control > software. Konstantin, thanks for your concern, but some of the byte sequences that Anton showed are "incorrect" in the sense that the receiver hardware should never generate them no matter what the (Logitech) remote is sending (unless I've misunderstood something of course). "0xdc 0xff 0xde" is a sequence which shouldn't happen. So even if the Logitech can be tweaked (and I'm sure Anton is grateful for the information), there is something wrong here and I'd like to get to the bottom of what it is. Kind regards, David -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
David, i see your point - as i mentioned i have no any knowledge of IR receiver part you're discussing and/or its Linux drivers and don't want just to spam, but my simple thinking is that if the Logitech Harmony universal remote control is with wrongly configured firmware it can emit something that is so messed up that makes the IR receiver part behaves in completely unpredictable way. anyway, at least Anton have some ideas to try... On Thu, Jul 5, 2012 at 5:45 PM, David Härdeman <david@hardeman.nu> wrote: > On Thu, 5 Jul 2012 17:39:18 +0300, Konstantin Dimitrov > <kosio.dimitrov@gmail.com> wrote: >> excuse me for my ignorance, but don't you think adjusting the IR >> receiver to universal remote control is fundamentally wrong, while the >> whole point of universal remote control like Logitech Harmony is to be >> adjusted to the IR receiver and be able to be adjusted to any IR >> receiver and not the other way around. so, that being said, my point >> is maybe the whole discussion here is just wild goose chase until >> those settings i mentioned in Logitech control software are not tried >> and there is no evidence that has already being done based on the >> information provided by Anton. we don't know what exactly those >> settings applied to Logitech Harmony firmware via Logitech control >> software do and it could be default pulse timings that are set trough >> them are just out of specification for RC6 and need to be manually >> refined using the Harmony firmware settings in question - once again >> after all universal remote control is supposed to be able to fit any >> IR receiver and any type of pulses and that's why provides series of >> different settings in order to do that - the issue seems more like >> misconfiguration of the universal remote control than Linux drivers >> problem. i'm just trying to save you time chasing not existing >> problems and don't mean anything else - i didn't even look at the >> source code you're discussing - i just have practical experience with >> Logitech Harmony 890 and thus i know keymaps and protocols are >> independently set from the proper pulse timings with Logitech control >> software. > > Konstantin, > > thanks for your concern, but some of the byte sequences that Anton showed > are "incorrect" in the sense that the receiver hardware should never > generate them no matter what the (Logitech) remote is sending (unless I've > misunderstood something of course). > > "0xdc 0xff 0xde" is a sequence which shouldn't happen. > > So even if the Logitech can be tweaked (and I'm sure Anton is grateful for > the information), there is something wrong here and I'd like to get to the > bottom of what it is. > > Kind regards, > David > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Index: linux/drivers/media/rc/winbond-cir.c =================================================================== --- linux.orig/drivers/media/rc/winbond-cir.c 2012-07-01 14:54:28.993475033 +1000 +++ linux/drivers/media/rc/winbond-cir.c 2012-07-01 14:55:50.500939910 +1000 @@ -358,7 +358,7 @@ wbcir_irq_rx(struct wbcir_data *data, st if (data->rxstate == WBCIR_RXSTATE_ERROR) continue; rawir.pulse = irdata & 0x80 ? false : true; - rawir.duration = US_TO_NS((irdata & 0x7F) * 10); + rawir.duration = US_TO_NS((irdata & 0x7F) * 6); ir_raw_event_store_with_filter(data->dev, &rawir); } @@ -874,8 +874,8 @@ wbcir_init_hw(struct wbcir_data *data) /* prescaler 1.0, tx/rx fifo lvl 16 */ outb(0x30, data->sbase + WBCIR_REG_SP3_EXCR2); - /* Set baud divisor to sample every 10 us */ - outb(0x0F, data->sbase + WBCIR_REG_SP3_BGDL); + /* Set baud divisor to sample every 6 us */ + outb(0x09, data->sbase + WBCIR_REG_SP3_BGDL); outb(0x00, data->sbase + WBCIR_REG_SP3_BGDH); /* Set CEIR mode */
When using my Logitech Harmony remote I get regular dropped events (about 1 in every 3). Adjusting the sample frequency to 6us so we sample at a multiple of an RC-6 pulse (444us) fixes it. Signed-off-by: Anton Blanchard <anton@samba.org> --- -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html