On Fri, May 14, 2021 at 01:38:11PM +0200, Johan Hovold wrote:

On Thu, May 06, 2021 at 01:44:54PM +0100, Sean Young wrote:

quoted

See http://www.usbuirt.com/

No proper commit message?

No sure what to say here what's not already in the first line of the commit.

Maybe I could mention the fact this uses fdti usb serial chip.

quoted

Signed-off-by: Sean Young <sean@mess.org>

quoted

+// Read IR in wideband mode. The byte stream is delivered in packets,
+// and the values which come in two bytes may straddle two packets
+static void uirt_wideband(struct uirt *uirt, u32 len)
+{
+	uint i, duration, carrier, pulses;
+
+	for (i = 2; i < len; i++) {
+		switch (uirt->rx_state) {
+		case RX_STATE_INTERSPACE_HIGH:
+			uirt->rx_state = RX_STATE_INTERSPACE_LOW;
+			break;
+		case RX_STATE_INTERSPACE_LOW:
+			uirt->rx_state = RX_STATE_ON_HIGH;
+			break;
+		case RX_STATE_ON_HIGH:
+			uirt->high = uirt->in[i];
+			uirt->rx_state = RX_STATE_ON_LOW;
+			break;
+		case RX_STATE_ON_LOW:
+			// duration is in 400ns units
+			duration = (uirt->high << 8) | uirt->in[i];
+			uirt->last_duration = duration;
+			ir_raw_event_store(uirt->rc, &((struct ir_raw_event) {
+				.duration = DIV_ROUND_CLOSEST(duration * 2, 5),
+				.pulse = true,
+			}));
+			uirt->rx_state = RX_STATE_FREQ_HIGH;
+			break;
+		case RX_STATE_FREQ_HIGH:
+			if (uirt->in[i] & 0x80) {
+				uirt->high = uirt->in[i] & 0x7f;
+				uirt->rx_state = RX_STATE_FREQ_LOW;
+				break;
+			}
+
+			uirt->high = 0;
+			fallthrough;
+		case RX_STATE_FREQ_LOW:
+			pulses = (uirt->high << 8) | uirt->in[i];
+			if (pulses) {
+				dev_dbg(uirt->dev, "carrier duration %u pulses %u",
+					uirt->last_duration, pulses);
+
+				// calculate the Hz of pulses in duration 400ns units
+				carrier = DIV_ROUND_CLOSEST_ULL(pulses * 10000000ull,
+								uirt->last_duration * 4);

Looks like uirt->last_duration comes from the external device and you
don't have any sanity checks to prevent division by zero here.

Yes, you are right. In fact, if this is zero the reported IR duration is
also garbage. I'll fix this.

quoted

+				ir_raw_event_store(uirt->rc, &((struct ir_raw_event) {
+					.carrier = carrier,
+					.carrier_report = true,
+				}));
+			}
+			uirt->rx_state = RX_STATE_OFF_HIGH;
+			break;
+		case RX_STATE_OFF_HIGH:
+			if (uirt->in[i] == 0xff) {
+				ir_raw_event_store(uirt->rc, &((struct ir_raw_event) {
+					.duration = IR_TIMEOUT,
+					.timeout = true,
+				}));
+				uirt->rx_state = RX_STATE_INTERSPACE_HIGH;
+			} else {
+				uirt->high = uirt->in[i];
+				uirt->rx_state = RX_STATE_OFF_LOW;
+			}
+			break;
+		case RX_STATE_OFF_LOW:
+			// duration is in 400ns units
+			duration = (uirt->high << 8) | uirt->in[i];
+			ir_raw_event_store(uirt->rc, &((struct ir_raw_event) {
+				.duration = DIV_ROUND_CLOSEST(duration * 2, 5),
+				.pulse = false,
+			}));
+			uirt->rx_state = RX_STATE_ON_HIGH;
+			break;
+		}
+	}
+
+	ir_raw_event_handle(uirt->rc);
+}
+
+static void uirt_response(struct uirt *uirt, u32 len)
+{
+	int i;
+
+	dev_dbg(uirt->dev, "state:%d data: %*phN\n", uirt->cmd_state, len, uirt->in);
+
+	// Do we have more IR to transmit
+	if (uirt->cmd_state == CMD_STATE_STREAMING_TX && len >= 2 &&
+	    uirt->tx_len && uirt->in[0] & FTDI_RS0_CTS) {
+		u32 len;
+		int err;
+
+		len = min_t(u32, uirt->tx_len, MAX_PACKET);
+
+		memcpy(uirt->out, uirt->tx_buf, len);
+		uirt->urb_out->transfer_buffer_length = len;
+
+		uirt->tx_len -= len;
+		uirt->tx_buf += len;
+
+		err = usb_submit_urb(uirt->urb_out, GFP_ATOMIC);
+		if (err != 0)
+			dev_warn(uirt->dev,
+				 "failed to submit out urb: %d\n", err);
+	}
+
+	// if we only have two bytes, it just gives us the serial line status
+	if (len <= 2)
+		return;
+
+	switch (uirt->cmd_state) {
+	case CMD_STATE_GETVERSION:
+		if (len == 10) {
+			// check checksum
+			u8 checksum = 0;
+
+			for (i = 2; i < len; i++)
+				checksum += uirt->in[i];
+
+			if (checksum != 0) {
+				dev_err(uirt->dev, "checksum does not match: %*phN\n",
+					len, uirt->in);
+				return;
+			}
+
+			dev_info(uirt->dev,
+				 "USB-UIRT firmware v%u.%u protocol v%u.%u %02u-%02u-%04u",
+				 uirt->in[2], uirt->in[3], uirt->in[4],
+				 uirt->in[5], uirt->in[6], uirt->in[7],
+				 2000 + uirt->in[8]);
+
+			complete(&uirt->cmd_done);
+			uirt->cmd_state = CMD_STATE_IRDATA;
+			return;
+		}
+		break;
+	case CMD_STATE_DOTXRAW:
+	case CMD_STATE_STREAMING_TX:
+	case CMD_STATE_SETMODERAW:
+	case CMD_STATE_SETMODEWIDEBAND:
+		if (len == 3) {
+			switch (uirt->in[2]) {
+			case 0x20:
+				// 0x20 transmitting is expected during streaming tx
+				if (uirt->cmd_state == CMD_STATE_STREAMING_TX)
+					return;
+
+				if (uirt->cmd_state == CMD_STATE_DOTXRAW)
+					complete(&uirt->cmd_done);
+				else
+					dev_err(uirt->dev, "device transmitting");
+				break;
+			case 0x21:
+				if (uirt->tx_len) {
+					dev_err(uirt->dev, "tx completed with %u left to send",
+						uirt->tx_len);
+				} else {
+					if (uirt->cmd_state == CMD_STATE_SETMODERAW)
+						uirt->wideband = false;
+					if (uirt->cmd_state == CMD_STATE_SETMODEWIDEBAND)
+						uirt->wideband = true;
+
+					complete(&uirt->cmd_done);
+				}
+				break;
+			case 0x80:
+				dev_err(uirt->dev, "checksum error");
+				break;
+			case 0x81:
+				dev_err(uirt->dev, "timeout");
+				break;
+			case 0x82:
+				dev_err(uirt->dev, "command error");
+				break;
+			default:
+				dev_err(uirt->dev, "unknown response");
+			}
+
+			uirt->cmd_state = CMD_STATE_IRDATA;
+			return;
+		}
+	default:
+		break;
+	}
+
+	if (uirt->wideband)
+		uirt_wideband(uirt, len);
+	else
+		uirt_raw_mode(uirt, len);
+}

