HCI_MAX_DEV is a bit too small.

HCI_MAX_DEV is a bit too small.

[Peter Dons Tychsen]

Hello,

HCI_MAX_DEV is set a bit low (16), causing hci_for_each_dev() to not
return all of the devices if you have more. This is not a disaster for
the library itself, as i can just copy hci_for_each_dev() and make a
version supporting more devices.

However, hcitool uses the library version of hci_for_each_dev(), so that
makes hcitool useless for a system with more devices. You could of
course fix this is hcitool, but changing HCI_MAX_DEV seem like the right
solution.

Can this be changed, or does it *need* to be 16?

If changed, it would be nice if it was raised to something like 256, to
keep the number in the power of 2. To be honest, i don't think this
should have been a constant at all, as the number of devices is
virtually endless. hci_for_each_dev() should probably have taken a MAX
parameter, but should not itself have set a limit. But to keep the API
intact, changing HCI_MAX_DEV could be a solution. 

Thanks,

/Pedro

Re: HCI_MAX_DEV is a bit too small.

[Marcel Holtmann]

Hi Pedro,

> HCI_MAX_DEV is set a bit low (16), causing hci_for_each_dev() to not
> return all of the devices if you have more. This is not a disaster for
> the library itself, as i can just copy hci_for_each_dev() and make a
> version supporting more devices.
> 
> However, hcitool uses the library version of hci_for_each_dev(), so that
> makes hcitool useless for a system with more devices. You could of
> course fix this is hcitool, but changing HCI_MAX_DEV seem like the right
> solution.
> 
> Can this be changed, or does it *need* to be 16?
> 
> If changed, it would be nice if it was raised to something like 256, to
> keep the number in the power of 2. To be honest, i don't think this
> should have been a constant at all, as the number of devices is
> virtually endless. hci_for_each_dev() should probably have taken a MAX
> parameter, but should not itself have set a limit. But to keep the API
> intact, changing HCI_MAX_DEV could be a solution. 

you are seriously considering the case of more than 16 Bluetooth devices
are real normal use cases. We are talking about 1 device vs. 2 and can
not even find an agreement for the UI cases there.

Anyhow, I agree with you that the number of devices should not have been
limited within the library. That was a bug we inherited from the initial
library design. However I am against just raising the number. We did
raise it once from 4 to 16 a long time ago. It is a serious memory issue
in the long running apps like bluetoothd.

So I could be convinced to add new functions to read/write the limit
from within an application itself. So that it can be changed without
re-compiling the library. Feel free to propose a patch.

Regards

Marcel

Re: HCI_MAX_DEV is a bit too small.

[Peter Dons Tychsen]

Hi M,

Thanks for the fast reply,

> you are seriously considering the case of more than 16 Bluetooth devices
> are real normal use cases. We are talking about 1 device vs. 2 and can
> not even find an agreement for the UI cases there.
I know that normal users do not use that many devices, but test systems
or bluetooth servers (gateways) can easily have more than 16.

> So I could be convinced to add new functions to read/write the limit
> from within an application itself. So that it can be changed without
> re-compiling the library. Feel free to propose a patch.
I will see what i can do. What is needed is probably a
hci_set_max_devices() call. Then hcitool could call that with the number
of devices needed.

Thanks,

/Pedro

Re: HCI_MAX_DEV is a bit too small.

[Iain Hibbert]

On Mon, 15 Mar 2010, Peter Dons Tychsen wrote:

> > So I could be convinced to add new functions to read/write the limit
> > from within an application itself. So that it can be changed without
> > re-compiling the library. Feel free to propose a patch.
>
> I will see what i can do. What is needed is probably a
> hci_set_max_devices() call. Then hcitool could call that with the number
> of devices needed.

A suggestion. I have used this construct in the past for something that
contains a hard coded limit where somebody might want to override it
without rebuilding, and requires no change to the API..

hci.h:
#define HCI_MAX_DEV	hci_max_dev()

hci.c:
unsigned int
hci_max_dev(void)
{
	static unsigned int max = 0;
	char *env, *ep;
	unsigned long v;

	if (max == 0) {
		max = 16;

		env = getenv("HCI_MAX_DEV");
		if (env == NULL)
			break;

		errno = 0;
		v = strtoul(env, &ep, 0);
		if (env[0] == '\0' || *ep != '\0')
			break;

		if (errno == ERANGE && v == ULONG_MAX)
			break;

		if (v > UINT_MAX)
			break;

		max = v;
	}

	return max;
}

regards,
iain

Re: HCI_MAX_DEV is a bit too small.

[Peter Dons Tychsen]

Hello again,

> > So I could be convinced to add new functions to read/write the limit
> > from within an application itself. So that it can be changed without
> > re-compiling the library. Feel free to propose a patch.
> I will see what i can do. What is needed is probably a
> hci_set_max_devices() call. Then hcitool could call that with the number
> of devices needed.

After some time looking at it more closely, i can see that the real
problem is not in hcilib, but in the stack itself. The real problem is
that there is no way of getting the actual number of devices with
reading out all of them... and thereby allocating space for MAX devices
(which could be endless). Chicken and Egg situation. What is really
needed is an ioctl to get the current count. This could be done by:

a) A new IOCTL called HCIGETDEVCOUNT that returns the device count.

or

b) Modifying HCIGETDEVLIST to return the device count (dev_num) if the
input count (dev_num) is zero (without actually returning the devices).
This would be backward compatible, as zero is not a valid input today.

Is any of this acceptable? Please advise what solutions could be
acceptable.

This would allow for hci_for_each_dev (and in turn the stack via IOCTL)
to use less resources, and to support more devices. This could deprecate
the need for MAX_HCI_DEV altogether, as it is not used anywhere else in
the lib. There is of-course allot of other software using that constant,
but maybe removing it over time would be good. It gives the user of the
lib a false idea of a non-existing limitation.

Thanks,

/Pedro