[bug#52575] [PATCH v3] services: bluetooth: Add missing config parameters

[Liliana Marie Prikler]

Hi,

Am Freitag, dem 17.12.2021 um 14:26 +0100 schrieb Demis Balbach:
> +@item @code{class} (default: @code{"0x000000"})
> +Default device class. Only the major and minor device class bits are
> considered.
Is it really a good idea to use a string-encoded number here?  Why not
a number?  Why not two?  A pair or list of symbols mayhaps?

> +@item @code{device-id} (default: @code{#f})
> +Use vendor id source (assigner), vendor, product and version
> information for
> +DID profile support. The values are separated by ":" and
> @var{assigner}, @var{VID},
> +@var{PID} and @var{version}.
> +
> +Possible values are:
> +
> +@itemize @bullet
> +@item
> +@code{#f} to disable it,
> +
> +@item
> +@code{"assigner:1234:5678:abcd"}, where @var{assigner} is either
> @code{usb} (default)
> +or @code{bluetooth}.
> +
> +@end itemize
You maybe want to have a <device-id> record here, but fair enough, a
string works too.

> +@item @code{privacy} (default: @code{'off})
> +Default privacy settings.
> +
> +@itemize @bullet
> +@item
> +@code{'off}: Disable local privacy
> +
> +@item
> +@code{'network/on}: A device will only accept advertising packets
> from peer
> +devices that contain private addresses. It may not be compatible
> with some
> +legacy devices since it requires the use of RPA(s) all the time
> +
> +@item
> +@code{'device}: A device in device privacy mode is only concerned
> about the
> +privacy of the device and will accept advertising packets from peer
> devices
> +that contain their Identity Address as well as ones that contain a
> private
> +address, even if the peer device has distributed its IRK in the past
> +
> +@end itemize
Why have a key network/on?  Why not use 'on and 'network and document,
that they function the same due to (insert implementation detail).

> +@item @code{experimental} (default: @code{'false})
> +Enables experimental features and interfaces, alternatively a list
> of UUIDs
> +can be given.
> +
> +Possible values:
> +
> +@itemize @bullet
> +@item
> +@code{'true}
> +
> +@item
> +@code{'false}
> +
> +@item
> +@code{'d4992530-b9ec-469f-ab01-6c481c47da1c}: BlueZ Experimental
> Debug,
> +
> +@item
> +@code{'671b10b5-42c0-4696-9227-eb28d1b049d6}: BlueZ Experimental
> Simultaneous Central and Peripheral,
> +
> +@item
> +@code{'15c0a148-c273-11ea-b3de-0242ac130004}: BlueZ Experimental LL
> privacy,
> +
> +@item
> +@code{'330859bc-7506-492d-9370-9a6f0614037f}: BlueZ Experimental
> Bluetooth Quality Report,
> +
> +@item
> +@code{'a6695ace-ee7f-4fb9-881a-5fac66c629af}: BlueZ Experimental
> Offload Codecs.
> +@end itemize
Why a list of UUIDs?  Wouldn't it make more sense to use symbols like
'debug, 'll-privacy, 'quality-report, etc.?
On that note, we have a UUID type, so use it.

> +@item @code{enable-adv-mon-interleave-scan} (default: @code{1})
> +Enable/Disable Advertisement Monitor interleave scan for power
> saving.
> +
> +Possible values are:
> +
> +@itemize @bullet
> +@item
> +@code{0}: disable
> +
> +@item
> +@code{1}: enabled.
> +@end itemize
Should be a boolean.

> +@item @code{cache} (default: @code{'always})
> +GATT attribute cache.
> +
> +Possible values are:
> +@itemize @bullet
> +@item
> +@code{'always}: Always cache attributes even for devices not paired,
> this is
> +recommended as it is best for interoperability, with more consistent
> +reconnection times and enables proper tracking of notifications for
> all
> +devices
> +
> +@item
> +@code{'yes}: Only cache attributes of paired devices
> +
> +@item
> +@code{'no}: Never cache attributes.
> +@end itemize
Might want to rename/alias 'yes to 'paired.

> +@item @code{key-size} (default: @code{0})
> +Minimum required Encryption Key Size for accessing secured
> characteristics.
> +
> +Possible values are:
> +@itemize @bullet
> +@item
> +@code{0}: Don't care
> +
> +@item
> +@code{7-16}
> +@end itemize
You probably want to write that as "an integer 7 <= N <= 16".

> +@itemize @bullet
> +@item
> +@code{23-517}
> +@end itemize
As above.

> +@item
> +@code{2-5}
> +@end itemize
As above.

> +@item @code{reconnect-uuids} (default: @code{'()})
> +The ReconnectUUIDs defines the set of remote services that should
> try
> +to be reconnected to in case of a link loss (link supervision
> +timeout). The policy plugin should contain a sane set of values by
> +default, but this list can be overridden here. By setting the list
> to
> +empty the reconnection feature gets disabled.
Again, make sure that your code works with UUIDs here if it doesn't
already.

> +@item @code{rssi-sampling-period} (default: @code{"0xFF"})
> +Default RSSI Sampling Period. This is used when a client registers
> an
> +advertisement monitor and leaves the RSSISamplingPeriod unset.
Use numbers, not strings.

I've only reviewed the documentation and not the implementation,
because that is the spec facing the user.  If some implementation
details look off, we can address those later on.

Cheers