[PATCH 1/1] Input: add sensable phantom driver

[PATCH 1/1] Input: add sensable phantom driver

[Jiri Slaby]

add sensable phantom driver

Signed-off-by: Jiri Slaby <jirislaby@gmail.com>

---
commit bb9798e15d86ada19f4d15e31124dc240df78899
tree 06d0ac31976d92128b4e43b4d009810292bdf7a0
parent 551535195b52a4d02b476bbbdf5ca613b8e1afa2
author Jiri Slaby <jirislaby@gmail.com> Wed, 07 Mar 2007 12:29:27 +0100
committer Jiri Slaby <jirislaby@gmail.com> Wed, 07 Mar 2007 12:29:27 +0100

 Documentation/ioctl-number.txt |    1 
 MAINTAINERS                    |    5 
 drivers/input/misc/Kconfig     |    9 +
 drivers/input/misc/Makefile    |    1 
 drivers/input/misc/phantom.c   |  447 ++++++++++++++++++++++++++++++++++++++++
 include/linux/phantom.h        |   48 ++++
 6 files changed, 511 insertions(+), 0 deletions(-)

diff --git a/Documentation/ioctl-number.txt b/Documentation/ioctl-number.txt
index 8f750c0..38da829 100644
--- a/Documentation/ioctl-number.txt
+++ b/Documentation/ioctl-number.txt
@@ -128,6 +128,7 @@ Code	Seq#	Include File		Comments
 					<mailto:zapman@interlan.net>
 'i'	00-3F	linux/i2o.h
 'j'	00-3F	linux/joystick.h
+'k'	00-0F	linux/phantom.h		Sensable PHANToM
 'l'	00-3F	linux/tcfs_fs.h		transparent cryptographic file system
 					<http://mikonos.dia.unisa.it/tcfs>
 'l'	40-7F	linux/udf_fs_i.h	in development:
diff --git a/MAINTAINERS b/MAINTAINERS
index 00fa7f1..d4ee114 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3076,6 +3076,11 @@ L: 	selinux@tycho.nsa.gov (subscribers-only, general discussion)
 W:	http://www.nsa.gov/selinux
 S:	Supported
 
+SENSABLE PHANTOM
+P:	Jiri Slaby
+M:	jirislaby@gmail.com
+S:	Maintained
+
 SERIAL ATA (SATA) SUBSYSTEM:
 P:	Jeff Garzik
 M:	jgarzik@pobox.com
diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
index 5694115..68fed97 100644
--- a/drivers/input/misc/Kconfig
+++ b/drivers/input/misc/Kconfig
@@ -98,4 +98,13 @@ config HP_SDC_RTC
 	  Say Y here if you want to support the built-in real time clock
 	  of the HP SDC controller.
 
+config PHANTOM
+	tristate "Sensable PHANToM"
+	depends on PCI
+	help
+	  Say Y here if you want to build a driver for Sensable PHANToM device.
+
+	  If you choose to build module, its name will be phantom. If unsure,
+	  say N here.
+
 endif
diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
index 9f08f27..3ab3cc2 100644
--- a/drivers/input/misc/Makefile
+++ b/drivers/input/misc/Makefile
@@ -13,3 +13,4 @@ obj-$(CONFIG_INPUT_WISTRON_BTNS)	+= wistron_btns.o
 obj-$(CONFIG_INPUT_ATLAS_BTNS)		+= atlas_btns.o
 obj-$(CONFIG_HP_SDC_RTC)		+= hp_sdc_rtc.o
 obj-$(CONFIG_INPUT_IXP4XX_BEEPER)	+= ixp4xx-beeper.o