Your code assumes that you'll always get one message per transfer, but
since the device uses a regular FTDI chip with a FIFO this isn't
guaranteed. 

I guess you're talking about that data can straddle multiple packets
because there is a fifo involved, or there can be more data before/after
a command response.

Let me see what I can do about that.

quoted

+
+static void uirt_out_callback(struct urb *urb)
+{
+	struct uirt *uirt = urb->context;
+
+	if (urb->status)
+		dev_warn(uirt->dev, "out urb status: %d\n", urb->status);

Disconnecting a device shouldn't print a warning.

Ok.

quoted

+}
+
+static void uirt_in_callback(struct urb *urb)
+{
+	struct uirt *uirt = urb->context;
+	int ret;
+
+	if (urb->status == 0)
+		uirt_response(uirt, urb->actual_length);
+	else
+		dev_dbg(uirt->dev, "in urb status: %d\n", urb->status);
+
+	ret = usb_submit_urb(urb, GFP_ATOMIC);
+	if (ret && ret != -ENODEV)
+		dev_warn(uirt->dev, "failed to resubmit urb: %d\n", ret);
+}

quoted

+static int init_ftdi(struct usb_device *udev)
+{
+	int err;
+
+	// set the baud rate
+	err = usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
+			      FTDI_SIO_SET_BAUDRATE_REQUEST,
+			      FTDI_SIO_SET_BAUDRATE_REQUEST_TYPE,
+			      0x4009, 0x0001,
+			      NULL, 0, USB_CTRL_SET_TIMEOUT);
+	if (err)
+		return err;
+
+	// enabling rts/cts flow control
+	err = usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
+			      FTDI_SIO_SET_FLOW_CTRL_REQUEST,
+			      FTDI_SIO_SET_FLOW_CTRL_REQUEST_TYPE,
+			      0, FTDI_SIO_RTS_CTS_HS,
+			      NULL, 0, USB_CTRL_SET_TIMEOUT);
+	if (err)
+		return err;

