MLX90614: migrate to new I2C API #181

orangecms · 2021-05-22T14:52:21Z

No description provided.

orangecms · 2021-05-22T15:03:47Z

This if for #182. With the changes, I can read from the sensor again. I'm not sure what to do about the baud rate change though. It's not strictly necessary.

Here's how:

from pslab import ScienceLab
from pslab.external.MLX90614 import MLX90614
from time import sleep

psl = ScienceLab()  # This initializes the I2C bus.
sensor = MLX90614()

data = sensor.getAmbientTemperature()
print(data)

sleep(2)

data = sensor.getObjectTemperature()
print(data)

yields

25.32000000000005
21.500000000000057

🎉

orangecms · 2021-05-22T15:13:34Z

@bessman I took the i2c bus part from your patch to migrate the SSD1306 driver. Now I'm not sure where I got it from. For reference, see #184

Edit: found it, the branch is in here and was for some reason even targeted by the PR automatically; I looked at your fork only 😅 https://github.com/fossasia/pslab-python/commits/ssd1306

bessman

In addition to the specific comments throughout the code, the module, class and its public methods need docstrings, and the methods should have type hints for input and return values.

Remember to add it to the lint whitelist in tox.ini.

bessman · 2021-06-15T08:06:56Z

pslab/external/MLX90614.py

+class MLX90614(I2CSlave):
+    _ADDRESS = 0x5A
+    _OBJADDR = 0x07
+    _AMBADDR = 0x06
    NUMPLOTS = 1


Looks like some class-level vars are unused (NUMPLOTS, PLOTNAMES, name). Let's remove them.

Yes, that is also what I see in other sensors here.
For the other PR, I dropped a note on that:
#192 (comment)

It was a previous attempt to abstract the sensors in a way, and at the same time, it is tightly coupling them with the UI. I am seeing a very similar thing in sigrok. Let's discuss this in the nxt meeting a bit further in depth please, because that information is needed in some form.

I'm imagining some kind of SensorMetaDataMixin class could be used for this.

bessman · 2021-06-15T08:11:25Z

pslab/external/MLX90614.py


-        self.source = self.OBJADDR
+        self.source = self._OBJADDR

        self.name = 'Passive IR temperature sensor'


self.name also seems unused.

bessman · 2021-06-15T08:15:48Z

pslab/external/MLX90614.py


