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 _______________________________________________ subsurface mailing list [email protected] http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface
