Better detect BT devices and LTE devices #242

posterzh · 2021-11-22T12:41:30Z

Why
Detect the BLE and LET devices in better way.

How
Get the BLE devices and LTE devices programatically using dbus.

References
#96
#88

shawaj · 2021-11-22T18:42:08Z

Dockerfile

+            pkg-config=0.29-6 \
+            libdbus-1-3 && \
+    # Because the PATH is already updated above, this command creates a new venv AND activates it
+    python3 -m venv /opt/venv && \


IIRC @marvinmarnold and @vpetersson mentioned previously that there was little point in using venv in containerised setup.

Think was in relation to hm-config but guess it applies here too

I still have a soft spot for venv since it makes python portable. But yea, we did agree to stop using it.

@posterzh hm-pktfwd is an example of the new pattern: https://github.com/NebraLtd/hm-pktfwd/blob/e067bef1be17f9d3c1036cbe1100b317d5cc5f57/Dockerfile#L65

Nice pattern.

@marvinmarnold I am not fussed either way! Whichever you think is best TBH

shawaj · 2021-11-22T18:44:48Z

Dockerfile

+RUN \
+    install_packages \
+        i2c-tools \
+        bluez \


Do we need bluez and network manager in the container if we are communicating with them over dbus?

I will check.

@shawaj Without the dependencies of bluez, network-manager, modem-manager, detecting BT, WIFI, LTE devices works well with only dbus dependency. I removed unnecessary dependencies in the Dockerfile.

shawaj · 2021-11-22T18:46:35Z

Dockerfile

+# Finally, use pip to install dependencies.
+RUN \
+    install_packages \
+            python3-dev=3.7.3-1 \


IIRC @vpetersson thought it best to unpin packages previously

vpetersson · 2021-11-23T08:45:09Z

Nice improvement, @posterzh!

posterzh · 2021-11-24T03:46:34Z

Detecting BT and LTE devices using dbus:

Diagnostic Web page:

Diagnostic container from the multistage docker build:

shawaj · 2021-11-24T05:11:16Z

Looks good to me, but probably worth @vpetersson or @marvinmarnold or someone else from @NebraLtd/developers taking a deeper look.

vpetersson · 2021-11-24T08:52:46Z

Good job @posterzh. This is a big improvement. I wonder however if we should move the detection logic to hm-pyhelper` instead as we should probably re-use this code in hm-diag too. Wdyt @marvinmarnold?

posterzh · 2021-11-24T11:27:22Z

Good job @posterzh. This is a big improvement. I wonder however if we should move the detection logic to hm-pyhelper` instead as we should probably re-use this code in hm-diag too. Wdyt @marvinmarnold?

FYI, in the hm-diag, the only feature used is to check the presence of BT/LTE devices. Furthermore, I added the utility methods to get the list of BT, WiFi, LTE devices with details(get_bt_devices(), get_wifi_devices(), get_lte_devices()). These details of devices are used in checking the presence for validating their state. Meanwhile, almost the same code can be used for managing and connecting WiFi devices using dbus feature.

marvinmarnold

This work looks really good. The code is very readable and clear.

This logic also needs to be added to https://github.com/NebraLtd/hm-diag/blob/00c716dd4980e35ac7436d9f59aebb17bec07535/hw_diag/diagnostics/bt_lte_diagnostic.py Until we have completed https://github.com/NebraLtd/Hotspot-Production-Tool/issues/28 we will continue to have parallel logic for /json and /initFile.txt.

Lastly, my typical comment. Could you please add some basic unit tests?

marvinmarnold · 2021-11-24T17:18:06Z

hw_diag/utilities/hardware.py

 from hm_pyhelper.miner_param import get_public_keys_rust
 from hm_pyhelper.hardware_definitions import variant_definitions, is_rockpi
 from hw_diag.utilities.shell import config_search_param

+log = logging.getLogger()


Can you use get_logger from hm_pyhelper?

Ok, make sense.

marvinmarnold · 2021-11-24T17:23:02Z

hw_diag/utilities/hardware.py

+
+
+if __name__ == '__main__':
+    print('set_diagnostics_bt_lte: %s' % set_diagnostics_bt_lte({}))


This should be log instead of print.

vpetersson · 2021-11-24T08:46:20Z

Dockerfile

-    pip3 install --no-cache-dir -r /tmp/build/requirements.txt && \
-    python3 setup.py install && \
-    rm -rf /tmp/build
+# Cleanup


You can remove this entirely. That's the whole purpose of using install_packages from Balena as it takes care of the cleanup.

Yes, the size of the docker image is same after removing the clean up.

vpetersson · 2021-11-24T08:47:30Z

