Unpack isn't working

[fakecrusader]

I am using this library with RPi3 but encounter runtime errors when reading the temperature or reference temperature. The temperature functions use "unpack" when returning the results of register reads. The read_register function returns 3 bytes and although the comments say the unpack will adjust it to the right size I get a runtime error instead saying 4 bytes are expected.
A bit of diagnostic work showed that the bytearray was there and contained 3 bytes. I don't understand the unpack syntax to the point where it should be adding a byte :

raw_temp = unpack(">i", self._read_register(_MAX31856_LTCBH_REG, 3))[0]

There just seems to be a null byte declaration stuck on the end with no link symbol such as '+' as you might expect. It doesn't throw a syntax error but also doesn't seem to work.

Researching similar unpack errors I came across "int.from_bytes()" as a newer Python feature. I gave it a try instead of the "unpack" code and it worked.

`def temperature(self):

    self._perform_one_shot_measurement()

    # unpack the 3-byte temperature as 4 bytes
    # raw_temp = unpack(">i", self._read_register(_MAX31856_LTCBH_REG, 3))[0]
    raw_temp = int.from_bytes(self._read_register(_MAX31856_LTCBH_REG, 3),'big')
    # shift to remove extra byte from unpack needing 4 bytes
    # raw_temp >>= 8

    # effectively shift raw_read >> 12 to convert pseudo-float
    temp_float = (raw_temp / 4096.0)

    return temp_float`

This gave me what look like accurate temperatures, pending further calibration.
Any idea why the original code failed for me? It seems the code isn't as portable as it could be?

[ladyada]

python3 is a little pickier than circuitpython on what you can pass into unpack

@kattni @brennen @caternuson can one of y'all check this out and fix

[siddacious]

I can take a look at this after work.

[ladyada]

thanks! yah plz stick to unpack, but i think you'll need to either extend or shrink the bytearray - circuitpython is blase' about the # of bytes matching the unpack string :)

[fakecrusader]

python3 is a little pickier than circuitpython on what you can pass into unpack

Does this mean that Circuitpython (or micropython?) had its own "unpack" implementation that is now replaced by the fact that it's running under Python3 on the RPi? Does that explain the odd (to me) syntax, i.e. it's not vanilla unpack?

[ladyada]

nope, struct.unpack has the same syntax, we just have more errorchecking in python3 - so we just have to be more strict in circuitpython :)

…

On Tue, Jan 8, 2019 at 2:40 PM fakecrusader ***@***.***> wrote: python3 is a little pickier than circuitpython on what you can pass into unpack Does this mean that Circuitpython (or micropython?) had its own "unpack" implementation that is now replaced by the fact that it's running under Python3 on the RPi? Does that explain the odd (to me) syntax, i.e. it's not vanilla unpack? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#4 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABKG0a65cnF3dldwUaeuy4KOwWL_kWenks5vBPQmgaJpZM4Z19UR> .

[caternuson]

@siddacious it's as @ladyada says, so will need to pad the bytearray somehow before calling unpack. Do you want to PR that?

Python 3 struct.unpack i expects 4 bytes:
https://docs.python.org/3/library/struct.html#format-characters
and can't take a joke.

Issue recreated:

pi@raspberrypi:~ $ python3
Python 3.5.3 (default, Sep 27 2018, 17:25:39) 
[GCC 6.3.0 20170516] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import board, busio
>>> import digitalio
>>> import adafruit_max31856
>>> spi = busio.SPI(board.SCK, board.MOSI, board.MISO)
>>> cs = digitalio.DigitalInOut(board.D21)
>>> cs.direction = digitalio.Direction.OUTPUT
>>> tc = adafruit_max31856.MAX31856(spi, cs)
>>> tc.temperature
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/local/lib/python3.5/dist-packages/adafruit_max31856.py", line 159, in temperature
    raw_temp = unpack(">i", self._read_register(_MAX31856_LTCBH_REG, 3))[0]
struct.error: unpack requires a bytes object of length 4
>>> 

