Skip to content

Extended library for TMP116, TMP117, TMP119 #17

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

ami3go
Copy link

@ami3go ami3go commented May 16, 2024

I extended library to be compatible with TMP116, TMP117, TMP119 devices.
All of them have same register map and resolution.
I added:

  • methods: .temperature_updated if application need to receive only updated values

  • modified configurable variables in this was that allow IDE use autocompletion:

    • such as verageCount.AVERAGE_32X, or MeasurementDelay.DELAY_0_0015_S
  • Added round(x,3) for temperature. Resolution is only 7m degC, there is no need to have more then 3 digits

Copy link
Contributor

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't sell other than TMP11x, so I'm going to inquire about whether we would want to include this in our own library. You're of course welcome to fork the library.

Do these each have different I2C addresses? There was inconsistency about that in the code examples.

Suggestion: instead of checking the device ID for each device, change to a single TMP11x class which accepts any of the device ID's. Assuming the I2C addresses differ, define constants for each address. That will take less code. (They don't differ inherently, I think i see.)

The additional names for autocompletion use up space in the library, so we'd avoid that. It's too bad -- I don't know of another way to make the names available easily.

@@ -41,13 +45,13 @@ PyPI <https://pypi.org/project/adafruit-circuitpython-tmp117/>`_. To install for

.. code-block:: shell

pip3 install adafruit-circuitpython-tmp117
pip3 install git+https://github.com/ami3go/Adafruit_CircuitPython_TMP11X.git
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you mean to do this.

Suggested change
pip3 install git+https://github.com/ami3go/Adafruit_CircuitPython_TMP11X.git
pip3 install adafruit-circuitpython-tmp11x

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello thanks for review, and your feedback.
That was my worry too, that you sell only one part and there is no logic to merge updated lib, because it could be confusing for your customers.
Thank for idea to integrate all 3 device into 1 class instead of 3 different.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding readme . i will correct it if you going to merge it.
just for now it would be confusing if someone going to install original lib and it will not work with tmp116


To install system-wide (this may be required in some cases):

.. code-block:: shell

sudo pip3 install adafruit-circuitpython-tmp117
sudo pip3 install git+https://github.com/ami3go/Adafruit_CircuitPython_TMP11X.git
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

@@ -56,7 +60,7 @@ To install in a virtual environment in your current project:
mkdir project-name && cd project-name
python3 -m venv .venv
source .venv/bin/activate
pip3 install adafruit-circuitpython-tmp117
pip3 install git+https://github.com/ami3go/Adafruit_CircuitPython_TMP11X.git
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

Comment on lines +76 to +78
t1 = TMP116(i2c_bus=i2c, address=0x49)
# t2 = TMP117(i2c_bus=i2c, address=0x49)
# t2 = TMP119(i2c_bus=i2c, address=0x49)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the I2C addresses all the same? If so don't bother to specify the address here, since the default is fine and makes usage easier.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It appears the default I2C address is the same. It's just that it can be varied via pin settings.

@@ -2,10 +2,10 @@
#
# SPDX-License-Identifier: MIT
"""
`adafruit_tmp117`
`adafruit_tmp11X`
Copy link
Contributor

@dhalbert dhalbert May 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use lower-case x for filename

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and TMP11X (upper case) for new classname.

Comment on lines +139 to +146
DELAY_0_0015_S = None
DELAY_0_125_S = None
DELAY_0_250_S = None
DELAY_0_500_S = None
DELAY_1_S = None
DELAY_4_S = None
DELAY_8_S = None
DELAY_16_S = None
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
DELAY_0_0015_S = None
DELAY_0_125_S = None
DELAY_0_250_S = None
DELAY_0_500_S = None
DELAY_1_S = None
DELAY_4_S = None
DELAY_8_S = None
DELAY_16_S = None

Comment on lines +164 to +165
WINDOW = None
HYSTERESIS = None
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
WINDOW = None
HYSTERESIS = None

Comment on lines +175 to +177
CONTINUOUS = None
ONE_SHOT = None
SHUTDOWN = None
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
CONTINUOUS = None
ONE_SHOT = None
SHUTDOWN = None

# currently set when `alert_status` is read, but not exposed
self.reset()
self.initialize()

# thsi methods is unique for each device
Copy link
Contributor

@dhalbert dhalbert May 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# thsi methods is unique for each device
# this method is unique for each device

Comment on lines 15 to 17
t1 = TMP116(i2c_bus=i2c, address=0x49)
# t2 = TMP116(i2c_bus=i2c, address=0x47)
# t3 = TMP116(i2c_bus=i2c, address=0x48)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, they do have different I2C addresses.

Comment on lines +22 to +23
It is forked from Adafruit_CircuitPython_TMP117, updated and extended to use it with TMP116 and TMP119

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
It is forked from Adafruit_CircuitPython_TMP117, updated and extended to use it with TMP116 and TMP119
``

Comment on lines +76 to +78
t1 = TMP116(i2c_bus=i2c, address=0x49)
# t2 = TMP117(i2c_bus=i2c, address=0x49)
# t2 = TMP119(i2c_bus=i2c, address=0x49)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It appears the default I2C address is the same. It's just that it can be varied via pin settings.

@@ -539,4 +565,17 @@ def _read_status(self) -> Tuple[int, int, int]:
return (high_alert, low_alert, data_ready)

def _read_temperature(self) -> float:
return self._raw_temperature * _TMP117_RESOLUTION
return round(self._raw_temperature * _TMP117_RESOLUTION, 3)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't round here, because the extra digits are still interesting when summing or averaging. Leave it up to the caller to round as needed.

Suggested change
return round(self._raw_temperature * _TMP117_RESOLUTION, 3)
return self._raw_temperature * _TMP117_RESOLUTION```

Also this specifically mentions the TMP117 resolution: does the value change? Should it be TMP11X_RESOLUTION?

@@ -2,10 +2,10 @@
#
# SPDX-License-Identifier: MIT
"""
`adafruit_tmp117`
`adafruit_tmp11X`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and TMP11X (upper case) for new classname.

pandas~=2.1.1
colorama~=0.4.6
nidaqmx~=1.0.0
VotschTechnikClimateChamber~=0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why have all of these modules been added to the requirements.txt file?

Are any / all of these absolutely necessary? Many of them don't have analogous modules in CircuitPython so there functionality will not be possible to use on microcontrollers.

I am in favor of removing these, unless if there are any that are strictly necessary for the operation of this library then they can remain.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants