Added Comparator functionality #98

RoaCode · 2024-07-23T21:21:52Z

Enabled use of Comparator output by adding controls for the COMP_QUE fields in the Config Register. Also added an example of use.

Added controls for the COMP_QUE fields in the Config Register that handle the Comparator output. Also added an example of use.

Added comments for new functions

tannewt

Thank you for the PR! I've added a few comments around the new APIs and their documentation.

adafruit_ads1x15/ads1x15.py

adafruit_ads1x15/analog_in.py

Added threshold properties, changed to "comparator_queue_length" property name, added "convert to voltage" and "convert to value" functions

RoaCode · 2024-07-24T19:49:21Z

Thanks for the feedback, @tannewt . I think I've addressed everything in the latest commit.

tannewt

Thanks for the changes! The docs look good. Just a couple more questions.

adafruit_ads1x15/ads1x15.py

Writing of the threshold registers moved to the setter functions, "threshold" spelled out in variables, improved readability of some comments.

RoaCode · 2024-07-25T21:04:13Z

Comments addressed. I2C writes to threshold registers moved to setter functions, "threshold" spelled out in variables, and improved description in comments.

tannewt

Two minor things. Looks good otherwise. Getting close! Thanks!

adafruit_ads1x15/ads1x15.py

tannewt · 2024-07-26T17:25:57Z

adafruit_ads1x15/ads1x15.py

+        comparator_low_threshold: int = 0x8000,
+        comparator_high_threshold: int = 0x7FF0,


Why is the low threshold default higher than the default high threshold?

This is tricky because the values are in 2's complement, but also they are 12-bit values stored in 16-bit registers where the 4 LSBs are 0. Because of this, 0x8000 is the lowest possible value (decimal -32768 in 16-bit or really -2048 in 12-bit) and 0x7FF0 is the highest possible value (decimal 32752 in 16-bit or really 2047 in 12-bit).
I expanded the comments to hopefully explain this better.

Ah! I think it'll be clearer to set the defaults to the actual values and have the properties do the conversion and masking.

I set the defaults to the actual (12-bit signed) values so that it is easier to read, and moved the 2's complement and bit shift to 16-bit to the setter functions.

Please use 16-bit values because this library is also for the ADS1115 which is 16-bit. In the math you can do self.bits to read if it is 12 or 16 bit.

I tried to copy the handling of the comparator low/high threshold properties just like the other properties, such as data rate. The subclasses handle the lower level chip differences (12 vs 16 bits, default settings, etc) and pass those back to the higher level ads1x15 class.
Also, the ADC data in the analog_in class is handled as a signed 16-bit integer, so I felt it was appropriate to similarly handle the comparator threshold values as signed 16-bit integers as well.

Fixed issues in ads1x15 init comment. Removed stray copy/paste and added more info for comparator thresholds.

Disabled instance attributes req for ADS1x15 init function and shortened "possible_comparator_queue_lengths" to "possible_comp_queue_lengths"

FoamyGuy · 2024-07-29T13:32:34Z

If you use the process in the guide here: https://learn.adafruit.com/improve-your-code-with-pylint/check-your-code-with-pre-commit to install pre-commit locally you can run it and it'll help you find and fix anything that the actions check will have issue with.

Trying to fix "too many instance attributes" issue

RoaCode · 2024-07-29T16:01:41Z

@FoamyGuy Thanks for the comment. I did have to change the pylint version (to v3.2.6 from v2.17.4) to get it to work, I believe because I am using python 3.12. I hope that is OK.

FoamyGuy · 2024-07-29T17:42:13Z

@RoaCode thank you!

Yes thats fine to change the pylint version for now. We're in the process of switching to use Ruff so this repo will get swapped over to that anyway. I've been contending with that same issue with pyt3.12 and pylint.

Threshold properties changed to 12-bit signed integers and conversion to unsigned 16-bit numbers moved to threshold setter functions.

Adapted functions to use bits properties and moved part specific settings to subclasses

tannewt

Thanks for continuing to iterate on this with me. It is the first comparator API in CircuitPython-land so I want to ensure we get it right.