hw_diag/utilities/hardware.py

 from hm_pyhelper.miner_param import get_public_keys_rust
 from hm_pyhelper.hardware_definitions import variant_definitions, is_rockpi
 from hw_diag.utilities.shell import config_search_param

+log = logging.getLogger()
+log.setLevel(logging.DEBUG)


This log level needs to be controlled centrally. IIRC we already have a framework for this. We don't want debug logging in production unless we're troubleshooting something.

vpetersson · 2021-11-24T08:48:30Z

hw_diag/utilities/hardware.py

@@ -15,25 +74,112 @@ def should_display_lte(diagnostics):
    return variant_data.get('CELLULAR')


+def get_ble_devices():
+    log.info("Get BLE devices.")


'Retrieving list of BLE device(s)"

vpetersson · 2021-11-24T08:48:54Z

hw_diag/utilities/hardware.py

+                    "Discovering": str(adapter.get("Discovering")),
+                })
+
+        log.info('BLE Devices: %s' % ble_devices)


"Found the following BLE devices:"

vpetersson · 2021-11-24T08:49:36Z

hw_diag/utilities/hardware.py

+        log.info('BLE Devices: %s' % ble_devices)
+    except dbus.exceptions.DBusException as ex:
+        log.error(ex.get_dbus_message())
+    except Exception as ex:


I think the standardized way is to do Exception as e. Not a big deal, just to keep things consistent.

I agree. I will keep in mind it.

vpetersson · 2021-11-24T08:50:25Z

hw_diag/utilities/hardware.py

+        log.info('WiFi Devices: %s' % wifi_devices)
+    except dbus.exceptions.DBusException as ex:
+        log.error(ex.get_dbus_message())
+    except Exception as ex:


as e (same as above).

vpetersson · 2021-11-24T08:50:59Z

hw_diag/utilities/hardware.py

+
+
+def get_lte_devices():
+    log.info("Get LTE devices.")


"Retrieving list of LTE device(s)."

vpetersson · 2021-11-24T08:51:04Z

hw_diag/utilities/hardware.py

+                })
+
+        log.info('LTE Devices: %s' % lte_devices)
+    except dbus.exceptions.DBusException as ex:


as e (same as above).

vpetersson · 2021-11-24T08:51:11Z

hw_diag/utilities/hardware.py

+        log.info('LTE Devices: %s' % lte_devices)
+    except dbus.exceptions.DBusException as ex:
+        log.error(ex.get_dbus_message())
+    except Exception as ex:


as e (same as above).

vpetersson · 2021-11-24T08:51:32Z

hw_diag/utilities/hardware.py

+
+
+if __name__ == '__main__':
+    print('set_diagnostics_bt_lte: %s' % set_diagnostics_bt_lte({}))


Let's avoid using print() in favor of logging.

vpetersson · 2021-11-25T09:09:53Z

@posterzh Please fix the linting errors.

vpetersson

Approved by me, but I would ideally like to see some unit tests here. Don't merge this before @marvinmarnold approves it.

vpetersson · 2021-11-25T09:17:21Z

Dockerfile

+    install_packages \
+            build-essential \
+            libdbus-glib-1-dev && \
+    pip3 install --no-cache-dir --target="$PYTHON_DEPENDENCIES_DIR" .


Nitpicking here, but I'd probably run this on a separate RUN line just to help with debugging if for whatever reason it would fail building.

It would help debugging.

Approved by me, but I would ideally like to see some unit tests here. Don't merge this before @marvinmarnold approves it.

Yeah, I am writing the unit test and a parallel logic in DiagnosticsReport. These pushes are just for In-progress.

I see. Please update the PR to 'Draft' mode if that is the case.

shawaj · 2021-11-25T15:48:09Z