+obj-$(CONFIG_PHANTOM)			+= phantom.o
diff --git a/drivers/input/misc/phantom.c b/drivers/input/misc/phantom.c
new file mode 100644
index 0000000..1dcf775
--- /dev/null
+++ b/drivers/input/misc/phantom.c
@@ -0,0 +1,447 @@
+/*
+ *  Copyright (C) 2005-2007 Jiri Slaby <jirislaby@gmail.com>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  You need an userspace library to cooperate with this library. It may be
+ *  obtained here:
+ *  http://www.fi.muni.cz/~xslaby/phantom.html
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/pci.h>
+#include <linux/fs.h>
+#include <linux/poll.h>
+#include <linux/interrupt.h>
+#include <linux/cdev.h>
+#include <linux/rtc.h>
+
+#include <asm/io.h>
+
+#include <linux/phantom.h>
+
+#define PHANTOM_VERSION		"n0.9.1"
+
+#define PHANTOM_MAX_MINORS	8
+
+#define phantom_write_cfgl(p, val, off) do {	\
+	writel(val, (p)->caddr + (off));	\
+	readl((p)->caddr + PHN_IRQCTL);	\
+} while (0)
+
+#define phantom_write_inl(p, val, off) do {	\
+	outl(val, (p)->ibase + (off));		\
+	inl((p)->ibase);			\
+} while (0)
+
+static struct class *phantom_class;
+static int phantom_major;
+
+struct phantom_device {
+	unsigned int opened;
+	void __iomem *caddr;
+	unsigned long ibase;
+	unsigned long obase;
+	struct phantom_status *stat;
+	unsigned long status;
+	unsigned int tCu[3];
+	unsigned int tFe[3];
+
+	wait_queue_head_t wait;
+	struct cdev cdev;
+
+	struct mutex open_lock;
+};
+
+static unsigned char phantom_devices[PHANTOM_MAX_MINORS];
+
+/*
+ * Update status of phantom; turn off and/or start task if necessary
+ *
+ * spinlock should be here for safety
+ */
+static int phantom_status(struct phantom_device *dev, unsigned long newstat)
+{
+	pr_debug("phantom_status %lx %lx\n", dev->status, newstat);
+
+	if (!(dev->status & PHB_RUNNING) && (newstat & PHB_RUNNING))
+		phantom_write_cfgl(dev, 0x43, PHN_IRQCTL);
+	else if ((dev->status & PHB_RUNNING) && !(newstat & PHB_RUNNING))
+		phantom_write_cfgl(dev, 0, PHN_IRQCTL);
+
+	dev->status = newstat;
+
+	return 0;
+}
+
+/******************************************************************************
+ * File ops
+ */
+
+static int phantom_ioctl(struct inode *inode, struct file *file, u_int cmd,
+	u_long arg)
+{
+	struct phantom_device *dev = file->private_data;
+
+	if (_IOC_TYPE(cmd) != PH_IOC_MAGIC ||
+			_IOC_NR(cmd) > PH_IOC_MAXNR)
+		return -ENOTTY;
+
+	switch (cmd) {
+	case PH_IOCSTART:
+		return phantom_status(dev, dev->status | PHB_RUNNING);
+	case PH_IOCSTOP:
+		return phantom_status(dev, dev->status & ~PHB_RUNNING);
+	case PH_IOCGET_STATUS:
+		if (put_user(dev->status, (int __user *)arg))
+			return -EFAULT;
+		break;
+	default:
+		return -ENOTTY;
+	}
+
+	return 0;
+}
+
+static int phantom_open(struct inode *inode, struct file *file)
+{
+	struct phantom_device *dev = container_of(inode->i_cdev,
+			struct phantom_device, cdev);
+
+	if (mutex_lock_interruptible(&dev->open_lock))
+		return -ERESTARTSYS;
+
+	if (dev->opened) {
+		mutex_unlock(&dev->open_lock);
+		return -EINVAL;
+	}
+
+	memset(dev->stat, 0, PAGE_ALIGN(sizeof(*dev->stat)));
+	memcpy(dev->stat->tCu, dev->tCu, sizeof(dev->tCu));
+	memcpy(dev->stat->tFe, dev->tFe, sizeof(dev->tFe));
+	dev->stat->reset = !!(dev->status & PHB_RESET);
+
+	file->private_data = dev;
+
+	dev->opened++;
+	mutex_unlock(&dev->open_lock);
+
+	return 0;
+}
+
+static int phantom_release(struct inode *inode, struct file *file)
+{
+	struct phantom_device *dev = file->private_data;
+
+	if (mutex_lock_interruptible(&dev->open_lock))
+		return -ERESTARTSYS;
+
+	dev->opened = 0;
+	phantom_status(dev, dev->status & ~PHB_RUNNING);
+	memcpy(dev->tCu, dev->stat->tCu, sizeof(dev->tCu));
+	memcpy(dev->tFe, dev->stat->tFe, sizeof(dev->tFe));
+	if (dev->stat->reset)
+		dev->status |= PHB_RESET;
+
+	mutex_unlock(&dev->open_lock);
+
+	return 0;
+}
+
+static unsigned int phantom_poll(struct file *file, poll_table *wait)
+{
+	struct phantom_device *dev = file->private_data;
+	unsigned int mask = 0;
+
+	pr_debug("phantom_poll: %u\n", dev->stat->counter);
+	poll_wait(file, &dev->wait, wait);
+	if (dev->stat->counter)
+		mask = POLLIN | POLLRDNORM;
+	else if ((dev->status & PHB_RUNNING) == 0)
+		mask = POLLIN | POLLRDNORM | POLLERR;
+	pr_debug("phantom_poll end: %x/%u\n", mask, dev->stat->counter);
+
+	return mask;
+}
+
+#define phantom_remap(io, vma, addr)	({				\
+	vma->vm_pgoff = (addr) >> PAGE_SHIFT;				\
+	io ## remap_pfn_range(vma, (vma)->vm_start, (vma)->vm_pgoff,	\
+		(vma)->vm_end - (vma)->vm_start, (vma)->vm_page_prot);	\
+})
+
+static int phantom_mmap(struct file *file, struct vm_area_struct *vma)
+{
+	struct phantom_device *dev = file->private_data;
+	int retval;
+
+	switch (vma->vm_pgoff) {
+	case 0:
+		retval = phantom_remap(, vma, virt_to_phys(dev->stat));
+		break;
+	case 1:
+		retval = phantom_remap(io_, vma, dev->ibase);
+		break;
+	default:
+		retval = phantom_remap(io_, vma, dev->obase);
+	}
+
+	return retval ? -EINVAL : 0;
+}
+
+static struct file_operations phantom_file_ops = {
+	.open = phantom_open,
+	.release = phantom_release,
+	.ioctl = phantom_ioctl,
+	.poll = phantom_poll,
+	.mmap = phantom_mmap,
+};
+
+static irqreturn_t phantom_isr(int irq, void *data)
+{
+	static unsigned int cnt;
+	struct phantom_device *dev = data;
+
+	if (!(inl(dev->ibase + PHN_CONTROL) & 0x10))
+		return IRQ_NONE;
+
+	phantom_write_inl(dev, 0, 0);
+	phantom_write_inl(dev, 0xc0, 0);
+	if (++cnt >= 22) { /* 22 times ~ 2kHz */
+		dev->stat->counter++;
+		wake_up_interruptible(&dev->wait);
+		cnt = 0;
+	}
+
+	return IRQ_HANDLED;
+}
+
+/******************************************************************************
+ * Init and deinit driver
+ */
+
+static unsigned int __devinit phantom_get_free(void)
+{
+	unsigned int i;
+
+	for (i = 0; i < PHANTOM_MAX_MINORS; i++)
+		if (phantom_devices[i] == 0)
+			break;
+
+	return i;
+}
+
+static int __devinit phantom_probe(struct pci_dev *pdev,
+	const struct pci_device_id *pci_id)
+{
+	struct phantom_device *pht;
+	unsigned int minor;
+	int retval;
+
+	retval = pci_enable_device(pdev);
+	if (retval)
+		goto err;
+
+	minor = phantom_get_free();
+	if (minor == PHANTOM_MAX_MINORS) {
+		dev_err(&pdev->dev, "too many devices found!\n");
+		retval = -EIO;
+		goto err;
+	}
+
+	phantom_devices[minor] = 1;
+
+	retval = pci_request_regions(pdev, "phantom");
+	if (retval)
+		goto err_null;
+
+	retval = -ENOMEM;
+	pht = kzalloc(sizeof(*pht), GFP_KERNEL);
+	if (pht == NULL) {
+		dev_err(&pdev->dev, "unable to allocate device\n");
+		goto err_reg;
+	}
+
+	pht->stat = (void *)__get_free_page(GFP_KERNEL | __GFP_WAIT);
+	if (pht->stat == NULL)
+		goto err_fr;
+
+	pht->caddr = pci_iomap(pdev, 0, 0);
+	if (pht->caddr == NULL) {
+		dev_err(&pdev->dev, "can't remap conf space\n");
+		goto err_frp;
+	}
+	pht->ibase = pci_resource_start(pdev, 2);
+	pht->obase = pci_resource_start(pdev, 3);
+
+	mutex_init(&pht->open_lock);
+	init_waitqueue_head(&pht->wait);
+
+	phantom_write_cfgl(pht, 0, PHN_IRQCTL);
+	retval = request_irq(pdev->irq, phantom_isr,
+			IRQF_SHARED | IRQF_DISABLED, "phantom", pht);
+	if (retval) {
+		dev_err(&pdev->dev, "can't establish ISR\n");
+		goto err_unm;
+	}
+
+	cdev_init(&pht->cdev, &phantom_file_ops);
+	pht->cdev.owner = THIS_MODULE;
+	retval = cdev_add(&pht->cdev, MKDEV(phantom_major, minor), 1);
+	if (retval) {
+		dev_err(&pdev->dev, "chardev registration failed\n");
+		goto err_irq;
+	}
+
+	device_create(phantom_class, &pdev->dev, MKDEV(phantom_major, minor),
+			"phantom%u", minor);
+
+	pci_set_drvdata(pdev, pht);
+
+	return 0;
+err_irq:
+	free_irq(pdev->irq, pht);
+err_unm:
+	pci_iounmap(pdev, pht->caddr);
+err_frp:
+	free_page((unsigned long)pht->stat);
+err_fr:
+	kfree(pht);
+err_reg:
+	pci_release_regions(pdev);
+err_null:
+	phantom_devices[minor] = 0;
+err:
+	return retval;
+}
+
+static void __devexit phantom_remove(struct pci_dev *pdev)
+{
+	struct phantom_device *pht = pci_get_drvdata(pdev);
+	unsigned int minor = MINOR(pht->cdev.dev);
+
+	device_destroy(phantom_class, MKDEV(phantom_major, minor));
+
+	cdev_del(&pht->cdev);
+
+	phantom_write_cfgl(pht, 0, PHN_IRQCTL);
+	free_irq(pdev->irq, pht);
+
+	pci_iounmap(pdev, pht->caddr);
+
+	free_page((unsigned long)pht->stat);
+
+	phantom_devices[minor] = 0;
+	kfree(pht);
+
+	pci_release_regions(pdev);
+}
+
+static int phantom_suspend(struct pci_dev *pdev, pm_message_t state)
+{
+	struct phantom_device *dev = pci_get_drvdata(pdev);
+
+	phantom_write_cfgl(dev, 0, PHN_IRQCTL);
+
+	return 0;
+}
+
+static int phantom_resume(struct pci_dev *pdev)
+{
+	struct phantom_device *dev = pci_get_drvdata(pdev);
+
+	phantom_write_cfgl(dev, 0, PHN_IRQCTL);
+
+	return 0;
+}
+
+static struct pci_device_id phantom_pci_tbl[] __devinitdata = {
+	{ PCI_DEVICE(PCI_VENDOR_ID_PLX, PCI_DEVICE_ID_PLX_9050),
+		.class = PCI_CLASS_BRIDGE_OTHER << 8, .class_mask = 0xffff00 },
+	{ 0, }
+};
+MODULE_DEVICE_TABLE(pci, phantom_pci_tbl);
+
+static struct pci_driver phantom_pci_driver = {
+	.name = "phantom",
+	.id_table = phantom_pci_tbl,
+	.probe = phantom_probe,
+	.remove = __devexit_p(phantom_remove),
+	.suspend = phantom_suspend,
+	.resume = phantom_resume
+};
+
+static ssize_t phantom_show_version(struct class *cls, char *buf)
+{
+	return sprintf(buf, PHANTOM_VERSION "\n");
+}
+
+static CLASS_ATTR(version, 0444, phantom_show_version, NULL);
+
+static int __init phantom_init(void)
+{
+	int retval;
+	dev_t dev;
+
+	phantom_class = class_create(THIS_MODULE, "phantom");
+	if (IS_ERR(phantom_class)) {
+		retval = PTR_ERR(phantom_class);
+		printk(KERN_ERR "phantom: can't register phantom class\n");
+		goto err;
+	}
+	retval = class_create_file(phantom_class, &class_attr_version);
+	if (retval) {
+		printk(KERN_ERR "phantom: can't create sysfs version file\n");
+		goto err_class;
+	}
+
+	retval = alloc_chrdev_region(&dev, 0, PHANTOM_MAX_MINORS, "phantom");
+	if (retval) {
+		printk(KERN_ERR "phantom: can't register character device\n");
+		goto err_attr;
+	}
+	phantom_major = MAJOR(dev);
+
+	retval = pci_register_driver(&phantom_pci_driver);
+	if (retval) {
+		printk(KERN_ERR "phantom: can't register pci driver\n");
+		goto err_unchr;
+	}
+
+	printk(KERN_INFO "Phantom Linux Driver, version " PHANTOM_VERSION ", "
+			"init OK\n");
+
+	return 0;
+err_unchr:
+	unregister_chrdev_region(dev, PHANTOM_MAX_MINORS);
+err_attr:
+	class_remove_file(phantom_class, &class_attr_version);
+err_class:
+	class_destroy(phantom_class);
+err:
+	return retval;
+}
+
+static void __exit phantom_exit(void)
+{
+	pci_unregister_driver(&phantom_pci_driver);
+
+	unregister_chrdev_region(MKDEV(phantom_major, 0), PHANTOM_MAX_MINORS);
+
+	class_remove_file(phantom_class, &class_attr_version);
+	class_destroy(phantom_class);
+
+	pr_debug(KERN_INFO "phantom: module successfully removed\n");
+}
+
+module_init(phantom_init);
+module_exit(phantom_exit);
+
+MODULE_AUTHOR("Jiri Slaby <jirislaby@gmail.com>");
+MODULE_DESCRIPTION("Sensable Phantom driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/phantom.h b/include/linux/phantom.h
new file mode 100644
index 0000000..57a8616
--- /dev/null
+++ b/include/linux/phantom.h
@@ -0,0 +1,48 @@
+/*
+ *  Copyright (C) 2005-2007 Jiri Slaby <jirislaby@gmail.com>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ */
+
+#ifndef __PHANTOM_H
+#define __PHANTOM_H
+
+/* some common public defines & structures */
+#define PH_IOC_MAGIC		'k'
+#define PH_IOCSTOP		_IO (PH_IOC_MAGIC, 1)
+#define PH_IOCSTART		_IO (PH_IOC_MAGIC, 2)
+#define PH_IOCGET_STATUS	_IOR(PH_IOC_MAGIC, 3, int)
+#define PH_IOC_MAXNR		3
+
+#define PHB_RUNNING		1
+#define PHB_RESET		2
+
+#define PHN_BUTTON_BIT		2
+#define PHN_CONTROL		6
+#define PHN_IRQCTL		0x4c
+#define PHN_ZERO_FORCE		2048
+
+/* pointer for location of shared memory - its useful to put
+ * shared memory to some special address - especially for debugging :-)
+ *
+ * process mmap-ing phantom memory should use this constant;
+ */
+
+struct phantom_status {
+	unsigned int counter;
+	unsigned int tCu[3];
+	unsigned int tFe[3];
+	unsigned int reset:1;
+};
+
+#define PHANTOM_MMAP_OFF_STAT	0x00000000
+#define PHANTOM_MMAP_LEN_STAT	sizeof(struct phantom_status)
+#define PHANTOM_MMAP_OFF_IBASE	0x00001000
+#define PHANTOM_MMAP_LEN_IBASE	0x00000020
+#define PHANTOM_MMAP_OFF_OBASE	0x00002000
+#define PHANTOM_MMAP_LEN_OBASE	0x00000020
+
+#endif

Re: [PATCH 1/1] Input: add sensable phantom driver

[Dmitry Torokhov]

Hi Jiri,

On 3/7/07, Jiri Slaby <jirislaby@gmail.com> wrote:
> add sensable phantom driver
...

General question - can this driver use force-feedback mecahnisms
already present in kernel instead of exporting raw datastream to
userspace. What are shortcomings of kernels force-feedback
implemenattion that make it insufficient for phantom?

> +
> +#define phantom_remap(io, vma, addr)   ({                              \
> +       vma->vm_pgoff = (addr) >> PAGE_SHIFT;                           \
> +       io ## remap_pfn_range(vma, (vma)->vm_start, (vma)->vm_pgoff,    \
> +               (vma)->vm_end - (vma)->vm_start, (vma)->vm_page_prot);  \
> +})
> +
> +static int phantom_mmap(struct file *file, struct vm_area_struct *vma)
> +{
> +       struct phantom_device *dev = file->private_data;
> +       int retval;
> +
> +       switch (vma->vm_pgoff) {
> +       case 0:
> +               retval = phantom_remap(, vma, virt_to_phys(dev->stat));

This really hurts my eyes. Is there any reason for using the macro here?
> +               break;
> +       case 1:
> +               retval = phantom_remap(io_, vma, dev->ibase);
> +               break;
> +       default:
> +               retval = phantom_remap(io_, vma, dev->obase);
> +       }
> +
> +       return retval ? -EINVAL : 0;
> +}
> +
> +
> +static int __devinit phantom_probe(struct pci_dev *pdev,
> +       const struct pci_device_id *pci_id)
> +{
> +       struct phantom_device *pht;
> +       unsigned int minor;
> +       int retval;
> +
> +       retval = pci_enable_device(pdev);
> +       if (retval)
> +               goto err;
> +
> +       minor = phantom_get_free();
> +       if (minor == PHANTOM_MAX_MINORS) {
> +               dev_err(&pdev->dev, "too many devices found!\n");
> +               retval = -EIO;
> +               goto err;
> +       }
> +
> +       phantom_devices[minor] = 1;

Locking? In face of multithreaded PCI probes it might be needed.

> +
> +       retval = pci_request_regions(pdev, "phantom");
> +       if (retval)
> +               goto err_null;
> +
> +       retval = -ENOMEM;
> +       pht = kzalloc(sizeof(*pht), GFP_KERNEL);
> +       if (pht == NULL) {
> +               dev_err(&pdev->dev, "unable to allocate device\n");
> +               goto err_reg;
> +       }
> +
> +       pht->stat = (void *)__get_free_page(GFP_KERNEL | __GFP_WAIT);
> +       if (pht->stat == NULL)
> +               goto err_fr;
> +
> +       pht->caddr = pci_iomap(pdev, 0, 0);
> +       if (pht->caddr == NULL) {
> +               dev_err(&pdev->dev, "can't remap conf space\n");
> +               goto err_frp;
> +       }
> +       pht->ibase = pci_resource_start(pdev, 2);
> +       pht->obase = pci_resource_start(pdev, 3);
> +
> +       mutex_init(&pht->open_lock);
> +       init_waitqueue_head(&pht->wait);
> +
> +       phantom_write_cfgl(pht, 0, PHN_IRQCTL);
> +       retval = request_irq(pdev->irq, phantom_isr,
> +                       IRQF_SHARED | IRQF_DISABLED, "phantom", pht);
> +       if (retval) {
> +               dev_err(&pdev->dev, "can't establish ISR\n");
> +               goto err_unm;
> +       }
> +
> +       cdev_init(&pht->cdev, &phantom_file_ops);
> +       pht->cdev.owner = THIS_MODULE;
> +       retval = cdev_add(&pht->cdev, MKDEV(phantom_major, minor), 1);
> +       if (retval) {
> +               dev_err(&pdev->dev, "chardev registration failed\n");
> +               goto err_irq;
> +       }
> +
> +       device_create(phantom_class, &pdev->dev, MKDEV(phantom_major, minor),
> +                       "phantom%u", minor);
> +

Error handling is needed.

-- 
Dmitry

Re: [PATCH 1/1] Input: add sensable phantom driver

[Jiri Slaby]

Dmitry Torokhov napsal(a):
> Hi Jiri,

Hi.

> On 3/7/07, Jiri Slaby <jirislaby@gmail.com> wrote:
>> add sensable phantom driver
> ...
> 
> General question - can this driver use force-feedback mecahnisms
> already present in kernel instead of exporting raw datastream to
> userspace. What are shortcomings of kernels force-feedback
> implemenattion that make it insufficient for phantom?

I didn't even think about it, sorry. Does any problem with rate about 2kHz come 
on your mind before I'll try to rewrite it?

>> +#define phantom_remap(io, vma, addr)   ({                              \
>> +       vma->vm_pgoff = (addr) >> PAGE_SHIFT;                           \
>> +       io ## remap_pfn_range(vma, (vma)->vm_start, (vma)->vm_pgoff,    \
>> +               (vma)->vm_end - (vma)->vm_start, (vma)->vm_page_prot);  \
>> +})
>> +
>> +static int phantom_mmap(struct file *file, struct vm_area_struct *vma)
>> +{
>> +       struct phantom_device *dev = file->private_data;
>> +       int retval;
>> +
>> +       switch (vma->vm_pgoff) {
>> +       case 0:
>> +               retval = phantom_remap(, vma, virt_to_phys(dev->stat));
> 
> This really hurts my eyes. Is there any reason for using the macro here?

Not necessarilly. (It remained here from times, when this driver was not 
intended to be merged, since rtc workaround. Now we have a source of the 2.4 
driver.). Anyway, if the ff layer was used, I'll get the whole mmap away.

>> +               break;
>> +       case 1:
>> +               retval = phantom_remap(io_, vma, dev->ibase);
>> +               break;
>> +       default:
>> +               retval = phantom_remap(io_, vma, dev->obase);
>> +       }
>> +
>> +       return retval ? -EINVAL : 0;
>> +}
>> +
>> +
>> +static int __devinit phantom_probe(struct pci_dev *pdev,
>> +       const struct pci_device_id *pci_id)
>> +{
>> +       struct phantom_device *pht;
>> +       unsigned int minor;
>> +       int retval;
>> +
>> +       retval = pci_enable_device(pdev);
>> +       if (retval)
>> +               goto err;
>> +
>> +       minor = phantom_get_free();
>> +       if (minor == PHANTOM_MAX_MINORS) {
>> +               dev_err(&pdev->dev, "too many devices found!\n");
>> +               retval = -EIO;
>> +               goto err;
>> +       }
>> +
>> +       phantom_devices[minor] = 1;
> 
> Locking? In face of multithreaded PCI probes it might be needed.

I think, this is the same issue?

<cite from="akpm" date="12/19/06">
On Sat, 16 Dec 2006 02:09:48 +0100 (CET)
Jiri Slaby <jirislaby@gmail.com> wrote:

 > isicom, fix probe race
 >
 > Fix two race conditions in the probe function with mutex.
 >
 > ...
 >
 >  static int __devinit isicom_probe(struct pci_dev *pdev,
 >       const struct pci_device_id *ent)
 >  {
 > +     static DEFINE_MUTEX(probe_lock);

hm.  How can isicom_probe() race with itself?  Even with the dreaded
multithreaded-pci-probing?  It's only called once, by a single thread.

Confused.
</cite>

What do you think? Greg?

>> +
>> +       retval = pci_request_regions(pdev, "phantom");
>> +       if (retval)
>> +               goto err_null;
>> +
>> +       retval = -ENOMEM;
>> +       pht = kzalloc(sizeof(*pht), GFP_KERNEL);
>> +       if (pht == NULL) {
>> +               dev_err(&pdev->dev, "unable to allocate device\n");
>> +               goto err_reg;
>> +       }
>> +
>> +       pht->stat = (void *)__get_free_page(GFP_KERNEL | __GFP_WAIT);
>> +       if (pht->stat == NULL)
>> +               goto err_fr;
>> +
>> +       pht->caddr = pci_iomap(pdev, 0, 0);
>> +       if (pht->caddr == NULL) {
>> +               dev_err(&pdev->dev, "can't remap conf space\n");
>> +               goto err_frp;
>> +       }
>> +       pht->ibase = pci_resource_start(pdev, 2);
>> +       pht->obase = pci_resource_start(pdev, 3);
>> +
>> +       mutex_init(&pht->open_lock);
>> +       init_waitqueue_head(&pht->wait);
>> +
>> +       phantom_write_cfgl(pht, 0, PHN_IRQCTL);
>> +       retval = request_irq(pdev->irq, phantom_isr,
>> +                       IRQF_SHARED | IRQF_DISABLED, "phantom", pht);
>> +       if (retval) {
>> +               dev_err(&pdev->dev, "can't establish ISR\n");
>> +               goto err_unm;
>> +       }
>> +
>> +       cdev_init(&pht->cdev, &phantom_file_ops);
>> +       pht->cdev.owner = THIS_MODULE;
>> +       retval = cdev_add(&pht->cdev, MKDEV(phantom_major, minor), 1);
>> +       if (retval) {
>> +               dev_err(&pdev->dev, "chardev registration failed\n");
>> +               goto err_irq;
>> +       }
>> +
>> +       device_create(phantom_class, &pdev->dev, MKDEV(phantom_major, 
>> minor),
>> +                       "phantom%u", minor);
>> +
> 
> Error handling is needed.

Creating of sysfs node is not requisite, isn't it? (But yes, I'll at least spit 
some warning out.)

thanks for comments,
-- 
http://www.fi.muni.cz/~xslaby/            Jiri Slaby
faculty of informatics, masaryk university, brno, cz
e-mail: jirislaby gmail com, gpg pubkey fingerprint:
B674 9967 0407 CE62 ACC8  22A0 32CC 55C3 39D4 7A7E

Re: [PATCH 1/1] Input: add sensable phantom driver

[Greg KH]

On Wed, Mar 07, 2007 at 05:38:59PM +0100, Jiri Slaby wrote:
> Dmitry Torokhov napsal(a):
> >>+static int __devinit phantom_probe(struct pci_dev *pdev,
> >>+       const struct pci_device_id *pci_id)
> >>+{
> >>+       struct phantom_device *pht;
> >>+       unsigned int minor;
> >>+       int retval;
> >>+
> >>+       retval = pci_enable_device(pdev);
> >>+       if (retval)
> >>+               goto err;
> >>+
> >>+       minor = phantom_get_free();
> >>+       if (minor == PHANTOM_MAX_MINORS) {
> >>+               dev_err(&pdev->dev, "too many devices found!\n");
> >>+               retval = -EIO;
> >>+               goto err;
> >>+       }
> >>+
> >>+       phantom_devices[minor] = 1;
> >
> >Locking? In face of multithreaded PCI probes it might be needed.
> 
> I think, this is the same issue?
> 
> <cite from="akpm" date="12/19/06">
> On Sat, 16 Dec 2006 02:09:48 +0100 (CET)
> Jiri Slaby <jirislaby@gmail.com> wrote:
> 
> > isicom, fix probe race
> >
> > Fix two race conditions in the probe function with mutex.
> >
> > ...
> >
> >  static int __devinit isicom_probe(struct pci_dev *pdev,
> >       const struct pci_device_id *ent)
> >  {
> > +     static DEFINE_MUTEX(probe_lock);
> 
> hm.  How can isicom_probe() race with itself?  Even with the dreaded
> multithreaded-pci-probing?  It's only called once, by a single thread.
> 
> Confused.
> </cite>
> 
> What do you think? Greg?

We used to be able to call the pci probe functions from different
threads for different devices.  But people seemed to like to enable
options that told them their box could blow up into tiny pieces and then
got upset when it did.  So I removed the option :(

So you don't have to worry about it now, but note that it could race
with the remove function if two different devices are in the system, one
being added and one removed at the same time.  So you should protect it
with a spinlock or something.

thanks,

greg k-h

Re: [PATCH 1/1] Input: add sensable phantom driver

[Dmitry Torokhov]

On 3/7/07, Jiri Slaby <jirislaby@gmail.com> wrote:
> Dmitry Torokhov napsal(a):
> > Hi Jiri,
>
> Hi.
>
> > On 3/7/07, Jiri Slaby <jirislaby@gmail.com> wrote:
> >> add sensable phantom driver
> > ...
> >
> > General question - can this driver use force-feedback mecahnisms
> > already present in kernel instead of exporting raw datastream to
> > userspace. What are shortcomings of kernels force-feedback
> > implemenattion that make it insufficient for phantom?
>
> I didn't even think about it, sorry. Does any problem with rate about 2kHz come
> on your mind before I'll try to rewrite it?

No, not really.

-- 
Dmitry

Re: [PATCH 1/1] Input: add sensable phantom driver

[Jiri Slaby]

Cc: Anssi Hannula <anssi.hannula@gmail.com>

On 3/7/07, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> On 3/7/07, Jiri Slaby <jirislaby@gmail.com> wrote:
> > Dmitry Torokhov napsal(a):
> > > Hi Jiri,
> >
> > Hi.
> >
> > > On 3/7/07, Jiri Slaby <jirislaby@gmail.com> wrote:
> > >> add sensable phantom driver
> > > ...
> > >
> > > General question - can this driver use force-feedback mecahnisms
> > > already present in kernel instead of exporting raw datastream to
> > > userspace. What are shortcomings of kernels force-feedback
> > > implemenattion that make it insufficient for phantom?
> >
> > I didn't even think about it, sorry. Does any problem with rate about 2kHz come
> > on your mind before I'll try to rewrite it?
>
> No, not really.

Hmm, after going through input and ff layer, I figured out, that it's
possible to pass only a direction to constant effect. I think, there
is no possibility to pass 3D force (or vector) to the ff layer, isn't
it?

thanks,
-- 
http://www.fi.muni.cz/~xslaby/            Jiri Slaby
faculty of informatics, masaryk university, brno, cz
e-mail: jirislaby gmail com, gpg pubkey fingerprint:
B674 9967 0407 CE62 ACC8  22A0 32CC 55C3 39D4 7A7E

FF layer restrictions [Was: [PATCH 1/1] Input: add sensable phantom driver]

[Jiri Slaby]

Why did you remove all Cced people? Anyway I filtered some of them out

johann deneux napsal(a):
> You are right, the direction in ff_effect is meant to be an angle.
> A dirty solution would be to use the 16 bits as two 8-bits angles. Or 

That would be a problem as I need 3x 16bits.

> maybe we should change the API. I don't think there are many 
> applications using force feedback yet, so maybe that should be ok?
> 
> If we change the API, we should remove the assumption that a device has 
> at most two axes to render effects. We could for instance have a 
> magnitude argument for each axis which is capable of rendering effects. 
> That might be necessary even for more common gaming devices like racing 
> wheels: One can think pedals could also be capable of force feedback 
> some day, not just the steering wheel.

I can do that, but in that case, I need to know how people (especially those 
input one) want me to do...

regards,
-- 
http://www.fi.muni.cz/~xslaby/            Jiri Slaby
faculty of informatics, masaryk university, brno, cz
e-mail: jirislaby gmail com, gpg pubkey fingerprint:
B674 9967 0407 CE62 ACC8  22A0 32CC 55C3 39D4 7A7E

Re: FF layer restrictions [Was: [PATCH 1/1] Input: add sensable phantom driver]

[Dmitry Torokhov]

On 3/13/07, Jiri Slaby <jirislaby@gmail.com> wrote:
> Why did you remove all Cced people? Anyway I filtered some of them out
>
> johann deneux napsal(a):
> > You are right, the direction in ff_effect is meant to be an angle.
> > A dirty solution would be to use the 16 bits as two 8-bits angles. Or
>
> That would be a problem as I need 3x 16bits.
>
> > maybe we should change the API. I don't think there are many
> > applications using force feedback yet, so maybe that should be ok?
> >
> > If we change the API, we should remove the assumption that a device has
> > at most two axes to render effects. We could for instance have a
> > magnitude argument for each axis which is capable of rendering effects.
> > That might be necessary even for more common gaming devices like racing
> > wheels: One can think pedals could also be capable of force feedback
> > some day, not just the steering wheel.
>
> I can do that, but in that case, I need to know how people (especially those
> input one) want me to do...
>

Since we have no idea how many programs (if any) are using force
feedback interface I would be wary of changing existing effcets and
rather add new set of 3D effects.

Do we have any idea if there any users of FF out there?

-- 
Dmitry

Re: FF layer restrictions [Was: [PATCH 1/1] Input: add sensable phantom driver]

[Jiri Slaby]

Dmitry Torokhov napsal(a):
> On 3/13/07, Jiri Slaby <jirislaby@gmail.com> wrote:
>> Why did you remove all Cced people? Anyway I filtered some of them out
>>
>> johann deneux napsal(a):
>> > You are right, the direction in ff_effect is meant to be an angle.
>> > A dirty solution would be to use the 16 bits as two 8-bits angles. Or
>>
>> That would be a problem as I need 3x 16bits.
>>
>> > maybe we should change the API. I don't think there are many
>> > applications using force feedback yet, so maybe that should be ok?
>> >
>> > If we change the API, we should remove the assumption that a device has
>> > at most two axes to render effects. We could for instance have a
>> > magnitude argument for each axis which is capable of rendering effects.
>> > That might be necessary even for more common gaming devices like racing
>> > wheels: One can think pedals could also be capable of force feedback
>> > some day, not just the steering wheel.
>>
>> I can do that, but in that case, I need to know how people (especially
>> those
>> input one) want me to do...
>>
> 
> Since we have no idea how many programs (if any) are using force
> feedback interface I would be wary of changing existing effcets and

I definitely agree.

> rather add new set of 3D effects.

I was thinking about having "raw" (e.g. FF_RAW) effect, which would be only
X "axes"/entries of u32, where X is about 10 for future use and simply
posting these values further to HW (maybe after clamping or driver specific
processing) from this array. This seems to be augmentation of FF_CONSTANT
but the fact, it doesn't compute forces from direction.

Also yet another one such as FF_VECTOR or FF_3D could be considered as one
posibility, but it's still the same -- to have no more than 3 entries to
pass forces...

> Do we have any idea if there any users of FF out there?

At least me :). I'm using it for wheel and joystick in modules for locally
developped multiplatform virtual reality system.

regards,
-- 
http://www.fi.muni.cz/~xslaby/            Jiri Slaby
faculty of informatics, masaryk university, brno, cz
e-mail: jirislaby gmail com, gpg pubkey fingerprint:
B674 9967 0407 CE62 ACC8  22A0 32CC 55C3 39D4 7A7E

Re: FF layer restrictions [Was: [PATCH 1/1] Input: add sensable phantom driver]

[Jiri Slaby]

Jiri Slaby napsal(a):
> Also yet another one such as FF_VECTOR or FF_3D could be considered as one
> posibility, but it's still the same -- to have no more than 3 entries to
> pass forces...

(for example johann's FF pedals)

-- 
http://www.fi.muni.cz/~xslaby/            Jiri Slaby
faculty of informatics, masaryk university, brno, cz
e-mail: jirislaby gmail com, gpg pubkey fingerprint:
B674 9967 0407 CE62 ACC8  22A0 32CC 55C3 39D4 7A7E

Re: FF layer restrictions [Was: [PATCH 1/1] Input: add sensable phantom driver]

[Anssi Hannula]

Jiri Slaby wrote:
> Dmitry Torokhov napsal(a):
>> On 3/13/07, Jiri Slaby <jirislaby@gmail.com> wrote:
>>> Why did you remove all Cced people? Anyway I filtered some of them out
>>>
>>> johann deneux napsal(a):
>>>> You are right, the direction in ff_effect is meant to be an angle.
>>>> A dirty solution would be to use the 16 bits as two 8-bits angles. Or
>>> That would be a problem as I need 3x 16bits.

Interesting. What kind of device is that? i.e. what is the third
direction value?

>>>> maybe we should change the API. I don't think there are many
>>>> applications using force feedback yet, so maybe that should be ok?
>>>>
>>>> If we change the API, we should remove the assumption that a device has
>>>> at most two axes to render effects. We could for instance have a
>>>> magnitude argument for each axis which is capable of rendering effects.
>>>> That might be necessary even for more common gaming devices like racing
>>>> wheels: One can think pedals could also be capable of force feedback
>>>> some day, not just the steering wheel.
>>> I can do that, but in that case, I need to know how people (especially
>>> those
>>> input one) want me to do...
>>>
>> Since we have no idea how many programs (if any) are using force
>> feedback interface I would be wary of changing existing effcets and
> 
> I definitely agree.
>
>> rather add new set of 3D effects.
> 
> I was thinking about having "raw" (e.g. FF_RAW) effect, which would be only
> X "axes"/entries of u32, where X is about 10 for future use and simply
> posting these values further to HW (maybe after clamping or driver specific
> processing) from this array. This seems to be augmentation of FF_CONSTANT
> but the fact, it doesn't compute forces from direction.

I don't like the idea of a driver-specific "raw" effect, I'd rather add
real effect types.

> Also yet another one such as FF_VECTOR or FF_3D could be considered as one
> posibility, but it's still the same -- to have no more than 3 entries to
> pass forces...
> 
>> Do we have any idea if there any users of FF out there?
> 
> At least me :). I'm using it for wheel and joystick in modules for locally
> developped multiplatform virtual reality system.

Wine and BZflag come to mind, though I think the support is quite
limited in both.

-- 
Anssi Hannula

Re: FF layer restrictions [Was: [PATCH 1/1] Input: add sensable phantom driver]

[Jiri Slaby]

Anssi Hannula napsal(a):
> Jiri Slaby wrote:
>> Dmitry Torokhov napsal(a):
>>> On 3/13/07, Jiri Slaby <jirislaby@gmail.com> wrote:
>>>> Why did you remove all Cced people? Anyway I filtered some of them out
>>>>
>>>> johann deneux napsal(a):
>>>>> You are right, the direction in ff_effect is meant to be an angle.
>>>>> A dirty solution would be to use the 16 bits as two 8-bits angles. Or
>>>> That would be a problem as I need 3x 16bits.
> 
> Interesting. What kind of device is that? i.e. what is the third
> direction value?

6DOF sensable phantom:
http://www.sensable.com/haptic-phantom-premium-6dof.htm

X,Y,Z,RX,RY,RZ+BUT as inputs
X,Y,Z motors as FF

regards,
-- 
http://www.fi.muni.cz/~xslaby/            Jiri Slaby
faculty of informatics, masaryk university, brno, cz
e-mail: jirislaby gmail com, gpg pubkey fingerprint:
B674 9967 0407 CE62 ACC8  22A0 32CC 55C3 39D4 7A7E

Re: FF layer restrictions [Was: [PATCH 1/1] Input: add sensable phantom driver]

[STenyaK (Bruno González)]

 On 3/14/07, Anssi Hannula <anssi.hannula@gmail.com> wrote:
 >  >> Do we have any idea if there any users of FF out there?
 > >
 > > At least me :). I'm using it for wheel and joystick in modules for locally
 > > developped multiplatform virtual reality system.
 >
 >  Wine and BZflag come to mind, though I think the support is quite
 > limited in both.
 >

There's also vDrift (racing sim), they added support very recently.
I plan to add it in my car sim too, but there's still nothing coded.

I have a question: if the force is to be 3D, why only 3 possible
values? What would they be, 3 torques or 3 forces? In the case of car
sims (ff steering wheels), only one axis of torque is usually used
(except for 6 dof platforms, as mentioned).

By the way, is this the correct list for asking trivial questions
(such as questions about how the ff api works), or is it only for
development of kernel?

(hope the mail arrives this time; the first one was apparently sent as
html, which is considered spam by the mailing list; and the
notification about that problem was considered spam by gmail too!)

-- 
Saludos,
     STenyaK

_______________________________________________
Site:   http://1ksurvivor.homeip.net  <1kSurvivor>
         http://motorsport-sim.org     <Motorsport>
         http://kwh.iespana.es         <KuantikalWareHouse>
         http://emuletutorial.info     <EmuleTutorial>
ICQ:    153709484
Mail:   stenyak AT gmail DOT com

Re: FF layer restrictions [Was: [PATCH 1/1] Input: add sensable phantom driver]

[Dmitry Torokhov]

On 3/14/07, STenyaK (Bruno González) <stenyak@gmail.com> wrote:
>
>
> On 3/14/07, Anssi Hannula <anssi.hannula@gmail.com> wrote:
> > >> Do we have any idea if there any users of FF out there?
> > >
> > > At least me :). I'm using it for wheel and joystick in modules for
> locally
> > > developped multiplatform virtual reality system.
> >
> > Wine and BZflag come to mind, though I think the support is quite
> > limited in both.
> >
>
> There's also vDrift (racing sim), they added support very recently.
> I plan to add it in my car sim too, but there's still nothing coded.
>
> I have a question: if the force is to be 3D, why only 3 possible values?
> What would they be, 3 torques or 3 forces? In the case of car sims (ff
> steering wheels), only one axis of torque is usually used (except for 6 dof
> platforms, as mentioned).
>

I wonder if we could somehow extend or augment FF envelope se we could
specify a plane for the effect.. Then a vector could be represented by
a sum 3 constant effects in 3 separate planes and we could also use
spring and other effects as well.

> By the way, is this the correct list for asking trivial questions (such as
> questions about how the ff api works), or is it only for development of
> kernel?

Since it is kernel-provided API linux-input list should be fine and I
think linux-kernel as well.

-- 
Dmitry

Re: FF layer restrictions [Was: [PATCH 1/1] Input: add sensable phantom driver]

[STenyaK (Bruno González)]

On 3/14/07, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > I have a question: if the force is to be 3D, why only 3 possible values?
> > What would they be, 3 torques or 3 forces? In the case of car sims (ff
> > steering wheels), only one axis of torque is usually used (except for 6 dof
> > platforms, as mentioned).
> >
>
> I wonder if we could somehow extend or augment FF envelope se we could
> specify a plane for the effect.. Then a vector could be represented by
> a sum 3 constant effects in 3 separate planes and we could also use
> spring and other effects as well.

Ideally, afaik we should use:
-3 values for translation force (linear force): x,y,z components of
the force vector.
-4 values for rotation force (torque): x,y,z,w components of the
quaternion. You can also use euler angles (and i think there are
another one or two notations), which is just 3 values, but i'm not
sure it will be a correct decision (due to the gimbal lock problem,
which may or may not be present in ff devices, dunno).

-- 
Saludos,
     STenyaK

_______________________________________________
Site:   http://1ksurvivor.homeip.net  <1kSurvivor>
         http://motorsport-sim.org     <Motorsport>
         http://kwh.iespana.es         <KuantikalWareHouse>
         http://emuletutorial.info     <EmuleTutorial>
ICQ:    153709484
Mail:   stenyak AT gmail DOT com

Re: FF layer restrictions [Was: [PATCH 1/1] Input: add sensable phantom driver]

[johann deneux]

On 3/14/07, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> On 3/14/07, STenyaK (Bruno González) <stenyak@gmail.com> wrote:
> >
> >
> > On 3/14/07, Anssi Hannula <anssi.hannula@gmail.com> wrote:
> > > >> Do we have any idea if there any users of FF out there?
> > > >
> > > > At least me :). I'm using it for wheel and joystick in modules for
> > locally
> > > > developped multiplatform virtual reality system.
> > >
> > > Wine and BZflag come to mind, though I think the support is quite
> > > limited in both.
> > >
> >
> > There's also vDrift (racing sim), they added support very recently.
> > I plan to add it in my car sim too, but there's still nothing coded.
> >
> > I have a question: if the force is to be 3D, why only 3 possible values?
> > What would they be, 3 torques or 3 forces? In the case of car sims (ff
> > steering wheels), only one axis of torque is usually used (except for 6 dof
> > platforms, as mentioned).
> >
>
> I wonder if we could somehow extend or augment FF envelope se we could
> specify a plane for the effect.. Then a vector could be represented by
> a sum 3 constant effects in 3 separate planes and we could also use
> spring and other effects as well.

