| Message ID | 1427824092-23163-2-git-send-email-a.seppala@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Mar 31, 2015 at 08:48:06PM +0300, Antti Seppälä wrote: >From: James Hogan <james@albanarts.com> > >Add a callback to raw ir handlers for encoding and modulating a scancode >to a set of raw events. This could be used for transmit, or for >converting a wakeup scancode filter to a form that is more suitable for >raw hardware wake up filters. > >Signed-off-by: James Hogan <james@albanarts.com> >Signed-off-by: Antti Seppälä <a.seppala@gmail.com> >Cc: David Härdeman <david@hardeman.nu> >--- > >Notes: > Changes in v3: > - Ported to apply against latest media-tree > > Changes in v2: > - 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. > > drivers/media/rc/rc-core-priv.h | 2 ++ > drivers/media/rc/rc-ir-raw.c | 37 +++++++++++++++++++++++++++++++++++++ > include/media/rc-core.h | 3 +++ > 3 files changed, 42 insertions(+) > >diff --git a/drivers/media/rc/rc-core-priv.h b/drivers/media/rc/rc-core-priv.h >index b68d4f76..122c25f 100644 >--- a/drivers/media/rc/rc-core-priv.h >+++ b/drivers/media/rc/rc-core-priv.h >@@ -25,6 +25,8 @@ struct ir_raw_handler { > > u64 protocols; /* which are handled by this handler */ > int (*decode)(struct rc_dev *dev, struct ir_raw_event event); >+ int (*encode)(u64 protocols, const struct rc_scancode_filter *scancode, >+ struct ir_raw_event *events, unsigned int max); > > /* These two should only be used by the lirc decoder */ > int (*raw_register)(struct rc_dev *dev); >diff --git a/drivers/media/rc/rc-ir-raw.c b/drivers/media/rc/rc-ir-raw.c >index b732ac6..dd47fe5 100644 >--- a/drivers/media/rc/rc-ir-raw.c >+++ b/drivers/media/rc/rc-ir-raw.c >@@ -246,6 +246,43 @@ static int change_protocol(struct rc_dev *dev, u64 *rc_type) > return 0; > } > >+/** >+ * ir_raw_encode_scancode() - Encode a scancode as raw events >+ * >+ * @protocols: permitted protocols >+ * @scancode: scancode filter describing a single scancode >+ * @events: array of raw events to write into >+ * @max: max number of raw events >+ * >+ * Attempts to encode the scancode as raw events. >+ * >+ * Returns: The number of events written. >+ * -ENOBUFS if there isn't enough space in the array to fit the >+ * encoding. In this case all @max events will have been written. >+ * -EINVAL if the scancode is ambiguous or invalid, or if no >+ * compatible encoder was found. >+ */ >+int ir_raw_encode_scancode(u64 protocols, Why a bitmask of protocols and not a single protocol enum? What's the use case for encoding a given scancode according to one out of a number of protocols (and not even knowing which one)?? >+ const struct rc_scancode_filter *scancode, >+ struct ir_raw_event *events, unsigned int max) >+{ >+ struct ir_raw_handler *handler; >+ int ret = -EINVAL; >+ >+ mutex_lock(&ir_raw_handler_lock); >+ list_for_each_entry(handler, &ir_raw_handler_list, list) { >+ if (handler->protocols & protocols && handler->encode) { >+ ret = handler->encode(protocols, scancode, events, max); >+ if (ret >= 0 || ret == -ENOBUFS) >+ break; >+ } >+ } >+ mutex_unlock(&ir_raw_handler_lock); >+ >+ return ret; >+} >+EXPORT_SYMBOL(ir_raw_encode_scancode); >+ > /* > * Used to (un)register raw event clients > */ >diff --git a/include/media/rc-core.h b/include/media/rc-core.h >index 2c7fbca..5703c08 100644 >--- a/include/media/rc-core.h >+++ b/include/media/rc-core.h >@@ -250,6 +250,9 @@ int ir_raw_event_store_edge(struct rc_dev *dev, enum raw_event_type type); > int ir_raw_event_store_with_filter(struct rc_dev *dev, > struct ir_raw_event *ev); > void ir_raw_event_set_idle(struct rc_dev *dev, bool idle); >+int ir_raw_encode_scancode(u64 protocols, >+ const struct rc_scancode_filter *scancode, >+ struct ir_raw_event *events, unsigned int max); > > static inline void ir_raw_event_reset(struct rc_dev *dev) > { >-- >2.0.5 >
On 19 May 2015 at 23:38, David Härdeman <david@hardeman.nu> wrote: > On Tue, Mar 31, 2015 at 08:48:06PM +0300, Antti Seppälä wrote: >>From: James Hogan <james@albanarts.com> >> >>Add a callback to raw ir handlers for encoding and modulating a scancode >>to a set of raw events. This could be used for transmit, or for >>converting a wakeup scancode filter to a form that is more suitable for >>raw hardware wake up filters. >> >>Signed-off-by: James Hogan <james@albanarts.com> >>Signed-off-by: Antti Seppälä <a.seppala@gmail.com> >>Cc: David Härdeman <david@hardeman.nu> >>--- >> >>Notes: >> Changes in v3: >> - Ported to apply against latest media-tree >> >> Changes in v2: >> - 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. >> >> drivers/media/rc/rc-core-priv.h | 2 ++ >> drivers/media/rc/rc-ir-raw.c | 37 +++++++++++++++++++++++++++++++++++++ >> include/media/rc-core.h | 3 +++ >> 3 files changed, 42 insertions(+) >> >>diff --git a/drivers/media/rc/rc-core-priv.h b/drivers/media/rc/rc-core-priv.h >>index b68d4f76..122c25f 100644 >>--- a/drivers/media/rc/rc-core-priv.h >>+++ b/drivers/media/rc/rc-core-priv.h >>@@ -25,6 +25,8 @@ struct ir_raw_handler { >> >> u64 protocols; /* which are handled by this handler */ >> int (*decode)(struct rc_dev *dev, struct ir_raw_event event); >>+ int (*encode)(u64 protocols, const struct rc_scancode_filter *scancode, >>+ struct ir_raw_event *events, unsigned int max); >> >> /* These two should only be used by the lirc decoder */ >> int (*raw_register)(struct rc_dev *dev); >>diff --git a/drivers/media/rc/rc-ir-raw.c b/drivers/media/rc/rc-ir-raw.c >>index b732ac6..dd47fe5 100644 >>--- a/drivers/media/rc/rc-ir-raw.c >>+++ b/drivers/media/rc/rc-ir-raw.c >>@@ -246,6 +246,43 @@ static int change_protocol(struct rc_dev *dev, u64 *rc_type) >> return 0; >> } >> >>+/** >>+ * ir_raw_encode_scancode() - Encode a scancode as raw events >>+ * >>+ * @protocols: permitted protocols >>+ * @scancode: scancode filter describing a single scancode >>+ * @events: array of raw events to write into >>+ * @max: max number of raw events >>+ * >>+ * Attempts to encode the scancode as raw events. >>+ * >>+ * Returns: The number of events written. >>+ * -ENOBUFS if there isn't enough space in the array to fit the >>+ * encoding. In this case all @max events will have been written. >>+ * -EINVAL if the scancode is ambiguous or invalid, or if no >>+ * compatible encoder was found. >>+ */ >>+int ir_raw_encode_scancode(u64 protocols, > > Why a bitmask of protocols and not a single protocol enum? What's the > use case for encoding a given scancode according to one out of a number > of protocols (and not even knowing which one)?? > I think bitmask was used simply for consistency reasons. Most of the rc-core handles protocols in a bitmask (u64 protocols or some variant of it). Especially in the decoders the dev->enabled_protocols is used to mean "decode any of these protocols but I don't care which one" and the encoders were written to follow that logic. From ir driver point of view it was also kind of nice to use the u64 enabled_wakeup_protocols from struct rc_dev which already exists and is manipulated with the same sysfs code as the enabled_protocols bitmask.
On Wed, May 20, 2015 at 07:46:21PM +0300, Antti Seppälä wrote: >On 19 May 2015 at 23:38, David Härdeman <david@hardeman.nu> wrote: >> On Tue, Mar 31, 2015 at 08:48:06PM +0300, Antti Seppälä wrote: >>>From: James Hogan <james@albanarts.com> >>> >>>Add a callback to raw ir handlers for encoding and modulating a scancode >>>to a set of raw events. This could be used for transmit, or for >>>converting a wakeup scancode filter to a form that is more suitable for >>>raw hardware wake up filters. >>> >>>Signed-off-by: James Hogan <james@albanarts.com> >>>Signed-off-by: Antti Seppälä <a.seppala@gmail.com> >>>Cc: David Härdeman <david@hardeman.nu> >>>--- >>> >>>Notes: >>> Changes in v3: >>> - Ported to apply against latest media-tree >>> >>> Changes in v2: >>> - 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. >>> >>> drivers/media/rc/rc-core-priv.h | 2 ++ >>> drivers/media/rc/rc-ir-raw.c | 37 +++++++++++++++++++++++++++++++++++++ >>> include/media/rc-core.h | 3 +++ >>> 3 files changed, 42 insertions(+) >>> >>>diff --git a/drivers/media/rc/rc-core-priv.h b/drivers/media/rc/rc-core-priv.h >>>index b68d4f76..122c25f 100644 >>>--- a/drivers/media/rc/rc-core-priv.h >>>+++ b/drivers/media/rc/rc-core-priv.h >>>@@ -25,6 +25,8 @@ struct ir_raw_handler { >>> >>> u64 protocols; /* which are handled by this handler */ >>> int (*decode)(struct rc_dev *dev, struct ir_raw_event event); >>>+ int (*encode)(u64 protocols, const struct rc_scancode_filter *scancode, >>>+ struct ir_raw_event *events, unsigned int max); >>> >>> /* These two should only be used by the lirc decoder */ >>> int (*raw_register)(struct rc_dev *dev); >>>diff --git a/drivers/media/rc/rc-ir-raw.c b/drivers/media/rc/rc-ir-raw.c >>>index b732ac6..dd47fe5 100644 >>>--- a/drivers/media/rc/rc-ir-raw.c >>>+++ b/drivers/media/rc/rc-ir-raw.c >>>@@ -246,6 +246,43 @@ static int change_protocol(struct rc_dev *dev, u64 *rc_type) >>> return 0; >>> } >>> >>>+/** >>>+ * ir_raw_encode_scancode() - Encode a scancode as raw events >>>+ * >>>+ * @protocols: permitted protocols >>>+ * @scancode: scancode filter describing a single scancode >>>+ * @events: array of raw events to write into >>>+ * @max: max number of raw events >>>+ * >>>+ * Attempts to encode the scancode as raw events. >>>+ * >>>+ * Returns: The number of events written. >>>+ * -ENOBUFS if there isn't enough space in the array to fit the >>>+ * encoding. In this case all @max events will have been written. >>>+ * -EINVAL if the scancode is ambiguous or invalid, or if no >>>+ * compatible encoder was found. >>>+ */ >>>+int ir_raw_encode_scancode(u64 protocols, >> >> Why a bitmask of protocols and not a single protocol enum? What's the >> use case for encoding a given scancode according to one out of a number >> of protocols (and not even knowing which one)?? >> > >I think bitmask was used simply for consistency reasons. Most of the >rc-core handles protocols in a bitmask (u64 protocols or some variant >of it). Yes, all the parts where multiple protocols make sense use a bitmap of protocols. If there's any part which uses a bitmap where only one protocol makes sense that'd be a bug... >Especially in the decoders the dev->enabled_protocols is used >to mean "decode any of these protocols but I don't care which one" and >the encoders were written to follow that logic. The fact that you might want to be able to receive and decode more than one protocol has no bearing on encoding when you're supposed to know what it is you want to encode.... >From ir driver point of view it was also kind of nice to use the u64 >enabled_wakeup_protocols from struct rc_dev which already exists and >is manipulated with the same sysfs code as the enabled_protocols >bitmask. But it makes no sense? "here's a scancode...I have no idea what it means since only knowing the protocol allows you to make any sense of the scancode...but please encode it to something...anything...."
On 20 May 2015 at 21:29, David Härdeman <david@hardeman.nu> wrote: > On Wed, May 20, 2015 at 07:46:21PM +0300, Antti Seppälä wrote: >>On 19 May 2015 at 23:38, David Härdeman <david@hardeman.nu> wrote: >>> On Tue, Mar 31, 2015 at 08:48:06PM +0300, Antti Seppälä wrote: >>>>From: James Hogan <james@albanarts.com> >>>> >>>>Add a callback to raw ir handlers for encoding and modulating a scancode >>>>to a set of raw events. This could be used for transmit, or for >>>>converting a wakeup scancode filter to a form that is more suitable for >>>>raw hardware wake up filters. >>>> >>>>Signed-off-by: James Hogan <james@albanarts.com> >>>>Signed-off-by: Antti Seppälä <a.seppala@gmail.com> >>>>Cc: David Härdeman <david@hardeman.nu> >>>>--- >>>> >>>>Notes: >>>> Changes in v3: >>>> - Ported to apply against latest media-tree >>>> >>>> Changes in v2: >>>> - 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. >>>> >>>> drivers/media/rc/rc-core-priv.h | 2 ++ >>>> drivers/media/rc/rc-ir-raw.c | 37 +++++++++++++++++++++++++++++++++++++ >>>> include/media/rc-core.h | 3 +++ >>>> 3 files changed, 42 insertions(+) >>>> >>>>diff --git a/drivers/media/rc/rc-core-priv.h b/drivers/media/rc/rc-core-priv.h >>>>index b68d4f76..122c25f 100644 >>>>--- a/drivers/media/rc/rc-core-priv.h >>>>+++ b/drivers/media/rc/rc-core-priv.h >>>>@@ -25,6 +25,8 @@ struct ir_raw_handler { >>>> >>>> u64 protocols; /* which are handled by this handler */ >>>> int (*decode)(struct rc_dev *dev, struct ir_raw_event event); >>>>+ int (*encode)(u64 protocols, const struct rc_scancode_filter *scancode, >>>>+ struct ir_raw_event *events, unsigned int max); >>>> >>>> /* These two should only be used by the lirc decoder */ >>>> int (*raw_register)(struct rc_dev *dev); >>>>diff --git a/drivers/media/rc/rc-ir-raw.c b/drivers/media/rc/rc-ir-raw.c >>>>index b732ac6..dd47fe5 100644 >>>>--- a/drivers/media/rc/rc-ir-raw.c >>>>+++ b/drivers/media/rc/rc-ir-raw.c >>>>@@ -246,6 +246,43 @@ static int change_protocol(struct rc_dev *dev, u64 *rc_type) >>>> return 0; >>>> } >>>> >>>>+/** >>>>+ * ir_raw_encode_scancode() - Encode a scancode as raw events >>>>+ * >>>>+ * @protocols: permitted protocols >>>>+ * @scancode: scancode filter describing a single scancode >>>>+ * @events: array of raw events to write into >>>>+ * @max: max number of raw events >>>>+ * >>>>+ * Attempts to encode the scancode as raw events. >>>>+ * >>>>+ * Returns: The number of events written. >>>>+ * -ENOBUFS if there isn't enough space in the array to fit the >>>>+ * encoding. In this case all @max events will have been written. >>>>+ * -EINVAL if the scancode is ambiguous or invalid, or if no >>>>+ * compatible encoder was found. >>>>+ */ >>>>+int ir_raw_encode_scancode(u64 protocols, >>> >>> Why a bitmask of protocols and not a single protocol enum? What's the >>> use case for encoding a given scancode according to one out of a number >>> of protocols (and not even knowing which one)?? >>> >> >>I think bitmask was used simply for consistency reasons. Most of the >>rc-core handles protocols in a bitmask (u64 protocols or some variant >>of it). > > Yes, all the parts where multiple protocols make sense use a bitmap of > protocols. If there's any part which uses a bitmap where only one > protocol makes sense that'd be a bug... > >>Especially in the decoders the dev->enabled_protocols is used >>to mean "decode any of these protocols but I don't care which one" and >>the encoders were written to follow that logic. > > The fact that you might want to be able to receive and decode more than > one protocol has no bearing on encoding when you're supposed to know > what it is you want to encode.... > > >From ir driver point of view it was also kind of nice to use the u64 >>enabled_wakeup_protocols from struct rc_dev which already exists and >>is manipulated with the same sysfs code as the enabled_protocols >>bitmask. > > But it makes no sense? "here's a scancode...I have no idea what it means > since only knowing the protocol allows you to make any sense of the > scancode...but please encode it to something...anything...." > Well it made sense from code simplicity point of view :) But yes current use cases will most likely be valid when encoding only to a single specific protocol at a time. However having a flexible api allows for future expansion though if a new use case is needed to be supported. And like I said earlier using bitmask is also consistent with what rc-core already has. That being said I don't object if you wish to propose a patch to refactor the bitmask to be an enumeration instead.
On Wed, May 20, 2015 at 10:26:54PM +0300, Antti Seppälä wrote: >On 20 May 2015 at 21:29, David Härdeman <david@hardeman.nu> wrote: >> On Wed, May 20, 2015 at 07:46:21PM +0300, Antti Seppälä wrote: >>>On 19 May 2015 at 23:38, David Härdeman <david@hardeman.nu> wrote: >>>> On Tue, Mar 31, 2015 at 08:48:06PM +0300, Antti Seppälä wrote: >>>>>From: James Hogan <james@albanarts.com> >>>>> >>>>>Add a callback to raw ir handlers for encoding and modulating a scancode >>>>>to a set of raw events. This could be used for transmit, or for >>>>>converting a wakeup scancode filter to a form that is more suitable for >>>>>raw hardware wake up filters. >>>>> >>>>>Signed-off-by: James Hogan <james@albanarts.com> >>>>>Signed-off-by: Antti Seppälä <a.seppala@gmail.com> >>>>>Cc: David Härdeman <david@hardeman.nu> >>>>>--- >>>>> >>>>>Notes: >>>>> Changes in v3: >>>>> - Ported to apply against latest media-tree >>>>> >>>>> Changes in v2: >>>>> - 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. >>>>> >>>>> drivers/media/rc/rc-core-priv.h | 2 ++ >>>>> drivers/media/rc/rc-ir-raw.c | 37 +++++++++++++++++++++++++++++++++++++ >>>>> include/media/rc-core.h | 3 +++ >>>>> 3 files changed, 42 insertions(+) >>>>> >>>>>diff --git a/drivers/media/rc/rc-core-priv.h b/drivers/media/rc/rc-core-priv.h >>>>>index b68d4f76..122c25f 100644 >>>>>--- a/drivers/media/rc/rc-core-priv.h >>>>>+++ b/drivers/media/rc/rc-core-priv.h >>>>>@@ -25,6 +25,8 @@ struct ir_raw_handler { >>>>> >>>>> u64 protocols; /* which are handled by this handler */ >>>>> int (*decode)(struct rc_dev *dev, struct ir_raw_event event); >>>>>+ int (*encode)(u64 protocols, const struct rc_scancode_filter *scancode, >>>>>+ struct ir_raw_event *events, unsigned int max); >>>>> >>>>> /* These two should only be used by the lirc decoder */ >>>>> int (*raw_register)(struct rc_dev *dev); >>>>>diff --git a/drivers/media/rc/rc-ir-raw.c b/drivers/media/rc/rc-ir-raw.c >>>>>index b732ac6..dd47fe5 100644 >>>>>--- a/drivers/media/rc/rc-ir-raw.c >>>>>+++ b/drivers/media/rc/rc-ir-raw.c >>>>>@@ -246,6 +246,43 @@ static int change_protocol(struct rc_dev *dev, u64 *rc_type) >>>>> return 0; >>>>> } >>>>> >>>>>+/** >>>>>+ * ir_raw_encode_scancode() - Encode a scancode as raw events >>>>>+ * >>>>>+ * @protocols: permitted protocols >>>>>+ * @scancode: scancode filter describing a single scancode >>>>>+ * @events: array of raw events to write into >>>>>+ * @max: max number of raw events >>>>>+ * >>>>>+ * Attempts to encode the scancode as raw events. >>>>>+ * >>>>>+ * Returns: The number of events written. >>>>>+ * -ENOBUFS if there isn't enough space in the array to fit the >>>>>+ * encoding. In this case all @max events will have been written. >>>>>+ * -EINVAL if the scancode is ambiguous or invalid, or if no >>>>>+ * compatible encoder was found. >>>>>+ */ >>>>>+int ir_raw_encode_scancode(u64 protocols, >>>> >>>> Why a bitmask of protocols and not a single protocol enum? What's the >>>> use case for encoding a given scancode according to one out of a number >>>> of protocols (and not even knowing which one)?? >>>> >>> >>>I think bitmask was used simply for consistency reasons. Most of the >>>rc-core handles protocols in a bitmask (u64 protocols or some variant >>>of it). >> >> Yes, all the parts where multiple protocols make sense use a bitmap of >> protocols. If there's any part which uses a bitmap where only one >> protocol makes sense that'd be a bug... >> >>>Especially in the decoders the dev->enabled_protocols is used >>>to mean "decode any of these protocols but I don't care which one" and >>>the encoders were written to follow that logic. >> >> The fact that you might want to be able to receive and decode more than >> one protocol has no bearing on encoding when you're supposed to know >> what it is you want to encode.... >> >> >From ir driver point of view it was also kind of nice to use the u64 >>>enabled_wakeup_protocols from struct rc_dev which already exists and >>>is manipulated with the same sysfs code as the enabled_protocols >>>bitmask. >> >> But it makes no sense? "here's a scancode...I have no idea what it means >> since only knowing the protocol allows you to make any sense of the >> scancode...but please encode it to something...anything...." >> > >Well it made sense from code simplicity point of view :) > >But yes current use cases will most likely be valid when encoding only >to a single specific protocol at a time. However having a flexible api >allows for future expansion though if a new use case is needed to be >supported. And like I said earlier using bitmask is also consistent >with what rc-core already has. * drivers/media/rc/img-ir/img-ir-hw.c only seems to support one protocol at a time * drivers/media/rc/rc-loopback.c doesn't care * drivers/media/rc/winbond-cir.c seems hardware-wise very similar to nuvoton-cir.c, was not converted to use the in-kernel encoders...has a private implementation * drivers/media/rc/nuvoton-cir.c is actually protocol agnostic but with your code it will encode according to a randomly chosen protocol among the enabled ones, one that will change depending on *module load order*...so unless I'm mistaken you'll actually get different pulse/space timings written to hardware depending on the order in which certain kernel modules have been loaded... Do you see why this is a bad idea? >That being said I don't object if you wish to propose a patch to >refactor the bitmask to be an enumeration instead. Ehrm...I propose the patches be reverted until they're fixed.
On 20 May 2015 at 23:45, David Härdeman <david@hardeman.nu> wrote: > On Wed, May 20, 2015 at 10:26:54PM +0300, Antti Seppälä wrote: >>On 20 May 2015 at 21:29, David Härdeman <david@hardeman.nu> wrote: >>> On Wed, May 20, 2015 at 07:46:21PM +0300, Antti Seppälä wrote: >>>>On 19 May 2015 at 23:38, David Härdeman <david@hardeman.nu> wrote: >>>>> On Tue, Mar 31, 2015 at 08:48:06PM +0300, Antti Seppälä wrote: >>>>>>From: James Hogan <james@albanarts.com> >>>>>> >>>>>>Add a callback to raw ir handlers for encoding and modulating a scancode >>>>>>to a set of raw events. This could be used for transmit, or for >>>>>>converting a wakeup scancode filter to a form that is more suitable for >>>>>>raw hardware wake up filters. >>>>>> >>>>>>Signed-off-by: James Hogan <james@albanarts.com> >>>>>>Signed-off-by: Antti Seppälä <a.seppala@gmail.com> >>>>>>Cc: David Härdeman <david@hardeman.nu> >>>>>>--- >>>>>> >>>>>>Notes: >>>>>> Changes in v3: >>>>>> - Ported to apply against latest media-tree >>>>>> >>>>>> Changes in v2: >>>>>> - 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. >>>>>> >>>>>> drivers/media/rc/rc-core-priv.h | 2 ++ >>>>>> drivers/media/rc/rc-ir-raw.c | 37 +++++++++++++++++++++++++++++++++++++ >>>>>> include/media/rc-core.h | 3 +++ >>>>>> 3 files changed, 42 insertions(+) >>>>>> >>>>>>diff --git a/drivers/media/rc/rc-core-priv.h b/drivers/media/rc/rc-core-priv.h >>>>>>index b68d4f76..122c25f 100644 >>>>>>--- a/drivers/media/rc/rc-core-priv.h >>>>>>+++ b/drivers/media/rc/rc-core-priv.h >>>>>>@@ -25,6 +25,8 @@ struct ir_raw_handler { >>>>>> >>>>>> u64 protocols; /* which are handled by this handler */ >>>>>> int (*decode)(struct rc_dev *dev, struct ir_raw_event event); >>>>>>+ int (*encode)(u64 protocols, const struct rc_scancode_filter *scancode, >>>>>>+ struct ir_raw_event *events, unsigned int max); >>>>>> >>>>>> /* These two should only be used by the lirc decoder */ >>>>>> int (*raw_register)(struct rc_dev *dev); >>>>>>diff --git a/drivers/media/rc/rc-ir-raw.c b/drivers/media/rc/rc-ir-raw.c >>>>>>index b732ac6..dd47fe5 100644 >>>>>>--- a/drivers/media/rc/rc-ir-raw.c >>>>>>+++ b/drivers/media/rc/rc-ir-raw.c >>>>>>@@ -246,6 +246,43 @@ static int change_protocol(struct rc_dev *dev, u64 *rc_type) >>>>>> return 0; >>>>>> } >>>>>> >>>>>>+/** >>>>>>+ * ir_raw_encode_scancode() - Encode a scancode as raw events >>>>>>+ * >>>>>>+ * @protocols: permitted protocols >>>>>>+ * @scancode: scancode filter describing a single scancode >>>>>>+ * @events: array of raw events to write into >>>>>>+ * @max: max number of raw events >>>>>>+ * >>>>>>+ * Attempts to encode the scancode as raw events. >>>>>>+ * >>>>>>+ * Returns: The number of events written. >>>>>>+ * -ENOBUFS if there isn't enough space in the array to fit the >>>>>>+ * encoding. In this case all @max events will have been written. >>>>>>+ * -EINVAL if the scancode is ambiguous or invalid, or if no >>>>>>+ * compatible encoder was found. >>>>>>+ */ >>>>>>+int ir_raw_encode_scancode(u64 protocols, >>>>> >>>>> Why a bitmask of protocols and not a single protocol enum? What's the >>>>> use case for encoding a given scancode according to one out of a number >>>>> of protocols (and not even knowing which one)?? >>>>> >>>> >>>>I think bitmask was used simply for consistency reasons. Most of the >>>>rc-core handles protocols in a bitmask (u64 protocols or some variant >>>>of it). >>> >>> Yes, all the parts where multiple protocols make sense use a bitmap of >>> protocols. If there's any part which uses a bitmap where only one >>> protocol makes sense that'd be a bug... >>> >>>>Especially in the decoders the dev->enabled_protocols is used >>>>to mean "decode any of these protocols but I don't care which one" and >>>>the encoders were written to follow that logic. >>> >>> The fact that you might want to be able to receive and decode more than >>> one protocol has no bearing on encoding when you're supposed to know >>> what it is you want to encode.... >>> >>> >From ir driver point of view it was also kind of nice to use the u64 >>>>enabled_wakeup_protocols from struct rc_dev which already exists and >>>>is manipulated with the same sysfs code as the enabled_protocols >>>>bitmask. >>> >>> But it makes no sense? "here's a scancode...I have no idea what it means >>> since only knowing the protocol allows you to make any sense of the >>> scancode...but please encode it to something...anything...." >>> >> >>Well it made sense from code simplicity point of view :) >> >>But yes current use cases will most likely be valid when encoding only >>to a single specific protocol at a time. However having a flexible api >>allows for future expansion though if a new use case is needed to be >>supported. And like I said earlier using bitmask is also consistent >>with what rc-core already has. > > * drivers/media/rc/img-ir/img-ir-hw.c > only seems to support one protocol at a time > > * drivers/media/rc/rc-loopback.c > doesn't care > > * drivers/media/rc/winbond-cir.c > seems hardware-wise very similar to nuvoton-cir.c, was not converted to > use the in-kernel encoders...has a private implementation > > * drivers/media/rc/nuvoton-cir.c > is actually protocol agnostic but with your code it will encode > according to a randomly chosen protocol among the enabled ones, one that > will change depending on *module load order*...so unless I'm mistaken > you'll actually get different pulse/space timings written to hardware > depending on the order in which certain kernel modules have been > loaded... > > Do you see why this is a bad idea? > I see that this is not optimal to leave the proper usage up to the user but in a way we must work with what we have. See below: >>That being said I don't object if you wish to propose a patch to >>refactor the bitmask to be an enumeration instead. > > Ehrm...I propose the patches be reverted until they're fixed. > Reverting the patch series will not fix what I think you consider broken. The series has very little to do with the sysfs api for wakeup protocols that is already in place and which makes drivers (or encoders) behave in a certain way. I'm talking about the /sys/class/rc/<rc-device>/wakeup_protocols file that was included around kernel 3.15 and is being already used by at least img-ir driver regardless of whether support for encoding ir scancodes is included or not. The wakeup_protocols allows user to select multiple protocols because it works in a similar fashion to protocols file. I think that you would like to introduce a change to the behavior of this file to only allow selecting single protocol at a time and error-out if multiple protocols are enabled but this would break an existing api to userspace which we are really not allowed to do. And unless I'm mistaken even if we decided to change the behavior of the wakeup_protcols file that change would have nothing to do with this ir-encoding patch series (besides allowing usage of enumerations instead of bitmasks).
On 2015-05-21 09:53, Antti Seppälä wrote: > On 20 May 2015 at 23:45, David Härdeman <david@hardeman.nu> wrote: >> On Wed, May 20, 2015 at 10:26:54PM +0300, Antti Seppälä wrote: >>> On 20 May 2015 at 21:29, David Härdeman <david@hardeman.nu> wrote: >>>> On Wed, May 20, 2015 at 07:46:21PM +0300, Antti Seppälä wrote: >>>>> On 19 May 2015 at 23:38, David Härdeman <david@hardeman.nu> wrote: >>>>>> On Tue, Mar 31, 2015 at 08:48:06PM +0300, Antti Seppälä wrote: >>>>>>> From: James Hogan <james@albanarts.com> >>>>>>> >>>>>>> Add a callback to raw ir handlers for encoding and modulating a >>>>>>> scancode >>>>>>> to a set of raw events. This could be used for transmit, or for >>>>>>> converting a wakeup scancode filter to a form that is more >>>>>>> suitable for >>>>>>> raw hardware wake up filters. >>>>>>> >>>>>>> Signed-off-by: James Hogan <james@albanarts.com> >>>>>>> Signed-off-by: Antti Seppälä <a.seppala@gmail.com> >>>>>>> Cc: David Härdeman <david@hardeman.nu> >>>>>>> --- >>>>>>> >>>>>>> Notes: >>>>>>> Changes in v3: >>>>>>> - Ported to apply against latest media-tree >>>>>>> >>>>>>> Changes in v2: >>>>>>> - 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. >>>>>>> >>>>>>> drivers/media/rc/rc-core-priv.h | 2 ++ >>>>>>> drivers/media/rc/rc-ir-raw.c | 37 >>>>>>> +++++++++++++++++++++++++++++++++++++ >>>>>>> include/media/rc-core.h | 3 +++ >>>>>>> 3 files changed, 42 insertions(+) >>>>>>> >>>>>>> diff --git a/drivers/media/rc/rc-core-priv.h >>>>>>> b/drivers/media/rc/rc-core-priv.h >>>>>>> index b68d4f76..122c25f 100644 >>>>>>> --- a/drivers/media/rc/rc-core-priv.h >>>>>>> +++ b/drivers/media/rc/rc-core-priv.h >>>>>>> @@ -25,6 +25,8 @@ struct ir_raw_handler { >>>>>>> >>>>>>> u64 protocols; /* which are handled by this handler */ >>>>>>> int (*decode)(struct rc_dev *dev, struct ir_raw_event >>>>>>> event); >>>>>>> + int (*encode)(u64 protocols, const struct >>>>>>> rc_scancode_filter *scancode, >>>>>>> + struct ir_raw_event *events, unsigned int >>>>>>> max); >>>>>>> >>>>>>> /* These two should only be used by the lirc decoder */ >>>>>>> int (*raw_register)(struct rc_dev *dev); >>>>>>> diff --git a/drivers/media/rc/rc-ir-raw.c >>>>>>> b/drivers/media/rc/rc-ir-raw.c >>>>>>> index b732ac6..dd47fe5 100644 >>>>>>> --- a/drivers/media/rc/rc-ir-raw.c >>>>>>> +++ b/drivers/media/rc/rc-ir-raw.c >>>>>>> @@ -246,6 +246,43 @@ static int change_protocol(struct rc_dev >>>>>>> *dev, u64 *rc_type) >>>>>>> return 0; >>>>>>> } >>>>>>> >>>>>>> +/** >>>>>>> + * ir_raw_encode_scancode() - Encode a scancode as raw events >>>>>>> + * >>>>>>> + * @protocols: permitted protocols >>>>>>> + * @scancode: scancode filter describing a single >>>>>>> scancode >>>>>>> + * @events: array of raw events to write into >>>>>>> + * @max: max number of raw events >>>>>>> + * >>>>>>> + * Attempts to encode the scancode as raw events. >>>>>>> + * >>>>>>> + * Returns: The number of events written. >>>>>>> + * -ENOBUFS if there isn't enough space in the array >>>>>>> to fit the >>>>>>> + * encoding. In this case all @max events will have >>>>>>> been written. >>>>>>> + * -EINVAL if the scancode is ambiguous or invalid, >>>>>>> or if no >>>>>>> + * compatible encoder was found. >>>>>>> + */ >>>>>>> +int ir_raw_encode_scancode(u64 protocols, >>>>>> >>>>>> Why a bitmask of protocols and not a single protocol enum? What's >>>>>> the >>>>>> use case for encoding a given scancode according to one out of a >>>>>> number >>>>>> of protocols (and not even knowing which one)?? >>>>>> >>>>> >>>>> I think bitmask was used simply for consistency reasons. Most of >>>>> the >>>>> rc-core handles protocols in a bitmask (u64 protocols or some >>>>> variant >>>>> of it). >>>> >>>> Yes, all the parts where multiple protocols make sense use a bitmap >>>> of >>>> protocols. If there's any part which uses a bitmap where only one >>>> protocol makes sense that'd be a bug... >>>> >>>>> Especially in the decoders the dev->enabled_protocols is used >>>>> to mean "decode any of these protocols but I don't care which one" >>>>> and >>>>> the encoders were written to follow that logic. >>>> >>>> The fact that you might want to be able to receive and decode more >>>> than >>>> one protocol has no bearing on encoding when you're supposed to know >>>> what it is you want to encode.... >>>> >>>> >From ir driver point of view it was also kind of nice to use the u64 >>>>> enabled_wakeup_protocols from struct rc_dev which already exists >>>>> and >>>>> is manipulated with the same sysfs code as the enabled_protocols >>>>> bitmask. >>>> >>>> But it makes no sense? "here's a scancode...I have no idea what it >>>> means >>>> since only knowing the protocol allows you to make any sense of the >>>> scancode...but please encode it to something...anything...." >>>> >>> >>> Well it made sense from code simplicity point of view :) >>> >>> But yes current use cases will most likely be valid when encoding >>> only >>> to a single specific protocol at a time. However having a flexible >>> api >>> allows for future expansion though if a new use case is needed to be >>> supported. And like I said earlier using bitmask is also consistent >>> with what rc-core already has. >> >> * drivers/media/rc/img-ir/img-ir-hw.c >> only seems to support one protocol at a time >> >> * drivers/media/rc/rc-loopback.c >> doesn't care >> >> * drivers/media/rc/winbond-cir.c >> seems hardware-wise very similar to nuvoton-cir.c, was not converted >> to >> use the in-kernel encoders...has a private implementation >> >> * drivers/media/rc/nuvoton-cir.c >> is actually protocol agnostic but with your code it will encode >> according to a randomly chosen protocol among the enabled ones, one >> that >> will change depending on *module load order*...so unless I'm mistaken >> you'll actually get different pulse/space timings written to hardware >> depending on the order in which certain kernel modules have been >> loaded... >> >> Do you see why this is a bad idea? >> > > I see that this is not optimal to leave the proper usage up to the > user but in a way we must work with what we have. See below: > >>> That being said I don't object if you wish to propose a patch to >>> refactor the bitmask to be an enumeration instead. >> >> Ehrm...I propose the patches be reverted until they're fixed. >> > > Reverting the patch series will not fix what I think you consider > broken. The series has very little to do with the sysfs api for wakeup > protocols that is already in place and which makes drivers (or > encoders) behave in a certain way. > > I'm talking about the /sys/class/rc/<rc-device>/wakeup_protocols file > that was included around kernel 3.15 and is being already used by at > least img-ir driver regardless of whether support for encoding ir > scancodes is included or not. The wakeup_protocols allows user to > select multiple protocols because it works in a similar fashion to > protocols file. > > I think that you would like to introduce a change to the behavior of > this file to only allow selecting single protocol at a time and > error-out if multiple protocols are enabled but this would break an > existing api to userspace which we are really not allowed to do. > > And unless I'm mistaken even if we decided to change the behavior of > the wakeup_protcols file that change would have nothing to do with > this ir-encoding patch series (besides allowing usage of enumerations > instead of bitmasks). I'm talking about ir_raw_encode_scancode() which is entirely broken in its current state. It will, given more than one enabled protocol, encode a scancode to pulse/space events according to the rules of a randomly chosen protocol. That random selection will be influenced by things like *module load order* (independent of the separate fact that passing multiple protocols to it is completely bogus in the first place). To be clear: the same scancode may be encoded differently depending on if you've load the nec decoder before or after the rc5 decoder! That kind of behavior can't go into a release kernel (Mauro...). Yes, the sysfs interface for selecting the wakeup protocol(s) is also not great and might need to be changed or augmented, but that's a different issue. (Resent, forgot to do reply-all). -- 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 21 May 2015 at 12:14, David Härdeman <david@hardeman.nu> wrote: > On 2015-05-21 09:53, Antti Seppälä wrote: >> >> On 20 May 2015 at 23:45, David Härdeman <david@hardeman.nu> wrote: >>> >>> On Wed, May 20, 2015 at 10:26:54PM +0300, Antti Seppälä wrote: >>>> >>>> On 20 May 2015 at 21:29, David Härdeman <david@hardeman.nu> wrote: >>>>> >>>>> On Wed, May 20, 2015 at 07:46:21PM +0300, Antti Seppälä wrote: >>>>>> >>>>>> On 19 May 2015 at 23:38, David Härdeman <david@hardeman.nu> wrote: >>>>>>> >>>>>>> On Tue, Mar 31, 2015 at 08:48:06PM +0300, Antti Seppälä wrote: >>>>>>>> >>>>>>>> From: James Hogan <james@albanarts.com> >>>>>>>> >>>>>>>> Add a callback to raw ir handlers for encoding and modulating a >>>>>>>> scancode >>>>>>>> to a set of raw events. This could be used for transmit, or for >>>>>>>> converting a wakeup scancode filter to a form that is more suitable >>>>>>>> for >>>>>>>> raw hardware wake up filters. >>>>>>>> >>>>>>>> Signed-off-by: James Hogan <james@albanarts.com> >>>>>>>> Signed-off-by: Antti Seppälä <a.seppala@gmail.com> >>>>>>>> Cc: David Härdeman <david@hardeman.nu> >>>>>>>> --- >>>>>>>> >>>>>>>> Notes: >>>>>>>> Changes in v3: >>>>>>>> - Ported to apply against latest media-tree >>>>>>>> >>>>>>>> Changes in v2: >>>>>>>> - 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. >>>>>>>> >>>>>>>> drivers/media/rc/rc-core-priv.h | 2 ++ >>>>>>>> drivers/media/rc/rc-ir-raw.c | 37 >>>>>>>> +++++++++++++++++++++++++++++++++++++ >>>>>>>> include/media/rc-core.h | 3 +++ >>>>>>>> 3 files changed, 42 insertions(+) >>>>>>>> >>>>>>>> diff --git a/drivers/media/rc/rc-core-priv.h >>>>>>>> b/drivers/media/rc/rc-core-priv.h >>>>>>>> index b68d4f76..122c25f 100644 >>>>>>>> --- a/drivers/media/rc/rc-core-priv.h >>>>>>>> +++ b/drivers/media/rc/rc-core-priv.h >>>>>>>> @@ -25,6 +25,8 @@ struct ir_raw_handler { >>>>>>>> >>>>>>>> u64 protocols; /* which are handled by this handler */ >>>>>>>> int (*decode)(struct rc_dev *dev, struct ir_raw_event event); >>>>>>>> + int (*encode)(u64 protocols, const struct rc_scancode_filter >>>>>>>> *scancode, >>>>>>>> + struct ir_raw_event *events, unsigned int max); >>>>>>>> >>>>>>>> /* These two should only be used by the lirc decoder */ >>>>>>>> int (*raw_register)(struct rc_dev *dev); >>>>>>>> diff --git a/drivers/media/rc/rc-ir-raw.c >>>>>>>> b/drivers/media/rc/rc-ir-raw.c >>>>>>>> index b732ac6..dd47fe5 100644 >>>>>>>> --- a/drivers/media/rc/rc-ir-raw.c >>>>>>>> +++ b/drivers/media/rc/rc-ir-raw.c >>>>>>>> @@ -246,6 +246,43 @@ static int change_protocol(struct rc_dev *dev, >>>>>>>> u64 *rc_type) >>>>>>>> return 0; >>>>>>>> } >>>>>>>> >>>>>>>> +/** >>>>>>>> + * ir_raw_encode_scancode() - Encode a scancode as raw events >>>>>>>> + * >>>>>>>> + * @protocols: permitted protocols >>>>>>>> + * @scancode: scancode filter describing a single scancode >>>>>>>> + * @events: array of raw events to write into >>>>>>>> + * @max: max number of raw events >>>>>>>> + * >>>>>>>> + * Attempts to encode the scancode as raw events. >>>>>>>> + * >>>>>>>> + * Returns: The number of events written. >>>>>>>> + * -ENOBUFS if there isn't enough space in the array to >>>>>>>> fit the >>>>>>>> + * encoding. In this case all @max events will have been >>>>>>>> written. >>>>>>>> + * -EINVAL if the scancode is ambiguous or invalid, or >>>>>>>> if no >>>>>>>> + * compatible encoder was found. >>>>>>>> + */ >>>>>>>> +int ir_raw_encode_scancode(u64 protocols, >>>>>>> >>>>>>> >>>>>>> Why a bitmask of protocols and not a single protocol enum? What's the >>>>>>> use case for encoding a given scancode according to one out of a >>>>>>> number >>>>>>> of protocols (and not even knowing which one)?? >>>>>>> >>>>>> >>>>>> I think bitmask was used simply for consistency reasons. Most of the >>>>>> rc-core handles protocols in a bitmask (u64 protocols or some variant >>>>>> of it). >>>>> >>>>> >>>>> Yes, all the parts where multiple protocols make sense use a bitmap of >>>>> protocols. If there's any part which uses a bitmap where only one >>>>> protocol makes sense that'd be a bug... >>>>> >>>>>> Especially in the decoders the dev->enabled_protocols is used >>>>>> to mean "decode any of these protocols but I don't care which one" and >>>>>> the encoders were written to follow that logic. >>>>> >>>>> >>>>> The fact that you might want to be able to receive and decode more than >>>>> one protocol has no bearing on encoding when you're supposed to know >>>>> what it is you want to encode.... >>>>> >>>>> >From ir driver point of view it was also kind of nice to use the u64 >>>>>> >>>>>> enabled_wakeup_protocols from struct rc_dev which already exists and >>>>>> is manipulated with the same sysfs code as the enabled_protocols >>>>>> bitmask. >>>>> >>>>> >>>>> But it makes no sense? "here's a scancode...I have no idea what it >>>>> means >>>>> since only knowing the protocol allows you to make any sense of the >>>>> scancode...but please encode it to something...anything...." >>>>> >>>> >>>> Well it made sense from code simplicity point of view :) >>>> >>>> But yes current use cases will most likely be valid when encoding only >>>> to a single specific protocol at a time. However having a flexible api >>>> allows for future expansion though if a new use case is needed to be >>>> supported. And like I said earlier using bitmask is also consistent >>>> with what rc-core already has. >>> >>> >>> * drivers/media/rc/img-ir/img-ir-hw.c >>> only seems to support one protocol at a time >>> >>> * drivers/media/rc/rc-loopback.c >>> doesn't care >>> >>> * drivers/media/rc/winbond-cir.c >>> seems hardware-wise very similar to nuvoton-cir.c, was not converted to >>> use the in-kernel encoders...has a private implementation >>> >>> * drivers/media/rc/nuvoton-cir.c >>> is actually protocol agnostic but with your code it will encode >>> according to a randomly chosen protocol among the enabled ones, one that >>> will change depending on *module load order*...so unless I'm mistaken >>> you'll actually get different pulse/space timings written to hardware >>> depending on the order in which certain kernel modules have been >>> loaded... >>> >>> Do you see why this is a bad idea? >>> >> >> I see that this is not optimal to leave the proper usage up to the >> user but in a way we must work with what we have. See below: >> >>>> That being said I don't object if you wish to propose a patch to >>>> refactor the bitmask to be an enumeration instead. >>> >>> >>> Ehrm...I propose the patches be reverted until they're fixed. >>> >> >> Reverting the patch series will not fix what I think you consider >> broken. The series has very little to do with the sysfs api for wakeup >> protocols that is already in place and which makes drivers (or >> encoders) behave in a certain way. >> >> I'm talking about the /sys/class/rc/<rc-device>/wakeup_protocols file >> that was included around kernel 3.15 and is being already used by at >> least img-ir driver regardless of whether support for encoding ir >> scancodes is included or not. The wakeup_protocols allows user to >> select multiple protocols because it works in a similar fashion to >> protocols file. >> >> I think that you would like to introduce a change to the behavior of >> this file to only allow selecting single protocol at a time and >> error-out if multiple protocols are enabled but this would break an >> existing api to userspace which we are really not allowed to do. >> >> And unless I'm mistaken even if we decided to change the behavior of >> the wakeup_protcols file that change would have nothing to do with >> this ir-encoding patch series (besides allowing usage of enumerations >> instead of bitmasks). > > > I'm talking about ir_raw_encode_scancode() which is entirely broken in its > current state. It will, given more than one enabled protocol, encode a > scancode to pulse/space events according to the rules of a randomly chosen > protocol. That random selection will be influenced by things like *module > load order* (independent of the separate fact that passing multiple > protocols to it is completely bogus in the first place). > > To be clear: the same scancode may be encoded differently depending on if > you've load the nec decoder before or after the rc5 decoder! That kind of > behavior can't go into a release kernel (Mauro...). > So... if the ir_raw_handler_list is sorted to eliminate the randomness caused by module load ordering you will be happy (or happier)? That is something that could be useful even for the ir-decoding functionality and might be worth a separate patch.
On 2015-05-21 13:51, Antti Seppälä wrote: > On 21 May 2015 at 12:14, David Härdeman <david@hardeman.nu> wrote: >> I'm talking about ir_raw_encode_scancode() which is entirely broken in >> its >> current state. It will, given more than one enabled protocol, encode a >> scancode to pulse/space events according to the rules of a randomly >> chosen >> protocol. That random selection will be influenced by things like >> *module >> load order* (independent of the separate fact that passing multiple >> protocols to it is completely bogus in the first place). >> >> To be clear: the same scancode may be encoded differently depending on >> if >> you've load the nec decoder before or after the rc5 decoder! That kind >> of >> behavior can't go into a release kernel (Mauro...). >> > > So... if the ir_raw_handler_list is sorted to eliminate the randomness > caused by module load ordering you will be happy (or happier)? No, cause it's a horrible hack. And the caller of ir_raw_handler_list() still has no idea of knowing (given more than one protocol) which protocol a given scancode will be encoded according to. > That is something that could be useful even for the ir-decoding > functionality and might be worth a separate patch. Useful how? -- 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 21 May 2015 at 15:30, David Härdeman <david@hardeman.nu> wrote: > On 2015-05-21 13:51, Antti Seppälä wrote: >> >> On 21 May 2015 at 12:14, David Härdeman <david@hardeman.nu> wrote: >>> >>> I'm talking about ir_raw_encode_scancode() which is entirely broken in >>> its >>> current state. It will, given more than one enabled protocol, encode a >>> scancode to pulse/space events according to the rules of a randomly >>> chosen >>> protocol. That random selection will be influenced by things like *module >>> load order* (independent of the separate fact that passing multiple >>> protocols to it is completely bogus in the first place). >>> >>> To be clear: the same scancode may be encoded differently depending on if >>> you've load the nec decoder before or after the rc5 decoder! That kind of >>> behavior can't go into a release kernel (Mauro...). >>> >> >> So... if the ir_raw_handler_list is sorted to eliminate the randomness >> caused by module load ordering you will be happy (or happier)? > > > No, cause it's a horrible hack. And the caller of ir_raw_handler_list() > still has no idea of knowing (given more than one protocol) which protocol a > given scancode will be encoded according to. > Okay, so how about "demuxing" the first protocol before handing them to encoder callback? Simply something like below: - if (handler->protocols & protocols && handler->encode) { + if (handler->protocols & ffs(protocols) && handler->encode) { Now the behavior is well-defined even when multiple protocols are selected. >> That is something that could be useful even for the ir-decoding >> functionality and might be worth a separate patch. > > > Useful how? Keeping dynamically filled lists sorted is a good practice if one wishes to achieve determinism in behavior (like running decoders always in certain order too).
On Thu, May 21, 2015 at 05:22:08PM +0300, Antti Seppälä wrote: >On 21 May 2015 at 15:30, David Härdeman <david@hardeman.nu> wrote: >> On 2015-05-21 13:51, Antti Seppälä wrote: >>> >>> On 21 May 2015 at 12:14, David Härdeman <david@hardeman.nu> wrote: >>>> >>>> I'm talking about ir_raw_encode_scancode() which is entirely broken in >>>> its >>>> current state. It will, given more than one enabled protocol, encode a >>>> scancode to pulse/space events according to the rules of a randomly >>>> chosen >>>> protocol. That random selection will be influenced by things like *module >>>> load order* (independent of the separate fact that passing multiple >>>> protocols to it is completely bogus in the first place). >>>> >>>> To be clear: the same scancode may be encoded differently depending on if >>>> you've load the nec decoder before or after the rc5 decoder! That kind of >>>> behavior can't go into a release kernel (Mauro...). >>>> >>> >>> So... if the ir_raw_handler_list is sorted to eliminate the randomness >>> caused by module load ordering you will be happy (or happier)? >> >> >> No, cause it's a horrible hack. And the caller of ir_raw_handler_list() >> still has no idea of knowing (given more than one protocol) which protocol a >> given scancode will be encoded according to. >> > >Okay, so how about "demuxing" the first protocol before handing them >to encoder callback? Simply something like below: > >- if (handler->protocols & protocols && handler->encode) { >+ if (handler->protocols & ffs(protocols) && handler->encode) { > >Now the behavior is well-defined even when multiple protocols are selected. Your patchset introduced ir_raw_encode_scancode() as well as the only two callers of that function: drivers/media/rc/nuvoton-cir.c drivers/media/rc/rc-loopback.c I realize that the sysfs wakeup_protocols file (which bakes several protocols into one label) makes defining *the* protocol difficult, but if you're going to add hacks like this, keep them to the sole driver using the API rather than the core API itself. That is, change nvt_ir_raw_change_wakeup_protocol() so that it picks a single protocol, no matter how many are passed to it (the img-ir driver already sets a precedent here so it wouldn't be an API change to change to a set of protocols which might be different than what the user suggested). (Also...yes, that'll make supporting several versions of e.g. RC6 impossible with the current sysfs code). Then change both ir_raw_encode_scancode() to take a single protocol enum and change the *encode function pointer in struct ir_raw_handler to also take a single protocol enum. That way encoders won't have to guess (using scanmasks...!?) what protocol they should encode to. And Mauro...I strongly suggest you revert all of this encoding stuff until this has been fixed...it's broken. >> >> Useful how? > >Keeping dynamically filled lists sorted is a good practice if one >wishes to achieve determinism in behavior (like running decoders >always in certain order too). That's a new "good practice" to me...
On 21 May 2015 at 22:40, David Härdeman <david@hardeman.nu> wrote: > On Thu, May 21, 2015 at 05:22:08PM +0300, Antti Seppälä wrote: >>On 21 May 2015 at 15:30, David Härdeman <david@hardeman.nu> wrote: >>> On 2015-05-21 13:51, Antti Seppälä wrote: >>>> >>>> On 21 May 2015 at 12:14, David Härdeman <david@hardeman.nu> wrote: >>>>> >>>>> I'm talking about ir_raw_encode_scancode() which is entirely broken in >>>>> its >>>>> current state. It will, given more than one enabled protocol, encode a >>>>> scancode to pulse/space events according to the rules of a randomly >>>>> chosen >>>>> protocol. That random selection will be influenced by things like *module >>>>> load order* (independent of the separate fact that passing multiple >>>>> protocols to it is completely bogus in the first place). >>>>> >>>>> To be clear: the same scancode may be encoded differently depending on if >>>>> you've load the nec decoder before or after the rc5 decoder! That kind of >>>>> behavior can't go into a release kernel (Mauro...). >>>>> >>>> >>>> So... if the ir_raw_handler_list is sorted to eliminate the randomness >>>> caused by module load ordering you will be happy (or happier)? >>> >>> >>> No, cause it's a horrible hack. And the caller of ir_raw_handler_list() >>> still has no idea of knowing (given more than one protocol) which protocol a >>> given scancode will be encoded according to. >>> >> >>Okay, so how about "demuxing" the first protocol before handing them >>to encoder callback? Simply something like below: >> >>- if (handler->protocols & protocols && handler->encode) { >>+ if (handler->protocols & ffs(protocols) && handler->encode) { >> >>Now the behavior is well-defined even when multiple protocols are selected. > > Your patchset introduced ir_raw_encode_scancode() as well as the only > two callers of that function: > > drivers/media/rc/nuvoton-cir.c > drivers/media/rc/rc-loopback.c > > I realize that the sysfs wakeup_protocols file (which bakes several > protocols into one label) makes defining *the* protocol difficult, but > if you're going to add hacks like this, keep them to the sole driver > using the API rather than the core API itself. > > That is, change nvt_ir_raw_change_wakeup_protocol() so that it picks a > single protocol, no matter how many are passed to it (the img-ir driver > already sets a precedent here so it wouldn't be an API change to change > to a set of protocols which might be different than what the user > suggested). (Also...yes, that'll make supporting several versions of > e.g. RC6 impossible with the current sysfs code). > I think that approach too is far from perfect as it leaves us with questions such as: How do we let the user know what variant of protocol the label "rc-6" really means? If in nvt we hardcode it to mean RC6-0-16 and a new driver cames along which chooses RC_TYPE_RC6_6A_24 how do we tell the user that the implementations differ? What if the scancode that was fed was really RC_TYPE_RC6_MCE? If only there were a sysfs api to set the exact variant life would be simpler... > Then change both ir_raw_encode_scancode() to take a single protocol enum > and change the *encode function pointer in struct ir_raw_handler to also > take a single protocol enum. > > That way encoders won't have to guess (using scanmasks...!?) what > protocol they should encode to. > > And Mauro...I strongly suggest you revert all of this encoding stuff > until this has been fixed...it's broken. > Let me shortly recap the points made so far about the encoding stuff: * If user enables multiple wakeup_protocols then the first matching one will be used to do the encoding. "First" being the module that was loaded first. * Currently the encoders use heuristics to determine the intended protocol variant from the scancode that was fed and from the length of the scanmask. This causes the programmer using the encoder to not know which exact variant will be used (until after the encoding is done when the information can be made available if needed). The way current sysfs api works using heuristics was a design choice since we don't have a better way to specify the protocol variant. Hope I didn't miss anything? So yeah.. The code isn't "broken" in a sense that it wouldn't work. It's more a question of what we want the api to look like. Mauro what do you think should be done?
On 2015-05-22 07:27, Antti Seppälä wrote: > On 21 May 2015 at 22:40, David Härdeman <david@hardeman.nu> wrote: >> On Thu, May 21, 2015 at 05:22:08PM +0300, Antti Seppälä wrote: >>> On 21 May 2015 at 15:30, David Härdeman <david@hardeman.nu> wrote: >>>> On 2015-05-21 13:51, Antti Seppälä wrote: >>>>> >>>>> On 21 May 2015 at 12:14, David Härdeman <david@hardeman.nu> wrote: >>>>>> >>>>>> I'm talking about ir_raw_encode_scancode() which is entirely >>>>>> broken in >>>>>> its >>>>>> current state. It will, given more than one enabled protocol, >>>>>> encode a >>>>>> scancode to pulse/space events according to the rules of a >>>>>> randomly >>>>>> chosen >>>>>> protocol. That random selection will be influenced by things like >>>>>> *module >>>>>> load order* (independent of the separate fact that passing >>>>>> multiple >>>>>> protocols to it is completely bogus in the first place). >>>>>> >>>>>> To be clear: the same scancode may be encoded differently >>>>>> depending on if >>>>>> you've load the nec decoder before or after the rc5 decoder! That >>>>>> kind of >>>>>> behavior can't go into a release kernel (Mauro...). >>>>>> >>>>> >>>>> So... if the ir_raw_handler_list is sorted to eliminate the >>>>> randomness >>>>> caused by module load ordering you will be happy (or happier)? >>>> >>>> >>>> No, cause it's a horrible hack. And the caller of >>>> ir_raw_handler_list() >>>> still has no idea of knowing (given more than one protocol) which >>>> protocol a >>>> given scancode will be encoded according to. >>>> >>> >>> Okay, so how about "demuxing" the first protocol before handing them >>> to encoder callback? Simply something like below: >>> >>> - if (handler->protocols & protocols && >>> handler->encode) { >>> + if (handler->protocols & ffs(protocols) && >>> handler->encode) { >>> >>> Now the behavior is well-defined even when multiple protocols are >>> selected. >> >> Your patchset introduced ir_raw_encode_scancode() as well as the only >> two callers of that function: >> >> drivers/media/rc/nuvoton-cir.c >> drivers/media/rc/rc-loopback.c >> >> I realize that the sysfs wakeup_protocols file (which bakes several >> protocols into one label) makes defining *the* protocol difficult, but >> if you're going to add hacks like this, keep them to the sole driver >> using the API rather than the core API itself. >> >> That is, change nvt_ir_raw_change_wakeup_protocol() so that it picks a >> single protocol, no matter how many are passed to it (the img-ir >> driver >> already sets a precedent here so it wouldn't be an API change to >> change >> to a set of protocols which might be different than what the user >> suggested). (Also...yes, that'll make supporting several versions of >> e.g. RC6 impossible with the current sysfs code). >> > > I think that approach too is far from perfect as it leaves us with > questions such as: How do we let the user know what variant of > protocol the label "rc-6" really means? If in nvt we hardcode it to > mean RC6-0-16 and a new driver cames along which chooses > RC_TYPE_RC6_6A_24 how do we tell the user that the implementations > differ? What if the scancode that was fed was really RC_TYPE_RC6_MCE? I never claimed it was perfect. For another (short-term, per-driver) solution, see the winbond-cir driver. > If only there were a sysfs api to set the exact variant life would be > simpler... Yes, and your patches made it harder to get to a sane solution. >> Then change both ir_raw_encode_scancode() to take a single protocol >> enum >> and change the *encode function pointer in struct ir_raw_handler to >> also >> take a single protocol enum. >> >> That way encoders won't have to guess (using scanmasks...!?) what >> protocol they should encode to. >> >> And Mauro...I strongly suggest you revert all of this encoding stuff >> until this has been fixed...it's broken. >> > > Let me shortly recap the points made so far about the encoding stuff: > > * If user enables multiple wakeup_protocols then the first matching > one will be used to do the encoding. "First" being the module that was > loaded first. That in itself would be a good enough reason to revert the patches. > * Currently the encoders use heuristics to determine the intended > protocol variant from the scancode that was fed and from the length of > the scanmask. This causes the programmer using the encoder to not know > which exact variant will be used (until after the encoding is done > when the information can be made available if needed). First, "currently" implies that the heuristics can later be changed. They can't once this becomes part of a released kernel (not without breaking the kernel API). Second, how do you plan to pass the information about the chosen protocol back to userspace? And what is userspace supposed to do with the information? * Userspace: please set the hardware to wake up if this RC6 mode0 command is received * Kernel: sure...I've set the hardware to wake up if a RC6 mode6a command is received * Userspace: WTF? Is the next step to go buy a new remote which matches what the kernel told you? Third, that the scanmask is suddenly used to determine the meaning of a scancode is in itself an API break. Fourth, the "programmer" is not the problem. The problem is the user(space). A user who writes e.g. a RC6-something wakeup scancode has no way of knowing according to which protocol the scancode will be interpreted. Meaning that even if: * the user knows he has a remote control in his hand which generates RC6 mode0 commands; and * he limits the wake protocols to "rc6"; and * he writes the correct scancode to sysfs then he still can't know that the hardware is correctly configured. And that might change depending on things like scanmask heuristics, module load order and the phase of the moon. > The way current > sysfs api works using heuristics was a design choice since we don't > have a better way to specify the protocol variant. > > Hope I didn't miss anything? You missed that once this goes in the API is going to be next to impossible to fix. > So yeah.. The code isn't "broken" in a sense that it wouldn't work. It's entirely broken in the sense that a user has no idea of knowing if the hardware has been properly configured to wake the computer or not. It's just as broken as the connect() system call would be if it randomly rewrote the destination address passed by the user, optionally connected, and most of the time returned zero independently of the result. I can't really believe we're still debating *if* this code should stay in it's present form. > It's more a question of what we want the api to look like. And if the current version is part of a released kernel we can never fix it. Look, there are fundamental issues right now in rc-core in that the only way a scancode can have any meaning is in a protocol-scancode tuple (single protocol, of course) and that information is missing in many places. That's something I'm trying to fix (see the updated set/get keytable ioctls for example) and Mauro seems to mostly want to forget about. Fixing it is probably going to be impossible without API breaks. Your code encodes more of that crap right in the core of rc-core and will require even more API breaks. -- 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 22 May 2015 at 13:33, David Härdeman <david@hardeman.nu> wrote: > On 2015-05-22 07:27, Antti Seppälä wrote: >> >> On 21 May 2015 at 22:40, David Härdeman <david@hardeman.nu> wrote: >>> >>> On Thu, May 21, 2015 at 05:22:08PM +0300, Antti Seppälä wrote: >>>> >>>> On 21 May 2015 at 15:30, David Härdeman <david@hardeman.nu> wrote: >>>>> >>>>> On 2015-05-21 13:51, Antti Seppälä wrote: >>>>>> >>>>>> >>>>>> On 21 May 2015 at 12:14, David Härdeman <david@hardeman.nu> wrote: >>>>>>> >>>>>>> >>>>>>> I'm talking about ir_raw_encode_scancode() which is entirely broken >>>>>>> in >>>>>>> its >>>>>>> current state. It will, given more than one enabled protocol, encode >>>>>>> a >>>>>>> scancode to pulse/space events according to the rules of a randomly >>>>>>> chosen >>>>>>> protocol. That random selection will be influenced by things like >>>>>>> *module >>>>>>> load order* (independent of the separate fact that passing multiple >>>>>>> protocols to it is completely bogus in the first place). >>>>>>> >>>>>>> To be clear: the same scancode may be encoded differently depending >>>>>>> on if >>>>>>> you've load the nec decoder before or after the rc5 decoder! That >>>>>>> kind of >>>>>>> behavior can't go into a release kernel (Mauro...). >>>>>>> >>>>>> >>>>>> So... if the ir_raw_handler_list is sorted to eliminate the randomness >>>>>> caused by module load ordering you will be happy (or happier)? >>>>> >>>>> >>>>> >>>>> No, cause it's a horrible hack. And the caller of ir_raw_handler_list() >>>>> still has no idea of knowing (given more than one protocol) which >>>>> protocol a >>>>> given scancode will be encoded according to. >>>>> >>>> >>>> Okay, so how about "demuxing" the first protocol before handing them >>>> to encoder callback? Simply something like below: >>>> >>>> - if (handler->protocols & protocols && handler->encode) { >>>> + if (handler->protocols & ffs(protocols) && >>>> handler->encode) { >>>> >>>> Now the behavior is well-defined even when multiple protocols are >>>> selected. >>> >>> >>> Your patchset introduced ir_raw_encode_scancode() as well as the only >>> two callers of that function: >>> >>> drivers/media/rc/nuvoton-cir.c >>> drivers/media/rc/rc-loopback.c >>> >>> I realize that the sysfs wakeup_protocols file (which bakes several >>> protocols into one label) makes defining *the* protocol difficult, but >>> if you're going to add hacks like this, keep them to the sole driver >>> using the API rather than the core API itself. >>> >>> That is, change nvt_ir_raw_change_wakeup_protocol() so that it picks a >>> single protocol, no matter how many are passed to it (the img-ir driver >>> already sets a precedent here so it wouldn't be an API change to change >>> to a set of protocols which might be different than what the user >>> suggested). (Also...yes, that'll make supporting several versions of >>> e.g. RC6 impossible with the current sysfs code). >>> >> >> I think that approach too is far from perfect as it leaves us with >> questions such as: How do we let the user know what variant of >> protocol the label "rc-6" really means? If in nvt we hardcode it to >> mean RC6-0-16 and a new driver cames along which chooses >> RC_TYPE_RC6_6A_24 how do we tell the user that the implementations >> differ? What if the scancode that was fed was really RC_TYPE_RC6_MCE? > > > I never claimed it was perfect. > > For another (short-term, per-driver) solution, see the winbond-cir driver. > Heh, funny you should mention that... Back in late 2013/early 2014 I submitted a patch for nuvoton which was modeled after winbond-cir. The feedback from that could be summarized as: - Scancodes should be used instead of raw pulse/spaces (the initial version of the patch worked without encoding) - Encoders should be generalized for others to use them too - Sysfs -api should be used instead of module parameters So the suggestions were a pretty much the exact opposite of what winbond-cir does. >> If only there were a sysfs api to set the exact variant life would be >> simpler... > > > Yes, and your patches made it harder to get to a sane solution. > >>> Then change both ir_raw_encode_scancode() to take a single protocol enum >>> and change the *encode function pointer in struct ir_raw_handler to also >>> take a single protocol enum. >>> >>> That way encoders won't have to guess (using scanmasks...!?) what >>> protocol they should encode to. >>> >>> And Mauro...I strongly suggest you revert all of this encoding stuff >>> until this has been fixed...it's broken. >>> >> >> Let me shortly recap the points made so far about the encoding stuff: >> >> * If user enables multiple wakeup_protocols then the first matching >> one will be used to do the encoding. "First" being the module that was >> loaded first. > > > That in itself would be a good enough reason to revert the patches. > >> * Currently the encoders use heuristics to determine the intended >> protocol variant from the scancode that was fed and from the length of >> the scanmask. This causes the programmer using the encoder to not know >> which exact variant will be used (until after the encoding is done >> when the information can be made available if needed). > > > First, "currently" implies that the heuristics can later be changed. They > can't once this becomes part of a released kernel (not without breaking the > kernel API). > > Second, how do you plan to pass the information about the chosen protocol > back to userspace? And what is userspace supposed to do with the > information? I don't plan to pass that information anywhere because there is no use case for it. Anyways that is besides the point. > * Userspace: please set the hardware to wake up if this RC6 mode0 command is > received > * Kernel: sure...I've set the hardware to wake up if a RC6 mode6a command is > received > * Userspace: WTF? > > Is the next step to go buy a new remote which matches what the kernel told > you? > > Third, that the scanmask is suddenly used to determine the meaning of a > scancode is in itself an API break. > > Fourth, the "programmer" is not the problem. The problem is the user(space). > A user who writes e.g. a RC6-something wakeup scancode has no way of knowing > according to which protocol the scancode will be interpreted. Meaning that > even if: > > * the user knows he has a remote control in his hand which generates RC6 > mode0 commands; and > * he limits the wake protocols to "rc6"; and > * he writes the correct scancode to sysfs > > then he still can't know that the hardware is correctly configured. And that > might change depending on things like scanmask heuristics, module load order > and the phase of the moon. > >> The way current >> sysfs api works using heuristics was a design choice since we don't >> have a better way to specify the protocol variant. >> >> Hope I didn't miss anything? > > > You missed that once this goes in the API is going to be next to impossible > to fix. > >> So yeah.. The code isn't "broken" in a sense that it wouldn't work. > > > It's entirely broken in the sense that a user has no idea of knowing if the > hardware has been properly configured to wake the computer or not. It's just > as broken as the connect() system call would be if it randomly rewrote the > destination address passed by the user, optionally connected, and most of > the time returned zero independently of the result. > I think you may be misunderstanding the sysfs api or at least the connect() analogue here as well as the userspace-kernelspace example above are actually not how the api works. Remember that userspace does not know about the protocol variants. Let me try to use your example and work-out how it really goes: * Userspace: please set the hardware to wake up if the scancode 0x800f040c is received. I know this is RC6 scancode because it came from the rc-6 decoder but I don't know the variant (and I don't really care) * Kernel: Ok (btw. based on the length of the scancode and the bit-pattern in the front I've determined this to be rc6-mce type scancode and encoded it accordingly) * Userspace: Got it So no changing anything behind users back or not leading to misconfigured hardware AFAICT. > I can't really believe we're still debating *if* this code should stay in > it's present form. > Of course we end up having a discussion when it looks like we undestand the code/api behavior differently and my goal is obviously to know what/how/where/when the code is "broken" to determine if it is true and if it can be fixed :) >> It's more a question of what we want the api to look like. > > > And if the current version is part of a released kernel we can never fix it. > > Look, there are fundamental issues right now in rc-core in that the only way > a scancode can have any meaning is in a protocol-scancode tuple (single > protocol, of course) and that information is missing in many places. That's > something I'm trying to fix (see the updated set/get keytable ioctls for > example) and Mauro seems to mostly want to forget about. Fixing it is > probably going to be impossible without API breaks. Your code encodes more > of that crap right in the core of rc-core and will require even more API > breaks. > I'm sorry that the encoding functionality clashes with your intentions of improving the rc-core. I guess Mauro likes encoders more than improving rc-core fundamentals :) Kidding aside the fact that this series got merged might suggest that you and Mauro don't necessarily share the same views about the future and possible api breaks of rc-core. Tell you what, I'll agree to reverting the series. In exchange I would hope that you and Mauro mutually agree and let me know on: - What are the issues that need to be fixed in the encoding series prefarably with how to fix them (e.g. module load order ambiquity, whether a new api is needed, or switching to a more limited functionality is desired like you suggested then so be it etc.) - When is a good chance to re-submit the series (e.g. after ioctl/scancode/whatever api break is done or some pending series is merged or some other core refactoring work is finished etc.) Deal?
On 2015-05-23 13:34, Antti Seppälä wrote: > On 22 May 2015 at 13:33, David Härdeman <david@hardeman.nu> wrote: >> On 2015-05-22 07:27, Antti Seppälä wrote: >>> I think that approach too is far from perfect as it leaves us with >>> questions such as: How do we let the user know what variant of >>> protocol the label "rc-6" really means? If in nvt we hardcode it to >>> mean RC6-0-16 and a new driver cames along which chooses >>> RC_TYPE_RC6_6A_24 how do we tell the user that the implementations >>> differ? What if the scancode that was fed was really RC_TYPE_RC6_MCE? >> >> >> I never claimed it was perfect. >> >> For another (short-term, per-driver) solution, see the winbond-cir >> driver. >> > > Heh, funny you should mention that... Back in late 2013/early 2014 I > submitted a patch for nuvoton which was modeled after winbond-cir. The > feedback from that could be summarized as: > - Scancodes should be used instead of raw pulse/spaces (the initial > version of the patch worked without encoding) > - Encoders should be generalized for others to use them too > - Sysfs -api should be used instead of module parameters > > So the suggestions were a pretty much the exact opposite of what > winbond-cir does. Again, it was a short-term suggestion. A long-term "real" solution requires a definitive identification of the intended protocol. One idea that I've had in the back of my head for a long time is to use the "flags" member of "struct rc_keymap_entry" in the new EVIOC[GS]KEYCODE_V2 ioctl variant (see http://www.spinics.net/lists/linux-media/msg88452.html). If a RC_POWERON flag was defined, it could be used for that purpose... >> It's entirely broken in the sense that a user has no idea of knowing >> if the >> hardware has been properly configured to wake the computer or not. >> It's just >> as broken as the connect() system call would be if it randomly rewrote >> the >> destination address passed by the user, optionally connected, and most >> of >> the time returned zero independently of the result. >> > > I think you may be misunderstanding the sysfs api or at least the > connect() analogue here as well as the userspace-kernelspace example > above are actually not how the api works. Remember that userspace does > not know about the protocol variants. Userspace most definately does. Keymaps are a good example of that. Distributing keymaps for a certain remote should be possible. > Let me try to use your example and work-out how it really goes: > * Userspace: please set the hardware to wake up if the scancode > 0x800f040c is received. I know this is RC6 scancode because it came > from the rc-6 decoder but I don't know the variant (and I don't really > care) First of all...you assume that the only way of generating a valid wakeup scancode is by using the same remote on the same hardware first to see what it generates (which - thanks to things like boneheaded decisions on NEC scancode generation and the previously mentioned heuristics - may differ). So distributing a keymap from a central repository or just looking up scancodes from a vendor datasheet is no longer feasible with this API. Second, you have absolutely nothing that ensures that the same heuristics are used in the decode/encode code paths and that the heuristics will remain in sync. > * Kernel: Ok (btw. based on the length of the scancode and the > bit-pattern in the front I've determined this to be rc6-mce type > scancode and encoded it accordingly) > * Userspace: Got it The whole "btw" part won't be passed to userspace...so there's nothing to "get" > So no changing anything behind users back or not leading to > misconfigured hardware AFAICT. ... > I'm sorry that the encoding functionality clashes with your intentions > of improving the rc-core. I guess Mauro likes encoders more than > improving rc-core fundamentals :) > Kidding aside the fact that this series got merged might suggest that > you and Mauro don't necessarily share the same views about the future > and possible api breaks of rc-core. If you've followed the development of rc-core during the last few years it should be pretty clear that Mauro has little to no long-term plan, understanding of the current issues or willingness to fix them. I wouldn't read too much into the fact that the code was merged. > Tell you what, I'll agree to reverting the series. In exchange I would > hope that you and Mauro mutually agree and let me know on: > - What are the issues that need to be fixed in the encoding series > prefarably with how to fix them (e.g. module load order ambiquity, > whether a new api is needed, or switching to a more limited > functionality is desired like you suggested then so be it etc.) > - When is a good chance to re-submit the series (e.g. after > ioctl/scancode/whatever api break is done or some pending series is > merged or some other core refactoring work is finished etc.) > > Deal? Mauro....wake up? I hope you're not planning to push the current code upstream??? -- 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 14 June 2015 at 02:44, David Härdeman <david@hardeman.nu> wrote: > One idea that I've had in the back of my head for a long time is to use the > "flags" member of "struct rc_keymap_entry" in the new EVIOC[GS]KEYCODE_V2 > ioctl variant (see http://www.spinics.net/lists/linux-media/msg88452.html). > > If a RC_POWERON flag was defined, it could be used for that purpose... > Ooh, that approach would indeed provide a cleaner api for setting the wakeup scancode. I like the idea though I haven't really had the chance to try it out. > ... >> >> I'm sorry that the encoding functionality clashes with your intentions >> of improving the rc-core. I guess Mauro likes encoders more than >> improving rc-core fundamentals :) >> Kidding aside the fact that this series got merged might suggest that >> you and Mauro don't necessarily share the same views about the future >> and possible api breaks of rc-core. > > > If you've followed the development of rc-core during the last few years it > should be pretty clear that Mauro has little to no long-term plan, > understanding of the current issues or willingness to fix them. I wouldn't > read too much into the fact that the code was merged. > >> Tell you what, I'll agree to reverting the series. In exchange I would >> hope that you and Mauro mutually agree and let me know on: >> - What are the issues that need to be fixed in the encoding series >> prefarably with how to fix them (e.g. module load order ambiquity, >> whether a new api is needed, or switching to a more limited >> functionality is desired like you suggested then so be it etc.) >> - When is a good chance to re-submit the series (e.g. after >> ioctl/scancode/whatever api break is done or some pending series is >> merged or some other core refactoring work is finished etc.) >> >> Deal? > > > Mauro....wake up? I hope you're not planning to push the current code > upstream??? > Yeah, I'm also inclined to target for a longer term solution with this so the current patches can be reverted. I think I now also have enough information to go and respin the patches to utilize the new EVIOCSKEYCODE_V2 if and when that gets included in the rc-core.
Em Sun, 14 Jun 2015 01:44:54 +0200 David Härdeman <david@hardeman.nu> escreveu: > On 2015-05-23 13:34, Antti Seppälä wrote: > > On 22 May 2015 at 13:33, David Härdeman <david@hardeman.nu> wrote: > >> On 2015-05-22 07:27, Antti Seppälä wrote: > >>> I think that approach too is far from perfect as it leaves us with > >>> questions such as: How do we let the user know what variant of > >>> protocol the label "rc-6" really means? If in nvt we hardcode it to > >>> mean RC6-0-16 and a new driver cames along which chooses > >>> RC_TYPE_RC6_6A_24 how do we tell the user that the implementations > >>> differ? What if the scancode that was fed was really RC_TYPE_RC6_MCE? > >> > >> > >> I never claimed it was perfect. > >> > >> For another (short-term, per-driver) solution, see the winbond-cir > >> driver. > >> > > > > Heh, funny you should mention that... Back in late 2013/early 2014 I > > submitted a patch for nuvoton which was modeled after winbond-cir. The > > feedback from that could be summarized as: > > - Scancodes should be used instead of raw pulse/spaces (the initial > > version of the patch worked without encoding) > > - Encoders should be generalized for others to use them too > > - Sysfs -api should be used instead of module parameters > > > > So the suggestions were a pretty much the exact opposite of what > > winbond-cir does. > > Again, it was a short-term suggestion. A long-term "real" solution > requires a definitive identification of the intended protocol. > > One idea that I've had in the back of my head for a long time is to use > the "flags" member of "struct rc_keymap_entry" in the new > EVIOC[GS]KEYCODE_V2 ioctl variant (see > http://www.spinics.net/lists/linux-media/msg88452.html). > > If a RC_POWERON flag was defined, it could be used for that purpose... > > >> It's entirely broken in the sense that a user has no idea of knowing > >> if the > >> hardware has been properly configured to wake the computer or not. > >> It's just > >> as broken as the connect() system call would be if it randomly rewrote > >> the > >> destination address passed by the user, optionally connected, and most > >> of > >> the time returned zero independently of the result. > >> > > > > I think you may be misunderstanding the sysfs api or at least the > > connect() analogue here as well as the userspace-kernelspace example > > above are actually not how the api works. Remember that userspace does > > not know about the protocol variants. > > Userspace most definately does. Keymaps are a good example of that. > Distributing keymaps for a certain remote should be possible. > > > Let me try to use your example and work-out how it really goes: > > * Userspace: please set the hardware to wake up if the scancode > > 0x800f040c is received. I know this is RC6 scancode because it came > > from the rc-6 decoder but I don't know the variant (and I don't really > > care) > > First of all...you assume that the only way of generating a valid wakeup > scancode is by using the same remote on the same hardware first to see > what it generates (which - thanks to things like boneheaded decisions on > NEC scancode generation and the previously mentioned heuristics - may > differ). So distributing a keymap from a central repository or just > looking up scancodes from a vendor datasheet is no longer feasible with > this API. > > Second, you have absolutely nothing that ensures that the same > heuristics are used in the decode/encode code paths and that the > heuristics will remain in sync. > > > * Kernel: Ok (btw. based on the length of the scancode and the > > bit-pattern in the front I've determined this to be rc6-mce type > > scancode and encoded it accordingly) > > * Userspace: Got it > > The whole "btw" part won't be passed to userspace...so there's nothing > to "get" > > > So no changing anything behind users back or not leading to > > misconfigured hardware AFAICT. > ... > > I'm sorry that the encoding functionality clashes with your intentions > > of improving the rc-core. I guess Mauro likes encoders more than > > improving rc-core fundamentals :) > > Kidding aside the fact that this series got merged might suggest that > > you and Mauro don't necessarily share the same views about the future > > and possible api breaks of rc-core. > > If you've followed the development of rc-core during the last few years > it should be pretty clear that Mauro has little to no long-term plan, > understanding of the current issues or willingness to fix them. I > wouldn't read too much into the fact that the code was merged. You completely missed the point. Adding new drivers and new features don't require much time to review, and don't require testing. What you're trying to send as "fixes" is actually series of patches that change the behavior of the subsystem, with would cause regressions. Btw, a lot of the pull requests you've sent over the past years did cause regressions. So, I can only review/apply them when I have a bunch of spare time to test them. As I don't usually have a bunch of spare time, nor we have a sub-maintainer for remote controllers that would have time for such tests, they're delayed. > > Tell you what, I'll agree to reverting the series. In exchange I would > > hope that you and Mauro mutually agree and let me know on: > > - What are the issues that need to be fixed in the encoding series > > prefarably with how to fix them (e.g. module load order ambiquity, > > whether a new api is needed, or switching to a more limited > > functionality is desired like you suggested then so be it etc.) > > - When is a good chance to re-submit the series (e.g. after > > ioctl/scancode/whatever api break is done or some pending series is > > merged or some other core refactoring work is finished etc.) > > > > Deal? > > Mauro....wake up? I hope you're not planning to push the current code > upstream??? What's there are planned to be sent upstream. If you think that something is not mature enough to be applied, please send a patch reverting it, with "[PATCH FIXES]" in the subject, clearly explaining why it should be reverted for me to analyze. Having Antti/James acks on that would help. Regards, Mauro -- 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 2015-06-18 23:23, Mauro Carvalho Chehab wrote: > Em Sun, 14 Jun 2015 01:44:54 +0200 > David Härdeman <david@hardeman.nu> escreveu: >> If you've followed the development of rc-core during the last few >> years >> it should be pretty clear that Mauro has little to no long-term plan, >> understanding of the current issues or willingness to fix them. I >> wouldn't read too much into the fact that the code was merged. > > You completely missed the point. Of course... > Adding new drivers and new features > don't require much time to review, and don't require testing. But a focus on adding new "features" (some of which further cement bad API) is dangerous when the foundations still need work. > What you're trying to send as "fixes" is actually series of patches > that > change the behavior of the subsystem, with would cause regressions. Some things can't be fixed without changing some behavior...assuming you're not talking about the patches that add a rc-core chardev...that is indeed a whole new direction and I can completely take onboard that you'd like to see a better RFC discussion with a document describing the interface, changes, rationale, etc....and I'd be happy to produce a document like that when I have the time (I'm guessing the LinuxTV wiki might be a good place). I have a total of six patches in my queue that are not related to the rc-core chardev: One fixes a uevent bug and should be trivial One converts rc-core to use an IDA, a cleanup which seems to fix a race as well One removes lirc as a "protocol" and is not an API change as you thought One prepares the lmedm04 driver for the next two patches The last two adds protocol info to the EVIOC[GS]KEYCODE_V2 ioctl The last two patches need the most careful scrutiny, but they are an attempt to finally fix a serious issue. I've already indicated that they are not 100% backwards compatible, but the corner cases they won't catch (can't catch) are pretty extreme. I'd be happy to discuss them further if you'd like. I have no plans to move on to the rc-core chardev discussion before the above patches have been dealt with. I don't think it's a good use of anyone's time. > Btw, a lot of the pull requests you've sent over the past years did > cause > regressions. Yes, trying to change/fix parts of the foundation of the rc-core code definitely carries a larger risk of regressions (especially when I don't even have the hardware). That's not a good reason to not try though. > So, I can only review/apply them when I have a bunch of > spare time to test them. As I don't usually have a bunch of spare time, > nor we have a sub-maintainer for remote controllers that would have > time for such tests, they're delayed. I think we're getting off-topic. >> Mauro....wake up? I hope you're not planning to push the current code >> upstream??? > > What's there are planned to be sent upstream. If you think that > something > is not mature enough to be applied, please send a patch reverting it, > with "[PATCH FIXES]" in the subject, clearly explaining why it should > be > reverted for me to analyze. Having Antti/James acks on that would help. This thread should already provide you with all the information you need why the patches should be reverted (including Antii saying the patches should be reverted). The current code includes hilarious "features" like producing different results depending on module load order and makes sure we'll be stuck with a bad API. Sending them upstream will look quite foolish... 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
On Tue, Jun 23, 2015 at 10:45:42PM +0200, David Härdeman wrote: >On 2015-06-18 23:23, Mauro Carvalho Chehab wrote: >>Em Sun, 14 Jun 2015 01:44:54 +0200 >>David Härdeman <david@hardeman.nu> escreveu: >>>Mauro....wake up? I hope you're not planning to push the current code >>>upstream??? >> >>What's there are planned to be sent upstream. If you think that something >>is not mature enough to be applied, please send a patch reverting it, >>with "[PATCH FIXES]" in the subject, clearly explaining why it should be >>reverted for me to analyze. Having Antti/James acks on that would help. > >This thread should already provide you with all the information you need why >the patches should be reverted (including Antii saying the patches should be >reverted). > >The current code includes hilarious "features" like producing different >results depending on module load order and makes sure we'll be stuck with a >bad API. Sending them upstream will look quite foolish... And now the patches have been submitted and comitted upstream. What's your plan? Leave it like this?
On Mon, Jun 29, 2015 at 09:05:24PM +0200, David Härdeman wrote: >On Tue, Jun 23, 2015 at 10:45:42PM +0200, David Härdeman wrote: >>On 2015-06-18 23:23, Mauro Carvalho Chehab wrote: >>>Em Sun, 14 Jun 2015 01:44:54 +0200 >>>David Härdeman <david@hardeman.nu> escreveu: >>>>Mauro....wake up? I hope you're not planning to push the current code >>>>upstream??? >>> >>>What's there are planned to be sent upstream. If you think that something >>>is not mature enough to be applied, please send a patch reverting it, >>>with "[PATCH FIXES]" in the subject, clearly explaining why it should be >>>reverted for me to analyze. Having Antti/James acks on that would help. >> >>This thread should already provide you with all the information you need why >>the patches should be reverted (including Antii saying the patches should be >>reverted). >> >>The current code includes hilarious "features" like producing different >>results depending on module load order and makes sure we'll be stuck with a >>bad API. Sending them upstream will look quite foolish... > >And now the patches have been submitted and comitted upstream. What's >your plan? Leave it like this? Mauro, I see that you've applied four of my patches...thanks for that...but the question is still what you plan to do about the patches that should be reverted....4.2-rc2 was recently released and I'm still not seeing any action on this while time is running out...?
Em Mon, 13 Jul 2015 19:47:50 +0200 David Härdeman <david@hardeman.nu> escreveu: > On Mon, Jun 29, 2015 at 09:05:24PM +0200, David Härdeman wrote: > >On Tue, Jun 23, 2015 at 10:45:42PM +0200, David Härdeman wrote: > >>On 2015-06-18 23:23, Mauro Carvalho Chehab wrote: > >>>Em Sun, 14 Jun 2015 01:44:54 +0200 > >>>David Härdeman <david@hardeman.nu> escreveu: > >>>>Mauro....wake up? I hope you're not planning to push the current code > >>>>upstream??? > >>> > >>>What's there are planned to be sent upstream. If you think that something > >>>is not mature enough to be applied, please send a patch reverting it, > >>>with "[PATCH FIXES]" in the subject, clearly explaining why it should be > >>>reverted for me to analyze. Having Antti/James acks on that would help. > >> > >>This thread should already provide you with all the information you need why > >>the patches should be reverted (including Antii saying the patches should be > >>reverted). > >> > >>The current code includes hilarious "features" like producing different > >>results depending on module load order and makes sure we'll be stuck with a > >>bad API. Sending them upstream will look quite foolish... > > > >And now the patches have been submitted and comitted upstream. What's > >your plan? Leave it like this? > > Mauro, I see that you've applied four of my patches...thanks for > that...but the question is still what you plan to do about the patches > that should be reverted....4.2-rc2 was recently released and I'm still > not seeing any action on this while time is running out...? What patches? I'm not seeing any such revert patches on my queue. At least patchwork doesn't show any pending patches from you: https://patchwork.linuxtv.org/project/linux-media/list/?submitter=96 Nor from James: https://patchwork.linuxtv.org/project/linux-media/list/?submitter=1270 Nor from Antti: https://patchwork.linuxtv.org/project/linux-media/list/?submitter=650 Are you sure you submitted any reverse patch? If so, please point the patches you sent for me to review/apply. Thanks, Mauro -- 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
diff --git a/drivers/media/rc/rc-core-priv.h b/drivers/media/rc/rc-core-priv.h index b68d4f76..122c25f 100644 --- a/drivers/media/rc/rc-core-priv.h +++ b/drivers/media/rc/rc-core-priv.h @@ -25,6 +25,8 @@ struct ir_raw_handler { u64 protocols; /* which are handled by this handler */ int (*decode)(struct rc_dev *dev, struct ir_raw_event event); + int (*encode)(u64 protocols, const struct rc_scancode_filter *scancode, + struct ir_raw_event *events, unsigned int max); /* These two should only be used by the lirc decoder */ int (*raw_register)(struct rc_dev *dev); diff --git a/drivers/media/rc/rc-ir-raw.c b/drivers/media/rc/rc-ir-raw.c index b732ac6..dd47fe5 100644 --- a/drivers/media/rc/rc-ir-raw.c +++ b/drivers/media/rc/rc-ir-raw.c @@ -246,6 +246,43 @@ static int change_protocol(struct rc_dev *dev, u64 *rc_type) return 0; } +/** + * ir_raw_encode_scancode() - Encode a scancode as raw events + * + * @protocols: permitted protocols + * @scancode: scancode filter describing a single scancode + * @events: array of raw events to write into + * @max: max number of raw events + * + * Attempts to encode the scancode as raw events. + * + * Returns: The number of events written. + * -ENOBUFS if there isn't enough space in the array to fit the + * encoding. In this case all @max events will have been written. + * -EINVAL if the scancode is ambiguous or invalid, or if no + * compatible encoder was found. + */ +int ir_raw_encode_scancode(u64 protocols, + const struct rc_scancode_filter *scancode, + struct ir_raw_event *events, unsigned int max) +{ + struct ir_raw_handler *handler; + int ret = -EINVAL; + + mutex_lock(&ir_raw_handler_lock); + list_for_each_entry(handler, &ir_raw_handler_list, list) { + if (handler->protocols & protocols && handler->encode) { + ret = handler->encode(protocols, scancode, events, max); + if (ret >= 0 || ret == -ENOBUFS) + break; + } + } + mutex_unlock(&ir_raw_handler_lock); + + return ret; +} +EXPORT_SYMBOL(ir_raw_encode_scancode); + /* * Used to (un)register raw event clients */ diff --git a/include/media/rc-core.h b/include/media/rc-core.h index 2c7fbca..5703c08 100644 --- a/include/media/rc-core.h +++ b/include/media/rc-core.h @@ -250,6 +250,9 @@ int ir_raw_event_store_edge(struct rc_dev *dev, enum raw_event_type type); int ir_raw_event_store_with_filter(struct rc_dev *dev, struct ir_raw_event *ev); void ir_raw_event_set_idle(struct rc_dev *dev, bool idle); +int ir_raw_encode_scancode(u64 protocols, + const struct rc_scancode_filter *scancode, + struct ir_raw_event *events, unsigned int max); static inline void ir_raw_event_reset(struct rc_dev *dev) {