Repository: ManiacalLabs/BiblioPixel
Pull request #1127 (2019-01-08 17:06:27)

Cache devices by hardware ID

rec: I tested this on the 12k, which is my goto hardware at this moment for multidevices.

However, as we know this didn't show the problem before.  At least it shows I didn't make it worse!
adammhaile: Ack, will test tonight

On Tue, Jan 8, 2019, 12:10 PM Tom Swirly <notifications@github.com wrote:

> I tested this on the 12k, which is my goto hardware at this moment for
> multidevices.
>
> However, as we know this didn't show the problem before. At least it shows
> I didn't make it worse!
>
> —
> You are receiving this because your review was requested.
> Reply to this email directly, view it on GitHub
> <https://github.com/ManiacalLabs/BiblioPixel/pull/1127#issuecomment-452377068>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AA6a6oIg7jSf0ZLyEg8L6tqGHf7JqHUwks5vBNDygaJpZM4Z15Rn>
> .
>

rec: So I have no idea why we are getting these failures - they're ubiquitous on new branches, nothing to do with this branch

The root error is this:

    raise VersionConflict(dist, req).with_context(dependent_req)
pkg_resources.VersionConflict: (pytest 3.0.7 (/home/travis/virtualenv/python3.4.6/lib/python3.4/site-packages), Requirement.parse('pytest>=3.6'))

But we don't specify a version of pytest at all.  And AFAIK none of our dependencies even specify pytest as a dependency!
adammhaile: Where do you see that?

On Tue, Jan 8, 2019, 12:44 PM Tom Swirly <notifications@github.com wrote:

> So I have no idea why we are getting these failures - they're ubiquitous
> on new branches, nothing to do with this branch
>
> The root error is this:
>
> raise VersionConflict(dist, req).with_context(dependent_req)
>
> pkg_resources.VersionConflict: (pytest 3.0.7
> (/home/travis/virtualenv/python3.4.6/lib/python3.4/site-packages),
> Requirement.parse('pytest>=3.6'))
>
> But we don't specify a version of pytest at all. And AFAIK none of our
> dependencies even specify pytest as a dependency!
>
> —
> You are receiving this because your review was requested.
> Reply to this email directly, view it on GitHub
> <https://github.com/ManiacalLabs/BiblioPixel/pull/1127#issuecomment-452388356>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AA6a6hCCXaAvP_qP2SCmv8Lwnh1xvSDnks5vBNjigaJpZM4Z15Rn>
> .
>

rec: Ah, forgot to paste this: https://travis-ci.org/ManiacalLabs/BiblioPixel/jobs/476942556

Note the "all checks failed" on this page.
adammhaile: This might be helpful? https://github.com/Lukasa/hyper/issues/328

On Tue, Jan 8, 2019 at 1:02 PM Tom Swirly <notifications@github.com> wrote:

> Ah, forgot to paste this:
> https://travis-ci.org/ManiacalLabs/BiblioPixel/jobs/476942556
>
> Note the "all checks failed" on this page.
>
> —
> You are receiving this because your review was requested.
> Reply to this email directly, view it on GitHub
> <https://github.com/ManiacalLabs/BiblioPixel/pull/1127#issuecomment-452394100>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AA6a6rCdEpG6z5puFzw22HE0PZ-COhxcks5vBN0agaJpZM4Z15Rn>
> .
>

adammhaile: @rec - Confirmed that works :D
rec: OK, so how do we fix this CI issue then?

I tried all sorts of things, without success.
adammhaile: I'm not really sure. I honestly don't get why it's only now happening...
I'll look more in a bit.

On Tue, Jan 8, 2019 at 5:10 PM Tom Swirly <notifications@github.com> wrote:

> OK, so how do we fix this CI issue then?
>
> I tried all sorts of things, without success.
>
> —
> You are receiving this because your review was requested.
> Reply to this email directly, view it on GitHub
> <https://github.com/ManiacalLabs/BiblioPixel/pull/1127#issuecomment-452470237>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AA6a6ssnn-YjC2lFHqGionHlG-tyFA8Rks5vBRdfgaJpZM4Z15Rn>
> .
>

adammhaile: Disable the coverage test for now and run it locally? That's what's
failing...

On Tue, Jan 8, 2019 at 5:11 PM Adam Haile <adammhaile@gmail.com> wrote:

> I'm not really sure. I honestly don't get why it's only now happening...
> I'll look more in a bit.
>
> On Tue, Jan 8, 2019 at 5:10 PM Tom Swirly <notifications@github.com>
> wrote:
>
>> OK, so how do we fix this CI issue then?
>>
>> I tried all sorts of things, without success.
>>
>> —
>> You are receiving this because your review was requested.
>> Reply to this email directly, view it on GitHub
>> <https://github.com/ManiacalLabs/BiblioPixel/pull/1127#issuecomment-452470237>,
>> or mute the thread
>> <https://github.com/notifications/unsubscribe-auth/AA6a6ssnn-YjC2lFHqGionHlG-tyFA8Rks5vBRdfgaJpZM4Z15Rn>
>> .
>>
>


