first working commit with the feature set of the Arduino driver #1

siddacious · 2018-10-02T16:17:53Z

caternuson

Looks good. Review suggestions are essentially cosmetic.

caternuson · 2018-10-02T16:25:00Z

adafruit_max31856.py

+  """Driver for the MAX31856 Universal Thermocouple Amplifier
+
+    :param ~busio.I2C i2c_bus: The I2C bus the DS3231 is connected to.
+    :param int address: The I2C address of the device.


Change to match SPI.

adafruit_max31856.py

caternuson · 2018-10-02T16:39:37Z

adafruit_max31856.py

+
+    float_low, float_high = val
+    int_low = int(float_low * 16)
+    int_high = int(float_high * 16)


This could be compacted a bit. Maybe instead of tuple unpacking, just grab them via index.

int_low = int(val[0] * 16) int_high = int(val[1] * 16)

caternuson · 2018-10-02T16:43:32Z

adafruit_max31856.py

+
+    # # assert on any fault
+    self._write_u8(_MAX31856_MASK_REG, 0x0)
+    # turn off open circuit faults?


Why? This is worth checking for.

After reading the datasheet again (and testing) this in fact turns on open circuit fault detection. Derp. I'll fix the comment.

caternuson · 2018-10-02T16:51:42Z

adafruit_max31856.py

+
+    self._write_u8(_MAX31856_CJLF_REG, int_low)
+    self._write_u8(_MAX31856_CJHF_REG, int_high)
+


No need for any variables here. This entire function could become these two lines:

self._write_u8(_MAX31856_CJLF_REG, int(val[0])) self._write_u8(_MAX31856_CJHF_REG, int(val[1]))

caternuson · 2018-10-02T16:53:42Z

adafruit_max31856.py

+    low = float( self._read_u8(_MAX31856_CJLF_REG))
+    high = float(self._read_u8(_MAX31856_CJHF_REG))
+    return (low, high)
+


Again here. Variables aren't used. So coud just:

return float(self._read_u8(_MAX31856_CJLF_REG)), float(self._read_u8(_MAX31856_CJHF_REG))

caternuson · 2018-10-02T16:56:05Z

adafruit_max31856.py

+
+
+  # these were shamelessly stolen from Tony's MAX31865 driver.
+  # I want to re-factor them to be more similar to the Arduino driver


Why? They look OK.

The Arduino code is a bit DRY-er, using a shared variable length read function from the set length read functions. Not the end of the world, just cleaner (and ever so slightly smaller I would presume)

caternuson · 2018-10-02T17:01:03Z

docs/conf.py

+# Uncomment the below if you use native CircuitPython modules such as
+# digitalio, micropython and busio. List the modules you use. Without it, the
+# autodoc module docs will fail to generate with a warning.
+# autodoc_mock_imports = ["digitalio", "busio"]


You'd discovered this when Travis ran. You'll need to uncomment and add the modules.

As long as you have requirements.txt appropriately populated, you should not need to automock anything.

caternuson · 2018-10-02T17:02:14Z

examples/max31856_simpletest.py

@@ -0,0 +1,17 @@
+import adafruit_max31856


move this import to last

kattni

There are files missing from this repo. @caternuson and I are looking into why, the same thing happened in a PR to another repo. The dotfiles are missing - .travis.yml, .readthedocs.yml and .gitignore. Once we've sorted the problem, we'll need another commit with the correct files.

siddacious · 2018-10-02T18:00:54Z

@kattni The missing dotfiles are my mistake; I copied the cookiecutter files into this repo from a different directory and omitted them because I forgot the -a option. I'll add them with the other fixes @caternuson requested.

What is the proper proceedure to add the new commit(s)? I can leave it as separate or squash it.

kattni · 2018-10-02T18:03:25Z

Simply commit and push to this PR. We don't mind having a lot of commits on a PR as long as they're part of it. If you really wanted to, you can squash locally, but we seriously don't mind. I've done plenty with a ton of little commits to fix things I missed.

tannewt · 2018-10-02T18:27:11Z

Specifically a PR tracks a branch. So, you can change the branch however you like and the PR will follow it.

tannewt

Thank you for the new driver! One comment about the property documentation.

tannewt · 2018-10-02T18:33:10Z

adafruit_max31856.py

+
+  @temperature_thresholds.setter
+  def temperature_thresholds(self, val):
+    """Set the thermocouple's low and high temperature thresholds as a tuple: `thermocouple.temperature_threshold = (low_threshold, high_threshold)`."""


You only need the getter documented for ReadTheDocs. The property comments should read like its a variable and not a function (So no "Read" or "Set" to begin with.)

I specifically documented the setter like this as it wasn't obvious to me that you would be able to assign it a tuple. Perhaps because the getter returns a tuple it should be obvious? Any specific recommendations would be welcome.

I take your second comment to mean the getter documentation should be:

The thermocouple's low and high temperature thresholds as a (low_temp, high_temp) tuple

Now that I write that it does seem to imply that you would set it with a tuple as well..