I think we'll need the thresholds to be ints in the 17-bit signed range so that the voltage to value conversion can produce the whole 16 bit unsigned range that value has been defined as.

tannewt · 2024-07-31T18:31:46Z

adafruit_ads1x15/ads1x15.py

+                          function. Value is 12-bit, 2's complement stored in
+                          16-bit register where 4 LSBs are 0. Defaults to 0x8000 (decimal -32768).


Suggested change

function. Value is 12-bit, 2's complement stored in

16-bit register where 4 LSBs are 0. Defaults to 0x8000 (decimal -32768).

function. Value is a signed 17-bit integer.

I just realized I forgot to update this comment in the higher level (ads1x15) class to be more generic, since the subclasses/chips handle this differently.
However, even for the ads1115, the conversion register (ADC data) and the threshold registers on the chip are only 16 bits total, whether or not you think of them as signed or unsigned, so there are only 2^16 (0 to 65535) possible values. So I don't understand why call it a 17-bit int? It would seem like python can handle it either way, as in the widths of ints don't have to be specified. But I agree that the comment should be correct so everyone understands.

tannewt · 2024-07-31T18:32:06Z

adafruit_ads1x15/ads1x15.py

+                          function. Value is 12-bit, 2's complement stored in
+                          16-bit register where 4 LSBs are 0. Defaults to 0x7FF0 (decimal 32752).


Suggested change

function. Value is 12-bit, 2's complement stored in

16-bit register where 4 LSBs are 0. Defaults to 0x7FF0 (decimal 32752).

function. Value is a signed 17-bit integer.

tannewt · 2024-07-31T18:32:38Z

