Changing the advertisements used by Advanced RPS Game to have manufacturer data declared and set in the desired serialization order.

Changing the id numbers on manufacturer data in the advertisements to use a broadly sequential set.
Removing unused RPS_VERSION. adafruit#1185

Author: Kevin J Walters

CLUE_Rock_Paper_Scissors/advanced/rps_advertisements.py

@@ -29,6 +29,8 @@
 # the protocol and a descriptor for the encryption type
 
 # From adafruit_ble.advertising
+# 0xFF is "Manufacturer Specific Data" as per list of types in
+# https://www.bluetooth.com/specifications/assigned-numbers/generic-access-profile/
 MANUFACTURING_DATA_ADT = 0xFF
 ADAFRUIT_COMPANY_ID = 0x0822
 
@@ -38,16 +40,14 @@
 
 # According to https://github.com/adafruit/Adafruit_CircuitPython_BLE/blob/master/adafruit_ble/advertising/adafruit.py
 # 0xf000 (to 0xffff) is for range for Adafruit customers
-GM_JOIN_ID = 0xfe30
-RPS_VERSION = 0xff30
-RPS_ROUND_ID = 0xff74
-RPS_ENC_DATA_ID = 0xff34
-RPS_KEY_DATA_ID = 0xff54
-RPS_ACK_ID = 0xff52
-# These ID numbers have all been carefully selected to obtain a particular
-# ordering of the fields within the ManufacturerData based on the current
-# dict key ordering - this is bad practice and FRAGILE as the prefix
-# matching falls apart if this changes
+
+# These four are used as part of prefix matching
+RPS_ENC_DATA_ID = 0xfe41
+RPS_KEY_DATA_ID = 0xfe42
+RPS_ROUND_ID = 0xfe43
+GM_JOIN_ID = 0xfe44
+
+RPS_ACK_ID = 0xfe51
 
 # Data formats for shared fields
 _DATA_FMT_ROUND = "B"
@@ -85,25 +85,24 @@ class RpsEncDataAdvertisement(Advertisement):
         key_encoding="<H"
     )
 
-    sequence_number = ManufacturerDataField(ADAFRUIT_SEQ_ID, "<" + _SEQ_FMT)
-    """Sequence number of the data. Used in acknowledgements."""
-
     enc_data = ManufacturerDataField(RPS_ENC_DATA_ID, "<" + _DATA_FMT_ENC_DATA)
     round_no = ManufacturerDataField(RPS_ROUND_ID, "<" + _DATA_FMT_ROUND)
+    sequence_number = ManufacturerDataField(ADAFRUIT_SEQ_ID, "<" + _SEQ_FMT)
+    """Sequence number of the data. Used in acknowledgements."""
     ack = ManufacturerDataField(RPS_ACK_ID, "<" + _DATA_FMT_ACK)
     """Round number starting at 1."""
 
-    def __init__(self, *, enc_data=None, round_no=None, ack=None, sequence_number=None):
-        """ack must be set to () to send this optional, data-less field."""
+    def __init__(self, *, enc_data=None, round_no=None, sequence_number=None, ack=None):
+        """enc_data must be either set here in the constructor or set first."""
         super().__init__()
         if enc_data is not None:
             self.enc_data = enc_data
         if round_no is not None:
             self.round_no = round_no
-        if ack is not None:
-            self.ack = ack
         if sequence_number is not None:
             self.sequence_number = sequence_number
+        if ack is not None:
+            self.ack = ack
 
 
 class RpsKeyDataAdvertisement(Advertisement):
@@ -136,25 +135,24 @@ class RpsKeyDataAdvertisement(Advertisement):
         key_encoding="<H"
     )
 
-    sequence_number = ManufacturerDataField(ADAFRUIT_SEQ_ID, "<" + _SEQ_FMT)
-    """Sequence number of the data. Used in acknowledgements."""
-
     key_data = ManufacturerDataField(RPS_KEY_DATA_ID, "<" + _DATA_FMT_KEY_DATA)
     round_no = ManufacturerDataField(RPS_ROUND_ID, "<" + _DATA_FMT_ROUND)
+    sequence_number = ManufacturerDataField(ADAFRUIT_SEQ_ID, "<" + _SEQ_FMT)
+    """Sequence number of the data. Used in acknowledgements."""
     ack = ManufacturerDataField(RPS_ACK_ID, "<" + _DATA_FMT_ACK)
     """Round number starting at 1."""
 
