Fix ble notifications in Python SDK (#396)

.github/workflows/python_sdk_test.yml

@@ -17,7 +17,7 @@ jobs:
         strategy:
             matrix:
                 os: [windows-latest, macos-latest, ubuntu-latest]
-                python-version: ["3.9", "3.10", "3.11"]
+                python-version: ['3.9', '3.10', '3.11']
                 include:
                     - os: ubuntu-latest
                       path: ~/.cache/pip
@@ -41,8 +41,8 @@ jobs:
                   key: ${{ runner.os }}-${{ matrix.python-version }}-pip-${{ hashFiles('demos/python/sdk_wireless_camera_control/poetry.lock') }}
                   restore-keys: ${{ runner.os }}-pip-
 
-            - name: Install Bluez on Ubuntu
-              run: sudo apt-get update && sudo apt install -y bluez
+            - name: Install prereqs on Ubuntu
+              run: sudo apt-get update && sudo apt install -y bluez graphviz
               if: ${{ matrix.os == 'ubuntu-latest'}}
 
             - name: Install dependencies
@@ -53,10 +53,16 @@ jobs:
                   pip install nox-poetry==1.0.3
                   pip install poetry
 
-            - name: Perform checks (format, types, lint, docstrings, unit tests, docs, safety)
+            - name: Perform checks (format, types, lint, docstrings, unit tests)
               working-directory: ./demos/python/sdk_wireless_camera_control/
               run: nox -p ${{ matrix.python-version }}
 
+            # Only test docs with latest Python on ubuntu since we need graphviz
+            - name: Test docs build
+              if: matrix.os == 'ubuntu-latest' &&  matrix.python-version == '3.11'
+              working-directory: ./demos/python/sdk_wireless_camera_control/
+              run: nox -s docs
+
             - name: Archive test report on failure
               uses: actions/upload-artifact@v2
               if: failure()

demos/python/sdk_wireless_camera_control/docs/changelog.rst

@@ -9,8 +9,12 @@ All notable changes to this project will be documented in this file.
 The format is based on `Keep a Changelog <https://keepachangelog.com/en/1.0.0/>`_,
 and this project adheres to `Semantic Versioning <https://semver.org/spec/v2.0.0.html>`_.
 
+Unreleased
+----------
+* Fix BLE notifications not being routed correctly
+
 0.14.0 (September-13-2022)
--------------------------
+--------------------------
 * NOTE! This is a major update and includes massive API breaking changes.
 * Move to asyncio-based framework
 * Add HERO 12 support

demos/python/sdk_wireless_camera_control/noxfile.py

@@ -6,7 +6,8 @@
 import nox
 from nox_poetry import session
 
-nox.options.sessions = "format", "lint", "tests", "docstrings", "docs"
+# Don't run docs by default since it needs graphviz.
+nox.options.sessions = "format", "lint", "tests", "docstrings"
 
 SUPPORTED_VERSIONS = [
     "3.9",
@@ -78,6 +79,6 @@ def docs(session) -> None:
         "autodoc-pydantic",
         "darglint",
     )
-    session.run("sphinx-build", "docs", "docs/build")
+    session.run("sphinx-build", "-W", "docs", "docs/build")
     # Clean up for Jekyll consumption
     session.run("rm", "-rf", "docs/build/.doctrees", "/docs/build/_sources", "/docs/build/_static/fonts", external=True)

demos/python/sdk_wireless_camera_control/open_gopro/demos/log_battery.py

@@ -17,7 +17,7 @@
 from open_gopro import WirelessGoPro, types
 from open_gopro.constants import StatusId
 from open_gopro.logger import set_stream_logging_level, setup_logging
-from open_gopro.util import add_cli_args_and_parse
+from open_gopro.util import add_cli_args_and_parse, ainput
 
 console = Console()
 
@@ -84,56 +84,55 @@ async def process_battery_notifications(update: types.UpdateType, value: int) ->
 
 async def main(args: argparse.Namespace) -> None:
     logger = setup_logging(__name__, args.log)
-    global SAMPLE_INDEX
 
     gopro: Optional[WirelessGoPro] = None
     try:
         async with WirelessGoPro(args.identifier, enable_wifi=False) as gopro:
             set_stream_logging_level(logging.ERROR)
 
-            if args.poll:
-                with console.status("[bold green]Polling the battery until it dies..."):
-                    while True:
-                        SAMPLES.append(
-                            Sample(
-                                index=SAMPLE_INDEX,
-                                percentage=(await gopro.ble_status.int_batt_per.get_value()).data,
-                                bars=(await gopro.ble_status.batt_level.get_value()).data,
+            async def log_battery() -> None:
+                global SAMPLE_INDEX
+                if args.poll:
+                    with console.status("[bold green]Polling the battery until it dies..."):
+                        while True:
+                            SAMPLES.append(
+                                Sample(
+                                    index=SAMPLE_INDEX,
+                                    percentage=(await gopro.ble_status.int_batt_per.get_value()).data,
+                                    bars=(await gopro.ble_status.batt_level.get_value()).data,
+                                )
                             )
-                        )
-                        console.print(str(SAMPLES[-1]))
-                        SAMPLE_INDEX += 1
-                        await asyncio.sleep(args.poll)
-            # Otherwise set up notifications
-            else:
-                global last_bars
-                global last_percentage
-
-                console.print("Configuring battery notifications...")
-                # Enable notifications of the relevant battery statuses. Also store initial values.
-                last_bars = (
-                    await gopro.ble_status.batt_level.register_value_update(process_battery_notifications)
-                ).data
-                last_percentage = (
-                    await gopro.ble_status.int_batt_per.register_value_update(process_battery_notifications)
-                ).data
-                # Append initial sample
-                SAMPLES.append(Sample(index=SAMPLE_INDEX, percentage=last_percentage, bars=last_bars))
-                console.print(str(SAMPLES[-1]))
-
-                # Start a thread to handle asynchronous battery level notifications
-                console.print("[bold green]Receiving battery notifications until it dies...")
-                while True:
-                    await asyncio.sleep(1)
+                            console.print(str(SAMPLES[-1]))
+                            SAMPLE_INDEX += 1
+                            await asyncio.sleep(args.poll)
+                else:  # Not polling. Set up notifications
+                    global last_bars
+                    global last_percentage
+
+                    console.print("Configuring battery notifications...")
+                    # Enable notifications of the relevant battery statuses. Also store initial values.
+                    last_bars = (
+                        await gopro.ble_status.batt_level.register_value_update(process_battery_notifications)
+                    ).data
+                    last_percentage = (
+                        await gopro.ble_status.int_batt_per.register_value_update(process_battery_notifications)
+                    ).data
+                    # Append initial sample
+                    SAMPLES.append(Sample(index=SAMPLE_INDEX, percentage=last_percentage, bars=last_bars))
+                    console.print(str(SAMPLES[-1]))
+                    console.print("[bold green]Receiving battery notifications until it dies...")
+
+            asyncio.create_task(log_battery())
+            await ainput("[purple]Press enter to exit.", console.print)
+            console.print("Exiting...")
 
     except KeyboardInterrupt:
         logger.warning("Received keyboard interrupt. Shutting down...")
-    if len(SAMPLES) > 0:
+    if SAMPLES:
         csv_location = Path(args.log.parent) / "battery_results.csv"
         dump_results_as_csv(csv_location)
     if gopro:
         await gopro.close()
-    console.print("Exiting...")
 
 
 def parse_arguments() -> argparse.Namespace:

demos/python/sdk_wireless_camera_control/open_gopro/gopro_wireless.py

@@ -27,7 +27,7 @@
 )
 from open_gopro.ble import BleakWrapperController, BleUUID
 from open_gopro.communicator_interface import GoProWirelessInterface, MessageRules
-from open_gopro.constants import ActionId, GoProUUIDs, QueryCmdId, SettingId, StatusId
+from open_gopro.constants import ActionId, GoProUUIDs, StatusId
 from open_gopro.gopro_base import GoProBase, GoProMessageInterface, MessageMethodType
 from open_gopro.logger import Logger
 from open_gopro.models.response import BleRespBuilder, GoProResp
@@ -538,12 +538,17 @@ async def _update_internal_state(self, update: types.UpdateType, value: int) ->
             logger.trace("Control setting encoded started")  # type: ignore
             self._encoding_started.set()
 
+    # TODO this needs unit testing
     async def _route_response(self, response: GoProResp) -> None:
         """After parsing response, route it to any stakeholders (such as registered listeners)
 
         Args:
             response (GoProResp): parsed response
         """
+        # Flatten data if possible
+        if response._is_query and not response._is_push:
+            response.data = list(response.data.values())[0]
+
         # Check if this is the awaited synchronous response (id matches). Note! these have to come in order.
         response_claimed = False
         if await self._sync_resp_wait_q.peek_front() == response.identifier:
@@ -554,10 +559,10 @@ async def _route_response(self, response: GoProResp) -> None:
         # If this wasn't the awaited synchronous response...
         if not response_claimed:
             logger.info(Logger.build_log_rx_str(response, asynchronous=True))
-        if isinstance(response.identifier, QueryCmdId):
+        if response._is_push:
             for update_id, value in response.data.items():
                 await self._notify_listeners(update_id, value)
-        elif isinstance(response.identifier, (StatusId, SettingId, ActionId)):
+        elif isinstance(response.identifier, ActionId):
             await self._notify_listeners(response.identifier, response.data)
 
     def _notification_handler(self, handle: int, data: bytearray) -> None:

demos/python/sdk_wireless_camera_control/open_gopro/models/response.py

@@ -143,6 +143,28 @@ def ok(self) -> bool:
         """
         return self.status in [ErrorCode.SUCCESS, ErrorCode.UNKNOWN]
 
+    @property
+    def _is_push(self) -> bool:
+        """Was this response an asynchronous push?
+
+        Returns:
+            bool: True if yes, False otherwise
+        """
+        return self.identifier in [
+            QueryCmdId.STATUS_VAL_PUSH,
+            QueryCmdId.SETTING_VAL_PUSH,
+            QueryCmdId.SETTING_CAPABILITY_PUSH,
+        ]
+
+    @property
+    def _is_query(self) -> bool:
+        """Is this response to a settings / status query?
+
+        Returns:
+            bool: True if yes, False otherwise
+        """
+        return isinstance(self.identifier, QueryCmdId)
+
 
 class RespBuilder(ABC, Generic[T]):
     """Common Response Builder Interface"""
@@ -437,12 +459,6 @@ def build(self) -> GoProResp:
 
             # Query (setting get value, status get value, etc.)
             if query_type:
-                is_multiple = self._identifier in [
-                    QueryCmdId.GET_CAPABILITIES_VAL,
-                    QueryCmdId.REG_CAPABILITIES_UPDATE,
-                    QueryCmdId.SETTING_CAPABILITY_PUSH,
-                ]
-
                 camera_state: types.CameraState = defaultdict(list)
                 self._status = ErrorCode(buf[0])
                 buf = buf[1:]
@@ -467,7 +483,11 @@ def build(self) -> GoProResp:
                             camera_state[param_id] = param_val
                             continue
                         # These can be more than 1 value so use a list
-                        if is_multiple:
+                        if self._identifier in [
+                            QueryCmdId.GET_CAPABILITIES_VAL,
+                            QueryCmdId.REG_CAPABILITIES_UPDATE,
+                            QueryCmdId.SETTING_CAPABILITY_PUSH,
+                        ]:
                             # Parse using parser from global map and append
                             camera_state[param_id].append(parser.parse(param_val))
                         else:
@@ -479,12 +499,8 @@ def build(self) -> GoProResp:
                         # isn't functionally critical
                         logger.warning(f"{param_id} does not contain a value {param_val}")
                         camera_state[param_id] = param_val
-                # Flatten if not multiple
-                if is_multiple:
-                    self._identifier = list(camera_state.keys())[0]
-                    parsed = list(camera_state.values())[0]
-                else:
-                    parsed = camera_state
+                parsed = camera_state
+
             else:  # Commands,  Protobuf, and direct Reads
                 if is_cmd := isinstance(self._identifier, CmdId):
                     # All (non-protobuf) commands have a status

demos/python/sdk_wireless_camera_control/pyproject.toml

@@ -1,6 +1,6 @@
 [tool.poetry]
 name = "open_gopro"
-version = "0.14.0"
+version = "0.14.1"
 description = "Open GoPro API and Examples"
 authors = ["Tim Camise <tcamise@gopro.com>"]
 readme = "README.md"
@@ -183,7 +183,7 @@ log_file_level = "DEBUG"
 log_file_format = "%(threadName)13s: %(name)40s:%(lineno)5d %(asctime)s.%(msecs)03d %(levelname)-8s | %(message)s"
 log_file_date_format = "%H:%M:%S"
 filterwarnings = "ignore::DeprecationWarning"
-# timeout = 10
+timeout = 10
 addopts = [
     "-s",
     "--capture=tee-sys",

demos/python/sdk_wireless_camera_control/tests/conftest.py

@@ -40,6 +40,7 @@
 from open_gopro.communicator_interface import GoProBle, GoProWifi
 from open_gopro.constants import CmdId, ErrorCode, GoProUUIDs, StatusId
 from open_gopro.exceptions import ConnectFailed, FailedToFindDevice
+from open_gopro.gopro_base import GoProBase
 from open_gopro.logger import set_logging_level, setup_logging
 from open_gopro.models.response import GoProResp
 from open_gopro.wifi import SsidState, WifiClient, WifiController
@@ -444,6 +445,7 @@ def close(self) -> None:
 @pytest.fixture(params=versions)
 async def mock_wireless_gopro_basic(request):
     test_client = MockWirelessGoPro(request.param)
+    GoProBase.HTTP_GET_RETRIES = 1
     yield test_client
     test_client.close()
 

demos/python/sdk_wireless_camera_control/tests/unit/test_responses.py

@@ -35,8 +35,10 @@ def test_push_response_no_parameter_values():
     assert builder.is_finished_accumulating
     r = builder.build()
     assert r.ok
-    assert r.identifier == SettingId.RESOLUTION
-    assert r.data == []
+    assert r.identifier == QueryCmdId.SETTING_CAPABILITY_PUSH
+    assert r.data[SettingId.RESOLUTION] == []
+    assert r.data[SettingId.FPS] == []
+    assert r.data[SettingId.VIDEO_FOV] == []
 
 
 test_read_receive = bytearray([0x64, 0x62, 0x32, 0x2D, 0x73, 0x58, 0x56, 0x2D, 0x66, 0x62, 0x38])

demos/python/sdk_wireless_camera_control/tests/unit/test_wireless_gopro.py

@@ -131,7 +131,7 @@ async def receive_encoding_status(id: types.UpdateType, value: bool):
         event.set()
 
     mock_wireless_gopro_basic.register_update(receive_encoding_status, StatusId.ENCODING)
-    not_encoding = bytearray([0x05, 0x13, 0x00, StatusId.ENCODING.value, 0x01, 0x00])
+    not_encoding = bytearray([0x05, 0x93, 0x00, StatusId.ENCODING.value, 0x01, 0x00])
     mock_wireless_gopro_basic._notification_handler(0xFF, not_encoding)
     await event.wait()
 
@@ -153,7 +153,7 @@ async def receive_encoding_status(id: types.UpdateType, value: bool):
         event.set()
 
     mock_wireless_gopro_basic.register_update(receive_encoding_status, StatusId.ENCODING)
-    not_encoding = bytearray([0x05, 0x13, 0x00, StatusId.ENCODING.value, 0x01, 0x00])
+    not_encoding = bytearray([0x05, 0x93, 0x00, StatusId.ENCODING.value, 0x01, 0x00])
     mock_wireless_gopro_basic._notification_handler(0xFF, not_encoding)
     await event.wait()