Good job @posterzh. This is a big improvement. I wonder however if we should move the detection logic to hm-pyhelper` instead as we should probably re-use this code in hm-diag too. Wdyt @marvinmarnold?

@vpetersson do you think we would need this in hm-config? We seem to be using some custom nmcli in there so not sure it'll be compatible https://github.com/NebraLtd/hm-config/blob/3b2e0fe138ab04533ffa44afa3d0a60d7972430e/gatewayconfig/processors/wifi_processor.py#L20

posterzh · 2021-11-26T02:25:34Z

This work looks really good. The code is very readable and clear.

This logic also needs to be added to https://github.com/NebraLtd/hm-diag/blob/00c716dd4980e35ac7436d9f59aebb17bec07535/hw_diag/diagnostics/bt_lte_diagnostic.py Until we have completed NebraLtd/Hotspot-Production-Tool#28 we will continue to have parallel logic for /json and /initFile.txt.

Lastly, my typical comment. Could you please add some basic unit tests?

@marvinmarnold
Unit tests are added.
What about using a separate BtDiagnostics and LteDiagnostics rather than BtLteDiagnostics?

hm-diag/hw_diag/diagnostics/bt_lte_diagnostic.py

Line 60 in c725318

class BtLteDiagnostics():

marvinmarnold

What about using a separate BtDiagnostics and LteDiagnostics rather than BtLteDiagnostics?

Good idea but lets not let that hold things up. Merge this and create a follow up ticket to do in a future sprint.

posterzh · 2021-11-26T19:47:39Z

@marvinmarnold I created a follow up ticket as above.

posterzh requested review from shawaj, marvinmarnold and vpetersson and removed request for marvinmarnold November 22, 2021 13:03

posterzh force-pushed the posterzh/better-detect-bt branch from ef3f6bc to 3178de9 Compare November 22, 2021 18:35

shawaj reviewed Nov 22, 2021

View reviewed changes

posterzh force-pushed the posterzh/better-detect-bt branch 4 times, most recently from aa12bc8 to 23de222 Compare November 23, 2021 08:27

posterzh force-pushed the posterzh/better-detect-bt branch from 23de222 to 0da1c07 Compare November 23, 2021 09:01

posterzh added 5 commits November 23, 2021 04:29

feat: detect BT, WiFi and LTE devices using dbus

85c08a9

refactor: unpin package versions

8829cdd

refactor: remove unnecessary dependencies since dbus is used

3d02bc2

refactor: stop using venv

65e7fd4

fix: hotfix packages

c97f52d

posterzh force-pushed the posterzh/better-detect-bt branch from 0da1c07 to 7907641 Compare November 23, 2021 09:29

refactor: copy Python dependencies between multi stage containers

ce9ae2c

posterzh force-pushed the posterzh/better-detect-bt branch from 7907641 to 03b9a44 Compare November 24, 2021 00:21

refactor: slim down the package dependencies

7fe3df3

posterzh force-pushed the posterzh/better-detect-bt branch from 03b9a44 to 7fe3df3 Compare November 24, 2021 03:41

posterzh marked this pull request as ready for review November 24, 2021 03:46

posterzh requested a review from a team as a code owner November 24, 2021 03:46

posterzh requested a review from shawaj November 24, 2021 03:47

marvinmarnold suggested changes Nov 24, 2021

View reviewed changes

vpetersson suggested changes Nov 24, 2021

View reviewed changes

posterzh added 2 commits November 25, 2021 02:35

refactor: update logging

6524432

refactor: remove unnecessary cleanup

1870e57

fix: lint

e90d8d9

vpetersson approved these changes Nov 25, 2021

View reviewed changes

chore: update Dockerfile

69a3197

posterzh marked this pull request as draft November 25, 2021 15:28

test: add BLE, WiFi, LTE test

4589bbd

marvinmarnold approved these changes Nov 26, 2021

View reviewed changes

posterzh marked this pull request as ready for review November 26, 2021 18:01

posterzh merged commit 625ea72 into master Nov 26, 2021

posterzh deleted the posterzh/better-detect-bt branch November 26, 2021 18:01

posterzh mentioned this pull request Nov 26, 2021

Update BtLteDiagnostic() to use the new method for detecting BT and LTE devices #252

Closed

shawaj mentioned this pull request Nov 26, 2021

Bluetooth detection: better way to identify BT interface #96

Closed

posterzh mentioned this pull request Nov 26, 2021

feat(diagnostics): better detect BT devices and LTE devices NebraLtd/helium-miner-software#280

Merged

marvinmarnold mentioned this pull request Dec 15, 2021

Bluetooth / WiFi not working properly on certain RockPi devices NebraLtd/hm-config#156

Closed

shawaj mentioned this pull request Dec 28, 2021

Add Multi-stage build to Dockerfile #141

Closed



		if __name__ == '__main__':
		print('set_diagnostics_bt_lte: %s' % set_diagnostics_bt_lte({}))

Better detect BT devices and LTE devices #242

Better detect BT devices and LTE devices #242

Conversation

posterzh commented Nov 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vpetersson commented Nov 23, 2021

posterzh commented Nov 24, 2021

shawaj commented Nov 24, 2021

vpetersson commented Nov 24, 2021

posterzh commented Nov 24, 2021 • edited Loading

marvinmarnold left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vpetersson commented Nov 25, 2021

vpetersson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shawaj commented Nov 25, 2021

posterzh commented Nov 26, 2021

marvinmarnold left a comment

Choose a reason for hiding this comment

posterzh commented Nov 26, 2021

posterzh commented Nov 24, 2021 •

edited

Loading