Initial commit #1
Conversation
adafruit_max31855.py
Outdated
| If ``raw`` is ``True``, return the values as 14- and 12- bit integers, | ||
| otherwise convert them to Celsuius degrees and return as floating point | ||
| numbers. | ||
| """ |
There was a problem hiding this comment.
Why provide raw access? What combine the reading of external and internal temps into one?
There was a problem hiding this comment.
You don't really have any choice about what you are reading from this chip. There are no registers, you just do a single read and get all the data all at once.
Raw access is useful for debugging or for getting more precision than the puny ~6 decimal digits supported by the float type. It generally opens the driver for use cases that we didn't anticipate, but that can still be useful, giving the user who knows what they are doing more power, but at the same time letting the common use cases stay simple.
There was a problem hiding this comment.
I considered other approaches:
- Having two separate methods (or properties) that would read all the data, but discard half of it and only return the other half. This is useless, as you really want to get the internal reference temperature from exactly the same measurement, as your temperature measurement. Doing two separate measurements defeats the whole purpose.
- Returning the temperature, and saving the internal reference to an attribute on this class. This introduces an asymmetry, where one property causes a new measurement to be made, but the other doesn't. I found that really confusing and surprising, so I decided against it.
- The problem with the former approach could be somewhat alleviated by naming the attribute appropriately, such as "last_measurement_internal_reference_temperature" or something similar, however that gets ridiculously long.
- Returning an object that behaves like a float for all operators, but has an additional attribute with the internal reference. That seemed too magical and with too much overhead.
There was a problem hiding this comment.
Although the device doesn't have registers, we can still choose to use properties to represent the device state. I think that having two properties, temperature and internal_temperature, that each read and discard the other temperature is fine. The Adafruit Arduino library does exactly that. Not having them in-sync is also OK by me because the device itself uses the internal temperature to adjust the other "hot end" temperature already. You shouldn't need to do it yourself, its just available in case you also want to know that value.
Regarding raw access, I agree it could be useful for some people. However, I think those people would have no trouble doing the SPI read themselves. I'd rather have this driver expose two simple properties which are easy for beginners who don't need that accuracy.
adafruit_max31855.py
Outdated
| # THE SOFTWARE. | ||
|
|
||
| import ustruct | ||
|
|
There was a problem hiding this comment.
Please add the description comment here. It provides the title of the API docs.
adafruit_max31855.py
Outdated
| If ``raw`` is ``True``, return the values as 14- and 12- bit integers, | ||
| otherwise convert them to Celsuius degrees and return as floating point | ||
| numbers. | ||
| """ |
There was a problem hiding this comment.
Although the device doesn't have registers, we can still choose to use properties to represent the device state. I think that having two properties, temperature and internal_temperature, that each read and discard the other temperature is fine. The Adafruit Arduino library does exactly that. Not having them in-sync is also OK by me because the device itself uses the internal temperature to adjust the other "hot end" temperature already. You shouldn't need to do it yourself, its just available in case you also want to know that value.
Regarding raw access, I agree it could be useful for some people. However, I think those people would have no trouble doing the SPI read themselves. I'd rather have this driver expose two simple properties which are easy for beginners who don't need that accuracy.
adafruit_max31855.py
Outdated
| if hasattr(self.cs, 'high'): | ||
| self.cs.high() | ||
| else: | ||
| self.cs.value = 1 |
There was a problem hiding this comment.
Please remove this. I understand its for MicroPython compatibility but I'd rather you use BusDevice to manage the CS line and create a BusDevice library for MicroPython. That way the compatiblity is done in one spot and not many.
Just fork this repo and edit this line for MicroPython: https://github.com/adafruit/Adafruit_CircuitPython_BusDevice/blob/master/adafruit_bus_device/spi_device.py#L64
Then you'd have every BusDevice based library and driver work with MicroPython.
This section would become:
with self.spi as spi:
spi.readinto(self.data)
|
Please don't merge yet. |
|
I now tested it with HUZZAH and M0, ready to merge. |
adafruit_max31855.py
Outdated
|
|
||
| @property | ||
| def temperature(self): | ||
| """Return measured temperature in degrees Celsius.""" |
There was a problem hiding this comment.
One small nitpick. This can just be "Thermocouple temperature in degree Celsius." because its a property rather than a function to the outside world.
adafruit_max31855.py
Outdated
|
|
||
| @property | ||
| def reference_temperature(self): | ||
| """Return internal reference temperature in degrees Celsius.""" |
There was a problem hiding this comment.
Same here: "Internal reference temperature in degrees Celsius."
|
Nitpicks applied. |
No description provided.