Does the device UART actually have RTS wired up?

I don't know TBH. However it does have CTS wired up.

quoted

+
+	// Set latency in milliseconds. The USB-UIRT will generate a
+	// urb every latency milliseconds (IR or not), so this should be
+	// set as high as possible.
+	//
+	// The USB-UIRT has a IR timeout of 14ms (i.e. it sends the 0xff
+	// byte after 14ms of IR silence. So if we set the timeout to a higher
+	// value, the 0xff just gets sent in a separate packet, and there
+	// is a small delay for the IR to be processed.
+	return usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
+			      FTDI_SIO_SET_LATENCY_TIMER_REQUEST,
+			      FTDI_SIO_SET_LATENCY_TIMER_REQUEST_TYPE,
+			      50, 0,

Magic constant; it's not entirely clear from the code (or comment) what
50 is here. The latency go as high as 255 IIRC.

You're right, the comment is not clear. I'll update it.

quoted

+			      NULL, 0, USB_CTRL_SET_TIMEOUT);
+}
+
+static int uirt_probe(struct usb_interface *intf,
+		      const struct usb_device_id *id)
+{
+	struct usb_host_interface *idesc = intf->cur_altsetting;
+	struct usb_device *usbdev = interface_to_usbdev(intf);
+	struct usb_endpoint_descriptor *ep_in = NULL;
+	struct usb_endpoint_descriptor *ep_out = NULL;
+	struct usb_endpoint_descriptor *ep = NULL;
+	struct uirt *uirt;
+	struct rc_dev *rc;
+	struct urb *urb;
+	int i, pipe, err = -ENOMEM;
+
+	for (i = 0; i < idesc->desc.bNumEndpoints; i++) {
+		ep = &idesc->endpoint[i].desc;
+
+		if (!ep_in && usb_endpoint_is_bulk_in(ep) &&
+		    usb_endpoint_maxp(ep) == MAX_PACKET)
+			ep_in = ep;
+
+		if (!ep_out && usb_endpoint_is_bulk_out(ep) &&
+		    usb_endpoint_maxp(ep) == MAX_PACKET)
+			ep_out = ep;
+	}

You should use usb_find_common_endpoints() instead of open coding even
if you'd still need to check the endpoint size with your implementation.

I was not aware of that function, thanks. This makes the code much shorter.

quoted

+
+	if (!ep_in || !ep_out) {
+		dev_err(&intf->dev, "required endpoints not found\n");
+		return -ENODEV;
+	}
+
+	uirt = kzalloc(sizeof(*uirt), GFP_KERNEL);
+	if (!uirt)
+		return -ENOMEM;
+
+	uirt->in = kmalloc(MAX_PACKET, GFP_KERNEL);
+	if (!uirt->in)
+		goto free_uirt;
+
+	uirt->out = kmalloc(MAX_PACKET, GFP_KERNEL);
+	if (!uirt->out)
+		goto free_uirt;
+
+	rc = rc_allocate_device(RC_DRIVER_IR_RAW);
+	if (!rc)
+		goto free_uirt;
+
+	urb = usb_alloc_urb(0, GFP_KERNEL);
+	if (!urb)
+		goto free_rcdev;
+
+	pipe = usb_rcvbulkpipe(usbdev, ep_in->bEndpointAddress);
+	usb_fill_bulk_urb(urb, usbdev, pipe, uirt->in, MAX_PACKET,
+			  uirt_in_callback, uirt);
+	uirt->urb_in = urb;
+
+	urb = usb_alloc_urb(0, GFP_KERNEL);
+	if (!urb)
+		goto free_rcdev;
+
+	pipe = usb_sndbulkpipe(usbdev, ep_out->bEndpointAddress);
+	usb_fill_bulk_urb(urb, usbdev, pipe, uirt->out, MAX_PACKET,
+			  uirt_out_callback, uirt);
+
+	uirt->dev = &intf->dev;
+	uirt->usbdev = usbdev;
+	uirt->rc = rc;
+	uirt->urb_out = urb;
+	uirt->rx_state = RX_STATE_INTERSPACE_HIGH;
+
+	err = usb_submit_urb(uirt->urb_in, GFP_KERNEL);
+	if (err != 0) {
+		dev_err(uirt->dev, "failed to submit read urb: %d\n", err);
+		goto free_rcdev;
+	}
+
+	err = init_ftdi(usbdev);
+	if (err) {
+		dev_err(uirt->dev, "failed to setup ftdi: %d\n", err);
+		goto free_rcdev;
+	}
+
+	err = uirt_setup(uirt);
+	if (err)
+		goto free_rcdev;
+
+	usb_make_path(usbdev, uirt->phys, sizeof(uirt->phys));
+
+	rc->device_name = "USB-UIRT";
+	rc->driver_name = KBUILD_MODNAME;
+	rc->input_phys = uirt->phys;
+	usb_to_input_id(usbdev, &rc->input_id);
+	rc->dev.parent = &intf->dev;
+	rc->priv = uirt;
+	rc->tx_ir = uirt_tx;
+	rc->s_tx_carrier = uirt_set_tx_carrier;
+	rc->s_learning_mode = uirt_set_rx_wideband;
+	rc->allowed_protocols = RC_PROTO_BIT_ALL_IR_DECODER;
+	rc->map_name = RC_MAP_RC6_MCE;
+	rc->rx_resolution = UNIT_US;
+	rc->timeout = IR_TIMEOUT;
+
+	uirt_set_tx_carrier(rc, 38000);
+
+	err = rc_register_device(rc);
+	if (err)
+		goto free_rcdev;
+
+	usb_set_intfdata(intf, uirt);
+
+	return 0;
+
+free_rcdev:
+	usb_kill_urb(uirt->urb_in);
+	usb_kill_urb(uirt->urb_out);

Looks lazy to kill the URBs before they've been allocated, better to add
another label even if it's not a bug.

Ok, will do.

quoted

+	usb_free_urb(uirt->urb_in);
+	usb_free_urb(uirt->urb_out);
+	rc_free_device(rc);
+free_uirt:
+	kfree(uirt->in);
+	kfree(uirt->out);
+	kfree(uirt);
+	return err;
+}

Thank you for the review. I'll send out a v4 when its ready.


Sean