Skip to content

Adding type annotations #36

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Nov 13, 2021
Merged

Adding type annotations #36

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2c5ee6a
Adding type annotations
fivesixzero Nov 7, 2021
06ed942
Adding type annotations (update)
fivesixzero Nov 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 38 additions & 25 deletions adafruit_apds9960/apds9960.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,13 @@
from adafruit_bus_device.i2c_device import I2CDevice
from micropython import const

try:
# Only used for typing
from typing import Tuple
from busio import I2C
except ImportError:
pass

__version__ = "0.0.0-auto.0"
__repo__ = "https://github.com/adafruit/Adafruit_CircuitPython_APDS9960.git"

Expand Down Expand Up @@ -143,7 +150,13 @@ class APDS9960:
_proximity_persistance = RWBits(4, APDS9960_PERS, 4)

def __init__(
self, i2c, *, address=0x39, integration_time=0x01, gain=0x01, rotation=0
self,
i2c: I2C,
*,
address: int = 0x39,
integration_time: int = 0x01,
gain: int = 0x01,
rotation: int = 0
):

self.buf129 = None
Copy link
Author

Choose a reason for hiding this comment

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

This buffer being declared as None but then getting used later in gesture() is a bit weird but understandable. Definitely causes a headache for some more aggressive linter configs though.

Not allocating it saves a decent chunk of memory but maybe there's a better way to handle it, like a gesture_enabled flag? Outside the scope here but maybe worth raising an issue for if you guys think its worth improving.

