Refactoring per request #1

caternuson · 2017-12-13T19:01:06Z

Pretty major refactor for this issue:
adafruit/circuitpython#338

deshipu · 2017-12-13T23:37:56Z

adafruit_onewire/bus.py

+
+class OneWireError(Exception):
+    """A class to represent a 1-Wire exception."""
+    pass


no need for pass if there is a docstring

i kind of like it there, makes it obvious that i intentionally left it blank, vs. accidentally deleting all the code

deshipu · 2017-12-13T23:40:25Z

adafruit_onewire/bus.py

+        if 'start' in kwargs:
+            start = kwargs['start']
+        if 'end' in kwargs:
+            end = kwargs['end']


This is usually done by having keywords argument defaulting to None, and checking if there are not None here:

def readinto(self, buf, start=0, end=None): if end is None: end = len(buf) ...

deshipu · 2017-12-13T23:42:20Z

adafruit_onewire/bus.py

+        if 'start' in kwargs:
+            start = kwargs['start']
+        if 'end' in kwargs:
+            end = kwargs['end']


nice again!

deshipu · 2017-12-13T23:43:04Z

adafruit_onewire/bus.py

+        for i in range(0xff): # pylint: disable=unused-variable
+            rom, diff = self._search_rom(rom, diff)
+            if rom:
+                devices += [OneWireAddress(rom)]


devices.append(OneWireAddress(rom))

yep. another left over from re-purposing some code.

deshipu · 2017-12-13T23:44:22Z

adafruit_onewire/bus.py

+        return devices
+
+    def _readbit(self):
+        return self._ow.read_bit()


You could do self._readbit = self._ow.read_bit in the __init__, then it would be one function call less.

i like doing it this way in case i ever want to come back and add additional behavior

You can still do it anyways — you can switch to this way at any moment.

is there any reason other than code style?

It's actually a bit worse style-wise, but it's much faster -- function calls are expensive, and you are avoiding one this way, and also one frame on the stack. CircuitPython has pretty low recursion limit, it's easy to hit it with a lot of nesting.

thanks. that's what i was after. 👍

deshipu · 2017-12-13T23:50:28Z

adafruit_onewire/bus.py

+                            next_diff = i
+                self._writebit(b)
+                if b:
+                    r_b |= 1 << bit


nitpick: this could be faster as r_b |= b << bit without the if.

good catch. that was borrowed code.

deshipu · 2017-12-13T23:51:22Z

adafruit_onewire/bus.py

+            rom[byte] = r_b
+        return rom, next_diff
+
+    def crc8(self, data): # pylint: disable=no-self-use


You could make this a @staticmethod if you don't use self.

deshipu · 2017-12-13T23:53:24Z

adafruit_onewire/device.py

+        :param int end: Index to write up to but not include
+        """
+        self._bus.readinto(buf, **kwargs)
+        if kwargs is None and len(buf) >= 8:


kwargs is never None — if there are no keyword arguments, it will be an empty dict {}, so you could write if not kwargs and ....

thanks. that was totally a mistake and a bug on my part!

deshipu · 2017-12-13T23:54:01Z

adafruit_onewire/device.py

+        :param int start: Index to start writing from
+        :param int end: Index to read up to but not include
+        """
+        return self._bus.write(buf, **kwargs)


You could also do self.write = self._bus.write here.

same comment as above

deshipu · 2017-12-13T23:55:12Z

adafruit_onewire/bus.py

+from micropython import const
+
+_SEARCH_ROM = const(0xF0)
+_MATCH_ROM = const(0x55)


if you do _MATCH_ROM = b'\x55'instead, you won't need to convert it to bytearray everywhere.

thanks, those converts were kind of kluge leftover from changing all calls to be general purpose and pass in arrays.

i changed in the other module, in this one there are no converts used

tannewt

Here are more comments with a bit of overlap with @deshipu

tannewt · 2017-12-13T23:17:20Z

README.rst

+    >>> for d in devices:
+    ...     print("ROM={}\tFamily=0x{:02x}".format(d.rom, d.family_code))
+    ...
+    ROM=bytearray(b'(\xff\xc3\x80\xc2\x16\x04\xc6') Family=0x28


Please don't put the prompt here so its easily copied.

tannewt · 2017-12-13T23:25:21Z

README.rst

+================
+
+To build this library locally you'll need to install the
+`circuitpython-travis-build-tools <https://github.com/adafruit/circuitpython-build-tools>`_ package.


