Skip to content
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

Adding library and example #1

Merged
merged 9 commits into from
Nov 12, 2023
Merged

Conversation

BlitzCityDIY
Copy link
Contributor

Adding library and example for the ADS7830 ADC (https://www.adafruit.com/product/5836). Arduino library: https://github.com/adafruit/Adafruit_ADS7830

@BlitzCityDIY BlitzCityDIY requested a review from tannewt November 6, 2023 20:37
Comment on lines 131 to 151
@property
def value(self) -> typing.List[int]:
"""Single-ended mode ADC values for all channels

:rtype: List[int]"""
values = []
for i in range(8):
single_value = self._single_channel(i)
values.append(single_value)
return values

@property
def differential_value(self) -> typing.List[int]:
"""Differential ADC values for all channel pairs

:rtype: List[int]"""
values = []
for i in range(0, 8, 2): # Iterate over channel pairs
diff_value = self._differential_channel(i)
values.append(diff_value)
return values
Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't have these read every value, every time because that'll be slow and do a bunch of work that the user may not need. Instead, take a look at this library: https://github.com/adafruit/Adafruit_CircuitPython_MCP3xxx/tree/main/adafruit_mcp3xxx

It provides a way to get the value directly and also has an AnalogIn compatibility class too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

awesome, thanks! that was really helpful to reference

@BlitzCityDIY BlitzCityDIY requested a review from tannewt November 7, 2023 14:44
Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

Thanks for the restructuring! It looks good. Just a few other comments on params.

adafruit_ads7830/ads7830.py Outdated Show resolved Hide resolved
adafruit_ads7830/ads7830.py Outdated Show resolved Hide resolved
@BlitzCityDIY BlitzCityDIY requested a review from tannewt November 9, 2023 01:08
Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

One more suggestion to remove the power down list. It isn't needed for verification or mapping.

Another thing to think about is the parameter naming (adc_pd for example). Using truncated words can make it harder to understand on its own. Full words tend to be easier to understand.

adafruit_ads7830/ads7830.py Outdated Show resolved Hide resolved
@BlitzCityDIY BlitzCityDIY requested a review from tannewt November 10, 2023 15:34
Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

Two minor suggestions you can ignore if you want. Thanks for the rename.

Comment on lines 86 to 90
self.power_down = 0
if not int_ref_power_down:
self.power_down |= 2
if not adc_power_down:
self.power_down |= 1
Copy link
Member

Choose a reason for hiding this comment

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

My example used a temporary variable because this will do reads and writes over i2c for each access.

adafruit_ads7830/ads7830.py Outdated Show resolved Hide resolved
@BlitzCityDIY BlitzCityDIY merged commit 66be365 into adafruit:main Nov 12, 2023
1 check passed
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.

2 participants