Expand Down Expand Up @@ -178,7 +191,7 @@ def __init__(
self._write8(APDS9960_GPULSE, (0x2 << 6) | 0x3)

## BOARD
def _reset_counts(self):
def _reset_counts(self) -> None:
"""Gesture detection internal counts"""
self._saw_down_start = 0
self._saw_up_start = 0
Expand All @@ -204,37 +217,37 @@ def _reset_counts(self):

## GESTURE ROTATION
@property
def rotation(self):
def rotation(self) -> int:
"""Gesture rotation offset. Acceptable values are 0, 90, 180, 270."""
return self._rotation

@rotation.setter
def rotation(self, new_rotation):
def rotation(self, new_rotation: int) -> None:
if new_rotation in [0, 90, 180, 270]:
self._rotation = new_rotation
else:
raise ValueError("Rotation value must be one of: 0, 90, 180, 270")

## GESTURE DETECTION
@property
def enable_gesture(self):
def enable_gesture(self) -> bool:
"""Gesture detection enable flag. True to enable, False to disable.
Note that when disabled, gesture mode is turned off"""
return self._gesture_enable

@enable_gesture.setter
def enable_gesture(self, enable_flag):
def enable_gesture(self, enable_flag: bool) -> None:
if not enable_flag:
self._gesture_mode = False
self._gesture_enable = enable_flag

def rotated_gesture(self, original_gesture):
def rotated_gesture(self, original_gesture: int) -> int:
"""Applies rotation offset to the given gesture direction and returns the result"""
directions = [1, 4, 2, 3]
new_index = (directions.index(original_gesture) + self._rotation // 90) % 4
return directions[new_index]

def gesture(self): # pylint: disable-msg=too-many-branches
def gesture(self) -> int: # pylint: disable-msg=too-many-branches
"""Returns gesture code if detected. =0 if no gesture detected
=1 if an UP, =2 if a DOWN, =3 if an LEFT, =4 if a RIGHT
"""
Expand All @@ -247,7 +260,7 @@ def gesture(self): # pylint: disable-msg=too-many-branches
if not self._gesture_valid:
return 0

time_mark = 0
time_mark = 0.0
gesture_received = 0
while True:

Expand Down Expand Up @@ -321,21 +334,21 @@ def gesture(self): # pylint: disable-msg=too-many-branches
return gesture_received

@property
def gesture_dimensions(self):
def gesture_dimensions(self) -> int:
"""Gesture dimension value: range 0-3"""
return self._read8(APDS9960_GCONF3)

@gesture_dimensions.setter
def gesture_dimensions(self, dims):
def gesture_dimensions(self, dims: int) -> None:
self._write8(APDS9960_GCONF3, dims & 0x03)

@property
def color_data_ready(self):
def color_data_ready(self) -> int:
"""Color data ready flag. zero if not ready, 1 is ready"""
return self._read8(APDS9960_STATUS) & 0x01

@property
def color_data(self):
def color_data(self) -> Tuple[int, int, int, int]:
"""Tuple containing r, g, b, c values"""
return (
self._color_data16(APDS9960_CDATAL + 2),
Expand All @@ -346,7 +359,7 @@ def color_data(self):

### PROXIMITY
@property
def proximity_interrupt_threshold(self):
def proximity_interrupt_threshold(self) -> Tuple[int, int, int]:
"""Tuple containing low and high threshold
followed by the proximity interrupt persistance.
When setting the proximity interrupt threshold values using a tuple of
Expand All @@ -359,7 +372,7 @@ def proximity_interrupt_threshold(self):
)

@proximity_interrupt_threshold.setter
def proximity_interrupt_threshold(self, setting_tuple):
def proximity_interrupt_threshold(self, setting_tuple: Tuple[int, ...]) -> None:
if setting_tuple:
self._write8(APDS9960_PILT, setting_tuple[0])
if len(setting_tuple) > 1:
Expand All @@ -370,57 +383,57 @@ def proximity_interrupt_threshold(self, setting_tuple):
self._proximity_persistance = persist

@property
def gesture_proximity_threshold(self):
def gesture_proximity_threshold(self) -> int:
"""Proximity threshold value: range 0-255"""
return self._read8(APDS9960_GPENTH)

@gesture_proximity_threshold.setter
def gesture_proximity_threshold(self, thresh):
def gesture_proximity_threshold(self, thresh: int) -> None:
self._write8(APDS9960_GPENTH, thresh & 0xFF)

@property
def proximity(self):
def proximity(self) -> int:
"""Proximity value: range 0-255"""
return self._read8(APDS9960_PDATA)

def clear_interrupt(self):
def clear_interrupt(self) -> None:
"""Clear all interrupts"""
self._writecmdonly(APDS9960_AICLEAR)

@property
def integration_time(self):
def integration_time(self) -> int:
"""Proximity integration time: range 0-255"""
return self._read8(APDS9960_ATIME)

@integration_time.setter
def integration_time(self, int_time):
def integration_time(self, int_time: int) -> None:
self._write8(APDS9960_ATIME, int_time & 0xFF)

# method for reading and writing to I2C
def _write8(self, command, abyte):
def _write8(self, command: int, abyte: int) -> None:
"""Write a command and 1 byte of data to the I2C device"""
buf = self.buf2
buf[0] = command
buf[1] = abyte
with self.i2c_device as i2c:
i2c.write(buf)

def _writecmdonly(self, command):
def _writecmdonly(self, command: int) -> None:
"""Writes a command and 0 bytes of data to the I2C device"""
buf = self.buf2
buf[0] = command
with self.i2c_device as i2c:
i2c.write(buf, end=1)

def _read8(self, command):
def _read8(self, command: int) -> int:
"""Sends a command and reads 1 byte of data from the I2C device"""
buf = self.buf2
buf[0] = command
with self.i2c_device as i2c:
i2c.write_then_readinto(buf, buf, out_end=1, in_end=1)
return buf[0]

def _color_data16(self, command):
def _color_data16(self, command: int) -> int:
"""Sends a command and reads 2 bytes of data from the I2C device
The returned data is low byte first followed by high byte"""
buf = self.buf2
Expand Down
4 changes: 2 additions & 2 deletions adafruit_apds9960/colorutility.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
__repo__ = "https://github.com/adafruit/Adafruit_CircuitPython_APDS9960.git"


def calculate_color_temperature(r, g, b):
def calculate_color_temperature(r: float, g: float, b: float) -> float:
Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure where these are used, so it was hard to infer the types expected for args here. There's definitely a lot of floating point math going on so it seemed logical to keep the return a float.

The functionality here also might be duplicated elsewhere or out of scope for the library if it isn't making use of it internally.

Copy link

@FoamyGuy FoamyGuy Nov 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think the types used for r, g, and b here should match the types returned inside the tuple from color_data up here: https://github.com/fivesixzero/Adafruit_CircuitPython_APDS9960/blob/2c5ee6a3a6453bac2217f3db9a70bb83f022d961/adafruit_apds9960/apds9960.py#L351

This line in one of the example scripts passes those individual values from that tuple into calculate_color_temperature() here: https://github.com/fivesixzero/Adafruit_CircuitPython_APDS9960/blob/2c5ee6a3a6453bac2217f3db9a70bb83f022d961/examples/apds9960_color_simpletest.py#L28

However I'm not certain whether that type would be int or float. I would need to hook up a sensor and look at the values it returns to know for sure. I think maybe this sensor is used on the CLUE or perhaps Feather Sense. I'll try to see if I have one and check the value types.

Copy link
Author

Choose a reason for hiding this comment

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

@FoamyGuy - Thanks for the review. :)

It makes total sense that the utility bundled here should operate on the same types that the library itself returns.

In the follow-up commit I changed the param types to int but kept the return float since we're doing some floating point math internally. If the caller wants an int it should be an easy conversion, I guess?

Changing it to int (and converting at return) would align it with the library as a whole though, at the small cost of potentially losing some (maybe not even required) precision in the output.

Copy link
Author

Choose a reason for hiding this comment

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

I'm pretty sure the color data returned is an int. The data is retrieved by grabbing 16-bit chunks of a register though, which feels a bit excesive for a simple int. I'll double-check this behavior (and the datasheet) today and test it on a Feather Sense and/or a Clue for good measure

Copy link
Author

Choose a reason for hiding this comment

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

Confirmed that the library indeed returns Tuple[int, int, int, int] in the current version of the code running on both a Clue and a Feather Bluefruit Sense.

Adafruit CircuitPython 7.1.0-beta.0 on 2021-11-12; Adafruit CLUE nRF52840 Express with nRF52840
>>> import board
>>> from adafruit_apds9960.apds9960 import APDS9960
>>> apds = APDS9960(board.I2C())
>>>
>>> color = apds.color_data
>>>
>>> type(color)
<class 'tuple'>
>>> len(color)
4
>>> type(color[0])
<class 'int'>
>>> type(color[1])
<class 'int'>
>>> type(color[2])
<class 'int'>
>>> type(color[3])
<class 'int'>

So we're good there. :)

"""Converts the raw R/G/B values to color temperature in degrees Kelvin"""

# 1. Map RGB values to their XYZ counterparts.
Expand All @@ -40,7 +40,7 @@ def calculate_color_temperature(r, g, b):
return cct


def calculate_lux(r, g, b):
def calculate_lux(r: float, g: float, b: float) -> float:
"""Calculate ambient light values"""
# This only uses RGB ... how can we integrate clear or calculate lux
# based exclusively on clear since this might be more reliable?
Expand Down