Re: [PATCH v2 01/25] media: lirc: implement scancode sending
On 09/10/17 17:11, Sean Young wrote:
> On Mon, Oct 09, 2017 at 11:14:28AM +0200, Hans Verkuil wrote:
>> On 05/10/17 10:45, Sean Young wrote:
> -snip-
>>> +/*
>>> + * struct lirc_scancode - decoded scancode with protocol for use with
>>> + * LIRC_MODE_SCANCODE
>>> + *
>>> + * @timestamp: Timestamp in nanoseconds using CLOCK_MONOTONIC when IR
>>> + * was decoded.
>>> + * @flags: should be 0 for transmit. When receiving scancodes,
>>> + * LIRC_SCANCODE_FLAG_TOGGLE or LIRC_SCANCODE_FLAG_REPEAT can be set
>>> + * depending on the protocol
>>> + * @target: target for transmit. Unused, set to 0.
>>> + * @source: source for receive. Unused, set to 0.
>>> + * @unused: set to 0.
>>> + * @rc_proto: see enum rc_proto
>>> + * @scancode: the scancode received or to be sent
>>> + */
>>> +struct lirc_scancode {
>>> + __u64 timestamp;
>>> + __u32 flags;
>>> + __u8target;
>>> + __u8source;
>>> + __u8unused;
>>> + __u8rc_proto;
>>> + __u64 scancode;
>>
>> I'm thinking how this will be implemented using CEC. Some RC commands take
>> arguments
>> (up to 4 bytes for the 0x67 (Tune Function) code), so how will they be
>> handled?
>>
>> See CEC table 6 in the HDMI 1.4 spec.
>>
>> Should they be part of the scancode, or would it be better to add a '__u8
>> args[8];'
>> field?
>>
>> I've no idea what makes sense, it's a weird corner case.
>
> I've given it some more thought.
>
> For cec remote control passthrough, you have the tv with the IR receiver (A),
> which then transmits CEC_MSG_USER_CONTROL_PRESSED and
> CEC_MSG_USER_CONTROL_RELEASED cec messages to the correct target, with
> arguments. Then on the target (B), it reads those commands and should execute
> them as if it received them itself.
>
> First of all (B) is already implemented in cec using rc-core. If RC
> passthrough is enabled, then cec will pass those keycodes to rc-core (which
> end up in an input device).
>
> So the problem we are trying to solve here is (A). How I would see this
> implemented is:
>
> 1) A physical IR receiver exists which has an rc-core driver and a /dev/lircN
>device. This is configured using ir-keytable to map to regular input events
>
> 2) A process receives input events, and decides that a particular key/command
>is not for itself (e.g. tell top set box to tune), so it knows what the
>target cec address is and the tune arguments, so it fills out a
>cec_msg with the target, CEC_MSG_USER_CONTROL_PRESSED, 0x67, arguments,
>and then transmits it using the ioctl CEC_TRANSMIT, followed by
>another CEC_MSG_USER_CONTROL_RELEASED cec_msg sent using ioctl
> CEC_TRANSMIT.
>
> In this way of viewing things, an rc-core device is either cec or lirc, and
> thus rc-core lirc devices have a /dev/lircN and rc-core cec devices have a
> /dev/cecN.
>
> So, the alternative which is being proposed is that a cec device has both
> a /dev/cecN and a /dev/lircN. In this case step 2) would look like:
>
> 2) A process receives input events, and decides that a particular key/command
>is not for itself (e.g. tell top set box to tune), so it knows what the
>target cec address is and the tune arguments, so it fills in a
>lirc_scancode with the target, CEC_MSG_USER_CONTROL_PRESSED, 0x67,
> arguments,
>and then transmits it using write() to the /dev/lircN device, which
>then passes it on to cec_transmit() in drivers/media/cec/cec-api.c
>(without having a cec_fh), and then another lirc_scancode is
>filled in CEC_MSG_USER_CONTROL_RELEASED and sent.
>
> Now, I think that this has a number of problems:
>
> - It's a lot of API for simply doing a CEC_TRANSMIT
>
> - and another chardev for a cec device (i.e. /dev/lircN).
>
> - lirc scancode tx deals with scancodes, for cec rc passthrough it isn't
>really scancodes.
>
> - Wiring this up is not going to be pretty or easy.
>
> - The lirc chardev has no other function other than sending
>CEC_MSG_USER_CONTROL_PRESSED and CEC_MSG_USER_CONTROL_RELEASED cec
> messages.
>
> So what I am proposing is that we don't use lirc for sending rc passthrough
> messages for cec.
>
> I hope this makes sense and where not, please *do* tell me exactly where I
> am wrong. I think that I missed something about the scancode tx idea.
You are right, it makes no sense. Let's forget about this.
Regards,
Hans
>
>>
>>> +};
>>> +
>>> +#define LIRC_SCANCODE_FLAG_TOGGLE 1
>>> +#define LIRC_SCANCODE_FLAG_REPEAT 2
>>
>> These flags need documentation.
>
> They do, fair point.
>
> Thanks,
>
> Sean
>
Re: [PATCH v2 01/25] media: lirc: implement scancode sending
On Mon, Oct 09, 2017 at 11:14:28AM +0200, Hans Verkuil wrote:
> On 05/10/17 10:45, Sean Young wrote:
-snip-
> > +/*
> > + * struct lirc_scancode - decoded scancode with protocol for use with
> > + * LIRC_MODE_SCANCODE
> > + *
> > + * @timestamp: Timestamp in nanoseconds using CLOCK_MONOTONIC when IR
> > + * was decoded.
> > + * @flags: should be 0 for transmit. When receiving scancodes,
> > + * LIRC_SCANCODE_FLAG_TOGGLE or LIRC_SCANCODE_FLAG_REPEAT can be set
> > + * depending on the protocol
> > + * @target: target for transmit. Unused, set to 0.
> > + * @source: source for receive. Unused, set to 0.
> > + * @unused: set to 0.
> > + * @rc_proto: see enum rc_proto
> > + * @scancode: the scancode received or to be sent
> > + */
> > +struct lirc_scancode {
> > + __u64 timestamp;
> > + __u32 flags;
> > + __u8target;
> > + __u8source;
> > + __u8unused;
> > + __u8rc_proto;
> > + __u64 scancode;
>
> I'm thinking how this will be implemented using CEC. Some RC commands take
> arguments
> (up to 4 bytes for the 0x67 (Tune Function) code), so how will they be
> handled?
>
> See CEC table 6 in the HDMI 1.4 spec.
>
> Should they be part of the scancode, or would it be better to add a '__u8
> args[8];'
> field?
>
> I've no idea what makes sense, it's a weird corner case.
I've given it some more thought.
For cec remote control passthrough, you have the tv with the IR receiver (A),
which then transmits CEC_MSG_USER_CONTROL_PRESSED and
CEC_MSG_USER_CONTROL_RELEASED cec messages to the correct target, with
arguments. Then on the target (B), it reads those commands and should execute
them as if it received them itself.
First of all (B) is already implemented in cec using rc-core. If RC
passthrough is enabled, then cec will pass those keycodes to rc-core (which
end up in an input device).
So the problem we are trying to solve here is (A). How I would see this
implemented is:
1) A physical IR receiver exists which has an rc-core driver and a /dev/lircN
device. This is configured using ir-keytable to map to regular input events
2) A process receives input events, and decides that a particular key/command
is not for itself (e.g. tell top set box to tune), so it knows what the
target cec address is and the tune arguments, so it fills out a
cec_msg with the target, CEC_MSG_USER_CONTROL_PRESSED, 0x67, arguments,
and then transmits it using the ioctl CEC_TRANSMIT, followed by
another CEC_MSG_USER_CONTROL_RELEASED cec_msg sent using ioctl CEC_TRANSMIT.
In this way of viewing things, an rc-core device is either cec or lirc, and
thus rc-core lirc devices have a /dev/lircN and rc-core cec devices have a
/dev/cecN.
So, the alternative which is being proposed is that a cec device has both
a /dev/cecN and a /dev/lircN. In this case step 2) would look like:
2) A process receives input events, and decides that a particular key/command
is not for itself (e.g. tell top set box to tune), so it knows what the
target cec address is and the tune arguments, so it fills in a
lirc_scancode with the target, CEC_MSG_USER_CONTROL_PRESSED, 0x67, arguments,
and then transmits it using write() to the /dev/lircN device, which
then passes it on to cec_transmit() in drivers/media/cec/cec-api.c
(without having a cec_fh), and then another lirc_scancode is
filled in CEC_MSG_USER_CONTROL_RELEASED and sent.
Now, I think that this has a number of problems:
- It's a lot of API for simply doing a CEC_TRANSMIT
- and another chardev for a cec device (i.e. /dev/lircN).
- lirc scancode tx deals with scancodes, for cec rc passthrough it isn't
really scancodes.
- Wiring this up is not going to be pretty or easy.
- The lirc chardev has no other function other than sending
CEC_MSG_USER_CONTROL_PRESSED and CEC_MSG_USER_CONTROL_RELEASED cec messages.
So what I am proposing is that we don't use lirc for sending rc passthrough
messages for cec.
I hope this makes sense and where not, please *do* tell me exactly where I
am wrong. I think that I missed something about the scancode tx idea.
>
> > +};
> > +
> > +#define LIRC_SCANCODE_FLAG_TOGGLE 1
> > +#define LIRC_SCANCODE_FLAG_REPEAT 2
>
> These flags need documentation.
They do, fair point.
Thanks,
Sean
Re: [PATCH v2 01/25] media: lirc: implement scancode sending
On 05/10/17 10:45, Sean Young wrote:
> This introduces a new lirc mode: scancode. Any device which can send raw IR
> can now also send scancodes.
>
> int main()
> {
> int mode, fd = open("/dev/lirc0", O_RDWR);
>
> mode = LIRC_MODE_SCANCODE;
> if (ioctl(fd, LIRC_SET_SEND_MODE, )) {
> // kernel too old or lirc does not support transmit
> }
> struct lirc_scancode scancode = {
> .scancode = 0x1e3d,
> .rc_proto = RC_PROTO_RC5,
> };
> write(fd, , sizeof(scancode));
> close(fd);
> }
>
> The other fields of lirc_scancode must be set to 0.
>
> Note that toggle (rc5, rc6) and repeats (nec) are not implemented. Nor is
> there a method for holding down a key for a period.
>
> Signed-off-by: Sean Young
> ---
> drivers/media/rc/ir-lirc-codec.c | 101
> ---
> drivers/media/rc/rc-core-priv.h | 2 +-
> include/media/rc-map.h | 54 +
> include/uapi/linux/lirc.h| 93 +++
> 4 files changed, 169 insertions(+), 81 deletions(-)
>
> diff --git a/drivers/media/rc/ir-lirc-codec.c
> b/drivers/media/rc/ir-lirc-codec.c
> index bd046c41a53a..ef0b8df88613 100644
> --- a/drivers/media/rc/ir-lirc-codec.c
> +++ b/drivers/media/rc/ir-lirc-codec.c
> @@ -107,7 +107,8 @@ static ssize_t ir_lirc_transmit_ir(struct file *file,
> const char __user *buf,
> {
> struct lirc_codec *lirc;
> struct rc_dev *dev;
> - unsigned int *txbuf; /* buffer with values to transmit */
> + unsigned int *txbuf = NULL;
> + struct ir_raw_event *raw = NULL;
> ssize_t ret = -EINVAL;
> size_t count;
> ktime_t start;
> @@ -121,16 +122,51 @@ static ssize_t ir_lirc_transmit_ir(struct file *file,
> const char __user *buf,
> if (!lirc)
> return -EFAULT;
>
> - if (n < sizeof(unsigned) || n % sizeof(unsigned))
> - return -EINVAL;
> + if (lirc->send_mode == LIRC_MODE_SCANCODE) {
> + struct lirc_scancode scan;
>
> - count = n / sizeof(unsigned);
> - if (count > LIRCBUF_SIZE || count % 2 == 0)
> - return -EINVAL;
> + if (n != sizeof(scan))
> + return -EINVAL;
>
> - txbuf = memdup_user(buf, n);
> - if (IS_ERR(txbuf))
> - return PTR_ERR(txbuf);
> + if (copy_from_user(, buf, sizeof(scan)))
> + return -EFAULT;
> +
> + if (scan.flags || scan.source || scan.target || scan.unused ||
> + scan.timestamp)
> + return -EINVAL;
> +
> + raw = kmalloc_array(LIRCBUF_SIZE, sizeof(*raw), GFP_KERNEL);
> + if (!raw)
> + return -ENOMEM;
> +
> + ret = ir_raw_encode_scancode(scan.rc_proto, scan.scancode,
> + raw, LIRCBUF_SIZE);
> + if (ret < 0)
> + goto out;
> +
> + count = ret;
> +
> + txbuf = kmalloc_array(count, sizeof(unsigned int), GFP_KERNEL);
> + if (!txbuf) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + for (i = 0; i < count; i++)
> + /* Convert from NS to US */
> + txbuf[i] = DIV_ROUND_UP(raw[i].duration, 1000);
> + } else {
> + if (n < sizeof(unsigned int) || n % sizeof(unsigned int))
> + return -EINVAL;
> +
> + count = n / sizeof(unsigned int);
> + if (count > LIRCBUF_SIZE || count % 2 == 0)
> + return -EINVAL;
> +
> + txbuf = memdup_user(buf, n);
> + if (IS_ERR(txbuf))
> + return PTR_ERR(txbuf);
> + }
>
> dev = lirc->dev;
> if (!dev) {
> @@ -156,24 +192,31 @@ static ssize_t ir_lirc_transmit_ir(struct file *file,
> const char __user *buf,
> if (ret < 0)
> goto out;
>
> - for (duration = i = 0; i < ret; i++)
> - duration += txbuf[i];
> -
> - ret *= sizeof(unsigned int);
> -
> - /*
> - * The lircd gap calculation expects the write function to
> - * wait for the actual IR signal to be transmitted before
> - * returning.
> - */
> - towait = ktime_us_delta(ktime_add_us(start, duration), ktime_get());
> - if (towait > 0) {
> - set_current_state(TASK_INTERRUPTIBLE);
> - schedule_timeout(usecs_to_jiffies(towait));
> + if (lirc->send_mode == LIRC_MODE_SCANCODE) {
> + ret = n;
> + } else {
> + for (duration = i = 0; i < ret; i++)
> + duration += txbuf[i];
> +
> +
> + ret *= sizeof(unsigned int);
> +
> + /*
> + * The lircd gap calculation expects the write function to
> + * wait for the actual IR signal to be transmitted before