I don't think struct ff_envelope is a good candidate: Spring effects
don't have any envelope.
We have plenty of space for new effect types. Why not add a bunch of
new types, and add more structs in the ff_effect union?

The other option where you somehow manage to specify planes and
combine several effects is a good solution for devices where the
driver is responsible for storing effects, but it's wasteful for
I-Force-like devices where the effects are stored on the device. You
would be using two (or more) effects in the device's memory where only
one would be used if you used all the potential of the device. That
is, unless the driver is smart enough to merge effects, but that seems
hard.

-- 
Johann

Re: FF layer restrictions [Was: [PATCH 1/1] Input: add sensable phantom driver]

[johann deneux]

On 3/14/07, STenyaK (Bruno González) <stenyak@gmail.com> wrote:
> On 3/14/07, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > > I have a question: if the force is to be 3D, why only 3 possible values?
> > > What would they be, 3 torques or 3 forces? In the case of car sims (ff
> > > steering wheels), only one axis of torque is usually used (except for 6 dof
> > > platforms, as mentioned).
> > >
> >
> > I wonder if we could somehow extend or augment FF envelope se we could
> > specify a plane for the effect.. Then a vector could be represented by
> > a sum 3 constant effects in 3 separate planes and we could also use
> > spring and other effects as well.
>