-    def __init__(self, *, key_data=None, round_no=None, ack=None, sequence_number=None):
-        """ack must be set to () to send this optional, data-less field."""
+    def __init__(self, *, key_data=None, round_no=None, sequence_number=None, ack=None):
+        """key_data must be either set here in the constructor or set first."""
         super().__init__()
         if key_data is not None:
             self.key_data = key_data
         if round_no is not None:
             self.round_no = round_no
-        if ack is not None:
-            self.ack = ack
         if sequence_number is not None:
             self.sequence_number = sequence_number
+        if ack is not None:
+            self.ack = ack
 
 
 class RpsRoundEndAdvertisement(Advertisement):
@@ -187,22 +185,21 @@ class RpsRoundEndAdvertisement(Advertisement):
         key_encoding="<H"
     )
 
+    round_no = ManufacturerDataField(RPS_ROUND_ID, "<" + _DATA_FMT_ROUND)
     sequence_number = ManufacturerDataField(ADAFRUIT_SEQ_ID, "<" + _SEQ_FMT)
     """Sequence number of the data. Used in acknowledgements."""
-
-    round_no = ManufacturerDataField(RPS_ROUND_ID, "<" + _DATA_FMT_ROUND)
     ack = ManufacturerDataField(RPS_ACK_ID, "<" + _DATA_FMT_ACK)
     """Round number starting at 1."""
 
-    def __init__(self, *, round_no=None, ack=None, sequence_number=None):
-        """ack must be set to () to send this optional, data-less field."""
+    def __init__(self, *, round_no=None, sequence_number=None, ack=None):
+        """round_no must be either set here in the constructor or set first."""
         super().__init__()
         if round_no is not None:
             self.round_no = round_no
-        if ack is not None:
-            self.ack = ack
         if sequence_number is not None:
             self.sequence_number = sequence_number
+        if ack is not None:
+            self.ack = ack
 
 
 class JoinGameAdvertisement(Advertisement):
@@ -235,7 +232,7 @@ class JoinGameAdvertisement(Advertisement):
     )
 
     game = ManufacturerDataField(GM_JOIN_ID, "<" + _DATA_FMT)
-    """RPS choice."""
+    """The name of the game, limited to eight characters."""
 
     def __init__(self, *, game=None):
         super().__init__()

CLUE_Rock_Paper_Scissors/tests/test_rps_advertisements.py