Played with a nominal fix:

>>> raw = tc._read_register(0x0c, 3)
>>> len(raw)
3
>>> unpack(">i", raw)[0]
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
struct.error: unpack requires a bytes object of length 4
>>> raw[0:0] = b'\x00'
>>> len(raw)
4
>>> unpack(">i", raw)[0]
92640
>>> 

NOTE be careful not to redefine your class level _BUFFER in fixing this. I think my example above does that.

>>> len(tc._BUFFER)
4

yeah python

[siddacious]

@siddacious Yup, I can PR it but I'll have to test on a metro/feather and get you or someone else to test on a PI because I don't have one.

[caternuson]

Cool. I can test on RPi setup. And good idea to make sure it works on CP - maybe CP requires things t be blasé and won't like you being serious. :)

[coffeegoat]

This problem still seems to exist when using the reference.temperature property. I get the following error:

File "ReadMax31856Fault4.py", line 35, in
CJTemp2 = max31856.reference_temperature
File "/home/pi/.local/lib/python3.5/site-packages/adafruit_max31856.py", line 174, in reference_temperature
raw_read = unpack(">h", self._read_register(_MAX31856_CJTH_REG, 2))[0]
struct.error: unpack requires a bytes object of length 2

I can read the value using a simple "_read_register" request, but the property is apparently nonworking. I found similar results with the reference_temperature_thresholds property, though the error is slightly different:

File "ReadMax31856Fault4.py", line 38, in
Threshold = max31856.reference_temperature_thresholds
File "/home/pi/.local/lib/python3.5/site-packages/adafruit_max31856.py", line 209, in reference_temperature_thresholds
return (float(unpack("b", self._read_register(_MAX31856_CJLF_REG, 1))[0]),
struct.error: unpack requires a bytes object of length 1

I'm barely python literate so I can't help with that but I'm working with a PI and a MAX31856 so I'd be happy to do some hardware testing.

[siddacious]

@coffeegoat Sorry to hear that! I'll take a look.

[caternuson]

Have the return be of specified length?
https://github.com/adafruit/Adafruit_CircuitPython_MAX31856/blob/master/adafruit_max31856.py#L274

[siddacious]

@caternuson That's a good idea. I can take a look at it in a while but if you want to give it a go, by all means do so.

[caternuson]

Nah, don't let me hold you up. Go for it. I was just passing by and took a looksie. Might be as simple as:

 return self._BUFFER[:length]

but not sure what kind of thread that might pull. So worth testing.

[coffeegoat]

I poked at it a bit more, and this snippet worked:

` def reference_temperature(self):
"""The temperature of the cold junction in degrees celsius. (read-only)"""
self._perform_one_shot_measurement()

    raw_read = unpack(">i", self._read_register(_MAX31856_CJTH_REG, 2))[0]

    raw_read >>=16

    cold_junction_temp = raw_read / 256.0

    return cold_junction_temp`

I'm not super clear on the syntax, but I modeled it after the read function with the key difference being the data type and made the assumption that we should only be seeing a couple of bits. It seems to give reasonable values.

[caternuson]

Maybe this should've been a new/separate issue, but hopefully this is an easy fix, so reopening.

PR submitted - checkout #6

[caternuson]

@coffeegoat Please try the 0.8.3 version of the library when it becomes available:
https://github.com/adafruit/Adafruit_CircuitPython_MAX31856/releases/tag/0.8.3

[coffeegoat]

@caternuson I'll check version 0.8.3 tomorrow morning, I've got a test running right now and don't want to swap libraries in the middle.

[coffeegoat]

We have success, that fixed it, was it only the minor change in to add:
return self._BUFFER[:length]

[caternuson]

Yep. That was the main change. Then had to add some padding to a call that requested 3 bytes that needed to get unpacked with an i, which wants 4 bytes. You can actually see all the code changes in the PR:
#6
Select the "Files changed" tab to view.

Glad that fixed it. Thanks for letting us know.