I would think one plane and one axis is all that's needed for a 3d vector, or?
Do we understand eachother?

> Ideally, afaik we should use:
> -3 values for translation force (linear force): x,y,z components of
> the force vector.
> -4 values for rotation force (torque): x,y,z,w components of the
> quaternion. You can also use euler angles (and i think there are
> another one or two notations), which is just 3 values, but i'm not
> sure it will be a correct decision (due to the gimbal lock problem,
> which may or may not be present in ff devices, dunno).

Same remark here. A torque is a 3-dimensional thing, just like a
force. What for do you need 4 components?

-- 
Johann

Re: FF layer restrictions [Was: [PATCH 1/1] Input: add sensable phantom driver]

[STenyaK (Bruno González)]

On 3/15/07, johann deneux <johann.deneux@gmail.com> wrote:
> On 3/14/07, STenyaK (Bruno González) <stenyak@gmail.com> wrote:
> > Ideally, afaik we should use:
> > -3 values for translation force (linear force): x,y,z components of
> > the force vector.
> > -4 values for rotation force (torque): x,y,z,w components of the
> > quaternion. You can also use euler angles (and i think there are
> > another one or two notations), which is just 3 values, but i'm not
> > sure it will be a correct decision (due to the gimbal lock problem,
> > which may or may not be present in ff devices, dunno).
>
> Same remark here. A torque is a 3-dimensional thing, just like a
> force. What for do you need 4 components?