-        self.source = self.OBJADDR
+        self.source = self._OBJADDR

        self.name = 'Passive IR temperature sensor'
        self.params = {'readReg': {'dataType': 'integer', 'min': 0, 'max': 0x20, 'prefix': 'Addr: '},


I wonder what this is? It seems like it's related to the readReg method, specifying its argument as an integer that should be between 0 and 0x20. From the rest of the code I gather that there are two register addresses of interest: _OBJADDR (0x07) and _AMBADDR (0x06). Does the sensor have other register addresses that could be interesting?

Either way, this variable is unused and should be removed. The information about register address range (0-0x20) should be moved to a docstring if it is relevant to the user.

bessman · 2021-06-15T08:23:00Z

pslab/external/MLX90614.py


        self.name = 'Passive IR temperature sensor'
        self.params = {'readReg': {'dataType': 'integer', 'min': 0, 'max': 0x20, 'prefix': 'Addr: '},
                       'select_source': ['object temperature', 'ambient temperature']}

-        try:


If the sensor works with the default baudrate (125 kHz), there is no need for this configuration code.

Even if the sensor requires the baudrate to not exceed some maximum value, merely instantiating the sensor should not automatically reconfigure the I2C bus, I think. The user might have multiple sensors connected to the I2C bus and have manually set the baudrate to a desired speed.

Instead we should check if the baudrate is low enough for this sensor, and raise a warning or even an exception if it is too high. However, there is currently no way to read back the configured I2C baudrate (#195).

bessman · 2021-06-15T08:24:11Z

pslab/external/MLX90614.py

+        #     print('switching baud to 100k')
+        #     self.I2C.configI2C(100e3)
+        # except Exception as e:
+        #     print('FAILED TO CHANGE BAUD RATE', e.message)

    def select_source(self, source):


This could be a _private method, I think.

bessman · 2021-06-15T08:24:53Z

pslab/external/MLX90614.py

        elif source == 'ambient temperature':
-            self.source = self.AMBADDR
+            self.source = self._AMBADDR

    def readReg(self, addr):


Unused? Seems like a leftover from debugging.

bessman · 2021-06-15T08:25:14Z

pslab/external/MLX90614.py

        elif source == 'ambient temperature':
-            self.source = self.AMBADDR
+            self.source = self._AMBADDR

    def readReg(self, addr):
        x = self.getVals(addr, 2)
        print(hex(addr), hex(x[0] | (x[1] << 8)))

    def getVals(self, addr, numbytes):


Could be _private, name should be in snake_case.

bessman · 2021-06-15T08:28:55Z

pslab/external/MLX90614.py


    def readReg(self, addr):
        x = self.getVals(addr, 2)
        print(hex(addr), hex(x[0] | (x[1] << 8)))

    def getVals(self, addr, numbytes):
-        vals = self.I2C.readBulk(self.ADDRESS, addr, numbytes)
+        vals = self.read(numbytes, addr)
        return vals

    def getRaw(self):


_private, snake_case.

The method itself could be clearer.

It fetches three bytes from the sensor, but only uses two of them. Why?

This: [((((vals[1] & 0x007f) << 8) + vals[0]) * 0.02) - 0.01 - 273.15] is some kind of conversion, but what exactly is going on?

The most significant bit of val[1] is discarded, why?

I think 0.02 is a resolution factor (0.02 degrees of resolution per bit), but what is 0.01? It looks like an offset, but if the resolution is 0.02 degC it makes no sense to have an offset of 0.01 degC.

273.15 is conversion from degK to degC.

The method returns False if fewer than three bytes are received from the sensor. Can this happen in normal operation, or is it indicative of a hardware problem? If the latter, we should raise an exception instead.

The return type can be either list or bool. Multiple return types should be avoided; better return an empty list instead of False.

bessman · 2021-06-15T08:29:37Z

pslab/external/MLX90614.py

@@ -54,15 +48,15 @@ def getRaw(self):
            return False

    def getObjectTemperature(self):
-        self.source = self.OBJADDR


Here we could use the select_source method for improved readability.

Good point, thank you!

bessman · 2021-06-15T08:29:52Z

pslab/external/MLX90614.py

        val = self.getRaw()
        if val:
            return val[0]
        else:
            return False

    def getAmbientTemperature(self):
-        self.source = self.AMBADDR


select_source

bessman · 2021-06-22T08:09:59Z

Data sheet: https://www.sparkfun.com/datasheets/Sensors/Temperature/MLX90614_rev001.pdf

orangecms force-pushed the mlx-90614-migrate branch from abadfe1 to 1e8242d Compare May 22, 2021 15:01

MLX90614: migrate to new I2C API

455f7d0

orangecms force-pushed the mlx-90614-migrate branch from 1e8242d to 455f7d0 Compare June 12, 2021 13:23

orangecms marked this pull request as ready for review June 12, 2021 13:23

bessman reviewed Jun 15, 2021

View reviewed changes

bessman merged commit b310d85 into fossasia:development Oct 15, 2023

orangecms deleted the mlx-90614-migrate branch October 15, 2023 16:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MLX90614: migrate to new I2C API #181

MLX90614: migrate to new I2C API #181

orangecms commented May 22, 2021

orangecms commented May 22, 2021

orangecms commented May 22, 2021 •

edited

Loading

bessman left a comment

bessman Jun 15, 2021

orangecms Jun 15, 2021

bessman Jun 15, 2021

bessman Jun 15, 2021

bessman Jun 15, 2021

bessman Jun 15, 2021

bessman Jun 15, 2021

bessman Jun 15, 2021

bessman Jun 15, 2021

bessman Jun 15, 2021 •

edited

Loading

bessman Jun 15, 2021

orangecms Jun 15, 2021

bessman Jun 15, 2021

bessman commented Jun 22, 2021

MLX90614: migrate to new I2C API #181

MLX90614: migrate to new I2C API #181

Conversation

orangecms commented May 22, 2021

orangecms commented May 22, 2021

orangecms commented May 22, 2021 • edited Loading

bessman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bessman Jun 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bessman commented Jun 22, 2021

orangecms commented May 22, 2021 •

edited

Loading

bessman Jun 15, 2021 •

edited

Loading