Em Wed, 14 Feb 2018 21:32:07 +0000 Sean Young <s...@mess.org> escreveu:
> Hi Mauro, > > On Wed, Feb 14, 2018 at 04:44:48PM -0200, Mauro Carvalho Chehab wrote: > > Hi Sean, > > > > Em Mon, 12 Feb 2018 20:03:18 +0000 > > Sean Young <s...@mess.org> escreveu: > > > > > Hi Mauro, > > > > > > Just very minor changes this time (other stuff is not ready yet). I would > > > really appreciate if you could cast an extra critical eye on the commit > > > "no need to check for transitions", just to be sure it is the right > > > change. > > > > Did you send all patches in separate? This is important to allow us > > to comment on an specific issue inside a patch... > > All the patches were emailed to linux-media, some of them on the same day > as the pull request. Maybe I should wait longer. The patch below was sent > out on the 28th of January. > > > > media: rc: no need to check for transitions No need to wait longer. Yet, it seems that I lost the above patch, as I couldn't find anything on my email with the above subject. Perhaps the e-mail got lost somehow on my inbox. > > > > I don't remember the exact reason for that, but, as far as I > > remember, on a few devices, a pulse (or space) event could be > > broken into two consecutive events of the same type, e. g., > > a pulse with a 125 ms could be broken into two pulses, like > > one with 100 ms and the other with 25 ms. > > If that is the case, then the IR decoders could not deal with this anyway. > For example, the first state transition rc6 is: > > if (!eq_margin(ev.duration, RC6_PREFIX_PULSE, RC6_UNIT)) > > So if ev.duration is not the complete duration, then decoding will fail; > I tried to explain in the commit message that if this was the case, then > decoding would not work so the check was unnecessary. > > > That's said, I'm not sure if the current implementation are > > adding the timings for both pulses into a single one. > > That depends on whether the driver uses ir_raw_event_store() or > ir_raw_event_store_with_filter(). The latter exists precisely for this > reason. OK. > > > For now, I'll keep this patch out of the merge. > > Ok. So in summary, I think: > > 1. Any driver which produces consequentive pulse events is broken > and should be fixed; Agreed. > 2. The IR decoders cannot deal with consequentive pulses and the current > prev_ev code does not help with this (possibly in very special > cases). Ok. Yet, maybe it would worth to produce a warning if this happen, and reset the state machine, as it would help to identify problems at the driver or at the hardware. Thanks, Mauro