Simplifying, 3 values defining axis of rotation, 1 defining module of
torque.. As i said, i'm not really sure we really need the 4; you most
probably are right.

-- 
Saludos,
     STenyaK

_______________________________________________
Site:   http://1ksurvivor.homeip.net  <1kSurvivor>
         http://motorsport-sim.org     <Motorsport>
         http://kwh.iespana.es         <KuantikalWareHouse>
         http://emuletutorial.info     <EmuleTutorial>
ICQ:    153709484
Mail:   stenyak AT gmail DOT com

Re: FF layer restrictions [Was: [PATCH 1/1] Input: add sensable phantom driver]

[Pavel Machek]

Hi!

> >Why did you remove all Cced people? Anyway I filtered 
> >some of them out
> >
> >johann deneux napsal(a):
> >> You are right, the direction in ff_effect is meant to 
> >be an angle.
> >> A dirty solution would be to use the 16 bits as two 
> >8-bits angles. Or
> >
> >That would be a problem as I need 3x 16bits.
> >
> >> maybe we should change the API. I don't think there 
> >are many
> >> applications using force feedback yet, so maybe that 
> >should be ok?
> >>
> >> If we change the API, we should remove the assumption 
> >that a device has
> >> at most two axes to render effects. We could for 
> >instance have a
> >> magnitude argument for each axis which is capable of 
> >rendering effects.
> >> That might be necessary even for more common gaming 
> >devices like racing
> >> wheels: One can think pedals could also be capable of 
> >force feedback
> >> some day, not just the steering wheel.
> >
> >I can do that, but in that case, I need to know how 
> >people (especially those
> >input one) want me to do...
> >
> 
> Since we have no idea how many programs (if any) are 
> using force
> feedback interface I would be wary of changing existing 
> effcets and
> rather add new set of 3D effects.
> 
> Do we have any idea if there any users of FF out there?

Number of linux games is quite low, so...

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: FF layer restrictions [Was: [PATCH 1/1] Input: add sensable phantom driver]

[johann deneux]

On 3/16/07, Pavel Machek <pavel@ucw.cz> wrote:
> Hi!
>
> > >Why did you remove all Cced people? Anyway I filtered
> > >some of them out
> > >
> > >johann deneux napsal(a):
> > >> You are right, the direction in ff_effect is meant to
> > >be an angle.
> > >> A dirty solution would be to use the 16 bits as two
> > >8-bits angles. Or
> > >
> > >That would be a problem as I need 3x 16bits.
> > >
> > >> maybe we should change the API. I don't think there
> > >are many
> > >> applications using force feedback yet, so maybe that
> > >should be ok?
> > >>
> > >> If we change the API, we should remove the assumption
> > >that a device has
> > >> at most two axes to render effects. We could for
> > >instance have a
> > >> magnitude argument for each axis which is capable of
> > >rendering effects.
> > >> That might be necessary even for more common gaming
> > >devices like racing
> > >> wheels: One can think pedals could also be capable of
> > >force feedback
> > >> some day, not just the steering wheel.
> > >
> > >I can do that, but in that case, I need to know how
> > >people (especially those
> > >input one) want me to do...
> > >
> >
> > Since we have no idea how many programs (if any) are
> > using force
> > feedback interface I would be wary of changing existing
> > effcets and
> > rather add new set of 3D effects.
> >
> > Do we have any idea if there any users of FF out there?
>
> Number of linux games is quite low, so...
>
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
>

Games are not the only application type using FF. I got a few
enquiries from universities working on robotics project.
I think keeping backward compatibility is not a problem here. The
problem is to make an extension that does not duplicate the
capabilities of the existing API. We don't want to have two ways of
specifying the same effects.

-- 
Johann

Re: FF layer restrictions [Was: [PATCH 1/1] Input: add sensable phantom driver]

[Jiri Slaby]

STenyaK (Bruno González) napsal(a):
> On 3/14/07, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
>> > I have a question: if the force is to be 3D, why only 3 possible
>> values?
>> > What would they be, 3 torques or 3 forces? In the case of car sims (ff
>> > steering wheels), only one axis of torque is usually used (except
>> for 6 dof
>> > platforms, as mentioned).
>> >
>>
>> I wonder if we could somehow extend or augment FF envelope se we could
>> specify a plane for the effect.. Then a vector could be represented by
>> a sum 3 constant effects in 3 separate planes and we could also use
>> spring and other effects as well.
> 
> Ideally, afaik we should use:
> -3 values for translation force (linear force): x,y,z components of
> the force vector.
> -4 values for rotation force (torque): x,y,z,w components of the
> quaternion. You can also use euler angles (and i think there are
> another one or two notations), which is just 3 values, but i'm not
> sure it will be a correct decision (due to the gimbal lock problem,
> which may or may not be present in ff devices, dunno).

