[PULL REQUEST] Subsurface branch - QtBluetooth serial implementation

[Dirk Hohndel]

Hi CLaudiu

On Mon, Jul 06, 2015 at 05:42:00PM +0300, Claudiu Olteanu wrote:
> Hi there,
> 
> I attached you some patches which add the QtBluetooth serial implementation.
> The Bluetooth download mode is accessible from the DownloadFromDiveComputer
> widget.
> 
> In order to connect to a Bluetooth device I decided to remove the SDP
> discovery because
> it doesn't work for all devices. For the moment I try to connect to the
> device using the RFCOMM
> channel 1 because this is the default channel of the SPP service for most
> devices.
> If this doesn't work I try again on RFCOMM channel number 5 (for Petrel2
> devices).

Thanks for sending a strong set of patches.
There is always this question of "what level of granularity is right for
commits". It's good not to show all steps of development. So don't add
things just to delete them in the next commit, etc. But it's also good to
try to make the commits fairly small so they are easy to read, they
ideally do just one thing and are incremental.

I have no simple rule and no simple tool that will tell you the right way
to do this (and different maintainers actually have different
preferences). To me a couple of your patches felt a little big, adding
quite a few things at once. I could have seen ways to split them into
smaller, self contained parts. But it's really hard for me to come up with
a concrete example that says "look, here, this is how it should be done".

Actually, here's one. The fake open function for the OSTC 2N. That could
have been its own commit, added before the next commit that adds all the
qtserialbluetooth stuff. That's a simple example, but maybe it illustrates
what I mean.

I won't ask you to refactor the commits because I don't think there's
anything wrong in what you did. Just something to look into for the
future.

> The last patch is not mandatory. It adds some extra logs which can be
> useful for
> debugging.

I took the last patch as well as I hope that others here on the list with
BT devices will help test the implementation and I'm guessing that we will
always want the more verbose output. We can remove these later when we
have it all working as expected.

/D