@@ -0,0 +1,133 @@
+# The MIT License (MIT)
+#
+# Copyright (c) 2020 Kevin J. Walters
+#
+# Permission is hereby granted, free of charge, to any person obtaining a copy
+# of this software and associated documentation files (the "Software"), to deal
+# in the Software without restriction, including without limitation the rights
+# to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+# copies of the Software, and to permit persons to whom the Software is
+# furnished to do so, subject to the following conditions:
+#
+# The above copyright notice and this permission notice shall be included in
+# all copies or substantial portions of the Software.
+#
+# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+# AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+# THE SOFTWARE.
+
+import sys
+import os
+
+import unittest
+from unittest.mock import MagicMock
+
+verbose = int(os.getenv('TESTVERBOSE', '2'))
+
+# PYTHONPATH needs to be set to find adafruit_ble
+
+# Mocking library used by adafruit_ble
+sys.modules['_bleio'] = MagicMock()
+
+# Borrowing the dhalbert/tannewt technique from adafruit/Adafruit_CircuitPython_Motor
+sys.path.insert(0, os.path.abspath(os.path.join(os.path.dirname(__file__), '..')))
+
+# import what we are testing or will test in future
+# pylint: disable=unused-import,wrong-import-position
+from rps_advertisements import JoinGameAdvertisement, \
+                               RpsEncDataAdvertisement, \
+                               RpsKeyDataAdvertisement, \
+                               RpsRoundEndAdvertisement
+
+# pylint: disable=line-too-long
+
+class Test_RpsEncDataAdvertisement(unittest.TestCase):
+
+    def test_bytes_order(self):
+        """Testing the order of data inside the manufacturer's field to ensure it follows the
+           fields are set in. This is new behaviour to benefit prefix matching."""
+
+        rpsedad1 = RpsEncDataAdvertisement(enc_data=b"FIRST", round_no=33, sequence_number=17)
+
+        # This checks value is not the old incorrect order
+        self.assertNotEqual(bytes(rpsedad1),
+                            b"\x16\xff\x22\x08\x03\x03\x00\x11\nA\xfeFIRST\x00\x00\x00\x03C\xfe\x21",
+                            msg="Checking order of serialised data for"
+                                " ackless RpsEncDataAdvertisement does"
+                                " not follow previous incorrect order")
+
+        # This check for correct order
+        self.assertEqual(bytes(rpsedad1),
+                         b"\x16\xff\x22\x08\x0a\x41\xfeFIRST\x00\x00\x00\x03C\xfe\x21\x03\x03\x00\x11",
+                         msg="Checking order of serialised data for"
+                             " ackless RpsEncDataAdvertisement")
+
+        rpsedad1.ack = 29
+        self.assertEqual(bytes(rpsedad1),
+                         b"\x1a\xff\x22\x08\nA\xfeFIRST\x00\x00\x00\x03C\xfe!\x03\x03\x00\x11\x03Q\xfe\x1d",
+                         msg="Checking order of serialised data for"
+                             " RpsEncDataAdvertisement with ack set post construction")
+
+
+class Test_RpsKeyDataAdvertisement(unittest.TestCase):
+
+    def test_bytes_order(self):
+        """Testing the order of data inside the manufacturer's field to ensure it follows the
+           fields are set in. This is new behaviour to benefit prefix matching."""
+
+        rpskdad1 = RpsKeyDataAdvertisement(key_data=b"FIRST", round_no=33, sequence_number=17)
+
+        # This checks value is not the old incorrect order
+        self.assertNotEqual(bytes(rpskdad1),
+                            b"\x16\xff\x22\x08\x03\x03\x00\x11\nB\xfeFIRST\x00\x00\x00\x03C\xfe\x21",
+                            msg="Checking order of serialised data for"
+                                " ackless RpsKeyDataAdvertisement does"
+                                " not follow previous incorrect order")
+
+        # This check for correct order
+        self.assertEqual(bytes(rpskdad1),
+                         b"\x16\xff\x22\x08\x0a\x42\xfeFIRST\x00\x00\x00\x03C\xfe\x21\x03\x03\x00\x11",
+                         msg="Checking order of serialised data for"
+                             " ackless RpsKeyDataAdvertisement")
+
+        rpskdad1.ack = 29
+        self.assertEqual(bytes(rpskdad1),
+                         b"\x1a\xff\x22\x08\nB\xfeFIRST\x00\x00\x00\x03C\xfe!\x03\x03\x00\x11\x03Q\xfe\x1d",
+                         msg="Checking order of serialised data for"
+                             " RpsKeyDataAdvertisement with ack set post construction")
+
+
+class Test_RpsRoundEndAdvertisement(unittest.TestCase):
+
+    def test_bytes_order(self):
+        """Testing the order of data inside the manufacturer's field to ensure it follows the
+           fields are set in. This is new behaviour to benefit prefix matching."""
+
+        rpsread1 = RpsRoundEndAdvertisement(round_no=133, sequence_number=201)
+
+        # This checks value is not the old incorrect order
+        self.assertNotEqual(bytes(rpsread1),
+                            b"\x0b\xff\x22\x08\x03\x03\x00\xc9\x03C\xfe\x85",
+                            msg="Checking order of serialised data for"
+                                " ackless RpsRoundEndAdvertisement does"
+                                " not follow previous incorrect order")
+
+        # This check for correct order
+        self.assertEqual(bytes(rpsread1),
+                         b"\x0b\xff\x22\x08\x03C\xfe\x85\x03\x03\x00\xc9",
+                         msg="Checking order of serialised data for"
+                             " ackless RpsRoundEndAdvertisement")
+
+        rpsread1.ack = 200
+        self.assertEqual(bytes(rpsread1),
+                         b"\x0f" b"\xff\x22\x08\x03C\xfe\x85\x03\x03\x00\xc9" b"\x03Q\xfe\xc8",
+                         msg="Checking order of serialised data for"
+                             " RpsRoundEndAdvertisement with ack set post construction")
+
+
+if __name__ == '__main__':
+    unittest.main(verbosity=verbose)