So, the resolution is? Since I want no longer have out-of-kernel driver, I
need to know how to implement the phantom driver -- merge it `as is', i.e.
control through mmio or rewrite ff layer somehow, and in that case how?

There seem to be only few possibilities:

- new 3D effect, which will be problematic in any other future use, that may
need more than 3 axis. no matter if torques or vector is passed -- depending
on device and programmer (as I need to compute torques from forces in FP).
Maybe struct with 2x 3 axis is OK

- "raw" effect, which may contain more axis, but this is ugly in Anssi's eyes

- something else? (did I forget something)

thanks,
-- 
http://www.fi.muni.cz/~xslaby/            Jiri Slaby
faculty of informatics, masaryk university, brno, cz
e-mail: jirislaby gmail com, gpg pubkey fingerprint:
B674 9967 0407 CE62 ACC8  22A0 32CC 55C3 39D4 7A7E

Re: FF layer restrictions [Was: [PATCH 1/1] Input: add sensable phantom driver]

[Jiri Slaby]

Jiri Slaby napsal(a):
> - something else? (did I forget something)

Maybe someone's (Dmitry's?) idea with envelopes?

regards,
-- 
http://www.fi.muni.cz/~xslaby/            Jiri Slaby
faculty of informatics, masaryk university, brno, cz
e-mail: jirislaby gmail com, gpg pubkey fingerprint:
B674 9967 0407 CE62 ACC8  22A0 32CC 55C3 39D4 7A7E

Re: FF layer restrictions [Was: [PATCH 1/1] Input: add sensable phantom driver]

[johann deneux]

On 3/21/07, Jiri Slaby <jirislaby@gmail.com> wrote:
> STenyaK (Bruno González) napsal(a):
> > On 3/14/07, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> >> > I have a question: if the force is to be 3D, why only 3 possible
> >> values?
> >> > What would they be, 3 torques or 3 forces? In the case of car sims (ff
> >> > steering wheels), only one axis of torque is usually used (except
> >> for 6 dof
> >> > platforms, as mentioned).
> >> >
> >>
> >> I wonder if we could somehow extend or augment FF envelope se we could
> >> specify a plane for the effect.. Then a vector could be represented by
> >> a sum 3 constant effects in 3 separate planes and we could also use
> >> spring and other effects as well.
> >
> > Ideally, afaik we should use:
> > -3 values for translation force (linear force): x,y,z components of
> > the force vector.
> > -4 values for rotation force (torque): x,y,z,w components of the
> > quaternion. You can also use euler angles (and i think there are
> > another one or two notations), which is just 3 values, but i'm not
> > sure it will be a correct decision (due to the gimbal lock problem,
> > which may or may not be present in ff devices, dunno).
>
> So, the resolution is? Since I want no longer have out-of-kernel driver, I
> need to know how to implement the phantom driver -- merge it `as is', i.e.
> control through mmio or rewrite ff layer somehow, and in that case how?
>
> There seem to be only few possibilities:
>
> - new 3D effect, which will be problematic in any other future use, that may
> need more than 3 axis. no matter if torques or vector is passed -- depending
> on device and programmer (as I need to compute torques from forces in FP).
> Maybe struct with 2x 3 axis is OK
>
> - "raw" effect, which may contain more axis, but this is ugly in Anssi's eyes
>
> - something else? (did I forget something)
>

I would suggest adding a new effect type (3d effect) and extending the
union in struct ff_effect.
Let me know if I'm too vague, I already suggested that solution but
got no answer. I wonder if my mail got lost, nobody understood what I
said, or if it's just a plain bad idea.

For future devices with more than 3 FF-enabled axes, we can just do
the same, or develop a whole new API. There shouldn't be any worry.

-- 
Johann

Re: FF layer restrictions [Was: [PATCH 1/1] Input: add sensable phantom driver]

[Dmitry Torokhov]