Gah, cookiecutter is out of date here. This shouldn't have travis in the name and below it should be installed explicitly. Sent you a PR for it here: adafruit/cookiecutter-adafruit-circuitpython#9

tannewt · 2017-12-13T23:35:14Z

adafruit_onewire/bus.py

+            raise OneWireError
+        return not reset
+
+    def readinto(self, buf, **kwargs):


**kwargs should be rarely used. Its only really useful when passing kwargs through to a sub-call. Using it otherwise hides incorrect, unused kwargs.

Here simply do readinto(self, buf, *, start=None, end=None). The *, tells Python that everything after it must be a kwarg instead of positional.

yah, my first time to try using them. got the idea from here:
https://github.com/adafruit/Adafruit_CircuitPython_BusDevice/blob/master/adafruit_bus_device/i2c_device.py#L65

tannewt · 2017-12-13T23:44:17Z

adafruit_onewire/bus.py

+            rom[byte] = r_b
+        return rom, next_diff
+
+    def crc8(self, data): # pylint: disable=no-self-use


Instead of disabling pylint, this can be a static method. That means it works independent of the actual object state. To do so, use the @staticmethod decorator and drop self.

@staticmethod def crc8(data):

tannewt · 2017-12-13T23:53:12Z

adafruit_onewire/bus.py

+
+        for byte in data:
+            crc ^= byte
+            for bit in range(8): # pylint: disable=unused-variable


do for _ in range(8):. _ is used to designate unused things.

tannewt · 2017-12-14T00:01:33Z

api.rst

+.. automodule:: onewire_bus
+   :members:
+
+.. automodule:: onewire_device


I think these will need to be adafruit_onewire.bus and adafruit_onewire.device.

thanks. nice catch. left over from a rename.

…itPython_OneWire

tannewt

Just a couple minor things.

tannewt · 2017-12-14T20:37:13Z

README.rst

+
+.. code-block:: shell
+
+    circuitpython-build-bundles --filename_prefix {% if cookiecutter.library_prefix %}{{ cookiecutter.library_prefix | lower }}-{% endif %}circuitpython-{{ cookiecutter.library_name | lower }} --library_location .


The cookiecutter templates need to be replaced. (I think it should be adafruit-circuitpython-onewire.)

tannewt · 2017-12-14T20:37:33Z

README.rst

+Introduction
+============
+
+.. image:: https://readthedocs.org/projects/adafruit-circuitpython-OneWire/badge/?version=latest


I think OneWire should be onewire

tannewt · 2017-12-14T20:39:11Z

adafruit_onewire/bus.py

+
+        for byte in data:
+            crc ^= byte
+            for bit in range(8): # pylint: disable=unused-variable


tannewt

Looks awesome! Thanks!

caternuson added 6 commits December 13, 2017 10:54

it works

f834355

added .pylintrc

74f40e2

linted

d37f5d8

fix travis

e82d39d

travis

a759ac3

suppressing pylint

54ca987

tannewt self-requested a review December 13, 2017 23:15

Fix typo "repesent" -> "represent"

52d6031

deshipu suggested changes Dec 13, 2017

View reviewed changes

Typo with represent

deaefb5

tannewt requested changes Dec 14, 2017

View reviewed changes

caternuson added 4 commits December 14, 2017 09:33

review updates

463c9eb

Merge branch 'master' of https://github.com/caternuson/Adafruit_Circu…

fa86ecd

…itPython_OneWire

change to mapped functions

dafbf07

and remove existing defs

a6f32ff

tannewt requested changes Dec 14, 2017

View reviewed changes

a few minor tweaks

344468a

tannewt approved these changes Dec 15, 2017

View reviewed changes

tannewt merged commit bb71e82 into adafruit:master Dec 15, 2017


		.. code-block:: shell

		circuitpython-build-bundles --filename_prefix {% if cookiecutter.library_prefix %}{{ cookiecutter.library_prefix \| lower }}-{% endif %}circuitpython-{{ cookiecutter.library_name \| lower }} --library_location .

Refactoring per request #1

Refactoring per request #1

Uh oh!

Conversation

caternuson commented Dec 13, 2017

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!

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!

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!

deshipu Dec 13, 2017 • 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!

tannewt 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!

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!

tannewt left a comment

deshipu Dec 13, 2017 •

edited

Loading