Yup, exactly. You can document the type of the "variable"

siddacious · 2018-10-04T16:35:37Z

I just pushed a rebased version that should address all the comments/suggestions. Let me know if I missed anything.

adafruit_max31856.py

caternuson · 2018-10-06T21:13:28Z

adafruit_max31856.py

+        sleep(0.250)  # MEME FIX autocalculate based on oversampling
+
+    # these were shamelessly stolen from Tony's MAX31865 driver.
+    # I want to re-factor them to be more similar to the Arduino driver


Remove these "note to self" comments. This follow up is best done by creating an issue.

caternuson · 2018-10-09T15:49:11Z

adafruit_max31856.py

+        low = round(((low_temp_high_byte << 8) | low_temp_low_byte)/16, 1)
+        high = round(((high_temp_high_byte << 8) | high_temp_low_byte)/16, 1)
+
+        return (low, high)


This is due to the same thing you were dealing with for temperatures.

>>> tc.temperature_thresholds = (20, -20) >>> tc.temperature_thresholds (20.0, 4076.0)

You may have to do some extra work in both the getter and setter methods to deal with this.

caternuson · 2018-10-09T15:50:17Z

adafruit_max31856.py

+        as a ``(low_temp, high_temp)`` tuple
+        """
+        return (float(self._read_u8(_MAX31856_CJLF_REG)),
+                float(self._read_u8(_MAX31856_CJHF_REG)))


Same comment as for temperature_thresholds.

>>> tc.reference_temperature_thresholds = (20, -20) >>> tc.reference_temperature_thresholds (20.0, 236.0) >>>

caternuson · 2018-10-12T21:45:04Z

Travis is kicking on a few pylint things. Fix those and I think I'm good. @tannewt - how about you?

Adafruit CircuitPython 3.0.2 on 2018-09-14; Adafruit Feather M4 Express with samd51j19
>>> import max_test as test
>>> tc = test.tc
>>> tc.temperature
23.7109
>>> tc.temperature
-1.23438
>>> tc.temperature_thresholds
(-2048.0, 2047.9)
>>> tc.temperature_thresholds = (-20, 50)
>>> tc.temperature_thresholds
(-20.0, 50.0)
>>> tc.fault
{'voltage': False, 'tc_low': False, 'tc_range': False, 'tc_high': False, 'cj_range': False, 'cj_high': False, 'open_tc': False, 'cj_low': False}
>>>

tannewt · 2018-10-15T06:36:43Z

Looks good to me! Thank you!

caternuson requested changes Oct 2, 2018

View reviewed changes

kattni suggested changes Oct 2, 2018

View reviewed changes

tannewt requested changes Oct 2, 2018

View reviewed changes

siddacious force-pushed the master branch from 8ba20a4 to efc9b8f Compare October 4, 2018 16:34

siddacious force-pushed the master branch 5 times, most recently from 054e77c to 4229b12 Compare October 5, 2018 21:29

kattni approved these changes Oct 6, 2018

View reviewed changes

kattni mentioned this pull request Oct 6, 2018

CircuitPython Libraries! adafruit/circuitpython#1246

Open

caternuson requested changes Oct 6, 2018

View reviewed changes

adafruit_max31856.py Show resolved Hide resolved

adafruit_max31856.py Show resolved Hide resolved

caternuson requested changes Oct 6, 2018

View reviewed changes

siddacious force-pushed the master branch 2 times, most recently from f39e48e to b21c8a2 Compare October 8, 2018 17:14

caternuson requested changes Oct 9, 2018

View reviewed changes

siddacious force-pushed the master branch from b21c8a2 to e4e91f5 Compare October 12, 2018 08:07

first working commit with the feature set of the Arduino driver

fab2ef6

siddacious force-pushed the master branch from e4e91f5 to fab2ef6 Compare October 12, 2018 22:05

caternuson approved these changes Oct 12, 2018

View reviewed changes

tannewt approved these changes Oct 15, 2018

View reviewed changes

tannewt merged commit 168c9ac into adafruit:master Oct 15, 2018


		self._write_u8(_MAX31856_CJLF_REG, int_low)
		self._write_u8(_MAX31856_CJHF_REG, int_high)



		# these were shamelessly stolen from Tony's MAX31865 driver.
		# I want to re-factor them to be more similar to the Arduino driver

first working commit with the feature set of the Arduino driver #1

first working commit with the feature set of the Arduino driver #1

Uh oh!

Conversation

siddacious commented Oct 2, 2018

Uh oh!

caternuson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

kattni Oct 2, 2018 • 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!

kattni left a comment

Choose a reason for hiding this comment

Uh oh!

siddacious commented Oct 2, 2018

Uh oh!

kattni commented Oct 2, 2018

Uh oh!

tannewt commented Oct 2, 2018

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!

siddacious commented Oct 4, 2018

Uh oh!

Uh oh!

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!

caternuson commented Oct 12, 2018

Uh oh!

tannewt commented Oct 15, 2018

Uh oh!

Uh oh!

kattni Oct 2, 2018 •

edited

Loading