| Message ID | cover.1620304986.git.sean@mess.org |
|---|---|
| Headers | show |
| Series | IR driver for USB-UIRT device | expand |
On Thu, May 06, 2021 at 01:44:52PM +0100, Sean Young wrote: > This is a new rc-core driver for the USB-UIRT which you can see here > http://www.usbuirt.com/ > > This device is supported in lirc, via the usb serial kernel driver. This > driver is both for rc-core, which means it can use kernel/BPF decoding > ec. Also this implement is superior because it can: > - support learning mode > - setting transmit carrier > - larger transmits using streaming tx command This looks like something which should have been implemented as a line-discipline or serdev driver instead of reimplementing a minimal on-off ftdi driver and tying it closely to the RC subsystem. Why can't you just add support for the above features to whatever subsystem is managing this device today? Serdev still doesn't support hotplugging unfortunately so that route may take a bit more work. Johan
On Mon, May 10, 2021 at 10:15:14AM +0200, Johan Hovold wrote: > On Thu, May 06, 2021 at 01:44:52PM +0100, Sean Young wrote: > > This is a new rc-core driver for the USB-UIRT which you can see here > > http://www.usbuirt.com/ > > > > This device is supported in lirc, via the usb serial kernel driver. This > > driver is both for rc-core, which means it can use kernel/BPF decoding > > ec. Also this implement is superior because it can: > > - support learning mode > > - setting transmit carrier > > - larger transmits using streaming tx command > > This looks like something which should have been implemented as a > line-discipline or serdev driver instead of reimplementing a minimal > on-off ftdi driver and tying it closely to the RC subsystem. The device is an infrared device, I'm not sure what it is lost by doing it this way. The "minimal on-off ftdi driver" is super trivial. > Why can't you just add support for the above features to whatever > subsystem is managing this device today? > > Serdev still doesn't support hotplugging unfortunately so that route may > take a bit more work. There seems to be at least three ways of attaching drivers to serial devices: serio, serdev, and line-discipline. All seem to have limitations, as you say none of them provide a way of hotplugging devices without user-space attaching them through an ioctl or so. If you want to go down this route, then ideally you'd want a quirk on fdti saying "attach usb-uirt serdev device to this pid/vid". Considering module dependencies, I don't know how that could work without again userspace getting involved. Getting userspace involved seem like a big song and dance because the device uses an fdti device, even though it's not a serial port because it's hardwired for infrared functions, no db9 connector in sight. Sean
On Tue, May 11, 2021 at 11:32:19AM +0100, Sean Young wrote: > On Mon, May 10, 2021 at 10:15:14AM +0200, Johan Hovold wrote: > > On Thu, May 06, 2021 at 01:44:52PM +0100, Sean Young wrote: > > > This is a new rc-core driver for the USB-UIRT which you can see here > > > http://www.usbuirt.com/ > > > > > > This device is supported in lirc, via the usb serial kernel driver. This > > > driver is both for rc-core, which means it can use kernel/BPF decoding > > > ec. Also this implement is superior because it can: > > > - support learning mode > > > - setting transmit carrier > > > - larger transmits using streaming tx command > > > > This looks like something which should have been implemented as a > > line-discipline or serdev driver instead of reimplementing a minimal > > on-off ftdi driver and tying it closely to the RC subsystem. > > The device is an infrared device, I'm not sure what it is lost by > doing it this way. The "minimal on-off ftdi driver" is super trivial. It's still code duplication (and I meant to say "one-off" above"). What is preventing you from supporting the above functionality through lirc? > > Why can't you just add support for the above features to whatever > > subsystem is managing this device today? > > > > Serdev still doesn't support hotplugging unfortunately so that route may > > take a bit more work. > > There seems to be at least three ways of attaching drivers to serial > devices: serio, serdev, and line-discipline. All seem to have limitations, > as you say none of them provide a way of hotplugging devices without > user-space attaching them through an ioctl or so. serio is also a line-discipline driver, which unlike serdev needs to be set up by user space. And the problem with serdev is that it does not (yet) support hotplugging (specifically hangups) so it can't be enabled for USB serial just yet. > If you want to go down this route, then ideally you'd want a quirk on > fdti saying "attach usb-uirt serdev device to this pid/vid". Considering > module dependencies, I don't know how that could work without again > userspace getting involved. We'd just reuse or add another matching mechanism for USB devices. This can be handled without user-space interaction just fine as long as you have a dedicated device id as you do here. > Getting userspace involved seem like a big song and dance because the > device uses an fdti device, even though it's not a serial port because > it's hardwired for infrared functions, no db9 connector in sight. Far from every USB serial device have a db9 connector (e.g. modems, barcode scanners, development board consoles, etc.) and you still have a UART in your device. In principle reimplementing a one-off ftdi driver is wrong but since parts of the infrastructure needed to avoid this is still missing it may be acceptable, especially if you can't get this to work with lirc. Johan
On Fri, May 14, 2021 at 01:16:47PM +0200, Johan Hovold wrote: > On Tue, May 11, 2021 at 11:32:19AM +0100, Sean Young wrote: > > On Mon, May 10, 2021 at 10:15:14AM +0200, Johan Hovold wrote: > > > On Thu, May 06, 2021 at 01:44:52PM +0100, Sean Young wrote: > > > > This is a new rc-core driver for the USB-UIRT which you can see here > > > > http://www.usbuirt.com/ > > > > > > > > This device is supported in lirc, via the usb serial kernel driver. This > > > > driver is both for rc-core, which means it can use kernel/BPF decoding > > > > ec. Also this implement is superior because it can: > > > > - support learning mode > > > > - setting transmit carrier > > > > - larger transmits using streaming tx command > > > > > > This looks like something which should have been implemented as a > > > line-discipline or serdev driver instead of reimplementing a minimal > > > on-off ftdi driver and tying it closely to the RC subsystem. > > > > The device is an infrared device, I'm not sure what it is lost by > > doing it this way. The "minimal on-off ftdi driver" is super trivial. > > It's still code duplication (and I meant to say "one-off" above"). > > What is preventing you from supporting the above functionality through > lirc? I guess you mean the userspace lirc daemon, as opposed to the /dev/lirc chardev. If you use the lirc daemon, you don't use rc-core which comes with IR decoding using BPF IR decoding or in-kernel decoders, automatic setup of rc keymaps via udev. None of the modern ir-ctl/ir-keytable tooling will work, including the IRP encoder/BPF compiler I'm working on (very slowly). The other nice thing is that IR TX feeds data from an urb interrupt handler, so you don't need to rely userspace being scheduled quickly enough to feed more data before the device runs out. > > > > Why can't you just add support for the above features to whatever > > > subsystem is managing this device today? > > > > > > Serdev still doesn't support hotplugging unfortunately so that route may > > > take a bit more work. > > > > There seems to be at least three ways of attaching drivers to serial > > devices: serio, serdev, and line-discipline. All seem to have limitations, > > as you say none of them provide a way of hotplugging devices without > > user-space attaching them through an ioctl or so. > > serio is also a line-discipline driver, which unlike serdev needs to be > set up by user space. serio is unusable for this case. serio does not allow more than one byte to be written nor does it have callbacks for CTS readiness. > And the problem with serdev is that it does not (yet) support > hotplugging (specifically hangups) so it can't be enabled for USB serial > just yet. > > > If you want to go down this route, then ideally you'd want a quirk on > > fdti saying "attach usb-uirt serdev device to this pid/vid". Considering > > module dependencies, I don't know how that could work without again > > userspace getting involved. > > We'd just reuse or add another matching mechanism for USB devices. This > can be handled without user-space interaction just fine as long as you > have a dedicated device id as you do here. Right, ok. I don't quite follow what you have in mind. If at all possible keep me in the loop for any patches for this, I'm happy to test/re-write this driver and the drivers/media/rc/ir_toy.c driver on top of any such patches. There are a bunch old serial usb device IR devices and even older non-usb serial devices that would be nice to have supported, if anyone is still using them. > > Getting userspace involved seem like a big song and dance because the > > device uses an fdti device, even though it's not a serial port because > > it's hardwired for infrared functions, no db9 connector in sight. > > Far from every USB serial device have a db9 connector (e.g. modems, > barcode scanners, development board consoles, etc.) and you still have a > UART in your device. > > In principle reimplementing a one-off ftdi driver is wrong but since > parts of the infrastructure needed to avoid this is still missing it may > be acceptable, especially if you can't get this to work with lirc. Thanks -- that's a good compromise. Sean
On Sat, May 15, 2021 at 10:22:26AM +0100, Sean Young wrote: > On Fri, May 14, 2021 at 01:16:47PM +0200, Johan Hovold wrote: > > On Tue, May 11, 2021 at 11:32:19AM +0100, Sean Young wrote: > > > On Mon, May 10, 2021 at 10:15:14AM +0200, Johan Hovold wrote: > > > > On Thu, May 06, 2021 at 01:44:52PM +0100, Sean Young wrote: > > > > > This is a new rc-core driver for the USB-UIRT which you can see here > > > > > http://www.usbuirt.com/ > > > > > > > > > > This device is supported in lirc, via the usb serial kernel driver. This > > > > > driver is both for rc-core, which means it can use kernel/BPF decoding > > > > > ec. Also this implement is superior because it can: > > > > > - support learning mode > > > > > - setting transmit carrier > > > > > - larger transmits using streaming tx command > > > > > > > > This looks like something which should have been implemented as a > > > > line-discipline or serdev driver instead of reimplementing a minimal > > > > on-off ftdi driver and tying it closely to the RC subsystem. > > > > > > The device is an infrared device, I'm not sure what it is lost by > > > doing it this way. The "minimal on-off ftdi driver" is super trivial. > > > > It's still code duplication (and I meant to say "one-off" above"). > > > > What is preventing you from supporting the above functionality through > > lirc? > > I guess you mean the userspace lirc daemon, as opposed to the /dev/lirc > chardev. If you use the lirc daemon, you don't use rc-core which comes with > IR decoding using BPF IR decoding or in-kernel decoders, automatic setup of > rc keymaps via udev. None of the modern ir-ctl/ir-keytable tooling will > work, including the IRP encoder/BPF compiler I'm working on (very slowly). Ok, but apart from BPF that sound like other stuff and not the three items you list above? Is there anything preventing those items from being implemented in user space? Obviously a user-space implementation (e.g. accessing the device through /dev/ttyUSB0) is not going to be able to use in-kernel RC functionality. For that you'd need to use either a line-discipline or serdev driver, that is, a kernel driver, but not everything has to live in the kernel. > The other nice thing is that IR TX feeds data from an urb interrupt handler, > so you don't need to rely userspace being scheduled quickly enough to feed > more data before the device runs out. The tty layer and tty drivers provide write buffering so that need not be an issue. > > > > Why can't you just add support for the above features to whatever > > > > subsystem is managing this device today? > > > > > > > > Serdev still doesn't support hotplugging unfortunately so that route may > > > > take a bit more work. > > > > > > There seems to be at least three ways of attaching drivers to serial > > > devices: serio, serdev, and line-discipline. All seem to have limitations, > > > as you say none of them provide a way of hotplugging devices without > > > user-space attaching them through an ioctl or so. > > > > serio is also a line-discipline driver, which unlike serdev needs to be > > set up by user space. > > serio is unusable for this case. serio does not allow more than one byte > to be written nor does it have callbacks for CTS readiness. Ok, but you could still implement this as an rc-core line-discipline or serdev driver. And when you use hardware flow control as you do here, you shouldn't need any callbacks for CTS events either, right? > > And the problem with serdev is that it does not (yet) support > > hotplugging (specifically hangups) so it can't be enabled for USB serial > > just yet. > > > > > If you want to go down this route, then ideally you'd want a quirk on > > > fdti saying "attach usb-uirt serdev device to this pid/vid". Considering > > > module dependencies, I don't know how that could work without again > > > userspace getting involved. > > > > We'd just reuse or add another matching mechanism for USB devices. This > > can be handled without user-space interaction just fine as long as you > > have a dedicated device id as you do here. > > Right, ok. I don't quite follow what you have in mind. If at all possible > keep me in the loop for any patches for this, I'm happy to test/re-write > this driver and the drivers/media/rc/ir_toy.c driver on top of any such > patches. Thanks for that pointer. Judging from a quick look, the new driver appears to based on this one. By abstracting the serial interface bits in a generic RC serdev/ldisc driver you may be able reuse more code, even if I'm not in a position to judge how much of the IR protocol bits that can be shared. > There are a bunch old serial usb device IR devices and even older non-usb > serial devices that would be nice to have supported, if anyone is still > using them. I noticed that drivers/media/rc/serial_ir.c also appears to use a similar approach of implementing a minimal one-off serial (8250?) driver and tying it closely to RC core. This one might also benefit from using the standard serial drivers for the transport and having an RC layer on top. Currently it appears to use module-parameters for configuration instead of devicetree or some to-be-implemented interface for instantiating serdev devices from user space. Johan
On Mon, May 17, 2021 at 11:30:39AM +0200, Johan Hovold wrote: > On Sat, May 15, 2021 at 10:22:26AM +0100, Sean Young wrote: > > On Fri, May 14, 2021 at 01:16:47PM +0200, Johan Hovold wrote: > > > On Tue, May 11, 2021 at 11:32:19AM +0100, Sean Young wrote: > > > > On Mon, May 10, 2021 at 10:15:14AM +0200, Johan Hovold wrote: > > > > > On Thu, May 06, 2021 at 01:44:52PM +0100, Sean Young wrote: > > > > > > This is a new rc-core driver for the USB-UIRT which you can see here > > > > > > http://www.usbuirt.com/ > > > > > > > > > > > > This device is supported in lirc, via the usb serial kernel driver. This > > > > > > driver is both for rc-core, which means it can use kernel/BPF decoding > > > > > > ec. Also this implement is superior because it can: > > > > > > - support learning mode > > > > > > - setting transmit carrier > > > > > > - larger transmits using streaming tx command > > > > > > > > > > This looks like something which should have been implemented as a > > > > > line-discipline or serdev driver instead of reimplementing a minimal > > > > > on-off ftdi driver and tying it closely to the RC subsystem. > > > > > > > > The device is an infrared device, I'm not sure what it is lost by > > > > doing it this way. The "minimal on-off ftdi driver" is super trivial. > > > > > > It's still code duplication (and I meant to say "one-off" above"). > > > > > > What is preventing you from supporting the above functionality through > > > lirc? > > > > I guess you mean the userspace lirc daemon, as opposed to the /dev/lirc > > chardev. If you use the lirc daemon, you don't use rc-core which comes with > > IR decoding using BPF IR decoding or in-kernel decoders, automatic setup of > > rc keymaps via udev. None of the modern ir-ctl/ir-keytable tooling will > > work, including the IRP encoder/BPF compiler I'm working on (very slowly). > > Ok, but apart from BPF that sound like other stuff and not the three > items you list above? Is there anything preventing those items from > being implemented in user space? Well, after IR is decoded, you want to send decoded scancodes/key codes to any input device, so your remote works just like any input device. > Obviously a user-space implementation (e.g. accessing the device through > /dev/ttyUSB0) is not going to be able to use in-kernel RC functionality. > For that you'd need to use either a line-discipline or serdev driver, > that is, a kernel driver, but not everything has to live in the kernel. No, of course not. A lot of kernel functionality could live in user space, for sure. But it doesn't. Even if the input problem can be resolved, the lirc daemon is pretty outdated. All the existing functionality in-kernel would have to be re-written for userspace, and it would be total duplication of code, which you do not like. You end up with a userspace implementation and a kernel space implementation. There are many other IR devices that can be controlled through libusb in userspace, which could work entirely in userspace. Same for i2c IR devices, those could work entirely from userspace too. I don't know what the state is of pci userspace drivers, but there certainly have been patches for that; the line is not so clear. I do think that the monolithic approach to kernels necessarily invokes discussions like these, and there are no perfect answers. > > The other nice thing is that IR TX feeds data from an urb interrupt handler, > > so you don't need to rely userspace being scheduled quickly enough to feed > > more data before the device runs out. > > The tty layer and tty drivers provide write buffering so that need not > be an issue. Ok. I don't know what the size of the write buffer is or what the maximum TX size is; the IR device supports infinite streaming. > > > > > Why can't you just add support for the above features to whatever > > > > > subsystem is managing this device today? > > > > > > > > > > Serdev still doesn't support hotplugging unfortunately so that route may > > > > > take a bit more work. > > > > > > > > There seems to be at least three ways of attaching drivers to serial > > > > devices: serio, serdev, and line-discipline. All seem to have limitations, > > > > as you say none of them provide a way of hotplugging devices without > > > > user-space attaching them through an ioctl or so. > > > > > > serio is also a line-discipline driver, which unlike serdev needs to be > > > set up by user space. > > > > serio is unusable for this case. serio does not allow more than one byte > > to be written nor does it have callbacks for CTS readiness. > > Ok, but you could still implement this as an rc-core line-discipline or > serdev driver. And when you use hardware flow control as you do here, > you shouldn't need any callbacks for CTS events either, right? I like the idea of serdev, but like you say it's not ready yet because of absence of hotplugging. See above about streaming. > > > And the problem with serdev is that it does not (yet) support > > > hotplugging (specifically hangups) so it can't be enabled for USB serial > > > just yet. > > > > > > > If you want to go down this route, then ideally you'd want a quirk on > > > > fdti saying "attach usb-uirt serdev device to this pid/vid". Considering > > > > module dependencies, I don't know how that could work without again > > > > userspace getting involved. > > > > > > We'd just reuse or add another matching mechanism for USB devices. This > > > can be handled without user-space interaction just fine as long as you > > > have a dedicated device id as you do here. > > > > Right, ok. I don't quite follow what you have in mind. If at all possible > > keep me in the loop for any patches for this, I'm happy to test/re-write > > this driver and the drivers/media/rc/ir_toy.c driver on top of any such > > patches. > > Thanks for that pointer. Judging from a quick look, the new driver > appears to based on this one. By abstracting the serial interface bits > in a generic RC serdev/ldisc driver you may be able reuse more code, > even if I'm not in a position to judge how much of the IR protocol bits > that can be shared. Yes, I agree. Once hotplugging is in place. If you have patches for that, please CC me and I can see if will work for IR drivers. > > There are a bunch old serial usb device IR devices and even older non-usb > > serial devices that would be nice to have supported, if anyone is still > > using them. > > I noticed that drivers/media/rc/serial_ir.c also appears to use a > similar approach of implementing a minimal one-off serial (8250?) driver > and tying it closely to RC core. This one might also benefit from using > the standard serial drivers for the transport and having an RC layer on > top. > > Currently it appears to use module-parameters for configuration instead > of devicetree or some to-be-implemented interface for instantiating > serdev devices from user space. serial_ir.c (called lirc_serial in the past) is a bit special. It uses an IR sensor connected directly to DCD and an output led connected to DTR, (depending on the configuration used). I don't think this can be done with a standard serial port driver. If it is possible, I'd like to know. It's a bit of an insane way of doing things, but it's super cheap to build. Sean
On Mon, May 17, 2021 at 11:35:22AM +0100, Sean Young wrote: > On Mon, May 17, 2021 at 11:30:39AM +0200, Johan Hovold wrote: > > On Sat, May 15, 2021 at 10:22:26AM +0100, Sean Young wrote: > > > On Fri, May 14, 2021 at 01:16:47PM +0200, Johan Hovold wrote: > > > > On Tue, May 11, 2021 at 11:32:19AM +0100, Sean Young wrote: > > > > > On Mon, May 10, 2021 at 10:15:14AM +0200, Johan Hovold wrote: > > > > > > On Thu, May 06, 2021 at 01:44:52PM +0100, Sean Young wrote: > > > > > > > This is a new rc-core driver for the USB-UIRT which you can see here > > > > > > > http://www.usbuirt.com/ > > > > > > > > > > > > > > This device is supported in lirc, via the usb serial kernel driver. This > > > > > > > driver is both for rc-core, which means it can use kernel/BPF decoding > > > > > > > ec. Also this implement is superior because it can: > > > > > > > - support learning mode > > > > > > > - setting transmit carrier > > > > > > > - larger transmits using streaming tx command > > > > > > > > > > > > This looks like something which should have been implemented as a > > > > > > line-discipline or serdev driver instead of reimplementing a minimal > > > > > > on-off ftdi driver and tying it closely to the RC subsystem. > > > > > > > > > > The device is an infrared device, I'm not sure what it is lost by > > > > > doing it this way. The "minimal on-off ftdi driver" is super trivial. > > > > > > > > It's still code duplication (and I meant to say "one-off" above"). > > > > > > > > What is preventing you from supporting the above functionality through > > > > lirc? > > > > > > I guess you mean the userspace lirc daemon, as opposed to the /dev/lirc > > > chardev. If you use the lirc daemon, you don't use rc-core which comes with > > > IR decoding using BPF IR decoding or in-kernel decoders, automatic setup of > > > rc keymaps via udev. None of the modern ir-ctl/ir-keytable tooling will > > > work, including the IRP encoder/BPF compiler I'm working on (very slowly). > > > > Ok, but apart from BPF that sound like other stuff and not the three > > items you list above? Is there anything preventing those items from > > being implemented in user space? > > Well, after IR is decoded, you want to send decoded scancodes/key codes > to any input device, so your remote works just like any input device. There is another advantage. IR decoding in userspace involves a lot more context switches/scheduling, and it can feel laggy when the cpu is under load (e.g. video decoding on the CPU). When you press pause/play/stop or so you expect the response the instantatiously. A 100ms delay is noticable. Alternatively the key-up events get delayed and you end up with multiple un-intended button repeats. None of this happens with kernel decoding and it feels very snappy. Sean
On Mon, May 17, 2021 at 11:35:22AM +0100, Sean Young wrote: > On Mon, May 17, 2021 at 11:30:39AM +0200, Johan Hovold wrote: > > On Sat, May 15, 2021 at 10:22:26AM +0100, Sean Young wrote: > > > On Fri, May 14, 2021 at 01:16:47PM +0200, Johan Hovold wrote: > > > > On Tue, May 11, 2021 at 11:32:19AM +0100, Sean Young wrote: > > > > > On Mon, May 10, 2021 at 10:15:14AM +0200, Johan Hovold wrote: > > > > > > On Thu, May 06, 2021 at 01:44:52PM +0100, Sean Young wrote: > > > > > > > This is a new rc-core driver for the USB-UIRT which you can see here > > > > > > > http://www.usbuirt.com/ > > > > > > > > > > > > > > This device is supported in lirc, via the usb serial kernel driver. This > > > > > > > driver is both for rc-core, which means it can use kernel/BPF decoding > > > > > > > ec. Also this implement is superior because it can: > > > > > > > - support learning mode > > > > > > > - setting transmit carrier > > > > > > > - larger transmits using streaming tx command > > > > > > > > > > > > This looks like something which should have been implemented as a > > > > > > line-discipline or serdev driver instead of reimplementing a minimal > > > > > > on-off ftdi driver and tying it closely to the RC subsystem. > > > > > > > > > > The device is an infrared device, I'm not sure what it is lost by > > > > > doing it this way. The "minimal on-off ftdi driver" is super trivial. > > > > > > > > It's still code duplication (and I meant to say "one-off" above"). > > > > > > > > What is preventing you from supporting the above functionality through > > > > lirc? > > > > > > I guess you mean the userspace lirc daemon, as opposed to the /dev/lirc > > > chardev. If you use the lirc daemon, you don't use rc-core which comes with > > > IR decoding using BPF IR decoding or in-kernel decoders, automatic setup of > > > rc keymaps via udev. None of the modern ir-ctl/ir-keytable tooling will > > > work, including the IRP encoder/BPF compiler I'm working on (very slowly). > > > > Ok, but apart from BPF that sound like other stuff and not the three > > items you list above? Is there anything preventing those items from > > being implemented in user space? > > Well, after IR is decoded, you want to send decoded scancodes/key codes > to any input device, so your remote works just like any input device. Isn't that already handled by lircd using uinput? > > Obviously a user-space implementation (e.g. accessing the device through > > /dev/ttyUSB0) is not going to be able to use in-kernel RC functionality. > > For that you'd need to use either a line-discipline or serdev driver, > > that is, a kernel driver, but not everything has to live in the kernel. > > No, of course not. A lot of kernel functionality could live in user space, > for sure. But it doesn't. > > Even if the input problem can be resolved, the lirc daemon is pretty outdated. > All the existing functionality in-kernel would have to be re-written for > userspace, and it would be total duplication of code, which you do not like. > You end up with a userspace implementation and a kernel space implementation. > > There are many other IR devices that can be controlled through libusb in > userspace, which could work entirely in userspace. Same for i2c IR > devices, those could work entirely from userspace too. I don't know what > the state is of pci userspace drivers, but there certainly have been patches > for that; the line is not so clear. > > I do think that the monolithic approach to kernels necessarily invokes > discussions like these, and there are no perfect answers. I hear you, but we still need to have those discussions from time to time to make sure our architecture is sane. One of the problems today with the kernel development process appears to be that too few questions are asked. If it builds, ship it... In this case the device in question can already be handled in user space by lirqd (at least to some degree) and we have infrastructure for writing in-kernel serial client drivers (i.e. ldisc/serdev). While neither option may support everything we need today, adding further one-off serial-device + client combo drivers is still a step in the wrong direction. But I think I've got that point across by now. > > > The other nice thing is that IR TX feeds data from an urb interrupt handler, > > > so you don't need to rely userspace being scheduled quickly enough to feed > > > more data before the device runs out. > > > > The tty layer and tty drivers provide write buffering so that need not > > be an issue. > > Ok. I don't know what the size of the write buffer is or what the maximum > TX size is; the IR device supports infinite streaming. Our tty drivers typically have at least a 4k buffer for transmission. Surely that should be enough for a remote control but perhaps there are other more demanding applications? > > Thanks for that pointer. Judging from a quick look, the new driver > > appears to based on this one. By abstracting the serial interface bits > > in a generic RC serdev/ldisc driver you may be able reuse more code, > > even if I'm not in a position to judge how much of the IR protocol bits > > that can be shared. > > Yes, I agree. Once hotplugging is in place. If you have patches for that, > please CC me and I can see if will work for IR drivers. Let's hope someone steps up to fund that work then. > > > There are a bunch old serial usb device IR devices and even older non-usb > > > serial devices that would be nice to have supported, if anyone is still > > > using them. > > > > I noticed that drivers/media/rc/serial_ir.c also appears to use a > > similar approach of implementing a minimal one-off serial (8250?) driver > > and tying it closely to RC core. This one might also benefit from using > > the standard serial drivers for the transport and having an RC layer on > > top. > > > > Currently it appears to use module-parameters for configuration instead > > of devicetree or some to-be-implemented interface for instantiating > > serdev devices from user space. > > serial_ir.c (called lirc_serial in the past) is a bit special. It uses > an IR sensor connected directly to DCD and an output led connected to DTR, > (depending on the configuration used). I don't think this can be done with > a standard serial port driver. If it is possible, I'd like to know. > > It's a bit of an insane way of doing things, but it's super cheap to build. Heh, ok. Perhaps we'll just have to live with this one then. Johan
On Mon, May 17, 2021 at 01:35:09PM +0100, Sean Young wrote: > On Mon, May 17, 2021 at 11:35:22AM +0100, Sean Young wrote: > > On Mon, May 17, 2021 at 11:30:39AM +0200, Johan Hovold wrote: > > > Ok, but apart from BPF that sound like other stuff and not the three > > > items you list above? Is there anything preventing those items from > > > being implemented in user space? > > > > Well, after IR is decoded, you want to send decoded scancodes/key codes > > to any input device, so your remote works just like any input device. > > There is another advantage. IR decoding in userspace involves a lot more > context switches/scheduling, and it can feel laggy when the cpu is under > load (e.g. video decoding on the CPU). When you press pause/play/stop or > so you expect the response the instantatiously. A 100ms delay is noticable. RT scheduling? Sounds like you should be able to handle this way faster than 100 ms. > Alternatively the key-up events get delayed and you end up with multiple > un-intended button repeats. None of this happens with kernel decoding and > it feels very snappy. Yeah, perhaps it's best handled in-kernel, but it seems we should be able to handle a simple key press within 20 ms or whatever the critical latency is here using either option (kernel or user-space driver). Johan
On Thu, May 20, 2021 at 03:31:34PM +0200, Johan Hovold wrote: > On Mon, May 17, 2021 at 11:35:22AM +0100, Sean Young wrote: > > On Mon, May 17, 2021 at 11:30:39AM +0200, Johan Hovold wrote: > > > On Sat, May 15, 2021 at 10:22:26AM +0100, Sean Young wrote: > > > > The other nice thing is that IR TX feeds data from an urb interrupt handler, > > > > so you don't need to rely userspace being scheduled quickly enough to feed > > > > more data before the device runs out. > > > > > > The tty layer and tty drivers provide write buffering so that need not > > > be an issue. > > > > Ok. I don't know what the size of the write buffer is or what the maximum > > TX size is; the IR device supports infinite streaming. > > Our tty drivers typically have at least a 4k buffer for transmission. > Surely that should be enough for a remote control but perhaps there are > other more demanding applications? That's more than enough. The most demanding consumer IR I know of, is an air conditioner which encodes temperature and a few others things. That's a maximum of 439 pulse/spaces which should into 1k. > > > > Thanks for that pointer. Judging from a quick look, the new driver > > > appears to based on this one. By abstracting the serial interface bits > > > in a generic RC serdev/ldisc driver you may be able reuse more code, > > > even if I'm not in a position to judge how much of the IR protocol bits > > > that can be shared. > > > > Yes, I agree. Once hotplugging is in place. If you have patches for that, > > please CC me and I can see if will work for IR drivers. > > Let's hope someone steps up to fund that work then. I'm just a volunteer. I've literally never heard anything about kernel work being funded by anyone. Would you mind giving a brief summary what is needed to implement hotplugging for serdev? I get the impression it's not a lot of work, but I'm probably missing something obvious. Sean
Sorry about the late reply on this one. On Fri, May 21, 2021 at 12:39:54PM +0100, Sean Young wrote: > On Thu, May 20, 2021 at 03:31:34PM +0200, Johan Hovold wrote: > > On Mon, May 17, 2021 at 11:35:22AM +0100, Sean Young wrote: > > > On Mon, May 17, 2021 at 11:30:39AM +0200, Johan Hovold wrote: > > > > Thanks for that pointer. Judging from a quick look, the new driver > > > > appears to based on this one. By abstracting the serial interface bits > > > > in a generic RC serdev/ldisc driver you may be able reuse more code, > > > > even if I'm not in a position to judge how much of the IR protocol bits > > > > that can be shared. > > > > > > Yes, I agree. Once hotplugging is in place. If you have patches for that, > > > please CC me and I can see if will work for IR drivers. > > > > Let's hope someone steps up to fund that work then. > > I'm just a volunteer. I've literally never heard anything about kernel work > being funded by anyone. Someone always pays whether it's a client, an employer or you yourself with your spare time. > Would you mind giving a brief summary what is needed to implement hotplugging > for serdev? I get the impression it's not a lot of work, but I'm probably > missing something obvious. First, it's the matching bits we already touched on where we would like to be able to use something like devicetree overlays to avoid rolling a new mechanism for every bus. But devicetree overlays has its issues currently (e.g. theres no user-space interface for providing them and last time I checked they could not be reverted). Second, serdev does not use the file abstraction and does not support hangups which is used to implement tty hotplugging (e.g. by signalling the consumer and making all file operations become noops after a disconnect). This would take some thinking-through to get right, and hopefully it can be done without having to update every current serdev driver. Retrofitting serdev into the tty layer wasn't painless and broke things here and there. Supporting hotplugging should be doable but it's not as straight-forward as it may sound. Johan
On Wed, Jun 23, 2021 at 03:10:20PM +0200, Johan Hovold wrote: > Sorry about the late reply on this one too. > > On Tue, May 25, 2021 at 02:25:49PM +0200, Oliver Neukum wrote: > > Am Donnerstag, den 20.05.2021, 15:31 +0200 schrieb Johan Hovold: > > > > Isn't that already handled by lircd using uinput? > > > > The problem with that reasoning, though it is true, is > > > > 1) We would need to remove a lot of subsystems if we took that > > to the logical conclusion. > > Removing code is always nice. ;) So rather than adding hotplug to serdev, we should remove line-discipline, serdev, and serio and all its drivers from the kernel? This is taking your own argument and applying it your code. > > 2) It makes runtime PM much harder > > Possibly, depends on the bus and device. > > > 3) We end up with two classes of LIRC devices > > We already do, right? That's kind of my point since we have lircd > supporting uinput. This is not an either-or situation, lircd is the "old" solution which is slowing being supplanted with rc-core. All the new keymaps are rc-core and do not work with lircd. The new rc-core tooling (in the v4l-utils package) does not work with lircd. lircd hasn't had any real patches merged for years now. There is whole new tooling in the works for rc-core which is not compatible with lircd. > > > I hear you, but we still need to have those discussions from time to > > > time to make sure our architecture is sane. One of the problems today > > > with the kernel development process appears to be that too few > > > questions > > > are asked. If it builds, ship it... > > > > Indeed, so, could we force a line discipline on a device on the kernel > > level? Code duplication is bad. > > Not sure I understand what you have mind here. serdev is sort of a > line-discipline which we'd "force" on a device if there's a matching > description in devicetree, while line disciplines always need to be > instantiated by user space. Or are you referring to ldisc/serdev code > reuse? I am pretty sure Oliver is suggesting that all ldisc/serdev code in the kernel is duplication of code which can be done in userspace, by your own argument. > > > But I think I've got that point across by now. > > > > Yes and and we need to think about the conclusion we draw from > > that point. It seems to me that an architecture that pushes data > > through the whole tty layer into a demon, then through uinput > > is definitely not elegant. > > The elegant answer is serdev, but it does not yet support the features > needed in this case (i.e. hotplugging). > > Since we already support user-space drivers for these devices, I see > nothing wrong with implementing support for another one in user space > unless there are strong reasons against doing so (e.g. performance, > pm or usability). But if uinput works then great, we're done. As discussed lircd has terrible latency, and lircd is out of date and unmaintained and does not work with modern tooling and keymaps. Also essentially your saying that any input device that connects to a serial port should be done in user space. There are a ton of kernel drivers doing exactly that, and that is why serio exists in the first place. Sean
On Thu, Jun 24, 2021 at 10:13:49AM +0100, Sean Young wrote: > On Wed, Jun 23, 2021 at 03:10:20PM +0200, Johan Hovold wrote: > > Sorry about the late reply on this one too. > > > > On Tue, May 25, 2021 at 02:25:49PM +0200, Oliver Neukum wrote: > > > Am Donnerstag, den 20.05.2021, 15:31 +0200 schrieb Johan Hovold: > > > > > > Isn't that already handled by lircd using uinput? > > > > > > The problem with that reasoning, though it is true, is > > > > > > 1) We would need to remove a lot of subsystems if we took that > > > to the logical conclusion. > > > > Removing code is always nice. ;) > > So rather than adding hotplug to serdev, we should remove line-discipline, > serdev, and serio and all its drivers from the kernel? This is taking > your own argument and applying it your code. Not at all. Not everything can be done in user space, but some things can. > > > 3) We end up with two classes of LIRC devices > > > > We already do, right? That's kind of my point since we have lircd > > supporting uinput. > > This is not an either-or situation, lircd is the "old" solution which is > slowing being supplanted with rc-core. All the new keymaps are rc-core and > do not work with lircd. The new rc-core tooling (in the v4l-utils package) > does not work with lircd. lircd hasn't had any real patches merged for years > now. > > There is whole new tooling in the works for rc-core which is not compatible > with lircd. Sure, you already explained that. I was just asking (earlier) why you didn't use the infrastructure that's already in place. If there are good reasons for not doing so then fine. > > > > I hear you, but we still need to have those discussions from time to > > > > time to make sure our architecture is sane. One of the problems today > > > > with the kernel development process appears to be that too few > > > > questions > > > > are asked. If it builds, ship it... > > > > > > Indeed, so, could we force a line discipline on a device on the kernel > > > level? Code duplication is bad. > > > > Not sure I understand what you have mind here. serdev is sort of a > > line-discipline which we'd "force" on a device if there's a matching > > description in devicetree, while line disciplines always need to be > > instantiated by user space. Or are you referring to ldisc/serdev code > > reuse? > > I am pretty sure Oliver is suggesting that all ldisc/serdev code in > the kernel is duplication of code which can be done in userspace, by your > own argument. See above. > > > > But I think I've got that point across by now. > > > > > > Yes and and we need to think about the conclusion we draw from > > > that point. It seems to me that an architecture that pushes data > > > through the whole tty layer into a demon, then through uinput > > > is definitely not elegant. > > > > The elegant answer is serdev, but it does not yet support the features > > needed in this case (i.e. hotplugging). > > > > Since we already support user-space drivers for these devices, I see > > nothing wrong with implementing support for another one in user space > > unless there are strong reasons against doing so (e.g. performance, > > pm or usability). But if uinput works then great, we're done. > > As discussed lircd has terrible latency, and lircd is out of date and > unmaintained and does not work with modern tooling and keymaps. > > Also essentially your saying that any input device that connects to a > serial port should be done in user space. There are a ton of kernel > drivers doing exactly that, and that is why serio exists in the first > place. I'm not, again see above. I'm saying that we should not make one-off copies of serial drivers if we can avoid it. In this case the limitations of lircd and the lack of hotplugging in serdev may be a sufficient reason for making an exception. As we've already discussed. Johan
On Thu, Jun 24, 2021 at 11:41:28AM +0200, Johan Hovold wrote: > I'm not, again see above. I'm saying that we should not make one-off > copies of serial drivers if we can avoid it. > > In this case the limitations of lircd and the lack of hotplugging in > serdev may be a sufficient reason for making an exception. As we've > already discussed. Great, thanks very much. I totally agree a serdev solution would be preferable. In that case, can have your Signed-off-by for the ftdi_sio.c change, or could this be merged through the usb tree please? Thanks Sean
On Fri, Jun 25, 2021 at 09:08:00AM +0100, Sean Young wrote: > On Thu, Jun 24, 2021 at 11:41:28AM +0200, Johan Hovold wrote: > > I'm not, again see above. I'm saying that we should not make one-off > > copies of serial drivers if we can avoid it. > > > > In this case the limitations of lircd and the lack of hotplugging in > > serdev may be a sufficient reason for making an exception. As we've > > already discussed. > > Great, thanks very much. I totally agree a serdev solution would be > preferable. > > In that case, can have your Signed-off-by for the ftdi_sio.c change, or > could this be merged through the usb tree please? Sure, I can take the ftdi change (for the current -rc) once the ir driver hits the media tree. Johan
This is a new rc-core driver for the USB-UIRT which you can see here http://www.usbuirt.com/ This device is supported in lirc, via the usb serial kernel driver. This driver is both for rc-core, which means it can use kernel/BPF decoding ec. Also this implement is superior because it can: - support learning mode - setting transmit carrier - larger transmits using streaming tx command Changes since v2: - Fixed race condition is disconnect - Removed superfluous kmalloc in short tx Changes since v1: - Review comments from Oliver Neukum - Simplified wideband read function Sean Young (3): USB: serial: move ftdi_sio.h into include directories media: rc: new driver for USB-UIRT device USB: serial: blacklist USB-UIRT when driver is selected drivers/media/rc/Kconfig | 11 + drivers/media/rc/Makefile | 1 + drivers/media/rc/uirt.c | 723 ++++++++++++++++++ drivers/usb/serial/ftdi_sio.c | 10 +- .../serial => include/linux/usb}/ftdi_sio.h | 0 5 files changed, 744 insertions(+), 1 deletion(-) create mode 100644 drivers/media/rc/uirt.c rename {drivers/usb/serial => include/linux/usb}/ftdi_sio.h (100%)