Patch:
diff --git a/bibliopixel/drivers/serial/devices.py b/bibliopixel/drivers/serial/devices.py
index b5bed23..4f8baa6 100644
--- a/bibliopixel/drivers/serial/devices.py
+++ b/bibliopixel/drivers/serial/devices.py
@@ -6,8 +6,23 @@ from ... drivers.return_codes import raise_error
 from ... util import log, util
 from ... project.importer import import_module
 
+_DEVICES_BY_HARDWARE_ID = {}
 
-class Devices(object):
+
+def Devices(hardware_id, baudrate):
+    devices = _DEVICES_BY_HARDWARE_ID.get(hardware_id)
+    if devices:
+        if devices.baudrate != baudrate:
+            log.warning(
+                'Expected baudrate %s but got %s', baudrate, devices.baudrate)
+    else:
+        devices = DevicesImpl(hardware_id, baudrate)
+        _DEVICES_BY_HARDWARE_ID[hardware_id] = devices
+
+    return devices
+
+
+class DevicesImpl(object):
     """Manage a list of serial devices.
 
     :param str hardware_id: A valid USB VID:PID pair such as "1D50:60AB"
@@ -17,20 +32,24 @@ class Devices(object):
     def __init__(self, hardware_id, baudrate):
         self.hardware_id = hardware_id
         self.baudrate = baudrate
+        self.devices = None
 
     def find_serial_devices(self):
         """Scan and report all compatible serial devices on system.
 
         :returns: List of discovered devices
         """
+        if self.devices is not None:
+            return self.devices
+
         self.devices = {}
         hardware_id = "(?i)" + self.hardware_id  # forces case insensitive
 
         for ports in serial.tools.list_ports.grep(hardware_id):
             port = ports[0]
             try:
-                id = self.get_device_id(port, self.baudrate)
-                ver = self._get_device_version(port, self.baudrate)
+                id = self.get_device_id(port)
+                ver = self._get_device_version(port)
             except:
                 log.debug('Error getting device_id for %s, %s',
                           port, self.baudrate)
@@ -85,31 +104,31 @@ class Devices(object):
         if fail:
             raise IOError(e)
 
-    def set_device_id(self, dev, id, baudrate=921600):
+    def set_device_id(self, dev, id):
         """Set device ID to new value.
 
         :param str dev: Serial device address/path
         :param id: Device ID to set
-        :param baudrate: Baudrate to use when connectinh (optional)
         """
         if id < 0 or id > 255:
             raise ValueError("ID must be an unsigned byte!")
-
-        com, code, ok = io.send_packet(CMDTYPE.SETID, 1, dev, baudrate, 5, id)
+        com, code, ok = io.send_packet(
+            CMDTYPE.SETID, 1, dev, self.baudrate, 5, id)
         if not ok:
             raise_error(code)
 
-    def get_device_id(self, dev, baudrate=921600):
+    def get_device_id(self, dev):
         """Get device ID at given address/path.
 
         :param str dev: Serial device address/path
         :param baudrate: Baudrate to use when connecting (optional)
         """
-        com, code, ok = io.send_packet(CMDTYPE.GETID, 0, dev, baudrate, 5)
+        com, code, ok = io.send_packet(CMDTYPE.GETID, 0, dev, self.baudrate, 5)
         if code is None:
             self.error(action='get_device_id')
         return code
 
-    def _get_device_version(self, dev, baudrate=921600):
-        com, code, ok = io.send_packet(CMDTYPE.GETVER, 0, dev, baudrate, 0.5)
+    def _get_device_version(self, dev):
+        com, code, ok = io.send_packet(
+            CMDTYPE.GETVER, 0, dev, self.baudrate, 0.5)
         return ok and io.read_byte(com) or 0


Test patch:
diff --git a/test/bibliopixel/drivers/serial/driver_test.py b/test/bibliopixel/drivers/serial/driver_test.py
index 7ada920..0394c10 100644
--- a/test/bibliopixel/drivers/serial/driver_test.py
+++ b/test/bibliopixel/drivers/serial/driver_test.py
@@ -7,13 +7,13 @@ from test.bibliopixel.drivers.dummy_test import clock_only_project
 
 class DriverTest(unittest.TestCase):
     @mock.patch('serial.Serial', autospec=True)
-    @mock.patch('bibliopixel.drivers.serial.driver.Devices', autospec=True)
-    def test_driver(self, Devices, Serial):
+    @mock.patch('bibliopixel.drivers.serial.devices.DevicesImpl', autospec=True)
+    def test_driver(self, DevicesImpl, Serial):
         device_id, port, version = 10, 23, 5
         dev, device_version = '/dev/wombat', 0
         device_map = {device_id: (port, version)}
 
-        instance = Devices.return_value
+        instance = DevicesImpl.return_value
         instance.find_serial_devices.return_value = device_map
         instance.get_device.return_value = device_id, dev, device_version
         instance.baudrate = 921600
@@ -36,7 +36,7 @@ class DriverTest(unittest.TestCase):
         expected = [
             mock.call().find_serial_devices(),
             mock.call().get_device(None)]
-        self.assertEqual(Devices.method_calls, expected)
+        self.assertEqual(DevicesImpl.method_calls, expected)
 
         expected = [
             call_write(1, 4, 0, 0, 9, 0, 1),