adafruit_ads1x15/ads1x15.py

    def __init__(
        self,
        i2c: I2C,
        gain: float = 1,
        data_rate: Optional[int] = None,
        mode: int = Mode.SINGLE,
+        comparator_queue_length: int = 0,
+        comparator_low_threshold: Optional[int] = None,


Suggested change

comparator_low_threshold: Optional[int] = None,

comparator_low_threshold: int = -32768,

tannewt · 2024-07-31T18:33:31Z

adafruit_ads1x15/ads1x15.py

    def __init__(
        self,
        i2c: I2C,
        gain: float = 1,
        data_rate: Optional[int] = None,
        mode: int = Mode.SINGLE,
+        comparator_queue_length: int = 0,
+        comparator_low_threshold: Optional[int] = None,
+        comparator_high_threshold: Optional[int] = None,


Suggested change

comparator_high_threshold: Optional[int] = None,

comparator_high_threshold: int = 32767,

tannewt · 2024-07-31T18:34:30Z

adafruit_ads1x15/ads1x15.py

+        self.comparator_low_threshold = (
+            self._comp_low_thres_default()
+            if comparator_low_threshold is None
+            else comparator_low_threshold
+        )
+        self.comparator_high_threshold = (
+            self._comp_high_thres_default()
+            if comparator_high_threshold is None
+            else comparator_high_threshold
+        )


The default is the same for both because the lower four bits are 1s in the high threshold.

Suggested change

self.comparator_low_threshold = (

self._comp_low_thres_default()

if comparator_low_threshold is None

else comparator_low_threshold

)

self.comparator_high_threshold = (

self._comp_high_thres_default()

if comparator_high_threshold is None

else comparator_high_threshold

)

self.comparator_low_threshold = comparator_low_threshold

self.comparator_high_threshold = comparator_high_threshold

You are correct. I mistakenly went to some effort to separate them when I didn't need to.

tannewt · 2024-07-31T18:46:04Z

adafruit_ads1x15/ads1x15.py

+
+        :param int value: 16-bit signed integer to write to register
+        """
+        if value < 0 or value > 65535:


Isn't this the right range to check against since it is signed?

Suggested change

if value < 0 or value > 65535:

if value < -32768 or value > 32767:

You are technically correct, but python will look at 0xFFFF as 65535 and say that it is out of range, instead of looking at it as -1 and say that it is in range. I feel like what I did was a workaround to make things work.

I just discovered "a_bytes_big = an_int.to_bytes(2, 'big', signed = True)" that should convert between signed integers and bytes easier. I'll try to work that in.

tannewt · 2024-07-31T18:49:10Z

adafruit_ads1x15/ads1015.py

-        return value >> 4
+        return value


Don't change this. You'll change how read() works otherwise. Only .value needs to be the 16bit standard.

In the ads1015 flow, I believe there are 2 unnecessary bit shifts. This is the right bit shift, which is immediately cancelled out by the left bit shift in the value property of analog_in.py. I removed both of these. There is no need to take in a 16-bit number, convert to 12, convert back to 16, and then report as 16. Removing both keeps .value at 16-bits for both ads1015 and ads1115.

adafruit_ads1x15/analog_in.py

tannewt · 2024-07-31T18:56:14Z

adafruit_ads1x15/analog_in.py

+        if value_int & 0x8000:
+            # Need to convert negative number through 2's complement
+            value_int -= 0x10000
+
+        # Need to bit shift if value is only 12-bits
+        value_int >>= 16 - self._ads.bits


Again. Don't worry about two's complement or the numbers of bits. This should be 16 bit unsigned or "17 bit" signed to voltage.

I think we are on different pages as far as how to represent value. From "Signed number representations" on Wikipedia, 8-bit signed goes from -127 to 127. 8-bit unsigned goes from 0 to 255. I think value should match the registers in the datasheet, which is 16 bit two's complement or signed.

Can you explain 17-bit to me?

Voltage is a float since we want it to have decimal places.

value for reading from an adc has been defined as 16-bit unsigned since we added the AnalogIn API. If you want this resolution signed, then you need to add another bit for the sign. That would make it 17 total bits. I realize that the comparator will only do 16-bit signed. This conversion from voltage to value should handle the full 16-bit unsigned range still though.

tannewt · 2024-07-31T18:57:08Z

adafruit_ads1x15/ads1x15.py

+    def comparator_high_threshold(self, value: int) -> None:
+        """Set comparator high threshold value for ADS1015 ADC
+
+        :param int value: 16-bit signed integer to write to register


Suggested change

:param int value: 16-bit signed integer to write to register

:param int value: 17-bit signed integer to write to register. The lowest bit is dropped.

Moved specific comments about threshold registers to subclasses

Conversion calculations handled with signed integers, removing 2s complement. Removed subclass comparator threshold defaults since both chips have same default values.

tannewt

Looks much better! Thank you! Just one more thing about the warning prints.

adafruit_ads1x15/ads1x15.py

tannewt

Looks good! Thank you!

RoaCode added 2 commits July 23, 2024 17:09

Added Comparator functionality

017fb9f

Added controls for the COMP_QUE fields in the Config Register that handle the Comparator output. Also added an example of use.

Improved Comments

c84a39d

Added comments for new functions

tannewt requested changes Jul 24, 2024

View reviewed changes

Addressed PR comments

7d98fde

Added threshold properties, changed to "comparator_queue_length" property name, added "convert to voltage" and "convert to value" functions

tannewt requested changes Jul 25, 2024

View reviewed changes

adafruit_ads1x15/ads1x15.py Outdated Show resolved Hide resolved

adafruit_ads1x15/ads1x15.py Outdated Show resolved Hide resolved

adafruit_ads1x15/ads1x15.py Outdated Show resolved Hide resolved

Moved threshold writes to setters

0aa1cd7

Writing of the threshold registers moved to the setter functions, "threshold" spelled out in variables, improved readability of some comments.

tannewt requested changes Jul 26, 2024

View reviewed changes

RoaCode added 2 commits July 26, 2024 13:46

Fixed ads1x15 init comment

29ee801

Fixed issues in ads1x15 init comment. Removed stray copy/paste and added more info for comparator thresholds.

Corrected Pylint issues

a730639

Disabled instance attributes req for ADS1x15 init function and shortened "possible_comparator_queue_lengths" to "possible_comp_queue_lengths"

Correcting pylint issues

f74e4f9

Trying to fix "too many instance attributes" issue

RoaCode added 2 commits July 29, 2024 17:24

Moved 2's complement calculation to threshold setters

4ba6144

Threshold properties changed to 12-bit signed integers and conversion to unsigned 16-bit numbers moved to threshold setter functions.

Moved threshold defaults to subclass

8ddd81c

Adapted functions to use bits properties and moved part specific settings to subclasses

tannewt requested changes Jul 31, 2024

View reviewed changes

RoaCode added 2 commits August 1, 2024 13:54

Updated comments with threshold register info

657b768

Moved specific comments about threshold registers to subclasses

Removed 2s Complement calculations

f7fd261

Conversion calculations handled with signed integers, removing 2s complement. Removed subclass comparator threshold defaults since both chips have same default values.

tannewt requested changes Aug 5, 2024

View reviewed changes

adafruit_ads1x15/ads1x15.py Outdated Show resolved Hide resolved

adafruit_ads1x15/ads1x15.py Outdated Show resolved Hide resolved

Removed bit truncation warning

3e31097

tannewt approved these changes Aug 6, 2024

View reviewed changes

tannewt merged commit bf390e7 into adafruit:main Aug 6, 2024
1 check passed

RoaCode deleted the comparator branch August 6, 2024 17:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Comparator functionality #98

Added Comparator functionality #98

RoaCode commented Jul 23, 2024

tannewt left a comment

RoaCode commented Jul 24, 2024

tannewt left a comment

RoaCode commented Jul 25, 2024

tannewt left a comment

tannewt Jul 26, 2024

RoaCode Jul 26, 2024

tannewt Jul 29, 2024

RoaCode Jul 29, 2024

tannewt Jul 30, 2024

RoaCode Jul 31, 2024

FoamyGuy commented Jul 29, 2024

RoaCode commented Jul 29, 2024

FoamyGuy commented Jul 29, 2024

tannewt left a comment

tannewt Jul 31, 2024

RoaCode Jul 31, 2024

tannewt Jul 31, 2024

tannewt Jul 31, 2024

tannewt Jul 31, 2024

tannewt Jul 31, 2024

RoaCode Aug 1, 2024

tannewt Jul 31, 2024

RoaCode Aug 1, 2024

RoaCode Aug 1, 2024

tannewt Jul 31, 2024

RoaCode Aug 1, 2024

tannewt Jul 31, 2024

RoaCode Aug 1, 2024

tannewt Aug 6, 2024

tannewt Jul 31, 2024

tannewt left a comment

tannewt left a comment

		comparator_low_threshold: int = 0x8000,
		comparator_high_threshold: int = 0x7FF0,

		function. Value is 12-bit, 2's complement stored in
		16-bit register where 4 LSBs are 0. Defaults to 0x8000 (decimal -32768).

	function. Value is 12-bit, 2's complement stored in
	16-bit register where 4 LSBs are 0. Defaults to 0x8000 (decimal -32768).
	function. Value is a signed 17-bit integer.

	comparator_low_threshold: Optional[int] = None,
	comparator_low_threshold: int = -32768,

	comparator_high_threshold: Optional[int] = None,
	comparator_high_threshold: int = 32767,

	if value < 0 or value > 65535:
	if value < -32768 or value > 32767:

	:param int value: 16-bit signed integer to write to register
	:param int value: 17-bit signed integer to write to register. The lowest bit is dropped.

Added Comparator functionality #98

Added Comparator functionality #98

Conversation

RoaCode commented Jul 23, 2024

tannewt left a comment

Choose a reason for hiding this comment

RoaCode commented Jul 24, 2024

tannewt left a comment

Choose a reason for hiding this comment

RoaCode commented Jul 25, 2024

tannewt 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

FoamyGuy commented Jul 29, 2024

RoaCode commented Jul 29, 2024

FoamyGuy commented Jul 29, 2024

tannewt 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

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

tannewt left a comment

Choose a reason for hiding this comment

tannewt left a comment

Choose a reason for hiding this comment