On 3/21/07, johann deneux <johann.deneux@gmail.com> wrote:
> On 3/21/07, Jiri Slaby <jirislaby@gmail.com> wrote:
> > STenyaK (Bruno González) napsal(a):
> > > On 3/14/07, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > >> > I have a question: if the force is to be 3D, why only 3 possible
> > >> values?
> > >> > What would they be, 3 torques or 3 forces? In the case of car sims (ff
> > >> > steering wheels), only one axis of torque is usually used (except
> > >> for 6 dof
> > >> > platforms, as mentioned).
> > >> >
> > >>
> > >> I wonder if we could somehow extend or augment FF envelope se we could
> > >> specify a plane for the effect.. Then a vector could be represented by
> > >> a sum 3 constant effects in 3 separate planes and we could also use
> > >> spring and other effects as well.
> > >
> > > Ideally, afaik we should use:
> > > -3 values for translation force (linear force): x,y,z components of
> > > the force vector.
> > > -4 values for rotation force (torque): x,y,z,w components of the
> > > quaternion. You can also use euler angles (and i think there are
> > > another one or two notations), which is just 3 values, but i'm not
> > > sure it will be a correct decision (due to the gimbal lock problem,
> > > which may or may not be present in ff devices, dunno).
> >
> > So, the resolution is? Since I want no longer have out-of-kernel driver, I
> > need to know how to implement the phantom driver -- merge it `as is', i.e.
> > control through mmio or rewrite ff layer somehow, and in that case how?
> >
> > There seem to be only few possibilities:
> >
> > - new 3D effect, which will be problematic in any other future use, that may
> > need more than 3 axis. no matter if torques or vector is passed -- depending
> > on device and programmer (as I need to compute torques from forces in FP).
> > Maybe struct with 2x 3 axis is OK
> >
> > - "raw" effect, which may contain more axis, but this is ugly in Anssi's eyes
> >
> > - something else? (did I forget something)
> >
>
> I would suggest adding a new effect type (3d effect) and extending the
> union in struct ff_effect.
> Let me know if I'm too vague, I already suggested that solution but
> got no answer. I wonder if my mail got lost, nobody understood what I
> said, or if it's just a plain bad idea.
>

My concern with a new 3D effect is that it will be a very "simple"
effect with only constant force apllied. That might be enough for
phantom but may not be sufficient for future devices. If we add
ability to specify a "plane" for an effect we will be able to add
envelopes on top of more complex effects and get desired combined
effcet. I would not worry aboput extra memory needed on I-force like
devices because they just would not support additional "planes". What
about phantom? Would it have enough memory?

On the other hand if you guys (Anssi, Johann, Jiri...) decide that a
simple new 3d effect is the most efficient solution for now and will
be enough for a few years (till we get FF v2 API) I will merge it.

-- 
Dmitry

Re: FF layer restrictions [Was: [PATCH 1/1] Input: add sensable phantom driver]

[Jiri Slaby]

Dmitry Torokhov napsal(a):
> On 3/21/07, johann deneux <johann.deneux@gmail.com> wrote:
>> I would suggest adding a new effect type (3d effect) and extending the
>> union in struct ff_effect.
>> Let me know if I'm too vague, I already suggested that solution but
>> got no answer. I wonder if my mail got lost, nobody understood what I
>> said, or if it's just a plain bad idea.
>>
> 
> My concern with a new 3D effect is that it will be a very "simple"
> effect with only constant force apllied. That might be enough for
> phantom but may not be sufficient for future devices. If we add
> ability to specify a "plane" for an effect we will be able to add
> envelopes on top of more complex effects and get desired combined
> effcet.

I didn't get this too much, because I don't understand the FF layer well so
far. How is this going to work? Let's say, we have 3 torque values computed
in US, and this structure:

struct ff_effect {
    __u16 direction;
    struct ff_trigger trigger;
    struct ff_replay replay;

    struct ff_constant_effect {
        __s16 level;
 	struct ff_envelope {
   	     __u16 attack_length;
    	     __u16 attack_level;
   	     __u16 fade_length;
     	     __u16 fade_level;
	};
    };
};

and need to pass the three 16bits torques into s16 ioaddr[0..2]. How?

> I would not worry aboput extra memory needed on I-force like
> devices because they just would not support additional "planes". What
> about phantom? Would it have enough memory?

It's a memless device -- values written into pci space are immediately felt
in FF (it's just PLX chip interface to some their proprietary chip or somewhat).

> On the other hand if you guys (Anssi, Johann, Jiri...) decide that a
> simple new 3d effect is the most efficient solution for now and will
> be enough for a few years (till we get FF v2 API) I will merge it.

I won't force any of mentioned solutions, but this seems the simplest one in
my eyes.

thanks for clues,
-- 
http://www.fi.muni.cz/~xslaby/            Jiri Slaby
faculty of informatics, masaryk university, brno, cz
e-mail: jirislaby gmail com, gpg pubkey fingerprint:
B674 9967 0407 CE62 ACC8  22A0 32CC 55C3 39D4 7A7E

Re: FF layer restrictions [Was: [PATCH 1/1] Input: add sensable phantom driver]

[johann deneux]

On 3/21/07, Jiri Slaby <jirislaby@gmail.com> wrote:
> Dmitry Torokhov napsal(a):
> > On 3/21/07, johann deneux <johann.deneux@gmail.com> wrote:
> >> I would suggest adding a new effect type (3d effect) and extending the
> >> union in struct ff_effect.
> >> Let me know if I'm too vague, I already suggested that solution but
> >> got no answer. I wonder if my mail got lost, nobody understood what I
> >> said, or if it's just a plain bad idea.
> >>
> >
> > My concern with a new 3D effect is that it will be a very "simple"
> > effect with only constant force apllied. That might be enough for
> > phantom but may not be sufficient for future devices. If we add
> > ability to specify a "plane" for an effect we will be able to add
> > envelopes on top of more complex effects and get desired combined
> > effcet.
>
> I didn't get this too much, because I don't understand the FF layer well so
> far. How is this going to work? Let's say, we have 3 torque values computed
> in US, and this structure:
>
> struct ff_effect {
>     __u16 direction;
>     struct ff_trigger trigger;
>     struct ff_replay replay;
>
>     struct ff_constant_effect {
>         __s16 level;
>         struct ff_envelope {
>              __u16 attack_length;
>              __u16 attack_level;
>              __u16 fade_length;
>              __u16 fade_level;
>         };
>     };
> };
>
> and need to pass the three 16bits torques into s16 ioaddr[0..2]. How?
>

Stupid question, I have forgotten the details of ioctl: Wouldn't the
following work?

struct ff_effect {
        __u16 type;
        __s16 id;
        __u16 direction;
        struct ff_trigger trigger;
        struct ff_replay replay;

        union {
                struct ff_constant_effect constant;
                struct ff_ramp_effect ramp;
                struct ff_periodic_effect periodic;
                struct ff_condition_effect condition[2]; /* One for each axis */
                struct ff_rumble_effect rumble;
        } u;

        /* New member: Specify a plane in the 3d space. */
        struct ff_plane plane;
};

Would that pose compatibility issues? If the input layer knows the
size of the struct the user-space application is sending, it knows if
it's safe to look into the "plane" member. If it is, and the device
driver is capable of handling 3d effects, then fine. If it is but the
device driver can't handle it, return an error code. If it isn't, just
do whatever it's currently doing.

Alternatively, one could leave ff_effect effect untouched and find
another way to specify the plane, e.g. another ioctl.

-- 
Johann

Re: FF layer restrictions [Was: [PATCH 1/1] Input: add sensable phantom driver]

[Dmitry Torokhov]

On Wednesday 21 March 2007 18:03, johann deneux wrote:
> On 3/21/07, Jiri Slaby <jirislaby@gmail.com> wrote:
> > Dmitry Torokhov napsal(a):
> > > On 3/21/07, johann deneux <johann.deneux@gmail.com> wrote:
> > >> I would suggest adding a new effect type (3d effect) and extending the
> > >> union in struct ff_effect.
> > >> Let me know if I'm too vague, I already suggested that solution but
> > >> got no answer. I wonder if my mail got lost, nobody understood what I
> > >> said, or if it's just a plain bad idea.
> > >>
> > >
> > > My concern with a new 3D effect is that it will be a very "simple"
> > > effect with only constant force apllied. That might be enough for
> > > phantom but may not be sufficient for future devices. If we add
> > > ability to specify a "plane" for an effect we will be able to add
> > > envelopes on top of more complex effects and get desired combined
> > > effcet.
> >
> > I didn't get this too much, because I don't understand the FF layer well so
> > far. How is this going to work? Let's say, we have 3 torque values computed
> > in US, and this structure:
> >
> > struct ff_effect {
> >     __u16 direction;
> >     struct ff_trigger trigger;
> >     struct ff_replay replay;
> >
> >     struct ff_constant_effect {
> >         __s16 level;
> >         struct ff_envelope {
> >              __u16 attack_length;
> >              __u16 attack_level;
> >              __u16 fade_length;
> >              __u16 fade_level;
> >         };
> >     };
> > };
> >
> > and need to pass the three 16bits torques into s16 ioaddr[0..2]. How?
> >
> 
> Stupid question, I have forgotten the details of ioctl: Wouldn't the
> following work?
> 
> struct ff_effect {
>         __u16 type;
>         __s16 id;
>         __u16 direction;
>         struct ff_trigger trigger;
>         struct ff_replay replay;
> 
>         union {
>                 struct ff_constant_effect constant;
>                 struct ff_ramp_effect ramp;
>                 struct ff_periodic_effect periodic;
>                 struct ff_condition_effect condition[2]; /* One for each axis */
>                 struct ff_rumble_effect rumble;
>         } u;
> 
>         /* New member: Specify a plane in the 3d space. */
>         struct ff_ruct ff_plane plane;
> };
> 
> Would that pose compatibility issues? If the input layer knows the
> size of the struct the user-space application is sending, it knows if
> it's safe to look into the "plane" member. If it is, and the device
> driver is capable of handling 3d effects, then fine. If it is but the
> device driver can't handle it, return an error code. If it isn't, just
> do whatever it's currently doing.

No, the kernel would not know that there is more data unless we add a new
ioctl number.

> 
> Alternatively, one could leave ff_effect effect untouched and find
> another way to specify the plane, e.g. another ioctl.

I was thinking about a new ioctl to specify a plane for a specific effect,
but probably extending the structure and having 2 distinct ioctls (with
and without plane) is cleaner.

-- 
Dmitry

Re: FF layer restrictions [Was: [PATCH 1/1] Input: add sensable phantom driver]

[Jiri Slaby]

Dmitry Torokhov napsal(a):
> On Wednesday 21 March 2007 18:03, johann deneux wrote:
>> On 3/21/07, Jiri Slaby <jirislaby@gmail.com> wrote:
>>> Dmitry Torokhov napsal(a):
>>>> On 3/21/07, johann deneux <johann.deneux@gmail.com> wrote:
>>>>> I would suggest adding a new effect type (3d effect) and extending the
>>>>> union in struct ff_effect.
>>>>> Let me know if I'm too vague, I already suggested that solution but
>>>>> got no answer. I wonder if my mail got lost, nobody understood what I
>>>>> said, or if it's just a plain bad idea.
>>>>>
>>>> My concern with a new 3D effect is that it will be a very "simple"
>>>> effect with only constant force apllied. That might be enough for
>>>> phantom but may not be sufficient for future devices. If we add
>>>> ability to specify a "plane" for an effect we will be able to add
>>>> envelopes on top of more complex effects and get desired combined
>>>> effcet.
>>> I didn't get this too much, because I don't understand the FF layer well so
>>> far. How is this going to work? Let's say, we have 3 torque values computed
>>> in US, and this structure:
>>>
>>> struct ff_effect {
>>>     __u16 direction;
>>>     struct ff_trigger trigger;
>>>     struct ff_replay replay;
>>>
>>>     struct ff_constant_effect {
>>>         __s16 level;
>>>         struct ff_envelope {
>>>              __u16 attack_length;
>>>              __u16 attack_level;
>>>              __u16 fade_length;
>>>              __u16 fade_level;
>>>         };
>>>     };
>>> };
>>>
>>> and need to pass the three 16bits torques into s16 ioaddr[0..2]. How?
>>>
>> Stupid question, 

Sorry to be out for so long, maybe I'm stupid idiot, that understands
nothing, but:

>> I have forgotten the details of ioctl: Wouldn't the
>> following work?

No, at least, as far as I understand this. I have computed _torques_, not
forces vector (this was misleading info in my first post), the question is
"how can I pass torques through plane and direction entries into KS?".

>> struct ff_effect {
>>         __u16 type;
>>         __s16 id;
>>         __u16 direction;
>>         struct ff_trigger trigger;
>>         struct ff_replay replay;
>>
>>         union {
>>                 struct ff_constant_effect constant;
>>                 struct ff_ramp_effect ramp;
>>                 struct ff_periodic_effect periodic;
>>                 struct ff_condition_effect condition[2]; /* One for each axis */
>>                 struct ff_rumble_effect rumble;
>>         } u;
>>
>>         /* New member: Specify a plane in the 3d space. */
>>         struct ff_ruct ff_plane plane;
>> };
[...]
>> Alternatively, one could leave ff_effect effect untouched and find
>> another way to specify the plane, e.g. another ioctl.
> 
> I was thinking about a new ioctl to specify a plane for a specific effect,
> but probably extending the structure and having 2 distinct ioctls (with
> and without plane) is cleaner.

This sounds good for me, if I (or someone) have forces vector, but it
doesn't help me in my situation.

thanks,
-- 
http://www.fi.muni.cz/~xslaby/            Jiri Slaby
faculty of informatics, masaryk university, brno, cz
e-mail: jirislaby gmail com, gpg pubkey fingerprint:
B674 9967 0407 CE62 ACC8  22A0 32CC 55C3 39D4 7A7E

Re: FF layer restrictions [Was: [PATCH 1/1] Input: add sensable phantom driver]

[johann deneux]

On 3/27/07, Jiri Slaby <jirislaby@gmail.com> wrote:
> Dmitry Torokhov napsal(a):
> > On Wednesday 21 March 2007 18:03, johann deneux wrote:
> >> On 3/21/07, Jiri Slaby <jirislaby@gmail.com> wrote:
> >>> Dmitry Torokhov napsal(a):
> >>>> On 3/21/07, johann deneux <johann.deneux@gmail.com> wrote:
> >>>>> I would suggest adding a new effect type (3d effect) and extending the
> >>>>> union in struct ff_effect.
> >>>>> Let me know if I'm too vague, I already suggested that solution but
> >>>>> got no answer. I wonder if my mail got lost, nobody understood what I
> >>>>> said, or if it's just a plain bad idea.
> >>>>>
> >>>> My concern with a new 3D effect is that it will be a very "simple"
> >>>> effect with only constant force apllied. That might be enough for
> >>>> phantom but may not be sufficient for future devices. If we add
> >>>> ability to specify a "plane" for an effect we will be able to add
> >>>> envelopes on top of more complex effects and get desired combined
> >>>> effcet.
> >>> I didn't get this too much, because I don't understand the FF layer well so
> >>> far. How is this going to work? Let's say, we have 3 torque values computed
> >>> in US, and this structure:
> >>>
> >>> struct ff_effect {
> >>>     __u16 direction;
> >>>     struct ff_trigger trigger;
> >>>     struct ff_replay replay;
> >>>
> >>>     struct ff_constant_effect {
> >>>         __s16 level;
> >>>         struct ff_envelope {
> >>>              __u16 attack_length;
> >>>              __u16 attack_level;
> >>>              __u16 fade_length;
> >>>              __u16 fade_level;
> >>>         };
> >>>     };
> >>> };
> >>>
> >>> and need to pass the three 16bits torques into s16 ioaddr[0..2]. How?
> >>>
> >> Stupid question,
>
> Sorry to be out for so long, maybe I'm stupid idiot, that understands
> nothing, but:

No, it's not you, there are still things that are unclear. For
instance I think Dmitry intended to combine several planes, which I
don't understand.

What we want is to specify a 3d vector, which can be done using two
angles and a magnitude.
One angle and the magnitude we already have in ff_effect, so all we
need is an additional angle, which can be specified using a new ioctl.
I originally thought it could be done without a new ioctl, just by
extending ff_struct, but that won't work.

>
> >> I have forgotten the details of ioctl: Wouldn't the
> >> following work?
>
> No, at least, as far as I understand this. I have computed _torques_, not
> forces vector (this was misleading info in my first post), the question is
> "how can I pass torques through plane and direction entries into KS?".

Torques and forces are both represented as 3d vectors. Are we
misunderstanding eachother maybe? By "vector" I mean a triplet of
numbers, when you say "torques" and "forces vector", do you mean that
each effect is composed of a bunch of torques?

-- 
Johann

Re: FF layer restrictions [Was: [PATCH 1/1] Input: add sensable phantom driver]

[Jiri Slaby]

johann deneux napsal(a):
> On 3/27/07, Jiri Slaby <jirislaby@gmail.com> wrote:
>> Dmitry Torokhov napsal(a):
>> > On Wednesday 21 March 2007 18:03, johann deneux wrote:
>> >> I have forgotten the details of ioctl: Wouldn't the
>> >> following work?
>>
>> No, at least, as far as I understand this. I have computed _torques_, not
>> forces vector (this was misleading info in my first post), the
>> question is
>> "how can I pass torques through plane and direction entries into KS?".
> 
> Torques and forces are both represented as 3d vectors.

Yes, may be.

> Are we
> misunderstanding eachother maybe? By "vector" I mean a triplet of
> numbers, when you say "torques" and "forces vector", do you mean that
> each effect is composed of a bunch of torques?

Ok, let's make things a little bit clear. I need to put somehow 3 short
values (torques -- to tell 3 motors how much to spin around) into the kernel
space via ff layer. I computed them using FP in US from forces (3d vector)
and the problem is, that I don't know how to transform torques to plane(s)
and dir and then back to torques -- this is what I can't figure out, since I
have no longer vector -- 3 values for each axis, which says how big is the
force in each axis -- but I have values (no axis) specific to each motor.

thanks for notes,
-- 
http://www.fi.muni.cz/~xslaby/            Jiri Slaby
faculty of informatics, masaryk university, brno, cz
e-mail: jirislaby gmail com, gpg pubkey fingerprint:
B674 9967 0407 CE62 ACC8  22A0 32CC 55C3 39D4 7A7E

Re: FF layer restrictions [Was: [PATCH 1/1] Input: add sensable phantom driver]

[johann deneux]

On 3/27/07, Jiri Slaby <jirislaby@gmail.com> wrote:
> johann deneux napsal(a):
> > On 3/27/07, Jiri Slaby <jirislaby@gmail.com> wrote:
> >> Dmitry Torokhov napsal(a):
> >> > On Wednesday 21 March 2007 18:03, johann deneux wrote:
> >> >> I have forgotten the details of ioctl: Wouldn't the
> >> >> following work?
> >>
> >> No, at least, as far as I understand this. I have computed _torques_, not
> >> forces vector (this was misleading info in my first post), the
> >> question is
> >> "how can I pass torques through plane and direction entries into KS?".
> >
> > Torques and forces are both represented as 3d vectors.
>
> Yes, may be.
>
> > Are we
> > misunderstanding eachother maybe? By "vector" I mean a triplet of
> > numbers, when you say "torques" and "forces vector", do you mean that
> > each effect is composed of a bunch of torques?
>
> Ok, let's make things a little bit clear. I need to put somehow 3 short
> values (torques -- to tell 3 motors how much to spin around) into the kernel
> space via ff layer. I computed them using FP in US from forces (3d vector)
> and the problem is, that I don't know how to transform torques to plane(s)
> and dir and then back to torques -- this is what I can't figure out, since I
> have no longer vector -- 3 values for each axis, which says how big is the
> force in each axis -- but I have values (no axis) specific to each motor.

I think I'm starting to understand. Looking back at the url you gave,
it appears your device is some kind of articulated arm, possibly with
one or more motor(s) at each joint. You are not really dealing with a
single torque applied at one point in space, but several torques
applied at several points in space.
For this particular device you have three values, but for other
devices it could just as well be any other number, right?
A realistic example would be a robot arm with claws, there would be a
torque for each joint, and some more for the claws.

-- 
Johann

Re: FF layer restrictions [Was: [PATCH 1/1] Input: add sensable phantom driver]

[Jiri Slaby]

johann deneux napsal(a):
> On 3/27/07, Jiri Slaby <jirislaby@gmail.com> wrote:
>> johann deneux napsal(a):
>> > Are we
>> > misunderstanding eachother maybe? By "vector" I mean a triplet of
>> > numbers, when you say "torques" and "forces vector", do you mean that
>> > each effect is composed of a bunch of torques?
>>
>> Ok, let's make things a little bit clear. I need to put somehow 3 short
>> values (torques -- to tell 3 motors how much to spin around) into the
>> kernel
>> space via ff layer. I computed them using FP in US from forces (3d
>> vector)
>> and the problem is, that I don't know how to transform torques to
>> plane(s)
>> and dir and then back to torques -- this is what I can't figure out,
>> since I
>> have no longer vector -- 3 values for each axis, which says how big is
>> the
>> force in each axis -- but I have values (no axis) specific to each motor.
> 
> I think I'm starting to understand. Looking back at the url you gave,
> it appears your device is some kind of articulated arm, possibly with
> one or more motor(s) at each joint. You are not really dealing with a
> single torque applied at one point in space, but several torques
> applied at several points in space.

Exactly as you write. Sorry for my previous bad definitions.

> For this particular device you have three values, but for other
> devices it could just as well be any other number, right?

Correct.

> A realistic example would be a robot arm with claws, there would be a
> torque for each joint, and some more for the claws.

For instance.

Ok, so how to deal with these devices? Does anybody have some idea? That's
what I was talking about somewhere in the beginning of this thread, the raw
values, because it seems too specific for letting kernel to cope with each
of these devices separately, but there might be a better idea in somebody's
head? But raw is ugly...

regards,
-- 
http://www.fi.muni.cz/~xslaby/            Jiri Slaby
faculty of informatics, masaryk university, brno, cz
e-mail: jirislaby gmail com, gpg pubkey fingerprint:
B674 9967 0407 CE62 ACC8  22A0 32CC 55C3 39D4 7A7E

Re: FF layer restrictions [Was: [PATCH 1/1] Input: add sensable phantom driver]

[johann deneux]

On 3/27/07, Jiri Slaby <jirislaby@gmail.com> wrote:

> Ok, so how to deal with these devices? Does anybody have some idea? That's
> what I was talking about somewhere in the beginning of this thread, the raw
> values, because it seems too specific for letting kernel to cope with each
> of these devices separately, but there might be a better idea in somebody's
> head? But raw is ugly...
>

What about adding a member to ff_effect which would be the number of the motor?
We can't change the layout of ff_effect too much though, so we have to
find unused bits and put them to work.

For instance, we could replace

__u16 type;

by

__u8 motor;
__u8 type;

since 16 bits seems way more than needed for the effect type.

Another possibility is to get rid of "trigger" and replace it by __u8
motor and some padding, since that's I-Force specific (are there other
drivers that implement that?). The goal of "trigger" is to start an
effect when a button is pressed, which can be done from userland
instead. It's not like we need micro-second latency when starting
effects.

As a reminder, here is the current definition of ff_effect:

struct ff_effect {
       __u16 type;
       __s16 id;
       __u16 direction;
       struct ff_trigger trigger;
       struct ff_replay replay;

       union {
               struct ff_constant_effect constant;
               struct ff_ramp_effect ramp;
               struct ff_periodic_effect periodic;
               struct ff_condition_effect condition[2]; /* One for each axis */
               struct ff_rumble_effect rumble;
       } u;
};

-- 
Johann

Re: FF layer restrictions [Was: [PATCH 1/1] Input: add sensable phantom driver]

[Dmitry Torokhov]

On Tuesday 27 March 2007 17:34, johann deneux wrote:
> What about adding a member to ff_effect which would be the number of the motor?
> We can't change the layout of ff_effect too much though, so we have to
> find unused bits and put them to work.
> 
> For instance, we could replace
> 
> __u16 type;
> 
> by
> 
> __u8 motor;
> __u8 type;
> 

Splitting type field seems to be a good idea.

-- 
Dmitry

Re: FF layer restrictions [Was: [PATCH 1/1] Input: add sensable phantom driver]

[Jiri Slaby]

Dmitry Torokhov napsal(a):
> On Tuesday 27 March 2007 17:34, johann deneux wrote:
>> What about adding a member to ff_effect which would be the number of the motor?
>> We can't change the layout of ff_effect too much though, so we have to
>> find unused bits and put them to work.
>>
>> For instance, we could replace
>>
>> __u16 type;
>>
>> by
>>
>> __u8 motor;
>> __u8 type;
>>
> 
> Splitting type field seems to be a good idea.

For me too, if 3-4 context switches for one effect are not too much in ~ kHz
freq. I'll try that.

thanks,
-- 
http://www.fi.muni.cz/~xslaby/            Jiri Slaby
faculty of informatics, masaryk university, brno, cz
e-mail: jirislaby gmail com, gpg pubkey fingerprint:
B674 9967 0407 CE62 ACC8  22A0 32CC 55C3 39D4 7A7E

Re: FF layer restrictions [Was: [PATCH 1/1] Input: add sensable phantom driver]

[Jiri Slaby]

Dmitry Torokhov napsal(a):
> On Tuesday 27 March 2007 17:34, johann deneux wrote:
>> What about adding a member to ff_effect which would be the number of the motor?
>> We can't change the layout of ff_effect too much though, so we have to
>> find unused bits and put them to work.
>>
>> For instance, we could replace
>>
>> __u16 type;
>>
>> by
>>
>> __u8 motor;
>> __u8 type;
>>
> 
> Splitting type field seems to be a good idea.

Maybe stupid question, but what about endianness + backward compatibility?
If we split it into motor,type sequence, it would break LE (untouched BE),
if we do type,motor, it is OK for LE (broken BE).

regards,
-- 
http://www.fi.muni.cz/~xslaby/            Jiri Slaby
faculty of informatics, masaryk university, brno, cz
e-mail: jirislaby gmail com, gpg pubkey fingerprint:
 B674 9967 0407 CE62 ACC8  22A0 32CC 55C3 39D4 7A7E

Hnus <hnus@fi.muni.cz> is an alias for /dev/null

Re: FF layer restrictions [Was: [PATCH 1/1] Input: add sensable phantom driver]

[Jiri Slaby]

Jiri Slaby napsal(a):
> Dmitry Torokhov napsal(a):
>> On Tuesday 27 March 2007 17:34, johann deneux wrote:
>>> What about adding a member to ff_effect which would be the number of the motor?
>>> We can't change the layout of ff_effect too much though, so we have to
>>> find unused bits and put them to work.
>>>
>>> For instance, we could replace
>>>
>>> __u16 type;
>>>
>>> by
>>>
>>> __u8 motor;
>>> __u8 type;
>>>
>> Splitting type field seems to be a good idea.
> 
> Maybe stupid question, but what about endianness + backward compatibility?
> If we split it into motor,type sequence, it would break LE (untouched BE),
> if we do type,motor, it is OK for LE (broken BE).

Aha, and the question is: do

#ifdef __BIG_ENDIAN
#else
#endif

?

regards,
-- 
http://www.fi.muni.cz/~xslaby/            Jiri Slaby
faculty of informatics, masaryk university, brno, cz
e-mail: jirislaby gmail com, gpg pubkey fingerprint:
 B674 9967 0407 CE62 ACC8  22A0 32CC 55C3 39D4 7A7E

Hnus <hnus@fi.muni.cz> is an alias for /dev/null

Re: FF layer restrictions [Was: [PATCH 1/1] Input: add sensable phantom driver]

[Dmitry Torokhov]

On 3/28/07, Jiri Slaby <jirislaby@gmail.com> wrote:
> Jiri Slaby napsal(a):
> > Dmitry Torokhov napsal(a):
> >> On Tuesday 27 March 2007 17:34, johann deneux wrote:
> >>> What about adding a member to ff_effect which would be the number of the motor?
> >>> We can't change the layout of ff_effect too much though, so we have to
> >>> find unused bits and put them to work.
> >>>
> >>> For instance, we could replace
> >>>
> >>> __u16 type;
> >>>
> >>> by
> >>>
> >>> __u8 motor;
> >>> __u8 type;
> >>>
> >> Splitting type field seems to be a good idea.
> >
> > Maybe stupid question, but what about endianness + backward compatibility?
> > If we split it into motor,type sequence, it would break LE (untouched BE),
> > if we do type,motor, it is OK for LE (broken BE).
>
> Aha, and the question is: do
>
> #ifdef __BIG_ENDIAN
> #else
> #endif
>
> ?
>

I am so tempted to say "screw BE, there is not many users of FF layer"
but I am afraid we will have to do that #ifdef-fu.

-- 
Dmitry

Re: FF layer restrictions [Was: [PATCH 1/1] Input: add sensable phantom driver]

[Anssi Hannula]

johann deneux wrote:
> On 3/27/07, Jiri Slaby <jirislaby@gmail.com> wrote:
> 
>> Ok, so how to deal with these devices? Does anybody have some idea?
>> That's
>> what I was talking about somewhere in the beginning of this thread,
>> the raw
>> values, because it seems too specific for letting kernel to cope with
>> each
>> of these devices separately, but there might be a better idea in
>> somebody's
>> head? But raw is ugly...
>>
> 
> What about adding a member to ff_effect which would be the number of the
> motor?
> We can't change the layout of ff_effect too much though, so we have to
> find unused bits and put them to work.

Actually, having thought more about this, I'm beginning to believe
Jiri's original FF_RAW approach could actually be a good option to add
3d effect support to the API. It would be similar to FF_CONSTANT but
instead of angle+magnitude the force is defined as s16 per every axis
(so it could actually be used with current drivers as well).

Alternatively we could define FF_3D_CONSTANT as an usual FF_CONSTANT
with an additional direction value, allowing a 3D force to be described
using 2x direction and magnitude.

Or we could define both ;)

Howewer, if I understood correctly that the 3 values the phantom device
takes are not really magnitudes along X, Y, Z, but some possibly
unaligned motors specific to this device, we cannot really abstract the
device motors into a 3D force effect. We could still add some FF_RAW
type, however, but with the values representing motors instead of axes,
with the exact function (direction) of each motor being left undefined
in the API (unlike the FF_RAW type I was talking a few paragraphs ago,
this type of course wouldn't be used at all by other drivers).


> Another possibility is to get rid of "trigger" and replace it by __u8
> motor and some padding, since that's I-Force specific (are there other
> drivers that implement that?). The goal of "trigger" is to start an
> effect when a button is pressed, which can be done from userland
> instead. It's not like we need micro-second latency when starting
> effects.

The generic PID driver implements that. I also think implementing the
support in software for the devices not supporting triggers in hw could
be quite easily done in ff-core.c or ff-memless.c.

(I also don't know what the triggers are useful for)

johann deneux also wrote in another message:
> The
> problem is to make an extension that does not duplicate the
> capabilities of the existing API. We don't want to have two ways of
> specifying the same effects.

How is this a problem?

-- 
Anssi Hannula