Fix rtl28xxu nec/rc5 receiver
Subsystems (29)
Sean Young
maintainercover Fix rtl28xxu nec/rc5 receiver
Since forever rc-core has used the timeout for two separate purposes:
the raw IR timeout and the key up delay. This problem has now shown itself in issues with the rtl28xxu driver, so these two patches split the timeout and then fix the rtl28xxu driver, which is now very responsive.
Sean Young (2):
media: rc-core: split IR timeout into rawir timeout and keyup delay
media: rtl28xxu: improve IR receiver
drivers/hid/hid-picolcd_cir.c | 3 ++- drivers/media/cec/core/cec-core.c | 2 +- drivers/media/cec/platform/seco/seco-cec.c | 2 +- drivers/media/pci/cx88/cx88-input.c | 3 ++- drivers/media/pci/saa7134/saa7134-input.c | 3 ++- drivers/media/pci/smipcie/smipcie-ir.c | 3 ++- drivers/media/rc/ene_ir.c | 11 ++++---- drivers/media/rc/fintek-cir.c | 3 ++- drivers/media/rc/gpio-ir-recv.c | 3 ++- drivers/media/rc/igorplugusb.c | 5 ++-- drivers/media/rc/iguanair.c | 2 +- drivers/media/rc/ir-hix5hd2.c | 3 ++- drivers/media/rc/ir-mce_kbd-decoder.c | 2 +- drivers/media/rc/ir_toy.c | 3 ++- drivers/media/rc/ite-cir.c | 3 ++- drivers/media/rc/lirc_dev.c | 6 ++--- drivers/media/rc/mceusb.c | 9 ++++--- drivers/media/rc/meson-ir.c | 3 ++- drivers/media/rc/mtk-cir.c | 3 ++- drivers/media/rc/nuvoton-cir.c | 3 ++- drivers/media/rc/rc-ir-raw.c | 10 ++++---- drivers/media/rc/rc-loopback.c | 5 ++-- drivers/media/rc/rc-main.c | 8 +++--- drivers/media/rc/redrat3.c | 5 ++-- drivers/media/rc/serial_ir.c | 7 +++--- drivers/media/rc/st_rc.c | 8 ++++-- drivers/media/rc/streamzap.c | 5 ++-- drivers/media/rc/sunxi-cir.c | 7 +++--- drivers/media/rc/ttusbir.c | 3 ++- drivers/media/rc/winbond-cir.c | 3 ++- drivers/media/rc/xbox_remote.c | 2 +- drivers/media/usb/dvb-usb-v2/dvb_usb.h | 3 ++- drivers/media/usb/dvb-usb-v2/dvb_usb_core.c | 3 ++- drivers/media/usb/dvb-usb-v2/rtl28xxu.c | 28 +++++++++++++++++++-- include/media/rc-core.h | 11 +++++--- 35 files changed, 120 insertions(+), 63 deletions(-)
Signature · 1 line
2.34.1
Sean Young
maintainerPATCH 1/2 media: rc-core: split IR timeout into rawir timeout and keyup delay
There are two timeouts relevant for IR processing:
- The rawir_timeout specifies how long an IR space or gap has to be before it is converted to an IR timeout. This signals the end of an message.
- The timeout for a key up event. When an keypress has been decoded a key down event is generated. After some time a key up event is sent, unless the original IR is repeated (if the user holds the button down), and then the key up is event is delayed. This delay should be what possible delay the driver or device has for decoding, for example is the device is polled.
--- drivers/hid/hid-picolcd_cir.c | 3 ++- drivers/media/cec/core/cec-core.c | 2 +- drivers/media/cec/platform/seco/seco-cec.c | 2 +- drivers/media/pci/cx88/cx88-input.c | 3 ++- drivers/media/pci/saa7134/saa7134-input.c | 3 ++- drivers/media/pci/smipcie/smipcie-ir.c | 3 ++- drivers/media/rc/ene_ir.c | 11 ++++++----- drivers/media/rc/fintek-cir.c | 3 ++- drivers/media/rc/gpio-ir-recv.c | 3 ++- drivers/media/rc/igorplugusb.c | 5 +++-- drivers/media/rc/iguanair.c | 2 +- drivers/media/rc/ir-hix5hd2.c | 3 ++- drivers/media/rc/ir-mce_kbd-decoder.c | 2 +- drivers/media/rc/ir_toy.c | 3 ++- drivers/media/rc/ite-cir.c | 3 ++- drivers/media/rc/lirc_dev.c | 6 +++--- drivers/media/rc/mceusb.c | 9 +++++---- drivers/media/rc/meson-ir.c | 3 ++- drivers/media/rc/mtk-cir.c | 3 ++- drivers/media/rc/nuvoton-cir.c | 3 ++- drivers/media/rc/rc-ir-raw.c | 10 +++++----- drivers/media/rc/rc-loopback.c | 5 +++-- drivers/media/rc/rc-main.c | 8 +++++--- drivers/media/rc/redrat3.c | 5 +++-- drivers/media/rc/serial_ir.c | 7 ++++--- drivers/media/rc/st_rc.c | 8 ++++++-- drivers/media/rc/streamzap.c | 5 +++-- drivers/media/rc/sunxi-cir.c | 7 ++++--- drivers/media/rc/ttusbir.c | 3 ++- drivers/media/rc/winbond-cir.c | 3 ++- drivers/media/rc/xbox_remote.c | 2 +- drivers/media/usb/dvb-usb-v2/dvb_usb.h | 3 ++- drivers/media/usb/dvb-usb-v2/dvb_usb_core.c | 3 ++- drivers/media/usb/dvb-usb-v2/rtl28xxu.c | 3 ++- include/media/rc-core.h | 11 +++++++---- 35 files changed, 96 insertions(+), 62 deletions(-)
drivers/hid/hid-picolcd_cir.c +2 -1
@@ -114,7 +114,8 @@ int picolcd_init_cir(struct picolcd_data *data, struct hid_report *report) rdev->dev.parent = &data->hdev->dev; rdev->driver_name =PICOLCD_NAME ; rdev->map_name =RC_MAP_RC6_MCE ; - rdev->timeout =MS_TO_US (100 ); + rdev->rawir_timeout =MS_TO_US (100 ); + rdev->keyup_delay =MS_TO_US (100 ); rdev->rx_resolution =1 ; ret =rc_register_device (rdev);
drivers/media/cec/core/cec-core.c +1 -1
@@ -312,7 +312,7 @@ struct cec_adapter *cec_allocate_adapter(const struct cec_adap_ops *ops, adap->rc->allowed_protocols =RC_PROTO_BIT_CEC ; adap->rc->priv = adap; adap->rc->map_name =RC_MAP_CEC ; - adap->rc->timeout =MS_TO_US (550 ); + adap->rc->keyup_delay =MS_TO_US (550 );#endif return adap; }
drivers/media/cec/platform/seco/seco-cec.c +1 -1
@@ -369,7 +369,7 @@ static int secocec_ir_probe(void *priv) cec->ir->allowed_protocols =RC_PROTO_BIT_RC5 ; cec->ir->priv = cec; cec->ir->map_name =RC_MAP_HAUPPAUGE ; - cec->ir->timeout =MS_TO_US (100 ); + cec->ir->keyup_delay =MS_TO_US (100 );/* Clear the status register */ status =smb_rd16 (SECOCEC_STATUS_REG_1 , &val);
drivers/media/pci/cx88/cx88-input.c +2 -1
@@ -479,7 +479,8 @@ int cx88_ir_init(struct cx88_core *core, struct pci_dev *pci) dev->scancode_mask = hardware_mask;if (ir->sampling) { - dev->timeout =MS_TO_US (10 );/* 10 ms */ + dev->rawir_timeout =MS_TO_US (10 );/* 10 ms */ + dev->keyup_delay =MS_TO_US (10 );/* 10 ms */ }else { dev->driver_type =RC_DRIVER_SCANCODE ; dev->allowed_protocols = rc_proto;
drivers/media/pci/saa7134/saa7134-input.c +2 -1
@@ -812,7 +812,8 @@ int saa7134_input_init1(struct saa7134_dev *dev) rc->map_name = ir_codes; rc->driver_name =MODULE_NAME ; rc->min_timeout =1 ; - rc->timeout =IR_DEFAULT_TIMEOUT ; + rc->rawir_timeout =IR_DEFAULT_TIMEOUT ; + rc->keyup_delay =IR_DEFAULT_TIMEOUT ; rc->max_timeout =10 *IR_DEFAULT_TIMEOUT ; err =rc_register_device (rc);
drivers/media/pci/smipcie/smipcie-ir.c +2 -1
@@ -156,7 +156,8 @@ int smi_ir_init(struct smi_dev *dev) rc_dev->dev.parent = &dev->pci_dev->dev; rc_dev->map_name = dev->info->rc_map; - rc_dev->timeout =SMI_SAMPLE_PERIOD *SMI_SAMPLE_IDLEMIN ; + rc_dev->rawir_timeout =SMI_SAMPLE_PERIOD *SMI_SAMPLE_IDLEMIN ; + rc_dev->keyup_delay =SMI_SAMPLE_PERIOD *SMI_SAMPLE_IDLEMIN ; rc_dev->rx_resolution =SMI_SAMPLE_PERIOD ; ir->rc_dev = rc_dev;
drivers/media/rc/ene_ir.c +6 -5
@@ -454,10 +454,10 @@ static void ene_rx_setup(struct ene_device *dev)if (dev->hw_learning_and_tx_capable) dev->rdev->tx_resolution = sample_period; -if (dev->rdev->timeout > dev->rdev->max_timeout) - dev->rdev->timeout = dev->rdev->max_timeout; -if (dev->rdev->timeout < dev->rdev->min_timeout) - dev->rdev->timeout = dev->rdev->min_timeout; +if (dev->rdev->rawir_timeout > dev->rdev->max_timeout) + dev->rdev->rawir_timeout = dev->rdev->max_timeout; +if (dev->rdev->rawir_timeout < dev->rdev->min_timeout) + dev->rdev->rawir_timeout = dev->rdev->min_timeout; }/* Enable the device for receive */
@@ -818,7 +818,8 @@ static void ene_setup_default_settings(struct ene_device *dev) dev->learning_mode_enabled = learning_mode_force;/* Set reasonable default timeout */ - dev->rdev->timeout =MS_TO_US (150 ); + dev->rdev->rawir_timeout =MS_TO_US (150 ); + dev->rdev->keyup_delay =MS_TO_US (150 ); }/* Upload all hardware settings at once. Used at load and resume time */
drivers/media/rc/fintek-cir.c +2 -1
@@ -524,7 +524,8 @@ static int fintek_probe(struct pnp_dev *pdev, const struct pnp_device_id *dev_id rdev->dev.parent = &pdev->dev; rdev->driver_name =FINTEK_DRIVER_NAME ; rdev->map_name =RC_MAP_RC6_MCE ; - rdev->timeout =1000 ; + rdev->rawir_timeout =1000 ; + rdev->keyup_delay =1000 ;/* rx resolution is hardwired to 50us atm, 1, 25, 100 also possible */ rdev->rx_resolution =CIR_SAMPLE_PERIOD ;
drivers/media/rc/gpio-ir-recv.c +2 -1
@@ -99,7 +99,8 @@ static int gpio_ir_recv_probe(struct platform_device *pdev) rcdev->dev.parent = dev; rcdev->driver_name =KBUILD_MODNAME ; rcdev->min_timeout =1 ; - rcdev->timeout =IR_DEFAULT_TIMEOUT ; + rcdev->rawir_timeout =IR_DEFAULT_TIMEOUT ; + rcdev->keyup_delay =MS_TO_US (40 ); rcdev->max_timeout =10 *IR_DEFAULT_TIMEOUT ; rcdev->allowed_protocols =RC_PROTO_BIT_ALL_IR_DECODER ; rcdev->map_name =of_get_property (np,"linux,rc-map-name" ,NULL );
drivers/media/rc/igorplugusb.c +3 -2
@@ -81,7 +81,7 @@ static void igorplugusb_irdata(struct igorplugusb *ir, unsigned len) }while (i != start);/* add a trailing space */ - rawir.duration = ir->rc->timeout; + rawir.duration = ir->rc->rawir_timeout; rawir.pulse = false;ir_raw_event_store_with_filter (ir->rc, &rawir);
@@ -204,7 +204,8 @@ static int igorplugusb_probe(struct usb_interface *intf, rc->priv = ir; rc->driver_name =DRIVER_NAME ; rc->map_name =RC_MAP_HAUPPAUGE ; - rc->timeout =MS_TO_US (100 ); + rc->rawir_timeout =MS_TO_US (100 ); + rc->keyup_delay =MS_TO_US (100 ); rc->rx_resolution =85 ; ir->rc = rc;
drivers/media/rc/iguanair.c +1 -1
@@ -461,7 +461,7 @@ static int iguanair_probe(struct usb_interface *intf, rc->driver_name =KBUILD_MODNAME ; rc->map_name =RC_MAP_RC6_MCE ; rc->min_timeout =1 ; - rc->timeout =IR_DEFAULT_TIMEOUT ; + rc->rawir_timeout =IR_DEFAULT_TIMEOUT ; rc->max_timeout =10 *IR_DEFAULT_TIMEOUT ; rc->rx_resolution =RX_RESOLUTION ;
drivers/media/rc/ir-hix5hd2.c +2 -1
@@ -310,7 +310,8 @@ static int hix5hd2_ir_probe(struct platform_device *pdev) rdev->input_id.product =0x0001 ; rdev->input_id.version =0x0100 ; rdev->rx_resolution =10 ; - rdev->timeout =IR_CFG_SYMBOL_MAXWIDTH *10 ; + rdev->rawir_timeout =IR_CFG_SYMBOL_MAXWIDTH *10 ; + rdev->keyup_delay =IR_CFG_SYMBOL_MAXWIDTH *10 ; ret =rc_register_device (rdev);if (ret <0 )
drivers/media/rc/ir-mce_kbd-decoder.c +1 -1
@@ -320,7 +320,7 @@ static int ir_mce_kbd_decode(struct rc_dev *dev, struct ir_raw_event ev) data->body);spin_lock (&data->keylock);if (scancode) { - delay =usecs_to_jiffies (dev->timeout) + + delay =usecs_to_jiffies (dev->keyup_delay) +msecs_to_jiffies (100 );mod_timer (&data->rx_timeout, jiffies + delay); }else {
drivers/media/rc/ir_toy.c +2 -1
@@ -489,7 +489,8 @@ static int irtoy_probe(struct usb_interface *intf, rc->allowed_protocols =RC_PROTO_BIT_ALL_IR_DECODER ; rc->map_name =RC_MAP_RC6_MCE ; rc->rx_resolution =UNIT_US ; - rc->timeout =IR_DEFAULT_TIMEOUT ; + rc->rawir_timeout =IR_DEFAULT_TIMEOUT ; + rc->keyup_delay =IR_DEFAULT_TIMEOUT ;/* * end of transmission is detected by absence of a usb packet
drivers/media/rc/ite-cir.c +2 -1
@@ -1377,7 +1377,8 @@ static int ite_probe(struct pnp_dev *pdev, const struct pnp_device_id/* FIFO threshold is 17 bytes, so 17 * 8 samples minimum */ rdev->min_timeout =17 *8 *ITE_BAUDRATE_DIVISOR * sample_period /1000 ; - rdev->timeout =IR_DEFAULT_TIMEOUT ; + rdev->rawir_timeout =IR_DEFAULT_TIMEOUT ; + rdev->keyup_delay =IR_DEFAULT_TIMEOUT ; rdev->max_timeout =10 *IR_DEFAULT_TIMEOUT ; rdev->rx_resolution =ITE_BAUDRATE_DIVISOR * sample_period /1000 ; rdev->tx_resolution =ITE_BAUDRATE_DIVISOR * sample_period /1000 ;
drivers/media/rc/lirc_dev.c +3 -3
@@ -545,15 +545,15 @@ static long lirc_ioctl(struct file *file, unsigned int cmd, unsigned long arg)else if (dev->s_timeout) ret = dev->s_timeout (dev, val);else - dev->timeout = val; + dev->rawir_timeout = val; }break ;case LIRC_GET_REC_TIMEOUT : -if (!dev->timeout) +if (!dev->rawir_timeout) ret = -ENOTTY ;else - val = dev->timeout; + val = dev->rawir_timeout;break ;case LIRC_SET_REC_TIMEOUT_REPORTS :
drivers/media/rc/mceusb.c +5 -4
@@ -1203,7 +1203,7 @@ static void mceusb_handle_command(struct mceusb_dev *ir, u8 *buf_in)switch (subcmd) {/* 2-byte return value commands */ case MCE_RSP_EQIRTIMEOUT : - ir->rc->timeout = (*hi <<8 | *lo) *MCE_TIME_UNIT ; + ir->rc->rawir_timeout = (*hi <<8 | *lo) *MCE_TIME_UNIT ;break ;case MCE_RSP_EQIRNUMPORTS : ir->num_txports = *hi;
@@ -1335,7 +1335,7 @@ static void mceusb_process_ir_data(struct mceusb_dev *ir, int buf_len) }else {struct ir_raw_event ev = { .timeout =1 , - .duration = ir->rc->timeout + .duration = ir->rc->rawir_timeout }; if (ir_raw_event_store_with_filter(ir->rc,
@@ -1615,7 +1615,8 @@ static struct rc_dev *mceusb_init_rc_dev(struct mceusb_dev *ir) rc->allowed_protocols =RC_PROTO_BIT_ALL_IR_DECODER ; rc->rx_resolution =MCE_TIME_UNIT ; rc->min_timeout =MCE_TIME_UNIT ; - rc->timeout =MS_TO_US (100 ); + rc->rawir_timeout =MS_TO_US (100 ); + rc->keyup_delay =MS_TO_US (100 );if (!mceusb_model[ir->model].broken_irtimeout) { rc->s_timeout = mceusb_set_timeout; rc->max_timeout =10 *IR_DEFAULT_TIMEOUT ;
@@ -1624,7 +1625,7 @@ static struct rc_dev *mceusb_init_rc_dev(struct mceusb_dev *ir)* If we can't set the timeout using CMD_SETIRTIMEOUT, we can * rely on software timeouts for timeouts < 100ms. */ - rc->max_timeout = rc->timeout; + rc->max_timeout = rc->rawir_timeout; }if (!ir->flags.no_tx) { rc->s_tx_mask = mceusb_set_tx_mask;
drivers/media/rc/meson-ir.c +2 -1
@@ -133,7 +133,8 @@ static int meson_ir_probe(struct platform_device *pdev) ir->rc->allowed_protocols =RC_PROTO_BIT_ALL_IR_DECODER ; ir->rc->rx_resolution =MESON_TRATE ; ir->rc->min_timeout =1 ; - ir->rc->timeout =IR_DEFAULT_TIMEOUT ; + ir->rc->rawir_timeout =IR_DEFAULT_TIMEOUT ; + ir->rc->keyup_delay =IR_DEFAULT_TIMEOUT ; ir->rc->max_timeout =10 *IR_DEFAULT_TIMEOUT ; ir->rc->driver_name =DRIVER_NAME ;
drivers/media/rc/mtk-cir.c +2 -1
@@ -343,7 +343,8 @@ static int mtk_ir_probe(struct platform_device *pdev) ir->rc->driver_name =MTK_IR_DEV ; ir->rc->allowed_protocols =RC_PROTO_BIT_ALL_IR_DECODER ; ir->rc->rx_resolution =MTK_IR_SAMPLE ; - ir->rc->timeout =MTK_MAX_SAMPLES * (MTK_IR_SAMPLE +1 ); + ir->rc->rawir_timeout =MTK_MAX_SAMPLES * (MTK_IR_SAMPLE +1 ); + ir->rc->keyup_delay =MTK_MAX_SAMPLES * (MTK_IR_SAMPLE +1 ); ret =devm_rc_register_device (dev, ir->rc);if (ret) {
drivers/media/rc/nuvoton-cir.c +2 -1
@@ -998,7 +998,8 @@ static int nvt_probe(struct pnp_dev *pdev, const struct pnp_device_id *dev_id) rdev->input_id.version = nvt->chip_minor; rdev->driver_name =NVT_DRIVER_NAME ; rdev->map_name =RC_MAP_RC6_MCE ; - rdev->timeout =MS_TO_US (100 ); + rdev->rawir_timeout =MS_TO_US (100 ); + rdev->keyup_delay =MS_TO_US (100 );/* rx resolution is hardwired to 50us atm, 1, 25, 100 also possible */ rdev->rx_resolution =CIR_SAMPLE_PERIOD ;#if 0
drivers/media/rc/rc-ir-raw.c +5 -5
@@ -185,8 +185,8 @@ int ir_raw_event_store_with_filter(struct rc_dev *dev, struct ir_raw_event *ev) }/* Enter idle mode if necessary */ -if (!ev->pulse && dev->timeout && - dev->raw->this_ev.duration >= dev->timeout) +if (!ev->pulse && dev->rawir_timeout && + dev->raw->this_ev.duration >= dev->rawir_timeout)ir_raw_event_set_idle (dev, true);return 1 ;
@@ -283,7 +283,7 @@ static int change_protocol(struct rc_dev *dev, u64 *rc_proto)if (dev->s_timeout) dev->s_timeout (dev, timeout);else - dev->timeout = timeout; + dev->rawir_timeout = timeout;return 0 ; }
@@ -559,7 +559,7 @@ static void ir_raw_edge_handle(struct timer_list *t)spin_lock_irqsave (&dev->raw->edge_spinlock, flags); interval =ktime_sub (ktime_get (), dev->raw->last_event); -if (ktime_to_us (interval) >= dev->timeout) { +if (ktime_to_us (interval) >= dev->rawir_timeout) {struct ir_raw_event ev = { .timeout = true, .duration =ktime_to_us (interval)
@@ -568,7 +568,7 @@ static void ir_raw_edge_handle(struct timer_list *t)ir_raw_event_store (dev, &ev); }else {mod_timer (&dev->raw->edge_handle, - jiffies +usecs_to_jiffies (dev->timeout - + jiffies +usecs_to_jiffies (dev->rawir_timeout -ktime_to_us (interval))); }spin_unlock_irqrestore (&dev->raw->edge_spinlock, flags);
drivers/media/rc/rc-loopback.c +3 -2
@@ -129,7 +129,7 @@ static int loop_tx_ir(struct rc_dev *dev, unsigned *txbuf, unsigned count)/* Fake a silence long enough to cause us to go idle */ rawir.pulse = false; - rawir.duration = dev->timeout; + rawir.duration = dev->rawir_timeout;ir_raw_event_store_with_filter (dev, &rawir);ir_raw_event_handle (dev);
@@ -226,7 +226,8 @@ static int __init loop_init(void) rc->allowed_protocols =RC_PROTO_BIT_ALL_IR_DECODER ; rc->allowed_wakeup_protocols =RC_PROTO_BIT_ALL_IR_ENCODER ; rc->encode_wakeup = true; - rc->timeout =IR_DEFAULT_TIMEOUT ; + rc->rawir_timeout =IR_DEFAULT_TIMEOUT ; + rc->keyup_delay =IR_DEFAULT_TIMEOUT ; rc->min_timeout =1 ; rc->max_timeout =IR_MAX_TIMEOUT ; rc->rx_resolution =1 ;
drivers/media/rc/rc-main.c +5 -3
@@ -737,7 +737,7 @@ static unsigned int repeat_period(int protocol)void rc_repeat (struct rc_dev *dev) {unsigned long flags; -unsigned int timeout =usecs_to_jiffies (dev->timeout) + +unsigned int timeout =usecs_to_jiffies (dev->keyup_delay) +msecs_to_jiffies (repeat_period (dev->last_protocol));struct lirc_scancode sc = { .scancode = dev->last_scancode, .rc_proto = dev->last_protocol,
@@ -855,7 +855,8 @@ void rc_keydown(struct rc_dev *dev, enum rc_proto protocol, u64 scancode,ir_do_keydown (dev, protocol, scancode, keycode, toggle);if (dev->keypressed) { - dev->keyup_jiffies = jiffies +usecs_to_jiffies (dev->timeout) + + dev->keyup_jiffies = jiffies + +usecs_to_jiffies (dev->keyup_delay) +msecs_to_jiffies (repeat_period (protocol));mod_timer (&dev->timer_keyup, dev->keyup_jiffies); }
@@ -1715,7 +1716,8 @@ struct rc_dev *rc_allocate_device(enum rc_driver_type type) dev->input_dev->setkeycode = ir_setkeycode;input_set_drvdata (dev->input_dev, dev); - dev->timeout =IR_DEFAULT_TIMEOUT ; + dev->rawir_timeout =IR_DEFAULT_TIMEOUT ; + dev->keyup_delay =IR_DEFAULT_TIMEOUT ;timer_setup (&dev->timer_keyup, ir_timer_keyup,0 );timer_setup (&dev->timer_repeat, ir_timer_repeat,0 );
drivers/media/rc/redrat3.c +3 -2
@@ -381,7 +381,7 @@ static void redrat3_process_ir_data(struct redrat3_dev *rr3)/* add a trailing space */ rawir.pulse = false; rawir.timeout = true; - rawir.duration = rr3->rc->timeout; + rawir.duration = rr3->rc->rawir_timeout;dev_dbg (dev,"storing trailing timeout with duration %d\n" , rawir.duration);ir_raw_event_store_with_filter (rr3->rc, &rawir);
@@ -948,7 +948,8 @@ static struct rc_dev *redrat3_init_rc_dev(struct redrat3_dev *rr3) rc->allowed_protocols =RC_PROTO_BIT_ALL_IR_DECODER ; rc->min_timeout =MS_TO_US (RR3_RX_MIN_TIMEOUT ); rc->max_timeout =MS_TO_US (RR3_RX_MAX_TIMEOUT ); - rc->timeout =redrat3_get_timeout (rr3); + rc->rawir_timeout =redrat3_get_timeout (rr3); + rc->keyup_delay = rc->rawir_timeout; rc->s_timeout = redrat3_set_timeout; rc->tx_ir = redrat3_transmit_ir; rc->s_tx_carrier = redrat3_set_tx_carrier;
drivers/media/rc/serial_ir.c +4 -3
@@ -385,7 +385,7 @@ static irqreturn_t serial_ir_irq_handler(int i, void *blah) }while (!(sinp (UART_IIR ) &UART_IIR_NO_INT ));/* still pending ? */ mod_timer (&serial_ir.timeout_timer, - jiffies +usecs_to_jiffies (serial_ir.rcdev->timeout)); + jiffies +usecs_to_jiffies (serial_ir.rcdev->rawir_timeout));ir_raw_event_handle (serial_ir.rcdev);
@@ -466,7 +466,7 @@ static void serial_ir_timeout(struct timer_list *unused) {struct ir_raw_event ev = { .timeout = true, - .duration = serial_ir.rcdev->timeout + .duration = serial_ir.rcdev->rawir_timeout };ir_raw_event_store_with_filter (serial_ir.rcdev, &ev);ir_raw_event_handle (serial_ir.rcdev);
@@ -526,7 +526,8 @@ static int serial_ir_probe(struct platform_device *dev) rcdev->driver_name =KBUILD_MODNAME ; rcdev->map_name =RC_MAP_RC6_MCE ; rcdev->min_timeout =1 ; - rcdev->timeout =IR_DEFAULT_TIMEOUT ; + rcdev->rawir_timeout =IR_DEFAULT_TIMEOUT ; + rcdev->keyup_delay =IR_DEFAULT_TIMEOUT ; rcdev->max_timeout =10 *IR_DEFAULT_TIMEOUT ; rcdev->rx_resolution =250 ;
drivers/media/rc/st_rc.c +6 -2
@@ -63,7 +63,10 @@ struct st_rc_device {static void st_rc_send_lirc_timeout (struct rc_dev *rdev) { -struct ir_raw_event ev = { .timeout = true, .duration = rdev->timeout }; +struct ir_raw_event ev = { + .timeout = true, + .duration = rdev->rawir_timeout + };ir_raw_event_store (rdev, &ev); }
@@ -299,7 +302,8 @@ static int st_rc_probe(struct platform_device *pdev) rdev->allowed_protocols =RC_PROTO_BIT_ALL_IR_DECODER ;/* rx sampling rate is 10Mhz */ rdev->rx_resolution =100 ; - rdev->timeout =MAX_SYMB_TIME ; + rdev->rawir_timeout =MAX_SYMB_TIME ; + rdev->keyup_delay =MAX_SYMB_TIME ; rdev->priv = rc_dev; rdev->open = st_rc_open; rdev->close = st_rc_close;
drivers/media/rc/streamzap.c +3 -2
@@ -198,7 +198,7 @@ static void streamzap_callback(struct urb *urb)if (sz->buf_in[i] ==SZ_TIMEOUT ) {struct ir_raw_event rawir = { .pulse = false, - .duration = sz->rdev->timeout + .duration = sz->rdev->rawir_timeout };sz_push (sz, rawir); }else {
@@ -334,7 +334,8 @@ static int streamzap_probe(struct usb_interface *intf, sz->decoder_state = PulseSpace;/* FIXME: don't yet have a way to set this */ - sz->rdev->timeout =SZ_TIMEOUT *SZ_RESOLUTION ; + sz->rdev->rawir_timeout =SZ_TIMEOUT *SZ_RESOLUTION ; + sz->rdev->keyup_delay =SZ_TIMEOUT *SZ_RESOLUTION ;#if 0 /* not yet supported, depends on patches from maxim */ /* see also: LIRC_GET_REC_RESOLUTION and LIRC_SET_REC_TIMEOUT */
drivers/media/rc/sunxi-cir.c +4 -3
@@ -164,7 +164,7 @@ static int sunxi_ir_set_timeout(struct rc_dev *rc_dev, unsigned int timeout)writel (REG_CIR_NTHR (SUNXI_IR_RXNOISE ) |REG_CIR_ITHR (ithr), ir->base +SUNXI_IR_CIR_REG ); - rc_dev->timeout =sunxi_ithr_to_usec (base_clk, ithr); + rc_dev->rawir_timeout =sunxi_ithr_to_usec (base_clk, ithr);return 0 ; }
@@ -195,7 +195,7 @@ static int sunxi_ir_hw_init(struct device *dev)writel (REG_CTL_MD , ir->base +SUNXI_IR_CTL_REG );/* Set noise threshold and idle threshold */ -sunxi_ir_set_timeout (ir->rc, ir->rc->timeout); +sunxi_ir_set_timeout (ir->rc, ir->rc->rawir_timeout);/* Invert Input Signal */ writel (REG_RXCTL_RPPI , ir->base +SUNXI_IR_RXCTL_REG );
@@ -324,7 +324,8 @@ static int sunxi_ir_probe(struct platform_device *pdev) ir->rc->allowed_protocols =RC_PROTO_BIT_ALL_IR_DECODER ;/* Frequency after IR internal divider with sample period in us */ ir->rc->rx_resolution = (USEC_PER_SEC / (b_clk_freq /64 )); - ir->rc->timeout =IR_DEFAULT_TIMEOUT ; + ir->rc->rawir_timeout =IR_DEFAULT_TIMEOUT ; + ir->rc->keyup_delay =IR_DEFAULT_TIMEOUT ; ir->rc->min_timeout =sunxi_ithr_to_usec (b_clk_freq,0 ); ir->rc->max_timeout =sunxi_ithr_to_usec (b_clk_freq,255 ); ir->rc->s_timeout = sunxi_ir_set_timeout;
drivers/media/rc/ttusbir.c +2 -1
@@ -307,7 +307,8 @@ static int ttusbir_probe(struct usb_interface *intf, rc->driver_name =DRIVER_NAME ; rc->map_name =RC_MAP_TT_1500 ; rc->min_timeout =1 ; - rc->timeout =IR_DEFAULT_TIMEOUT ; + rc->rawir_timeout =IR_DEFAULT_TIMEOUT ; + rc->keyup_delay =IR_DEFAULT_TIMEOUT ; rc->max_timeout =10 *IR_DEFAULT_TIMEOUT ;/*
drivers/media/rc/winbond-cir.c +2 -1
@@ -1071,7 +1071,8 @@ wbcir_probe(struct pnp_dev *device, const struct pnp_device_id *dev_id) data->dev->priv = data; data->dev->dev.parent = &device->dev; data->dev->min_timeout =1 ; - data->dev->timeout =IR_DEFAULT_TIMEOUT ; + data->dev->rawir_timeout =IR_DEFAULT_TIMEOUT ; + data->dev->keyup_delay =IR_DEFAULT_TIMEOUT ; data->dev->max_timeout =10 *IR_DEFAULT_TIMEOUT ; data->dev->rx_resolution =2 ; data->dev->allowed_protocols =RC_PROTO_BIT_ALL_IR_DECODER ;
drivers/media/rc/xbox_remote.c +1 -1
@@ -157,7 +157,7 @@ static void xbox_remote_rc_init(struct xbox_remote *xbox_remote) rdev->device_name = xbox_remote->rc_name; rdev->input_phys = xbox_remote->rc_phys; - rdev->timeout =MS_TO_US (10 ); + rdev->keyup_delay =MS_TO_US (10 );usb_to_input_id (xbox_remote->udev, &rdev->input_id); rdev->dev.parent = &xbox_remote->interface->dev;
drivers/media/usb/dvb-usb-v2/dvb_usb.h +2 -1
@@ -132,7 +132,8 @@ struct dvb_usb_rc {unsigned int interval;enum rc_driver_type driver_type;bool bulk_mode; -int timeout; +u32 rawir_timeout; +u32 keyup_delay; };/**
drivers/media/usb/dvb-usb-v2/dvb_usb_core.c +2 -1
@@ -150,7 +150,8 @@ static int dvb_usbv2_remote_init(struct dvb_usb_device *d) dev->map_name = d->rc.map_name; dev->allowed_protocols = d->rc.allowed_protos; dev->change_protocol = d->rc.change_protocol; - dev->timeout = d->rc.timeout; + dev->rawir_timeout = d->rc.rawir_timeout; + dev->keyup_delay = d->rc.keyup_delay; dev->priv = d; ret =rc_register_device (dev);
drivers/media/usb/dvb-usb-v2/rtl28xxu.c +2 -1
@@ -1809,7 +1809,8 @@ static int rtl2832u_get_rc_config(struct dvb_usb_device *d, rc->query = rtl2832u_rc_query; rc->interval =200 ;/* we program idle len to 0xc0, set timeout to one less */ - rc->timeout =0xbf *51 ; + rc->rawir_timeout =0xbf *51 ; + rc->keyup_delay =MS_TO_US (210 );return 0 ; }
include/media/rc-core.h +7 -4
@@ -123,9 +123,11 @@ struct lirc_fh {* @last_protocol: protocol of last keypress * @last_scancode: scancode of last keypress * @last_toggle: toggle value of last command -* @timeout: optional time after which device stops sending data -* @min_timeout: minimum timeout supported by device -* @max_timeout: maximum timeout supported by device +* @rawir_timeout: length space to convert into IR timeout +* @keyup_delay: timeout before keyup event. This should be the maximum +* delay between reading IR events. +* @min_timeout: minimum rawir timeout supported by device +* @max_timeout: maximum rawir timeout supported by device * @rx_resolution : resolution (in us) of input sampler * @tx_resolution: resolution (in us) of output sampler * @lirc_dev: lirc device
@@ -190,7 +192,8 @@ struct rc_dev {enum rc_proto last_protocol;u64 last_scancode;u8 last_toggle; -u32 timeout; +u32 rawir_timeout; +u32 keyup_delay;u32 min_timeout;u32 max_timeout;u32 rx_resolution;
Signature · 1 line
2.34.1
Sean Young
maintainerPATCH 2/2 media: rtl28xxu: improve IR receiver
In reply to Sean YoungThis device presents an IR buffer, which can be read and cleared. Clearing the buffer is racey with receiving IR, so wait until the IR message is finished before clearing it.
This should minimize the chance of the buffer clear happening while IR is being received, although we cannot avoid this completely.
--- drivers/media/usb/dvb-usb-v2/rtl28xxu.c | 27 +++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-)
drivers/media/usb/dvb-usb-v2/rtl28xxu.c +25 -2
@@ -1720,6 +1720,7 @@ static int rtl2832u_rc_query(struct dvb_usb_device *d) {IR_RX_BUF_CTRL, 0x80, 0xff}, {IR_RX_CTRL, 0x80, 0xff}, }; +u32 idle_length;/* init remote controller */ if (!dev->rc_active) {
@@ -1770,6 +1771,28 @@ static int rtl2832u_rc_query(struct dvb_usb_device *d)if (ret) goto err; +dev_dbg (&d->intf->dev,"IR_RX_BUF=%*ph\n" , len, buf); + +/* if the receiver is not idle yet, do not process */ + idle_length =0 ; +if (len >2 ) { +if (!(buf[len -1 ] &0x80 )) + idle_length += buf[len -1 ]; +if (!(buf[len -2 ] &0x80 )) + idle_length += buf[len -2 ]; + } + +if (idle_length <0xbf ) { +/* +* If the IR does not end with a space equal to the idle +* length, then the IR is not complete yet and more is to +* arrive shortly. If we process it and flush the buffer now, +* we end up missing IR. +*/ +dev_dbg (&d->intf->dev,"ignoring idle=%x\n" , idle_length); +return 0 ; + } +/* let hw receive new code */ for (i =0 ; i <ARRAY_SIZE (refresh_tab); i++) { ret = rtl28xxu_wr_reg_mask(d, refresh_tab[i].reg,
@@ -1807,10 +1830,10 @@ static int rtl2832u_get_rc_config(struct dvb_usb_device *d, rc->allowed_protos =RC_PROTO_BIT_ALL_IR_DECODER ; rc->driver_type =RC_DRIVER_IR_RAW ; rc->query = rtl2832u_rc_query; - rc->interval =200 ; + rc->interval =100 ;/* we program idle len to 0xc0, set timeout to one less */ rc->rawir_timeout =0xbf *51 ; - rc->keyup_delay =MS_TO_US (210 ); + rc->keyup_delay =MS_TO_US (110 );return 0 ; }
Signature · 1 line
2.34.1
Re: [PATCH 2/2] media: rtl28xxu: improve IR receiver
Hi Sean,
I finally took the time to get a deeper understanding of the infrared remote control subsystem. I think that I now understand the translation into key-down, key-up, and key-repeat events. For the RC5 protocol, rc_repeat() will not be called by ir-rc5-decoder.c but instead, ir_do_keydown() will handle the repeat. For lirc_scancode_event() it will never set the LIRC_SCANCODE_FLAG_REPEAT bit, even if !new_event and the protocol does support the toggle bit. That might qualify as a bug.
Quoted 6 lines · Sean Young
This device presents an IR buffer, which can be read and cleared. Clearing the buffer is racey with receiving IR, so wait until the IR message is finished before clearing it.
This should minimize the chance of the buffer clear happening while IR is being received, although we cannot avoid this completely.
I just realized that this limitation of the interface may be causing exactly what I was observing when I was testing this. If a constant stream of data is being received because a button is being held down, a buffer overflow or wrap-around glitch is inevitable, maybe expect if the wrap-around occurs exactly at the 128-byte boundary.
How about the following improvement? If IR_RX_BC is a simple cursor to the 128-byte IR_RX_BUF, then rtl2832u_rc_query() could avoid sending refresh_tab[] but simply remember where the previous call left off. We could always read the 128 bytes at IR_RX_BUF, and process everything between the previous position reported by IR_RX_BC and the current position reported by IR_RX_BC, and treat buf[] as a ring buffer.
Last time I tested it, the patch was a significant improvement. I think that "perfect" is the enemy of "good enough", and the patch should be included in the kernel.
Best regards,
Marko
Re: [PATCH 2/2] media: rtl28xxu: improve IR receiver
Quoted 1 line · Marko Mäkelä
How about the following improvement? If
IR_RX_BCis a simple cursor to the 128-byteIR_RX_BUF, thenrtl2832u_rc_query()could avoid sending refresh_tab[] but simply remember where the previous call left off. We could always read the 128 bytes atIR_RX_BUF, and process everything between the previous position reported byIR_RX_BCand the current position reported byIR_RX_BC, and treat buf[] as a ring buffer.
I experimented with this on the 5.19.0-rc3 kernel. With the attached patch applied on top of this patch series, "ir-keytables -t" reported only one RC5 encoded key-down event. I had to unplug and plug in the adapter in order to receive another RC5 event. The refresh command seems to be necessary for the device to store and forward further IR data.
Quoted 1 line
Last time I tested it, the patch was a significant improvement. I think that "perfect" is the enemy of "good enough", and the patch should be included in the kernel.
The remaining problem definitely is a limitation of the interface. There is little that can be done to work around it.
Marko
drivers/media/usb/dvb-usb-v2/rtl28xxu.c +8 -40
@@ -1711,16 +1711,10 @@ static int rtl2831u_get_rc_config(struct dvb_usb_device *d,static int rtl2832u_rc_query (struct dvb_usb_device *d) { -int ret, i, len; +int ret, i, end;struct rtl28xxu_dev *dev = d->priv;struct ir_raw_event ev = {};u8 buf[128 ]; -static const struct rtl28xxu_reg_val_mask refresh_tab[] = { - {IR_RX_IF ,0x03 ,0xff }, - {IR_RX_BUF_CTRL ,0x80 ,0xff }, - {IR_RX_CTRL ,0x80 ,0xff }, - }; -u32 idle_length;/* init remote controller */ if (!dev->rc_active) {
@@ -1761,48 +1755,22 @@ static int rtl2832u_rc_query(struct dvb_usb_device *d) goto exit; ret =rtl28xxu_rd_reg (d,IR_RX_BC , &buf[0 ]); -if (ret || buf[0 ] >sizeof (buf)) +if (ret) goto err; - len = buf[0 ]; + i = dev->rc_bc; + end = dev->rc_bc = buf[0 ] &0x7f ;/* read raw code from hw */ - ret =rtl28xxu_rd_regs (d,IR_RX_BUF , buf, len); + ret =rtl28xxu_rd_regs (d,IR_RX_BUF , buf,sizeof buf);if (ret) goto err; -dev_dbg (&d->intf->dev,"IR_RX_BUF=%*ph\n" , len, buf); - -/* if the receiver is not idle yet, do not process */ - idle_length =0 ; -if (len >2 ) { -if (!(buf[len -1 ] &0x80 )) - idle_length += buf[len -1 ]; -if (!(buf[len -2 ] &0x80 )) - idle_length += buf[len -2 ]; - } - -if (idle_length <0xbf ) { -/* -* If the IR does not end with a space equal to the idle -* length, then the IR is not complete yet and more is to -* arrive shortly. If we process it and flush the buffer now, -* we end up missing IR. -*/ -dev_dbg (&d->intf->dev,"ignoring idle=%x\n" , idle_length); -return 0 ; - } - -/* let hw receive new code */ -for (i =0 ; i <ARRAY_SIZE (refresh_tab); i++) { - ret =rtl28xxu_wr_reg_mask (d, refresh_tab[i].reg, - refresh_tab[i].val, refresh_tab[i].mask); -if (ret) - goto err; - } +dev_dbg (&d->intf->dev,"IR_RX_BUF=%d,%*ph\n" , end, + (int )sizeof buf, buf);/* pass data to Kernel IR decoder */ -for (i =0 ; i < len; i++) { +for (; i != end; i++, i &=0x7f ) { ev.pulse = buf[i] >>7 ; ev.duration =51 * (buf[i] &0x7f );ir_raw_event_store_with_filter (d->rc_dev, &ev);
drivers/media/usb/dvb-usb-v2/rtl28xxu.h +1 -0
@@ -64,6 +64,7 @@ struct rtl28xxu_dev {u8 tuner;char *tuner_name;u8 page;/* integrated demod active register page */ +u8 rc_bc;struct i2c_adapter *demod_i2c_adapter;bool rc_active;bool new_i2c_write;
Sean Young
maintainerRe: [PATCH 2/2] media: rtl28xxu: improve IR receiver
Quoted 14 lines · Marko Mäkelä
Quoted 6 lines · Marko Mäkelä
How about the following improvement? If
IR_RX_BCis a simple cursor to the 128-byteIR_RX_BUF, thenrtl2832u_rc_query()could avoid sending refresh_tab[] but simply remember where the previous call left off. We could always read the 128 bytes atIR_RX_BUF, and process everything between the previous position reported byIR_RX_BCand the current position reported byIR_RX_BC, and treat buf[] as a ring buffer.I experimented with this on the 5.19.0-rc3 kernel. With the attached patch applied on top of this patch series, "ir-keytables -t" reported only one RC5 encoded key-down event. I had to unplug and plug in the adapter in order to receive another RC5 event. The refresh command seems to be necessary for the device to store and forward further IR data.
Quoted 6 lines
Quoted 3 lines
Last time I tested it, the patch was a significant improvement. I think that "perfect" is the enemy of "good enough", and the patch should be included in the kernel.
The remaining problem definitely is a limitation of the interface. There is little that can be done to work around it.
This particular device is really badly designed. Unless we come up with a better solution to work around its limitations, it's just broken.
Sean
Sean Young
maintainerRe: [PATCH 2/2] media: rtl28xxu: improve IR receiver
In reply to Marko MäkeläHi Marko,
Quoted 7 lines · Marko Mäkelä
I finally took the time to get a deeper understanding of the infrared remote control subsystem. I think that I now understand the translation into key-down, key-up, and key-repeat events. For the RC5 protocol,
rc_repeat()will not be called byir-rc5-decoder.cbut instead,ir_do_keydown()will handle the repeat. Forlirc_scancode_event()it will never set theLIRC_SCANCODE_FLAG_REPEATbit, even if !new_event and the protocol does support the toggle bit. That might qualify as a bug.
You are right, this was missed. Patches welcome.
Quoted 14 lines
Quoted 6 lines · Sean Young
This device presents an IR buffer, which can be read and cleared. Clearing the buffer is racey with receiving IR, so wait until the IR message is finished before clearing it.
This should minimize the chance of the buffer clear happening while IR is being received, although we cannot avoid this completely.
I just realized that this limitation of the interface may be causing exactly what I was observing when I was testing this. If a constant stream of data is being received because a button is being held down, a buffer overflow or wrap-around glitch is inevitable, maybe expect if the wrap-around occurs exactly at the 128-byte boundary.
Quoted 6 lines
How about the following improvement? If
IR_RX_BCis a simple cursor to the 128-byteIR_RX_BUF, thenrtl2832u_rc_query()could avoid sending refresh_tab[] but simply remember where the previous call left off. We could always read the 128 bytes atIR_RX_BUF, and process everything between the previous position reported byIR_RX_BCand the current position reported byIR_RX_BC, and treat buf[] as a ring buffer.
This is a great idea. Very original.
Quoted 3 lines
Last time I tested it, the patch was a significant improvement. I think that "perfect" is the enemy of "good enough", and the patch should be included in the kernel.
The idea sounds really good. I'll review/test the patch and get back to you.
Thanks,
Sean
Re: [PATCH 2/2] media: rtl28xxu: improve IR receiver
Hi Sean,
Quoted 11 lines · Sean Young
Hi Marko,
Quoted 7 lines · Marko Mäkelä
I finally took the time to get a deeper understanding of the infrared remote control subsystem. I think that I now understand the translation into key-down, key-up, and key-repeat events. For the RC5 protocol,
rc_repeat()will not be called byir-rc5-decoder.cbut instead,ir_do_keydown()will handle the repeat. Forlirc_scancode_event()it will never set theLIRC_SCANCODE_FLAG_REPEATbit, even if !new_event and the protocol does support the toggle bit. That might qualify as a bug.
Quoted 1 line
You are right, this was missed. Patches welcome.
Attached (for 5.19.0-rc3, on top of the two commits of this patch series).
I thought that it would be the least amount of trouble to slightly change the interpretation of the "toggle" parameter of rc_keydown(). My intention was to use the values 1 and 2 when the toggle flag is present. Any nonzero values would work.
I am not that familiar with updating the modules, and I suspect that instead of actually testing this code, I was testing the "ring buffer" patch that I posted yesterday. I could not use the rmmod/insmod approach for testing this change, because the rc_core module was in use by the display driver. So, I can only say that the patch compiled for me. A few FIXME places are indicated where I am not sure that a correct (nonzero) toggle value would be computed.
An alternative approach would be to use the value toggle=1 for the case when the toggle bit is set, and toggle>1 for the case when it is not set. Basically, change things like 1+!!x to 1+!x in the callers, and change the condition toggle > 1 to toggle == 1 in rc-main.c. In that way, any old driver that would use the toggle values 0 and 1 would still generate LIRC_SCANCODE_FLAG_TOGGLE. But then, the repeat_event logic would only work half the time (when toggle!=0).
Marko
From 29a5c2a00653f49c854109b2f2c8f99b4431430f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marko=20M=C3=A4kel=C3=A4?= <[email protected]>
Date: Tue, 28 Jun 2022 07:59:17 +0300
Subject: [PATCH] rc_keydown(): Report LIRC_SCANCODE_FLAG_REPEAT based on
toggle bit
Drivers that will not call rc_repeat() will let rc_keydown() create repeat events. For the LIRC interface, the repeat flag would only be set by rc_repeat(), never by rc_keydown().
We change the meaning of the toggle parameter: Drivers that invoke rc_repeat() by themselves should always pass toggle=0, while protocols that include a toggle bit should pass toggle>0, with the value 1 meaning that the toggle bit is clear.
This is largely untested code. See FIXME comments. Also, an interface change for bpf_rc_keydown() might have to be documented.
--- drivers/media/cec/platform/seco/seco-cec.c | 3 +- drivers/media/i2c/ir-kbd-i2c.c | 4 +- drivers/media/pci/bt8xx/bttv-input.c | 2 +- drivers/media/pci/ttpci/budget-ci.c | 4 +- drivers/media/rc/bpf-lirc.c | 2 +- drivers/media/rc/img-ir/img-ir-rc5.c | 2 +- drivers/media/rc/img-ir/img-ir-rc6.c | 2 +- drivers/media/rc/imon.c | 2 +- drivers/media/rc/ir-jvc-decoder.c | 3 +- drivers/media/rc/ir-rc5-decoder.c | 2 +- drivers/media/rc/ir-rc6-decoder.c | 4 +- drivers/media/rc/ir-rcmm-decoder.c | 2 +- drivers/media/rc/rc-main.c | 9 ++-- drivers/media/usb/dvb-usb-v2/az6007.c | 2 +- drivers/media/usb/dvb-usb-v2/dvbsky.c | 2 +- drivers/media/usb/dvb-usb-v2/rtl28xxu.c | 48 ++++----------------- drivers/media/usb/dvb-usb-v2/rtl28xxu.h | 1 + drivers/media/usb/dvb-usb/dib0700_core.c | 2 +- drivers/media/usb/dvb-usb/dib0700_devices.c | 2 +- drivers/media/usb/dvb-usb/ttusb2.c | 3 +- drivers/media/usb/em28xx/em28xx-input.c | 4 +- drivers/staging/media/av7110/av7110_ir.c | 2 +- 22 files changed, 40 insertions(+), 67 deletions(-)
drivers/media/cec/platform/seco/seco-cec.c +2 -1
@@ -416,7 +416,8 @@ static int secocec_ir_rx(struct secocec_data *priv) addr = (val &SECOCEC_IR_ADDRESS_MASK ) >>SECOCEC_IR_ADDRESS_SHL ; toggle = (val &SECOCEC_IR_TOGGLE_MASK ) >>SECOCEC_IR_TOGGLE_SHL ; -rc_keydown (cec->ir,RC_PROTO_RC5 ,RC_SCANCODE_RC5 (addr, key), toggle); +rc_keydown (cec->ir,RC_PROTO_RC5 ,RC_SCANCODE_RC5 (addr, key), +1 + toggle);dev_dbg (dev,"IR key pressed: 0x%02x addr 0x%02x toggle 0x%02x\n" , key, addr, toggle);
drivers/media/i2c/ir-kbd-i2c.c +2 -2
@@ -99,7 +99,7 @@ static int get_key_haup_common(struct IR_i2c *ir, enum rc_proto *protocol, *protocol =RC_PROTO_RC5 ; *scancode =RC_SCANCODE_RC5 (dev, code); - *ptoggle = toggle; + *ptoggle = toggle +1 ;return 1 ; }else if (size ==6 && (buf[0 ] &0x40 )) {
@@ -108,7 +108,7 @@ static int get_key_haup_common(struct IR_i2c *ir, enum rc_proto *protocol, vendor =get_unaligned_be16 (buf +1 );if (vendor ==0x800f ) { - *ptoggle = (dev &0x80 ) !=0 ; + *ptoggle =1 + ((dev &0x80 ) !=0 ); *protocol =RC_PROTO_RC6_MCE ; dev &=0x7f ; dev_dbg(&ir->rc->dev,
drivers/media/pci/bt8xx/bttv-input.c +1 -1
@@ -228,7 +228,7 @@ static void bttv_rc5_timer_end(struct timer_list *t) } scancode =RC_SCANCODE_RC5 (system, command); -rc_keydown (ir->dev,RC_PROTO_RC5 , scancode, toggle); +rc_keydown (ir->dev,RC_PROTO_RC5 , scancode, toggle +1 );dprintk ("scancode %x, toggle %x\n" , scancode, toggle); }
drivers/media/pci/ttpci/budget-ci.c +2 -2
@@ -147,13 +147,13 @@ static void msp430_ir_interrupt(struct tasklet_struct *t)if (budget_ci->ir.full_rc5) {rc_keydown (dev,RC_PROTO_RC5 ,RC_SCANCODE_RC5 (budget_ci->ir.rc5_device, budget_ci->ir.ir_key), - !!(command &0x20 )); +1 + !!(command &0x20 ));return ; }/* FIXME: We should generate complete scancodes for all devices */ rc_keydown (dev,RC_PROTO_UNKNOWN , budget_ci->ir.ir_key, - !!(command &0x20 )); +1 + !!(command &0x20 )); }static int msp430_ir_init (struct budget_ci *budget_ci)
drivers/media/rc/bpf-lirc.c +1 -1
@@ -42,7 +42,7 @@ BPF_CALL_4(bpf_rc_keydown, u32*, sample, u32, protocol, u64, scancode, ctrl = container_of(sample, struct ir_raw_event_ctrl, bpf_sample); -rc_keydown (ctrl->dev, protocol, scancode, toggle !=0 ); +rc_keydown (ctrl->dev, protocol, scancode, (u8 ) toggle);return 0 ; }
drivers/media/rc/img-ir/img-ir-rc5.c +1 -1
@@ -31,7 +31,7 @@ static int img_ir_rc5_scancode(int len, u64 raw, u64 enabled_protocols, request->protocol =RC_PROTO_RC5 ; request->scancode = addr <<8 | cmd; - request->toggle = tgl; + request->toggle =1 + tgl;return IMG_IR_SCANCODE ; }
drivers/media/rc/img-ir/img-ir-rc6.c +1 -1
@@ -52,7 +52,7 @@ static int img_ir_rc6_scancode(int len, u64 raw, u64 enabled_protocols, request->protocol =RC_PROTO_RC6_0 ; request->scancode = addr <<8 | cmd; - request->toggle = trl2; + request->toggle =1 + trl2;return IMG_IR_SCANCODE ; }
drivers/media/rc/imon.c +1 -1
@@ -1552,7 +1552,7 @@ static int imon_parse_press_type(struct imon_context *ictx,/* mce-specific button handling, no keyup events */ else if (ktype == IMON_KEY_MCE) { - ictx->rc_toggle = buf[2 ]; + ictx->rc_toggle =1 + buf[2 ];/* FIXME: test this */ press_type =1 ;/* incoherent or irrelevant data */
drivers/media/rc/ir-jvc-decoder.c +2 -1
@@ -129,7 +129,8 @@ static int ir_jvc_decode(struct rc_dev *dev, struct ir_raw_event ev) scancode = (bitrev8 ((data->bits >>8 ) &0xff ) <<8 ) | (bitrev8 ((data->bits >>0 ) &0xff ) <<0 );dev_dbg (&dev->dev,"JVC scancode 0x%04x\n" , scancode); -rc_keydown (dev,RC_PROTO_JVC , scancode, data->toggle); +rc_keydown (dev,RC_PROTO_JVC , scancode, +1 + data->toggle); data->first = false; data->old_bits = data->bits; }else if (data->bits == data->old_bits) {
drivers/media/rc/ir-rc5-decoder.c +1 -1
@@ -157,7 +157,7 @@ static int ir_rc5_decode(struct rc_dev *dev, struct ir_raw_event ev)dev_dbg (&dev->dev,"RC5(x/sz) scancode 0x%06x (p: %u, t: %u)\n" , scancode, protocol, toggle); -rc_keydown (dev, protocol, scancode, toggle); +rc_keydown (dev, protocol, scancode,1 + toggle); data->state =STATE_INACTIVE ;return 0 ; }
drivers/media/rc/ir-rc6-decoder.c +2 -2
@@ -212,7 +212,7 @@ static int ir_rc6_decode(struct rc_dev *dev, struct ir_raw_event ev)switch (rc6_mode (data)) {case RC6_MODE_0 : scancode = data->body; - toggle = data->toggle; + toggle =1 + data->toggle; protocol =RC_PROTO_RC6_0 ;dev_dbg (&dev->dev,"RC6(0) scancode 0x%04x (toggle: %u)\n" , scancode, toggle);
@@ -241,7 +241,7 @@ static int ir_rc6_decode(struct rc_dev *dev, struct ir_raw_event ev)case RC6_6A_KATHREIN_CC :case RC6_6A_ZOTAC_CC : protocol =RC_PROTO_RC6_MCE ; - toggle = !!(scancode &RC6_6A_MCE_TOGGLE_MASK ); + toggle =1 + !!(scancode &RC6_6A_MCE_TOGGLE_MASK ); scancode &= ~RC6_6A_MCE_TOGGLE_MASK ;break ;default :
drivers/media/rc/ir-rcmm-decoder.c +1 -1
@@ -148,7 +148,7 @@ static int ir_rcmm_decode(struct rc_dev *dev, struct ir_raw_event ev)break ;if (rcmm_mode (data)) { - toggle = !!(0x8000 & data->bits); + toggle =1 + !!(0x8000 & data->bits); scancode = data->bits & ~0x8000 ; }else { toggle =0 ;
drivers/media/rc/rc-main.c +5 -4
@@ -782,18 +782,19 @@ static void ir_do_keydown(struct rc_dev *dev, enum rc_proto protocol, {bool new_event = (!dev->keypressed || dev->last_protocol != protocol || - dev->last_scancode != scancode || - dev->last_toggle != toggle); + dev->last_scancode != scancode); +bool repeat_event = !new_event && toggle && dev->last_toggle == toggle;struct lirc_scancode sc = { .scancode = scancode, .rc_proto = protocol, - .flags = toggle ?LIRC_SCANCODE_FLAG_TOGGLE :0 , + .flags = (toggle >1 ?LIRC_SCANCODE_FLAG_TOGGLE :0 ) | + (repeat_event ?LIRC_SCANCODE_FLAG_REPEAT :0 ), .keycode = keycode };if (dev->allowed_protocols !=RC_PROTO_BIT_CEC )lirc_scancode_event (dev, &sc); -if (new_event && dev->keypressed) +if (!repeat_event && dev->keypressed)ir_do_keyup (dev, false);if (scancode <=U32_MAX )
drivers/media/usb/dvb-usb-v2/az6007.c +1 -1
@@ -224,7 +224,7 @@ static int az6007_rc_query(struct dvb_usb_device *d) proto =RC_PROTO_NEC32 ; } -rc_keydown (d->rc_dev, proto, code, st->data[5 ]); +rc_keydown (d->rc_dev, proto, code,1 + st->data[5 ]);/* FIXME */ return 0 ; }
drivers/media/usb/dvb-usb-v2/dvbsky.c +1 -1
@@ -192,7 +192,7 @@ static int dvbsky_rc_query(struct dvb_usb_device *d) rc5_system = (code &0x7C0 ) >>6 ; toggle = (code &0x800 ) ?1 :0 ; scancode = rc5_system <<8 | rc5_command; -rc_keydown (d->rc_dev,RC_PROTO_RC5 , scancode, toggle); +rc_keydown (d->rc_dev,RC_PROTO_RC5 , scancode,1 + toggle); }return 0 ; }
drivers/media/usb/dvb-usb-v2/rtl28xxu.c +8 -40
72 lines not on this page · raw message
drivers/media/usb/dvb-usb-v2/rtl28xxu.h +1 -0
@@ -64,6 +64,7 @@ struct rtl28xxu_dev {u8 tuner;char *tuner_name;u8 page;/* integrated demod active register page */ +u8 rc_bc;struct i2c_adapter *demod_i2c_adapter;bool rc_active;bool new_i2c_write;
drivers/media/usb/dvb-usb/dib0700_core.c +1 -1
@@ -780,7 +780,7 @@ static void dib0700_rc_urb_completion(struct urb *purb)default :deb_data ("RC5 protocol\n" ); protocol =RC_PROTO_RC5 ; - toggle = poll_reply->report_id; + toggle =1 + poll_reply->report_id;/* FIXME */ keycode =RC_SCANCODE_RC5 (poll_reply->rc5.system, poll_reply->rc5.data);if ((poll_reply->rc5.data ^ poll_reply->rc5.not_data) !=0xff ) {
drivers/media/usb/dvb-usb/dib0700_devices.c +1 -1
@@ -565,7 +565,7 @@ static int dib0700_rc_query_old_firmware(struct dvb_usb_device *d)/* RC-5 protocol changes toggle bit on new keypress */ protocol =RC_PROTO_RC5 ; scancode =RC_SCANCODE_RC5 (st->buf[3 -2 ], st->buf[3 -3 ]); - toggle = st->buf[3 -1 ]; + toggle =1 + st->buf[3 -1 ];/* FIXME: test this */ break ; }
drivers/media/usb/dvb-usb/ttusb2.c +2 -1
@@ -456,7 +456,8 @@ static int tt3650_rc_query(struct dvb_usb_device *d)/* got a "press" event */ st->last_rc_key =RC_SCANCODE_RC5 (rx[3 ], rx[2 ]);deb_info ("%s: cmd=0x%02x sys=0x%02x\n" , __func__, rx[2 ], rx[3 ]); -rc_keydown (d->rc_dev,RC_PROTO_RC5 , st->last_rc_key, rx[1 ]); +rc_keydown (d->rc_dev,RC_PROTO_RC5 , st->last_rc_key, +1 + rx[1 ]);/* FIXME: test this */ }else if (st->last_rc_key) {rc_keyup (d->rc_dev); st->last_rc_key =0 ;
drivers/media/usb/em28xx/em28xx-input.c +2 -2
@@ -340,12 +340,12 @@ static void em28xx_ir_handle_key(struct em28xx_IR *ir)rc_keydown (ir->rc, poll_result.protocol, poll_result.scancode, - poll_result.toggle_bit); +1 + poll_result.toggle_bit); else rc_keydown(ir->rc,RC_PROTO_UNKNOWN , poll_result.scancode & 0xff, - poll_result.toggle_bit); + 1 + poll_result.toggle_bit);if (ir->dev->chip_id ==CHIP_ID_EM2874 || ir->dev->chip_id ==CHIP_ID_EM2884 )
drivers/staging/media/av7110/av7110_ir.c +1 -1
@@ -64,7 +64,7 @@ void av7110_ir_handler(struct av7110 *av7110, u32 ircom)return ; } -rc_keydown (rcdev, proto, scancode, toggle !=0 ); +rc_keydown (rcdev, proto, scancode,1 + !!toggle); } }
Signature · 1 line
2.36.1
Sean Young
maintainerRe: [PATCH 2/2] media: rtl28xxu: improve IR receiver
Hi,
Quoted 12 lines · Marko Mäkelä
Quoted 11 lines · Sean Young
Hi Marko,
Quoted 7 lines · Marko Mäkelä
I finally took the time to get a deeper understanding of the infrared remote control subsystem. I think that I now understand the translation into key-down, key-up, and key-repeat events. For the RC5 protocol,
rc_repeat()will not be called byir-rc5-decoder.cbut instead,ir_do_keydown()will handle the repeat. Forlirc_scancode_event()it will never set theLIRC_SCANCODE_FLAG_REPEATbit, even if !new_event and the protocol does support the toggle bit. That might qualify as a bug.
Quoted 8 lines
Quoted 1 line
You are right, this was missed. Patches welcome.
Attached (for 5.19.0-rc3, on top of the two commits of this patch series).
I thought that it would be the least amount of trouble to slightly change the interpretation of the "toggle" parameter of
rc_keydown(). My intention was to use the values 1 and 2 when the toggle flag is present. Any nonzero values would work.
I don't understand why this is needed.
Quoted 7 lines
I am not that familiar with updating the modules, and I suspect that instead of actually testing this code, I was testing the "ring buffer" patch that I posted yesterday. I could not use the rmmod/insmod approach for testing this change, because the
rc_coremodule was in use by the display driver. So, I can only say that the patch compiled for me. A few FIXME places are indicated where I am not sure that a correct (nonzero) toggle value would be computed.
A patch needs to be tested. Just rebuild the entire kernel and boot from that.
Quoted 58 lines
An alternative approach would be to use the value toggle=1 for the case when the toggle bit is set, and toggle>1 for the case when it is not set. Basically, change things like 1+!!x to 1+!x in the callers, and change the condition toggle > 1 to toggle == 1 in
rc-main.c. In that way, any old driver that would use the toggle values 0 and 1 would still generateLIRC_SCANCODE_FLAG_TOGGLE. But then, therepeat_eventlogic would only work half the time (when toggle!=0).Marko
Quoted 1 line
From
29a5c2a00653f49c854109b2f2c8f99b4431430fMon Sep 17 00:00:00 2001From: =?UTF-8?q?Marko=20M=C3=A4kel=C3=A4?= <[email protected]>
Date: Tue, 28 Jun 2022 07:59:17 +0300
Subject: [PATCH]rc_keydown(): ReportLIRC_SCANCODE_FLAG_REPEATbased on
toggle bitDrivers that will not call
rc_repeat()will letrc_keydown()create repeat events. For the LIRC interface, the repeat flag would only be set byrc_repeat(), never byrc_keydown().We change the meaning of the toggle parameter: Drivers that invoke
rc_repeat()by themselves should always pass toggle=0, while protocols that include a toggle bit should pass toggle>0, with the value 1 meaning that the toggle bit is clear.This is largely untested code. See FIXME comments. Also, an interface change for
bpf_rc_keydown()might have to be documented.--- drivers/media/cec/platform/seco/seco-cec.c | 3 +- drivers/media/i2c/ir-kbd-i2c.c | 4 +- drivers/media/pci/bt8xx/bttv-input.c | 2 +- drivers/media/pci/ttpci/budget-ci.c | 4 +- drivers/media/rc/bpf-lirc.c | 2 +- drivers/media/rc/img-ir/img-ir-rc5.c | 2 +- drivers/media/rc/img-ir/img-ir-rc6.c | 2 +- drivers/media/rc/imon.c | 2 +- drivers/media/rc/ir-jvc-decoder.c | 3 +- drivers/media/rc/ir-rc5-decoder.c | 2 +- drivers/media/rc/ir-rc6-decoder.c | 4 +- drivers/media/rc/ir-rcmm-decoder.c | 2 +- drivers/media/rc/rc-main.c | 9 ++-- drivers/media/usb/dvb-usb-v2/az6007.c | 2 +- drivers/media/usb/dvb-usb-v2/dvbsky.c | 2 +- drivers/media/usb/dvb-usb-v2/rtl28xxu.c | 48 ++++----------------- drivers/media/usb/dvb-usb-v2/rtl28xxu.h | 1 + drivers/media/usb/dvb-usb/dib0700_core.c | 2 +- drivers/media/usb/dvb-usb/dib0700_devices.c | 2 +- drivers/media/usb/dvb-usb/ttusb2.c | 3 +- drivers/media/usb/em28xx/em28xx-input.c | 4 +- drivers/staging/media/av7110/av7110_ir.c | 2 +- 22 files changed, 40 insertions(+), 67 deletions(-)drivers/media/cec/platform/seco/seco-cec.c +0
-0
drivers/media/cec/platform/seco/seco-cec.c static int secocec_ir_rx(struct secocec_data *priv) · line 420
Quoted 6 lines
drivers/media/cec/platform/seco/seco-cec.c +2
-1@@ -416,7 +416,8 @@ static int secocec_ir_rx(struct secocec_data *priv) addr = (val &SECOCEC_IR_ADDRESS_MASK ) >>SECOCEC_IR_ADDRESS_SHL ; toggle = (val &SECOCEC_IR_TOGGLE_MASK ) >>SECOCEC_IR_TOGGLE_SHL ; -rc_keydown (cec->ir,RC_PROTO_RC5 ,RC_SCANCODE_RC5 (addr, key), toggle); +rc_keydown (cec->ir,RC_PROTO_RC5 ,RC_SCANCODE_RC5 (addr, key), +1 + toggle);
You can't change the toggle value because you want a repeat flag. This makes no sense.
-snip-
drivers/media/rc/rc-main.c static void ir_do_keydown(struct rc_dev *dev, enum rc_proto protocol, · line 786
Quoted 10 lines
drivers/media/rc/rc-main.c +2
-2@@ -782,18 +782,19 @@ static void ir_do_keydown(struct rc_dev *dev, enum rc_proto protocol, {bool new_event = (!dev->keypressed || dev->last_protocol != protocol || - dev->last_scancode != scancode || - dev->last_toggle != toggle); + dev->last_scancode != scancode); +bool repeat_event = !new_event && toggle && dev->last_toggle == toggle;
Why this change?
drivers/media/rc/rc-main.c static void ir_do_keydown(struct rc_dev *dev, enum rc_proto protocol, · line 790
Quoted 5 lines
drivers/media/rc/rc-main.c +2
-1struct lirc_scancode sc = { .scancode = scancode, .rc_proto = protocol, - .flags = toggle ?LIRC_SCANCODE_FLAG_TOGGLE :0 , + .flags = (toggle >1 ?LIRC_SCANCODE_FLAG_TOGGLE :0 ) | + (repeat_event ?LIRC_SCANCODE_FLAG_REPEAT :0 ),
Why not simply (!new_event ? LIRC_SCANCODE_FLAG_REPEAT : 0) and be done with it?
Quoted 16 lines
drivers/media/rc/rc-main.c +1
-1.keycode = keycode };if (dev->allowed_protocols !=RC_PROTO_BIT_CEC )lirc_scancode_event (dev, &sc); -if (new_event && dev->keypressed) +if (!repeat_event && dev->keypressed)ir_do_keyup (dev, false);if (scancode <=U32_MAX )drivers/media/usb/dvb-usb-v2/az6007.c +0
-0
drivers/media/usb/dvb-usb-v2/az6007.c static int az6007_rc_query(struct dvb_usb_device *d) · line 227
Quoted 5 lines
drivers/media/usb/dvb-usb-v2/az6007.c +1
-1@@ -224,7 +224,7 @@ static int az6007_rc_query(struct dvb_usb_device *d) proto =RC_PROTO_NEC32 ; } -rc_keydown (d->rc_dev, proto, code, st->data[5 ]); +rc_keydown (d->rc_dev, proto, code,1 + st->data[5 ]);/* FIXME */
I still have no idea what you are trying to achieve with this.
Sean
Re: [PATCH 2/2] media: rtl28xxu: improve IR receiver
Quoted 17 lines · Sean Young
Hi,
Quoted 14 lines · Marko Mäkelä
Quoted 12 lines · Sean Young
Hi Marko,
Quoted 7 lines · Marko Mäkelä
I finally took the time to get a deeper understanding of the infrared remote control subsystem. I think that I now understand the translation into key-down, key-up, and key-repeat events. For the RC5 protocol,
rc_repeat()will not be called byir-rc5-decoder.cbut instead,ir_do_keydown()will handle the repeat. Forlirc_scancode_event()it will never set theLIRC_SCANCODE_FLAG_REPEATbit, even if !new_event and the protocol does support the toggle bit. That might qualify as a bug.You are right, this was missed. Patches welcome.
Quoted 8 lines
Quoted 6 lines
Attached (for 5.19.0-rc3, on top of the two commits of this patch series).
I thought that it would be the least amount of trouble to slightly change the interpretation of the "toggle" parameter of
rc_keydown(). My intention was to use the values 1 and 2 when the toggle flag is present. Any nonzero values would work.I don't understand why this is needed.
For protocols that do not use a toggle bit, the last parameter of rc_keydown() will usually be toggle=0, and explicit calls to rc_repeat() will be issued when needed. For those protocols, I thought that we would not want rc_keydown() to set any LIRC_SCANCODE_FLAG_REPEAT flag under any circumstances.
Quoted 1 line
A patch needs to be tested. Just rebuild the entire kernel and boot from that.
Yes, I will do that for revising my patch.
Quoted 6 lines
drivers/media/cec/platform/seco/seco-cec.c static int secocec_ir_rx(struct secocec_data *priv) · line 420
Quoted 3 lines
drivers/media/cec/platform/seco/seco-cec.c +2
-1-rc_keydown (cec->ir,RC_PROTO_RC5 ,RC_SCANCODE_RC5 (addr, key), toggle); +rc_keydown (cec->ir,RC_PROTO_RC5 ,RC_SCANCODE_RC5 (addr, key), +1 + toggle);You can't change the toggle value because you want a repeat flag. This makes no sense.
[snip]
Quoted 13 lines
drivers/media/rc/rc-main.c static void ir_do_keydown(struct rc_dev *dev, enum rc_proto protocol, · line 786
Quoted 10 lines
drivers/media/rc/rc-main.c +2
-2@@ -782,18 +782,19 @@ static void ir_do_keydown(struct rc_dev *dev, enum rc_proto protocol, {bool new_event = (!dev->keypressed || dev->last_protocol != protocol || - dev->last_scancode != scancode || - dev->last_toggle != toggle); + dev->last_scancode != scancode); +bool repeat_event = !new_event && toggle && dev->last_toggle == toggle;Why this change?
Quoted 7 lines
drivers/media/rc/rc-main.c static void ir_do_keydown(struct rc_dev *dev, enum rc_proto protocol, · line 790
Quoted 5 lines
drivers/media/rc/rc-main.c +2
-1struct lirc_scancode sc = { .scancode = scancode, .rc_proto = protocol, - .flags = toggle ?LIRC_SCANCODE_FLAG_TOGGLE :0 , + .flags = (toggle >1 ?LIRC_SCANCODE_FLAG_TOGGLE :0 ) | + (repeat_event ?LIRC_SCANCODE_FLAG_REPEAT :0 ),Why not simply (!new_event ?
LIRC_SCANCODE_FLAG_REPEAT: 0) and be done with it?
Drivers that invoke rc_repeat() do not want rc_keydown() to ever set LIRC_SCANCODE_FLAG_REPEAT. The patch slightly changed the meaning of toggle: it *must* be 0 if and only if the protocol does not implement a toggle bit. If it does, the values must alternate between 1 and some greater-than-1 value.
A cleaner alternative could be to retain the interface of rc_keydown() as is, and add a new function, say, rc_keydown_or_repeat(), which would generate key-repeat events from the toggle bit.
Best regards,
Marko
Sean Young
maintainerRe: [PATCH 2/2] media: rtl28xxu: improve IR receiver
Quoted 25 lines · Marko Mäkelä
Quoted 24 lines · Sean Young
Hi,
Quoted 20 lines · Marko Mäkelä
Quoted 11 lines · Sean Young
Hi Marko,
Quoted 7 lines · Marko Mäkelä
I finally took the time to get a deeper understanding of the infrared remote control subsystem. I think that I now understand the translation into key-down, key-up, and key-repeat events. For the RC5 protocol,
rc_repeat()will not be called byir-rc5-decoder.cbut instead,ir_do_keydown()will handle the repeat. Forlirc_scancode_event()it will never set theLIRC_SCANCODE_FLAG_REPEATbit, even if !new_event and the protocol does support the toggle bit. That might qualify as a bug.Quoted 1 line
You are right, this was missed. Patches welcome.
Attached (for 5.19.0-rc3, on top of the two commits of this patch series).
I thought that it would be the least amount of trouble to slightly change the interpretation of the "toggle" parameter of
rc_keydown(). My intention was to use the values 1 and 2 when the toggle flag is present. Any nonzero values would work.
Quoted 7 lines
Quoted 1 line
I don't understand why this is needed.
For protocols that do not use a toggle bit, the last parameter of
rc_keydown()will usually be toggle=0, and explicit calls torc_repeat()will be issued when needed. For those protocols, I thought that we would not wantrc_keydown()to set anyLIRC_SCANCODE_FLAG_REPEATflag under any circumstances.
Toggle and repeat are distinct concepts.
rc_repeat() is for protocols which have a special repeat message, which carry no information other that "repeat the last message". However, all protocols repeat. Whether they use a special repeat message or not.
It's right that if a protocol repeats a message, LIRC_SCANCODE_FLAG_REPEAT is set.
Sean
Re: [PATCH 2/2] media: rtl28xxu: improve IR receiver
Hi Sean,
Quoted 7 lines · Sean Young
Quoted 5 lines · Marko Mäkelä
For protocols that do not use a toggle bit, the last parameter of
rc_keydown()will usually be toggle=0, and explicit calls torc_repeat()will be issued when needed. For those protocols, I thought that we would not wantrc_keydown()to set anyLIRC_SCANCODE_FLAG_REPEATflag under any circumstances.
Quoted 8 lines
Toggle and repeat are distinct concepts.
rc_repeat()is for protocols which have a special repeat message, which carry no information other that "repeat the last message". However, all protocols repeat. Whether they use a special repeat message or not.It's right that if a protocol repeats a message,
LIRC_SCANCODE_FLAG_REPEATis set.
Is it right to set the flag when a message is being repeated due to user effort (repeatedly pressing and releasing a button, instead of holding the button down)? If it is, is it consistent to avoid setting the flag when a protocol uses a toggle bit (say, RC5)? In that case, the toggle bit would change its value each time the button is pressed, and your suggested change to rc_keydown() would not set the repeat flag.
As far as I understand, the change that you suggested would set the LIRC_SCANCODE_FLAG_REPEAT if I repeatedly press a button on the NEC protocol remote control, but not on an RC5 remote control.
I tested the attached patch (which was created on 5.19-rc5, which failed to boot on my system for unrelated reasons) on Linux 5.17, on top of your fixes to rtl28xxu and rc-core.
By the way, the patch that I sent earlier accidentally included my futile attempt at avoiding buffer overrun on the rtl28xxu. That is probably why my previous test failed.
With the NEC protocol of the bundled remote of the Astrometa DVB-T2 adapter, the "repeat" flag was occasionally set when I repeated keypresses too quickly. I think that this was because the key matrix scanning algorithm on the remote control did not get a reading of "no key pressed" between two key presses. When I used an RC5 based remote (that of Hauppauge Nova-T PCI 90002), I only saw the "repeat" flag in the output of "ir-keytable -t" when holding a button down. If I repeated keypresses manually, the toggle bit was changing between them. In other words, for these two remote controls, the patch works exactly like I intended.
One might think that it is not necessary to make difference between long button presses (which should generate repeat events) and short button presses that are quickly repeated by the user. I can think of a user-space application that would intentionally ignore repeat events for some buttons where it would make little sense. For example, when the number button 1 is pressed for a long time, the application might choose not to repeat the keypress, but "demand" multiple separate button presses by the user, if the channel should really be switched to 11, 111, or 1111. The intention of ignoring "repeat" events would be to avoid "punishing" users who are pressing a button longer, possibly compensating for unreliable IR signal reception.
If the user wants to quickly switch to channel 111 by quickly pressing the button three times, it should not be misreported as an auto-repeated event, but reported as 3 LIRC events without the "repeat" flag, and as 3 pairs of keydown and keyup events.
On the other hand, there should be no reason for an application to not honor repeat events for a volume button. That is of course up to the application to decide, not the kernel.
If you agree that this patch is on the right track, an interface for the new function rc_keydown_or_repeat() may have to be exposed to the BPF interface as well.
Marko
From 1a1825e6ae737d840e2330db454e0426f7658d4f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marko=20M=C3=A4kel=C3=A4?= <[email protected]>
Date: Mon, 4 Jul 2022 12:07:26 +0300
Subject: [PATCH] media: rc: Report LIRC_SCANCODE_FLAG_REPEAT when not using
rc_repeat()
The flag LIRC_SCANCODE_FLAG_REPEAT is never set on rc_keydown(). Previously it was only set by rc_repeat(). The new function rc_keydown_or_repeat() is like rc_keydown(), but it will automatically set the LIRC_SCANCODE_FLAG_REPEAT flag for repeated events.
For protocols for which rc_repeat() will not be invoked, calls to the new function rc_keydown_or_repeat() will replace calls to rc_keydown().
Protocols that rely on rc_repeat() will continue to use rc_keydown(), so that the LIRC_SCANCODE_FLAG_REPEAT flag will only be set for auto-generated repeat events when a button is being held down, and not when the user is repeatedly pressing a button.
TODO: Introduce bpf_rc_keydown_or_repeat()
--- drivers/media/cec/platform/seco/seco-cec.c | 3 +- drivers/media/i2c/ir-kbd-i2c.c | 2 +- drivers/media/pci/bt8xx/bttv-input.c | 2 +- drivers/media/pci/ttpci/budget-ci.c | 7 ++-- drivers/media/rc/imon.c | 5 +-- drivers/media/rc/ir-rc5-decoder.c | 2 +- drivers/media/rc/ir-rc6-decoder.c | 2 +- drivers/media/rc/ir-rcmm-decoder.c | 4 ++- drivers/media/rc/rc-main.c | 40 ++++++++++++++++++--- drivers/media/usb/dvb-usb-v2/az6007.c | 2 +- drivers/media/usb/dvb-usb-v2/dvbsky.c | 3 +- drivers/media/usb/dvb-usb/dib0700_core.c | 10 +++--- drivers/media/usb/dvb-usb/dib0700_devices.c | 7 ++-- drivers/media/usb/dvb-usb/ttusb2.c | 3 +- drivers/media/usb/em28xx/em28xx-input.c | 8 ++--- drivers/staging/media/av7110/av7110_ir.c | 2 +- include/media/rc-core.h | 2 ++ 17 files changed, 71 insertions(+), 33 deletions(-)
drivers/media/cec/platform/seco/seco-cec.c +2 -1
@@ -429,7 +429,8 @@ static int secocec_ir_rx(struct secocec_data *priv) addr = (val &SECOCEC_IR_ADDRESS_MASK ) >>SECOCEC_IR_ADDRESS_SHL ; toggle = (val &SECOCEC_IR_TOGGLE_MASK ) >>SECOCEC_IR_TOGGLE_SHL ; -rc_keydown (cec->ir,RC_PROTO_RC5 ,RC_SCANCODE_RC5 (addr, key), toggle); +rc_keydown_or_repeat (cec->ir,RC_PROTO_RC5 ,RC_SCANCODE_RC5 (addr, key), + toggle);dev_dbg (dev,"IR key pressed: 0x%02x addr 0x%02x toggle 0x%02x" , key, addr, toggle);
drivers/media/i2c/ir-kbd-i2c.c +1 -1
@@ -299,7 +299,7 @@ static int ir_key_poll(struct IR_i2c *ir)if (rc) {dev_dbg (&ir->rc->dev,"%s: proto = 0x%04x, scancode = 0x%08x\n" , __func__, protocol, scancode); -rc_keydown (ir->rc, protocol, scancode, toggle); +rc_keydown_or_repeat (ir->rc, protocol, scancode, toggle); }return 0 ; }
drivers/media/pci/bt8xx/bttv-input.c +1 -1
@@ -228,7 +228,7 @@ static void bttv_rc5_timer_end(struct timer_list *t) } scancode =RC_SCANCODE_RC5 (system, command); -rc_keydown (ir->dev,RC_PROTO_RC5 , scancode, toggle); +rc_keydown_or_repeat (ir->dev,RC_PROTO_RC5 , scancode, toggle);dprintk ("scancode %x, toggle %x\n" , scancode, toggle); }
drivers/media/pci/ttpci/budget-ci.c +4 -3
@@ -145,9 +145,10 @@ static void msp430_ir_interrupt(struct tasklet_struct *t)return ;if (budget_ci->ir.full_rc5) { -rc_keydown (dev,RC_PROTO_RC5 , -RC_SCANCODE_RC5 (budget_ci->ir.rc5_device, budget_ci->ir.ir_key), - !!(command &0x20 )); +rc_keydown_or_repeat (dev,RC_PROTO_RC5 , +RC_SCANCODE_RC5 (budget_ci->ir.rc5_device, + budget_ci->ir.ir_key), + !!(command &0x20 ));return ; }
drivers/media/rc/imon.c +3 -2
@@ -1664,8 +1664,9 @@ static void imon_incoming_packet(struct imon_context *ictx,else return ; -rc_keydown (ictx->rdev, proto, ictx->rc_scancode, - ictx->rc_toggle); +rc_keydown_or_repeat (ictx->rdev, proto, + ictx->rc_scancode, + ictx->rc_toggle);spin_lock_irqsave (&ictx->kc_lock, flags); ictx->last_keycode = ictx->kc;
drivers/media/rc/ir-rc5-decoder.c +1 -1
@@ -157,7 +157,7 @@ static int ir_rc5_decode(struct rc_dev *dev, struct ir_raw_event ev)dev_dbg (&dev->dev,"RC5(x/sz) scancode 0x%06x (p: %u, t: %u)\n" , scancode, protocol, toggle); -rc_keydown (dev, protocol, scancode, toggle); +rc_keydown_or_repeat (dev, protocol, scancode, toggle); data->state =STATE_INACTIVE ;return 0 ; }
drivers/media/rc/ir-rc6-decoder.c +1 -1
@@ -263,7 +263,7 @@ static int ir_rc6_decode(struct rc_dev *dev, struct ir_raw_event ev) goto out; } -rc_keydown (dev, protocol, scancode, toggle); +rc_keydown_or_repeat (dev, protocol, scancode, toggle); data->state =STATE_INACTIVE ;return 0 ; }
drivers/media/rc/ir-rcmm-decoder.c +3 -1
@@ -156,7 +156,9 @@ static int ir_rcmm_decode(struct rc_dev *dev, struct ir_raw_event ev) }if (dev->enabled_protocols &RC_PROTO_BIT_RCMM32 ) { -rc_keydown (dev,RC_PROTO_RCMM32 , scancode, toggle); +rc_keydown_or_repeat (dev,RC_PROTO_RCMM32 , + data->bits & ~0x8000 , + !!(0x8000 & data->bits)); data->state =STATE_INACTIVE ;return 0 ; }
drivers/media/rc/rc-main.c +36 -4
@@ -773,12 +773,13 @@ EXPORT_SYMBOL_GPL(rc_repeat);* @scancode: the scancode of the keypress * @keycode: the keycode of the keypress * @toggle: the toggle value of the keypress +* @do_repeat: whether to set the LIRC_SCANCODE_FLAG_REPEAT when needed * * This function is used internally to register a keypress, it must be * called with keylock held. */ static void ir_do_keydown (struct rc_dev *dev,enum rc_proto protocol, -u64 scancode,u32 keycode,u8 toggle) +u64 scancode,u32 keycode,u8 toggle,bool do_repeat) { bool new_event = (!dev->keypressed || dev->last_protocol != protocol ||
@@ -786,7 +787,8 @@ static void ir_do_keydown(struct rc_dev *dev, enum rc_proto protocol, dev->last_toggle != toggle);struct lirc_scancode sc = { .scancode = scancode, .rc_proto = protocol, - .flags = toggle ?LIRC_SCANCODE_FLAG_TOGGLE :0 , + .flags = (toggle ?LIRC_SCANCODE_FLAG_TOGGLE :0 ) | + (do_repeat && !new_event ?LIRC_SCANCODE_FLAG_REPEAT :0 ), .keycode = keycode };
@@ -852,7 +854,7 @@ void rc_keydown(struct rc_dev *dev, enum rc_proto protocol, u64 scancode,u32 keycode =rc_g_keycode_from_table (dev, scancode);spin_lock_irqsave (&dev->keylock, flags); -ir_do_keydown (dev, protocol, scancode, keycode, toggle); +ir_do_keydown (dev, protocol, scancode, keycode, toggle, false);if (dev->keypressed) { dev->keyup_jiffies = jiffies +
@@ -864,6 +866,36 @@ void rc_keydown(struct rc_dev *dev, enum rc_proto protocol, u64 scancode, }EXPORT_SYMBOL_GPL (rc_keydown); +/** +* rc_keydown_or_repeat() - generates input event for a key press +* @dev: the struct rc_dev descriptor of the device +* @protocol: the protocol for the keypress +* @scancode: the scancode for the keypress +* @toggle: the toggle value (protocol dependent, if the protocol doesn't +* support toggle values, this should be set to zero) +* +* This routine is used to signal that a key has been pressed or is +* being held down on the remote control. +*/ +void rc_keydown_or_repeat (struct rc_dev *dev,enum rc_proto protocol, +u64 scancode,u8 toggle) +{ +unsigned long flags; +u32 keycode =rc_g_keycode_from_table (dev, scancode); + +spin_lock_irqsave (&dev->keylock, flags); +ir_do_keydown (dev, protocol, scancode, keycode, toggle, true); + +if (dev->keypressed) { + dev->keyup_jiffies = jiffies + +usecs_to_jiffies (dev->keyup_delay) + +msecs_to_jiffies (repeat_period (protocol)); +mod_timer (&dev->timer_keyup, dev->keyup_jiffies); + } +spin_unlock_irqrestore (&dev->keylock, flags); +} +EXPORT_SYMBOL_GPL (rc_keydown_or_repeat); +/** * rc_keydown_notimeout() - generates input event for a key press without * an automatic keyup event at a later time
@@ -883,7 +915,7 @@ void rc_keydown_notimeout(struct rc_dev *dev, enum rc_proto protocol,u32 keycode =rc_g_keycode_from_table (dev, scancode);spin_lock_irqsave (&dev->keylock, flags); -ir_do_keydown (dev, protocol, scancode, keycode, toggle); +ir_do_keydown (dev, protocol, scancode, keycode, toggle, false);spin_unlock_irqrestore (&dev->keylock, flags); }EXPORT_SYMBOL_GPL (rc_keydown_notimeout);
drivers/media/usb/dvb-usb-v2/az6007.c +1 -1
@@ -224,7 +224,7 @@ static int az6007_rc_query(struct dvb_usb_device *d) proto =RC_PROTO_NEC32 ; } -rc_keydown (d->rc_dev, proto, code, st->data[5 ]); +rc_keydown_or_repeat (d->rc_dev, proto, code, st->data[5 ]);return 0 ; }
drivers/media/usb/dvb-usb-v2/dvbsky.c +2 -1
@@ -192,7 +192,8 @@ static int dvbsky_rc_query(struct dvb_usb_device *d) rc5_system = (code &0x7C0 ) >>6 ; toggle = (code &0x800 ) ?1 :0 ; scancode = rc5_system <<8 | rc5_command; -rc_keydown (d->rc_dev,RC_PROTO_RC5 , scancode, toggle); +rc_keydown_or_repeat (d->rc_dev,RC_PROTO_RC5 , scancode, + toggle); }return 0 ; }
drivers/media/usb/dvb-usb/dib0700_core.c +4 -6
@@ -711,7 +711,6 @@ static void dib0700_rc_urb_completion(struct urb *purb)struct dib0700_rc_response *poll_reply;enum rc_proto protocol;u32 keycode; -u8 toggle;deb_info ("%s()\n" , __func__);if (d->rc_dev ==NULL ) {
@@ -743,8 +742,6 @@ static void dib0700_rc_urb_completion(struct urb *purb)switch (d->props.rc.core.protocol) {case RC_PROTO_BIT_NEC : - toggle =0 ; -/* NEC protocol sends repeat code as 0 0 0 FF */ if (poll_reply->nec.system == 0x00 && poll_reply->nec.not_system == 0x00 &&
@@ -780,7 +777,6 @@ static void dib0700_rc_urb_completion(struct urb *purb)default :deb_data ("RC5 protocol\n" ); protocol =RC_PROTO_RC5 ; - toggle = poll_reply->report_id; keycode =RC_SCANCODE_RC5 (poll_reply->rc5.system, poll_reply->rc5.data);if ((poll_reply->rc5.data ^ poll_reply->rc5.not_data) !=0xff ) {
@@ -791,10 +787,12 @@ static void dib0700_rc_urb_completion(struct urb *purb) goto resubmit; } -break ; +rc_keydown_or_repeat (d->rc_dev, protocol, keycode, + poll_reply->report_id); + goto resubmit; } -rc_keydown (d->rc_dev, protocol, keycode, toggle); +rc_keydown (d->rc_dev, protocol, keycode,0 ); resubmit:/* Clean the buffer before we requeue */
drivers/media/usb/dvb-usb/dib0700_devices.c +3 -4
@@ -517,7 +517,6 @@ static int dib0700_rc_query_old_firmware(struct dvb_usb_device *d) {enum rc_proto protocol;u32 scancode; -u8 toggle;int i;struct dib0700_state *st = d->priv;
@@ -558,18 +557,18 @@ static int dib0700_rc_query_old_firmware(struct dvb_usb_device *d) protocol =RC_PROTO_NEC ; scancode =RC_SCANCODE_NEC (st->buf[3 -2 ], st->buf[3 -3 ]); - toggle =0 ; +rc_keydown (d->rc_dev, protocol, scancode,0 );break ;default :/* RC-5 protocol changes toggle bit on new keypress */ protocol =RC_PROTO_RC5 ; scancode =RC_SCANCODE_RC5 (st->buf[3 -2 ], st->buf[3 -3 ]); - toggle = st->buf[3 -1 ]; +rc_keydown_or_repeat (d->rc_dev, protocol, scancode, + st->buf[3 -1 ]);break ; } -rc_keydown (d->rc_dev, protocol, scancode, toggle);return 0 ; }
drivers/media/usb/dvb-usb/ttusb2.c +2 -1
@@ -456,7 +456,8 @@ static int tt3650_rc_query(struct dvb_usb_device *d)/* got a "press" event */ st->last_rc_key =RC_SCANCODE_RC5 (rx[3 ], rx[2 ]);deb_info ("%s: cmd=0x%02x sys=0x%02x\n" , __func__, rx[2 ], rx[3 ]); -rc_keydown (d->rc_dev,RC_PROTO_RC5 , st->last_rc_key, rx[1 ]); +rc_keydown_or_repeat (d->rc_dev,RC_PROTO_RC5 , st->last_rc_key, + rx[1 ]); }else if (st->last_rc_key) {rc_keyup (d->rc_dev); st->last_rc_key =0 ;
drivers/media/usb/em28xx/em28xx-input.c +4 -4
@@ -337,10 +337,10 @@ static void em28xx_ir_handle_key(struct em28xx_IR *ir) poll_result.toggle_bit, poll_result.read_count, poll_result.scancode);if (ir->full_code) -rc_keydown (ir->rc, - poll_result.protocol, - poll_result.scancode, - poll_result.toggle_bit); +rc_keydown_or_repeat (ir->rc, + poll_result.protocol, + poll_result.scancode, + poll_result.toggle_bit);else rc_keydown(ir->rc, RC_PROTO_UNKNOWN,
drivers/staging/media/av7110/av7110_ir.c +1 -1
@@ -64,7 +64,7 @@ void av7110_ir_handler(struct av7110 *av7110, u32 ircom)return ; } -rc_keydown (rcdev, proto, scancode, toggle !=0 ); +rc_keydown_or_repeat (rcdev, proto, scancode, !!toggle); } }
include/media/rc-core.h +2 -0
@@ -287,6 +287,8 @@ void rc_unregister_device(struct rc_dev *dev);void rc_repeat (struct rc_dev *dev);void rc_keydown (struct rc_dev *dev,enum rc_proto protocol,u64 scancode,u8 toggle); +void rc_keydown_or_repeat (struct rc_dev *dev,enum rc_proto protocol, +u64 scancode,u8 toggle);void rc_keydown_notimeout (struct rc_dev *dev,enum rc_proto protocol,u64 scancode,u8 toggle);void rc_keyup (struct rc_dev *dev);
Signature · 1 line
2.36.1
Sean Young
maintainerRe: [PATCH 2/2] media: rtl28xxu: improve IR receiver
Hi Marko,
Quoted 14 lines · Marko Mäkelä
Quoted 13 lines · Sean Young
Quoted 5 lines · Marko Mäkelä
For protocols that do not use a toggle bit, the last parameter of
rc_keydown()will usually be toggle=0, and explicit calls torc_repeat()will be issued when needed. For those protocols, I thought that we would not wantrc_keydown()to set anyLIRC_SCANCODE_FLAG_REPEATflag under any circumstances.Toggle and repeat are distinct concepts.
rc_repeat()is for protocols which have a special repeat message, which carry no information other that "repeat the last message". However, all protocols repeat. Whether they use a special repeat message or not.
Quoted 6 lines
Quoted 2 lines
It's right that if a protocol repeats a message,
LIRC_SCANCODE_FLAG_REPEATis set.Is it right to set the flag when a message is being repeated due to user effort (repeatedly pressing and releasing a button, instead of holding the button down)?
The problem here is that the nec repeat is used by some remotes, but not others. Some nec remotes repeat the entire code every time. Our generic nec decoder cannot distinguish between the two. So, our nec decoder interprets both a nec repeat and a repeated code as "button being held down".
rc5 is a much nicer protocol and explicitly uses a toggle bit to specify the button has been released/pressed. Some protocols use more than one bit for toggle, in case a toggle was lost due to packet loss.
Quoted 2 lines
If it is, is it consistent to avoid setting the flag when a protocol uses a toggle bit (say, RC5)?
No, RC5 repeats the same message if a button is being held down with the same toggle value. We should get a LIRC_SCANCODE_FLAG_REPEAT in this case.
Quoted 3 lines
In that case, the toggle bit would
change its value each time the button is pressed, and your suggested change torc_keydown()would not set the repeat flag.
If we can distinguish between press/release vs hold then it is not a repeat. If it is being held down. then it is a repeat.
Quoted 3 lines
As far as I understand, the change that you suggested would set the
LIRC_SCANCODE_FLAG_REPEATif I repeatedly press a button on the NEC protocol remote control, but not on an RC5 remote control.
RC5 too.
Quoted 3 lines
I tested the attached patch (which was created on 5.19-rc5, which failed to boot on my system for unrelated reasons) on Linux 5.17, on top of your fixes to rtl28xxu and rc-core.
You'll need to fix this.
Quoted 11 lines
One might think that it is not necessary to make difference between long button presses (which should generate repeat events) and short button presses that are quickly repeated by the user. I can think of a user-space application that would intentionally ignore repeat events for some buttons where it would make little sense. For example, when the number button 1 is pressed for a long time, the application might choose not to repeat the keypress, but "demand" multiple separate button presses by the user, if the channel should really be switched to 11, 111, or 1111. The intention of ignoring "repeat" events would be to avoid "punishing" users who are pressing a button longer, possibly compensating for unreliable IR signal reception.
The input layer create autorepeat key events for keys that are being held down.
Quoted 4 lines
If the user wants to quickly switch to channel 111 by quickly pressing the button three times, it should not be misreported as an auto-repeated event, but reported as 3 LIRC events without the "repeat" flag, and as 3 pairs of keydown and keyup events.
Ideally yes, if we can distinguish between the two.
FWIW I'm (slowly) working on new tooling that allows you specify the IR protocol in IRP format. This would allow you say for NEC:
{38.4k,564}<1,-1|1,-3>(16,-8,D:8,S:8,F:8,~F:8,1,^108m)* [D:0..255,S:0..255=255-D,F:0..255]
For remotes that repeat the entire code each time, and
{38.4k,564}<1,-1|1,-3>(16,-8,D:8,S:8,F:8,~F:8,1,^108m,(16,-4,1,^108m)*) [D:0..255,S:0..255=255-D,F:0..255]]]
For remotes that send nec repeats. This would be compiled down BPF. I'm still working on the decoder and I haven't started on the BPF compilation side yet (the encoder is fully working).
See https://github.com/seanyoung/cir/
Quoted 3 lines
On the other hand, there should be no reason for an application to not honor repeat events for a volume button. That is of course up to the application to decide, not the kernel.
Well, that's not the way things work. Keys have autorepeats which are generated kernel-side. I think libinput wants to change this to user space but certainly not application side.
Quoted 3 lines
If you agree that this patch is on the right track, an interface for the new function
rc_keydown_or_repeat()may have to be exposed to the BPF interface as well.
I'm not sure why that is needed.
Sean
Re: [PATCH 2/2] media: rtl28xxu: improve IR receiver
Hi Sean,
Quoted 17 lines · Sean Young
Hi Marko,
Quoted 14 lines · Marko Mäkelä
Quoted 13 lines · Sean Young
Quoted 5 lines · Marko Mäkelä
For protocols that do not use a toggle bit, the last parameter of
rc_keydown()will usually be toggle=0, and explicit calls torc_repeat()will be issued when needed. For those protocols, I thought that we would not wantrc_keydown()to set anyLIRC_SCANCODE_FLAG_REPEATflag under any circumstances.Toggle and repeat are distinct concepts.
rc_repeat()is for protocols which have a special repeat message, which carry no information other that "repeat the last message". However, all protocols repeat. Whether they use a special repeat message or not.
Quoted 8 lines
Quoted 6 lines
Quoted 2 lines
It's right that if a protocol repeats a message,
LIRC_SCANCODE_FLAG_REPEATis set.Is it right to set the flag when a message is being repeated due to user effort (repeatedly pressing and releasing a button, instead of holding the button down)?
The problem here is that the nec repeat is used by some remotes, but not others. Some nec remotes repeat the entire code every time. Our generic nec decoder cannot distinguish between the two. So, our nec decoder interprets both a nec repeat and a repeated code as "button being held down".
I see. My experience of remote control protocols is mostly limited to RC5.
Quoted 5 lines
Quoted 3 lines
As far as I understand, the change that you suggested would set the
LIRC_SCANCODE_FLAG_REPEATif I repeatedly press a button on the NEC protocol remote control, but not on an RC5 remote control.RC5 too.
I think that it is depends on the implementation of the firmware that runs on the remote control unit. If the button is released and quickly pressed again so that no keyboard matrix scan was scheduled to run in between, then the firmware could report it as a repeat event. Alternatively, every second IR message could be lost, so that the receiver would observe the same toggle bit value for every IR message that it receives.
Quoted 3 lines
Quoted 1 line
I tested the attached patch (which was created on 5.19-rc5, which failed to boot on my system for unrelated reasons) on Linux 5.17, on top of your fixes to rtl28xxu and rc-core.
You'll need to fix this.
The 5.19-rc5 boot failure could have been related to LUKS setup on that machine, because a kernel panic message was displayed before I was being prompted for an encryption key. The modules would not have been loaded at that point, so I do not think that it is related to my modifications.
When compiled for the v5.17 kernel release tag on another computer, the patch that implements rc_keydown_or_repeat() worked for me.
It does not look like there are many changes in drivers/media/rc between 5.17 and 5.19.
Quoted 3 lines
Quoted 1 line
If the user wants to quickly switch to channel 111 by quickly pressing the button three times, it should not be misreported as an auto-repeated event, but reported as 3 LIRC events without the "repeat" flag, and as 3 pairs of keydown and keyup events.
Ideally yes, if we can distinguish between the two.
Okay, I understand that we indeed cannot always do that.
Quoted 1 line
This could open up many possibilities. Would the decoded events also be available via some low-level interface to user-space programs, in addition to the input event driver?
Quoted 3 lines
Quoted 1 line
On the other hand, there should be no reason for an application to not honor repeat events for a volume button. That is of course up to the application to decide, not the kernel.
Well, that's not the way things work. Keys have autorepeats which are generated kernel-side. I think libinput wants to change this to user space but certainly not application side.
It is how https://tvdr.de works. I have been using it via two interfaces: a built-in LIRC interface, and vdr-plugin-remote for the Linux input device driver. In http://git.tvdr.de/vdr.git you can find that in some cases, normal key-presses are being distinguished from repeat events (git grep -w k_Repeat). This is the reason why I brought up the missing LIRC_SCANCODE_FLAG_REPEAT in the RC5 protocol decoder: VDR with the LIRC interface would observe that the user is repeatedly pressing a button even when the button is being held down.
Quoted 5 lines
Quoted 3 lines
If you agree that this patch is on the right track, an interface for the new function
rc_keydown_or_repeat()may have to be exposed to the BPF interface as well.I'm not sure why that is needed.
I am attaching a minimal version of the patch, just one hunk, like you suggested earlier. I did not observe any bad effects with either remote control unit I tested it with: RC5 and NEC, again, on the rtl28xxu and with your two commits, on the v5.17 tag.
Best regards,
Marko
From 7bdda9eccd704076297a0d0802c8638b4562c703 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marko=20M=C3=A4kel=C3=A4?= <[email protected]>
Date: Mon, 4 Jul 2022 21:56:08 +0300
Subject: [PATCH] media: rc: Always report LIRC repeat flag
The flag LIRC_SCANCODE_FLAG_REPEAT was never set by rc_keydown(). Previously it was only set by rc_repeat(), but not all protocol decoders invoke that function.
--- drivers/media/rc/rc-main.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/media/rc/rc-main.c +2 -1
@@ -786,7 +786,8 @@ static void ir_do_keydown(struct rc_dev *dev, enum rc_proto protocol, dev->last_toggle != toggle);struct lirc_scancode sc = { .scancode = scancode, .rc_proto = protocol, - .flags = toggle ?LIRC_SCANCODE_FLAG_TOGGLE :0 , + .flags = (toggle ?LIRC_SCANCODE_FLAG_TOGGLE :0 ) | + (!new_event ?LIRC_SCANCODE_FLAG_REPEAT :0 ), .keycode = keycode };
Signature · 1 line
2.36.1
Sean Young
maintainerRe: [PATCH 2/2] media: rtl28xxu: improve IR receiver
Hi Marko,
Quoted 15 lines · Marko Mäkelä
Quoted 8 lines · Sean Young
Quoted 5 lines · Marko Mäkelä
Quoted 1 line · Sean Young
On Sun, Jul 03, 2022 at 08:02:14PM +0300, Marko Mäkelä wrote:
I tested the attached patch (which was created on 5.19-rc5, which failed to boot on my system for unrelated reasons) on Linux 5.17, on top of your fixes to rtl28xxu and rc-core.
You'll need to fix this.
The 5.19-rc5 boot failure could have been related to LUKS setup on that machine, because a kernel panic message was displayed before I was being prompted for an encryption key. The modules would not have been loaded at that point, so I do not think that it is related to my modifications.
Quoted 5 lines
When compiled for the v5.17 kernel release tag on another computer, the patch that implements
rc_keydown_or_repeat()worked for me.It does not look like there are many changes in
drivers/media/rcbetween 5.17 and 5.19.
Your patch needs a Signed-off-by and it should not be attached, it should be inline in your email.
Quoted 5 lines
Quoted 1 line
This could open up many possibilities. Would the decoded events also be available via some low-level interface to user-space programs, in addition to the input event driver?
The plan was for it to run once, generate an eBPF program, attach that an exit. The eBPF program sends the decoded stuff to the lirc chardev in this struct:
This is the struct you're amending with LIRC_SCANCODE_FLAG_REPEAT.
Will that be sufficient for your needs?
Sean
Re: [PATCH 2/2] media: rtl28xxu: improve IR receiver
Hi Sean,
Quoted 16 lines · Sean Young
Quoted 15 lines · Marko Mäkelä
Quoted 8 lines · Sean Young
Quoted 5 lines · Marko Mäkelä
Quoted 1 line · Sean Young
On Sun, Jul 03, 2022 at 08:02:14PM +0300, Marko Mäkelä wrote:
I tested the attached patch (which was created on 5.19-rc5, which failed to boot on my system for unrelated reasons) on Linux 5.17, on top of your fixes to rtl28xxu and rc-core.
You'll need to fix this.
The 5.19-rc5 boot failure could have been related to LUKS setup on that machine, because a kernel panic message was displayed before I was being prompted for an encryption key. The modules would not have been loaded at that point, so I do not think that it is related to my modifications.
Quoted 8 lines
Quoted 5 lines
When compiled for the v5.17 kernel release tag on another computer, the patch that implements
rc_keydown_or_repeat()worked for me.It does not look like there are many changes in
drivers/media/rcbetween 5.17 and 5.19.Your patch needs a
Signed-off-byand it should not be attached, it should be inline in your email.
Thank you for your patience. I hope that I got it right. It would be my very first patch submission to the Linux kernel. I did not see it appear on this list archive yet. You are Cc'd.
Quoted 10 lines
Quoted 5 lines
Quoted 1 line
This could open up many possibilities. Would the decoded events also be available via some low-level interface to user-space programs, in addition to the input event driver?
The plan was for it to run once, generate an eBPF program, attach that an exit. The eBPF program sends the decoded stuff to the lirc chardev in this struct:
Quoted 5 lines
This is the struct you're amending with
LIRC_SCANCODE_FLAG_REPEAT.Will that be sufficient for your needs?
I think that it should cover the most common types of remote control units.
I can name an example of a complex IR remote control, which I think would be challenging to repurpose for controlling anything else than the original type of device. But, I would think that something Bluetooth or WLAN based on a touchscreen device will replace IR in such applications. The remote control of my air conditioner presents all settings on a local LCD. On every change, maybe after a short timeout of inactivity, it will send a long IR message with all the settings. The 32 bits of keycode or 64 bits of scancode would not be sufficient for that.
Marko
Sean Young
maintainerRe: [PATCH 2/2] media: rtl28xxu: improve IR receiver
Hi Marko,
Quoted 23 lines · Marko Mäkelä
Quoted 22 lines · Sean Young
Quoted 20 lines · Marko Mäkelä
Quoted 8 lines · Sean Young
Quoted 5 lines · Marko Mäkelä
Quoted 1 line · Sean Young
On Sun, Jul 03, 2022 at 08:02:14PM +0300, Marko Mäkelä wrote:
I tested the attached patch (which was created on 5.19-rc5, which failed to boot on my system for unrelated reasons) on Linux 5.17, on top of your fixes to rtl28xxu and rc-core.
You'll need to fix this.
The 5.19-rc5 boot failure could have been related to LUKS setup on that machine, because a kernel panic message was displayed before I was being prompted for an encryption key. The modules would not have been loaded at that point, so I do not think that it is related to my modifications.
When compiled for the v5.17 kernel release tag on another computer, the patch that implements
rc_keydown_or_repeat()worked for me.It does not look like there are many changes in
drivers/media/rcbetween 5.17 and 5.19.
Quoted 6 lines
Quoted 2 lines
Your patch needs a
Signed-off-byand it should not be attached, it should be inline in your email.Thank you for your patience. I hope that I got it right. It would be my very first patch submission to the Linux kernel. I did not see it appear on this list archive yet. You are Cc'd.
Thanks, I'll have a look.
Quoted 10 lines
Quoted 10 lines
Quoted 5 lines
Quoted 1 line
This could open up many possibilities. Would the decoded events also be available via some low-level interface to user-space programs, in addition to the input event driver?
The plan was for it to run once, generate an eBPF program, attach that an exit. The eBPF program sends the decoded stuff to the lirc chardev in this struct:
Quoted 7 lines
Quoted 5 lines
This is the struct you're amending with
LIRC_SCANCODE_FLAG_REPEAT.Will that be sufficient for your needs?
I think that it should cover the most common types of remote control units.
That is the hope.
Quoted 8 lines
I can name an example of a complex IR remote control, which I think would be challenging to repurpose for controlling anything else than the original type of device. But, I would think that something Bluetooth or WLAN based on a touchscreen device will replace IR in such applications. The remote control of my air conditioner presents all settings on a local LCD. On every change, maybe after a short timeout of inactivity, it will send a long IR message with all the settings. The 32 bits of keycode or 64 bits of scancode would not be sufficient for that.
There certainly can be an eBPF decoder for this type of IR, but I'm not sure what the use case is, because it's not key information. Maybe there is something else in eBPF which is more suitable.
BTW I'm sure it's possible to control an air conditioner with the cir tool above, using the IRP language. Would be great to see something like that. Transmission is already working, so just requires reverse engineering the protocol and writing some IRP for it. :)
Sean