Avoid unnecessary bluetooth device address scanning. #4120

notrege · 2024-03-05T13:16:29Z

Describe the pull request:

Pull request long description:

When switching from a non-bluetooth computer to a bluetooh computer an unnecessary bluetooth scan may be forced. This change will avoid the scan if the bluetooth device address is known.

I discovered this issue when sharing a laptop to upload dives. If I upload dives using a bluetooth computer I have to rescan the device address after a dive is uploaded with a different computer. This change uses the existing device address instead of scanning. A scan can be forces by clearing/rechecking the "Choose Bluetooth download mode" checkbox.

Changes made:

There are two changes. First, set the device address using the prefs value when a dive computer is selected. Second, avoid scanning if the device address is already known.

Related issues:

Additional information:

Release note:

Documentation change:

Mentions:

When switching from a non-bluetooth computer to a bluetooh computer an unnecessary bluetooth scan may be forced. This patch will avoid the scan if the bluetooth device address is known. Signed-off-by: jme <32236882+notrege@users.noreply.github.com>

Avoid unnecessary bluetooth device address scanning.

mikeller

Congratulations on your first pull request!

A couple of remarks that need attention.

mikeller · 2024-03-05T20:41:36Z

desktop-widgets/downloadfromdivecomputer.cpp

 void DownloadFromDCWidget::DC##num##Clicked() \
 { \
 	ui.vendor->setCurrentIndex(ui.vendor->findText(qPrefDiveComputer::vendor##num())); \
+	ui.device->setCurrentText(qPrefDiveComputer::device##num()); \


This seems to be doing the same as what we are already doing in https://github.com/subsurface/subsurface/pull/4120/files#diff-a0bde534c60cabf020b85f0079e0a8d567f5465514e8a59629b3a50a228d17f9R160 - but with slightly different connotations, like not overriding the selection for serial devices (which probably makes sense), and not doing it for MacOS where it does not seem to be working.

The core of the problem seems to be that https://github.com/subsurface/subsurface/pull/4120/files#diff-a0bde534c60cabf020b85f0079e0a8d567f5465514e8a59629b3a50a228d17f9L156 is triggering enableBluetoothMode(), which is springing a bluetooth selection dialogue - so I think it will be better if we just move this line down below ui.device->setCurrentIndex().

Thanks very much for the help and for looking at this. I'm not sure why it should be different for serial vs Bluetooth devices, but I must be missing something. In my test case, I have an Oceanic atom using /dev/tty.usbserial-20030001 and a G2 using a Bluetooth address. With the changes above, I seem to be able to switch back and forth between the two without a Bluetooth scan or losing the serial mount point. I'm doing on this on a sadly old Macbook running Catalina (10.15.7). Won't your suggestion of moving this line down below ui.device-setCurrentIndex() be skipped on MacOs?

The thinking behind not using persisted serial port data in #3840 is that on today's computers practically all serial devices are connected through USB, and on most operating systems have dynamic identifiers (based on the order they were detected in). So in most cases the serial port identifier may or may not have changed. Not sure how this is for MacOS, but the device identifier that you supplied looks a lot like it is dependent on the USB bus ID, which will change if a different port is used.
Re not using remembered bluetooth ids on MacOS, this is based on #2158 (comment), so you'll have to work out with @dirkhh if this is still relevant.

@dirkhh: Do you have any insights about #2158 (comment) still being valid, or should this exclusion be dropped?

From comments that others have made, and from a quick experiment, it seems like Apple may have fixed that.

Sounds good @dirkhh, thanks for checking.
@notrege we still have the problem that this line essentially doubles up what https://github.com/subsurface/subsurface/pull/4120/files#diff-a0bde534c60cabf020b85f0079e0a8d567f5465514e8a59629b3a50a228d17f9R160 already does, and that it overwrites the serial port if it has already been selected, even if the 'remembered' serial port is not currently available.
So I'd probably remove this line, and instead drop the !isMacOs (and the comment about it) in the code below, and then move ui.bluetoothMode->setChecked(isBluetoothDevice) to below ui.device->setCurrentIndex(deviceIndex(qPrefDiveComputer::device##num())).

mikeller · 2024-03-05T20:48:45Z

desktop-widgets/downloadfromdivecomputer.cpp

+	if (state == Qt::Checked) {
+      		if (ui.device->currentText().isEmpty()) {
+         		selectRemoteBluetoothDevice();
+   		} else {


You've moved the else clause from the outer if to the (new) inner if - which is not the way this should be working.

Also note that coding style in Subsurface calls for tab as tabs (and not spaces), and for if / else with single statements to be without curly braces.

Sorry about that. I'm afraid that happened when retyping the code in git.

FWIW, the coding style does not state that single statements go without curly braces - the only relevant point I saw was:

o both sides of an if / else clause either use or do not use curly braces:

All good.
In essence, the purpose of having a defined coding style in a project and adhering to it is to avoid unneeded 'noise' from people reworking the style of sections of code when they are reworking or extending it. As such the 'coding style' is much more 'the style used for a majority of the code in a project' than it is a document that tries to codify this style. And in subsurface the predominant style is to not have curly braces for single if / else statements.

Replaces spaces with tabs and corrected misplaced braces (embarassing...). Signed-off-by: jme <32236882+notrege@users.noreply.github.com>

mikeller · 2024-03-05T23:38:47Z

desktop-widgets/downloadfromdivecomputer.cpp

-	else
+	if (state == Qt::Checked) {
+		if (ui.device->currentText().isEmpty()) 
+         		selectRemoteBluetoothDevice();


One more case of spaces - it often pays to enable 'show whitespace' in your editor before submitting code changes.

another space that should have been a tab. Signed-off-by: jme <32236882+notrege@users.noreply.github.com>

mikeller · 2024-03-07T00:42:52Z

desktop-widgets/downloadfromdivecomputer.cpp

-		selectRemoteBluetoothDevice();
-	else
+	if (state == Qt::Checked) {
+		if (ui.device->currentText().isEmpty()) 


I think this will work in an undesirable way if a serial port is selected for the device, and the user then checks the bluetooth box - in this case the serial port device selection will be retained. I think a better way to do this will be to use isBluetoothAddress() here to check if the device is a bluetooth device.

Thanks - that's better than just checking for a device id.

Per Michael's suggestion, use isBluetoothAddress as check before skipping Bluetooth scan. Signed-off-by: jme <32236882+notrege@users.noreply.github.com>

notrege · 2024-03-13T05:09:48Z

I can only speak anecdotally - the change works fine on my relatively old laptop running Catalina. The comment struck me as odd given that I can upload without scanning as long as I don't change the computer. It wasn't an issue until my wife started sharing my laptop on dive trips. If I change to a non-bluetooth and then back to my G2 I was forced to rescan. I can only guess that I misunderstood the comment. thank you both for looking at this, Jim

…

On Wed, Mar 13, 2024, 09:27 Dirk Hohndel ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In desktop-widgets/downloadfromdivecomputer.cpp <#4120 (comment)> : > @@ -149,6 +149,7 @@ int DownloadFromDCWidget::deviceIndex(QString deviceText) void DownloadFromDCWidget::DC##num##Clicked() \ { \ ui.vendor->setCurrentIndex(ui.vendor->findText(qPrefDiveComputer::vendor##num())); \ + ui.device->setCurrentText(qPrefDiveComputer::device##num()); \ From comments that others have made, and from a quick experiment, it seems like Apple may have fixed that. — Reply to this email directly, view it on GitHub <#4120 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHV6KUUNB5LRTWN66E4IDSTYX6TPDAVCNFSM6AAAAABEHD7RYGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSMZTGAYDQNBQGY> . You are receiving this because you authored the thread.Message ID: ***@***.***>

notrege · 2024-03-13T10:04:20Z

Thanks - am diving this week (and enjoying not rescanning) so I'll resume with this when I'm back. thanks for the help with this, Jim

…

On Wed, Mar 13, 2024, 14:04 Michael Keller ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In desktop-widgets/downloadfromdivecomputer.cpp <#4120 (comment)> : > @@ -149,6 +149,7 @@ int DownloadFromDCWidget::deviceIndex(QString deviceText) void DownloadFromDCWidget::DC##num##Clicked() \ { \ ui.vendor->setCurrentIndex(ui.vendor->findText(qPrefDiveComputer::vendor##num())); \ + ui.device->setCurrentText(qPrefDiveComputer::device##num()); \ Sounds good @dirkhh <https://github.com/dirkhh>, thanks for checking. @notrege <https://github.com/notrege> we still have the problem that this line essentially doubles up what https://github.com/subsurface/subsurface/pull/4120/files#diff-a0bde534c60cabf020b85f0079e0a8d567f5465514e8a59629b3a50a228d17f9R160 already does, and that it overwrites the serial port if it has already been selected, even if this serial port is not currently available. So I'd probably remove this line, and instead drop the !isMacOs (and the comment about it) in the code below. — Reply to this email directly, view it on GitHub <#4120 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHV6KUTNILQCZ5I2MGLERTTYX7T7XAVCNFSM6AAAAABEHD7RYGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSMZTGIZDOMJUGY> . You are receiving this because you were mentioned.Message ID: ***@***.***>

Preserve bluetooth address if we have it (don't rescan) and for MacOS save the mount point if we have it. As best as I can tell, the mount points for USB devices do not change on MacOs regardless of the USB port being used. Signed-off-by: jme <32236882+notrege@users.noreply.github.com>

mikeller

This is starting to look good now!

mikeller · 2024-04-14T04:04:30Z

desktop-widgets/downloadfromdivecomputer.cpp

+	is /dev/tty.usbserial-20030001 and it doesn't change regardless of the \
+	USB port being used.   If it isn't saved, it must be reentered. */ \
+	if (isBluetoothDevice || isMacOs) \
+		ui.device->setCurrentText(qPrefDiveComputer::device##num()); \


I think it will be better to set the device by index, like how it is done in https://github.com/subsurface/subsurface/pull/4120/files#diff-a0bde534c60cabf020b85f0079e0a8d567f5465514e8a59629b3a50a228d17f9L159 - this way, if the remembered device does not currently exist (i.e. the dive computer is not present / detected) the device will be cleared.

The change I made in enableBluetoothMode() depends on the value of ui.device->currentText(). Consequently, it must be set before enableBluetoothMode is called (via ui.bluetoothMode->setChecked(). If bluetoothMode is not checked, enableBluetoothMode sets the device index to -1. Consequently, either the device index must be set both before and after enableBluetoothMode(), or we need a different flag, such as setting CurrentText, for enableBluetoothMode to skip bluetooth scanning.

The change I made in enableBluetoothMode() depends on the value of ui.device->currentText(). Consequently, it must be set before enableBluetoothMode is called (via ui.bluetoothMode->setChecked().

Fully agreed on this part - my point is just that it is probably a better way to set this indirectly, through ui.device->setCurrentIndex(deviceIndex(qPrefDiveComputer::device##num())) (might also need a QCoreApplication::processEvents() to get ui.device->currentText() to update).

If bluetoothMode is not checked, enableBluetoothMode sets the device index to -1.

Good point, I had not considered the case of remembering the non-bluetooth devices on MacOS.
I suspect that the right thing there is to add another refinement to enableBluetoothMode(), to only set the index to -1 on disabling bluetooth if the selected device is a bluetooth device - equivalent to what you are doing when enabling bluetooth.

mikeller · 2024-04-14T04:05:32Z

desktop-widgets/downloadfromdivecomputer.cpp

 	ui.bluetoothMode->setChecked(isBluetoothDevice); \
-	if (ui.device->currentIndex() == -1 || (isBluetoothDevice && !isMacOs)) \
-		/* macOS seems to have a problem connecting to remembered bluetooth devices  if it hasn't already had a connection in the current session */ \
+	if (ui.device->currentIndex() == -1 || isBluetoothDevice) \


Not sure we still need this - the device is already set before https://github.com/subsurface/subsurface/pull/4120/files#diff-a0bde534c60cabf020b85f0079e0a8d567f5465514e8a59629b3a50a228d17f9L159.

mikeller · 2024-04-14T04:07:04Z

desktop-widgets/downloadfromdivecomputer.cpp

-		/* macOS seems to have a problem connecting to remembered bluetooth devices  if it hasn't already had a connection in the current session */ \
+	if (ui.device->currentIndex() == -1 || isBluetoothDevice) \
 		ui.device->setCurrentIndex(deviceIndex(qPrefDiveComputer::device##num())); \
 	if (isMacOs) { \


Not really required for this, but since you're already testing this with MacOS: Do you want to test if this extra code is still needed for MacOS, or if we can get a simplification by removing this - as stated, this is working fine without the extra code on other platforms.

I removed this and don't see any adverse effects. Given that I don't have a Windows or Linux platform to test on, it would be very helpful if someone could make sure I didn't break anything on Linux or Windows.

Happy to test this, but since this code is not run (and potentially even optimised away at build time) on non-MacOS I am fairly confident that removing it won't break anything there.

Removed the MacOs specific code and added comment explaining why ui.device.Text must be set before the ui.bluetoothMode change handler runs. Signed-off-by: jme <32236882+notrege@users.noreply.github.com>

Removed blank lines - forgot that blank lines without a continuation \ would break the macro. Embarrassing to say the least. I need to figure out how to upload tested code into git. Signed-off-by: jme <32236882+notrege@users.noreply.github.com>

Based on Michael's suggestion, added code to refine enableBluetoothMode. Signed-off-by: jme <32236882+notrege@users.noreply.github.com>

mikeller · 2024-04-16T19:41:44Z

desktop-widgets/downloadfromdivecomputer.cpp

+		if (!isBluetoothAddress(ui.device->currentText()))
+			selectRemoteBluetoothDevice();
+	} else
+		if (isBluetoothAddress(ui.device->currentText()) || QSysInfo::kernelType() != "darwin")


I did some testing on linux with this, and this is now removing the remembered filesystem path for Garmin devices - so I think it will be better if we remove the || QSysInfo::kernelType() != "darwin" - the fact that we are loading the device from a list of the detected devices with ui.device->setCurrentIndex(deviceIndex(qPrefDiveComputer::device##num())) will make sure that we are not setting a non-existent device.

Thanks for testing this - removed Mac specific behavior and any non-Bluetooth mount points will be preserved.

If Bluetooth isn't enabled, don't clear non Bluetooth address. There was an earlier concern that USB mount point shouldn't be preserved because they may change. This behavior is different on a Mac where the USB serial mount points tend to be persistent. Michael tested this on Linux and suggested on saving the mount points for Linux and Windows. Signed-off-by: jme <32236882+notrege@users.noreply.github.com>

notrege · 2024-04-16T20:55:30Z

Woo hoo! Thanks for all the help with this Michael.

…

On Tue, Apr 16, 2024, 21:51 Michael Keller ***@***.***> wrote: ***@***.**** approved this pull request. — Reply to this email directly, view it on GitHub <#4120 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHV6KURCWCZVTBYZUDYB7WDY5WFMLAVCNFSM6AAAAABEHD7RYGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDAMBUGU3TMNZSGI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

mikeller · 2024-04-16T22:03:28Z

Thanks for sticking with this @notrege - the result looks very clean and is a reduction in complexity, which is always welcome.
Tested in linux.

Since the removal of this special case for MacOS was confirmed to be working in the case of building with bluetooth support in subsurface#4120, we should probably remove it from the code used when building without bluetooth support as well. Signed-off-by: Michael Keller <mikeller@042.ch>

Since the removal of this special case for MacOS was confirmed to be working in the case of building with bluetooth support in #4120, we should probably remove it from the code used when building without bluetooth support as well. Signed-off-by: Michael Keller <mikeller@042.ch>

Since the removal of this special case for MacOS was confirmed to be working in the case of building with bluetooth support in subsurface#4120, we should probably remove it from the code used when building without bluetooth support as well. Signed-off-by: Michael Keller <mikeller@042.ch>

notrege added 2 commits March 5, 2024 09:05

Merge pull request #1 from notrege/jme-patch-1

7f76c3f

Avoid unnecessary bluetooth device address scanning.

mikeller reviewed Mar 5, 2024

View reviewed changes

Update downloadfromdivecomputer.cpp

c80dc32

Replaces spaces with tabs and corrected misplaced braces (embarassing...). Signed-off-by: jme <32236882+notrege@users.noreply.github.com>

mikeller reviewed Mar 5, 2024

View reviewed changes

Update downloadfromdivecomputer.cpp

0a236f1

another space that should have been a tab. Signed-off-by: jme <32236882+notrege@users.noreply.github.com>

mikeller reviewed Mar 7, 2024

View reviewed changes

notrege added 2 commits March 7, 2024 16:25

Merge branch 'subsurface:master' into master

9c582aa

Update downloadfromdivecomputer.cpp

32c41c4

Per Michael's suggestion, use isBluetoothAddress as check before skipping Bluetooth scan. Signed-off-by: jme <32236882+notrege@users.noreply.github.com>

mikeller reviewed Apr 14, 2024

View reviewed changes

notrege added 3 commits April 15, 2024 16:19

Update downloadfromdivecomputer.cpp

3f0ce38

Removed the MacOs specific code and added comment explaining why ui.device.Text must be set before the ui.bluetoothMode change handler runs. Signed-off-by: jme <32236882+notrege@users.noreply.github.com>

Update downloadfromdivecomputer.cpp

b3d79fc

Removed blank lines - forgot that blank lines without a continuation \ would break the macro. Embarrassing to say the least. I need to figure out how to upload tested code into git. Signed-off-by: jme <32236882+notrege@users.noreply.github.com>

Update downloadfromdivecomputer.cpp

ef1d154

Based on Michael's suggestion, added code to refine enableBluetoothMode. Signed-off-by: jme <32236882+notrege@users.noreply.github.com>

mikeller reviewed Apr 16, 2024

View reviewed changes

mikeller approved these changes Apr 16, 2024

View reviewed changes

mikeller merged commit 77281f8 into subsurface:master Apr 16, 2024

mikeller mentioned this pull request Apr 18, 2024

Desktop: Remove Unneeded Special Case for MacOS. #4157

Merged

7 tasks

Uh oh!

Avoid unnecessary bluetooth device address scanning. #4120

Avoid unnecessary bluetooth device address scanning. #4120

Uh oh!

Conversation

notrege commented Mar 5, 2024

Describe the pull request:

Pull request long description:

Changes made:

Related issues:

Additional information:

Release note:

Documentation change:

Mentions:

Uh oh!

mikeller left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikeller Mar 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

notrege commented Mar 13, 2024 via email

Uh oh!

notrege commented Mar 13, 2024 via email

Uh oh!

mikeller left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikeller Apr 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

notrege commented Apr 16, 2024 via email

Uh oh!

mikeller commented Apr 16, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

mikeller Mar 13, 2024 •

edited

Loading

mikeller Apr 15, 2024 •

edited

Loading