-
Notifications
You must be signed in to change notification settings - Fork 2.1k
drivers: add support for ADS1115 ADC #21613
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
Conversation
benpicco
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello and welcome to RIOT!
Nice first driver, I added some comments - the static test scripts also added their 2¢
drivers/include/ads1115.h
Outdated
| */ | ||
| typedef struct { | ||
| i2c_t i2c; /**< I2C device */ | ||
| uint8_t addr; /**< I2C address */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this configurable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by ‘configurable’? This should depend on the board you're using. If you mean that it can be modified dynamically after init, no, that's not the case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, some devices have a fixed I2C address (then there is no need to make it configurable in the code), for others the I2C address can be set via strap resistors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok so should I keep this like that ?
drivers/include/ads1115.h
Outdated
| ads1115_comp_mode_t comp_mode; /**< Comparator mode */ | ||
| ads1115_comp_polarity_t comp_polarity; /**< Comparator polarity */ | ||
| ads1115_comp_latch_t comp_latch; /**< Comparator latch */ | ||
| ads1115_comp_queue_t comp_queue; /**< Comparator queue */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the device only have a single input channel / input pin?
Otherwise it could make sense to set those at run-time for the different channels depending on what one wants to measure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No the device can handle four different channel. So maybe I would have to add 4 set functions these four parameters so it can be configure at run-time. What do you think ?
drivers/include/ads1115.h
Outdated
| typedef struct { | ||
| i2c_t i2c; /**< I2C device */ | ||
| uint8_t addr; /**< I2C address */ | ||
| ads1115_mux_t mux; /**< Input multiplexer configuration */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You also have ads1115_set_ain_ch_input(), why also the compile-time config?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just to make sure we have a default configuration with a default channel entry. Should I remove it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A single way to set the configuration is enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but the user may want to get the inputs from differents channels during run-time so the ads1115_set_ain_ch_input() should be necessary, right ?
drivers/ads1115/ads1115.c
Outdated
| i2c_acquire(DEV); | ||
|
|
||
| // Read conversion register | ||
| if (i2c_read_regs(DEV, ADDR, ADS1115_REG_CONVERSION, buf, 2, 0) < 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need to wait for a converstion to finish?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly I don´t know. Do you think the call to i2c_read_regs is a blocking call or we directly read the conversion register ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The data sheet should be able to tell you 😉
Devices can implement clock stretching if they are not ready yet, but this is rather rare.
How is the sampling started?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In single-shot mode (when bit[8] MODE = 1), writing a 1 to the OS bit (bit[15]) starts a conversion.
In continuous mode (when bit[8] MODE=0), the device get the input value continuously and store it into the conversion register
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally think that reading in the conversion register is not a blocking action and we shouldn't wait
drivers/include/ads1115.h
Outdated
| */ | ||
| typedef enum { | ||
| ADS1115_COMP_MODE_TRADITIONAL = 0, /**< Traditional comparator (default) */ | ||
| ADS1115_COMP_MODE_WINDOW = 1 /**< Window comparator */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds like it would generate an interrupt if the comperator matches, but you don't have anything like that in the code - if you don't need it, just leave it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't actually use it, I just add it to have a fully customisable configuration as explained on pages 25-26 of the datasheet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But what good is it to configure something that you then can not use?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes you right, it for the scalability and to be sure that the default config is loaded when calling the init function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@benpicco What do you think about that ?
2e9f348 to
9d7167f
Compare
Co-authored-by: benpicco <[email protected]>
9d7167f to
b71cc2f
Compare
crasbe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your addition of the ADS1115 driver. In the review I have addressed some general styling related things and Doxygen related issues.
Also, as in the other PR, please adapt the title and commit message according to our commit convention: https://github.com/RIOT-OS/RIOT/blob/master/CONTRIBUTING.md#commit-conventions
A possible title and commit message could be: "drivers: add support for ADS1115 ADC" or similar.
a4c2fe4 to
915158c
Compare
|
@crasbe Do you know why the Uncrustify formatter doesn't reformat lines longer than 100 characters? It causes the static test to fail, and manually fixing those lines is quite tedious. |
|
Unfortunately I am not super familiar with |
crasbe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't have to re-request a review on every change, I get an e-mail notification anyway 😅
| ADS1115_PARAMS | ||
| }; | ||
|
|
||
| /** @} */ /* close @ingroup drivers_ads1115 */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /** @} */ /* close @ingroup drivers_ads1115 */ | |
| /** @} */ /* close @ingroup drivers_ads1115 */ |
| ADS1115_PGA_0_512V, /**< FSR = ±0.512V */ | ||
| ADS1115_PGA_0_256V, /**< FSR = ±0.256V */ | ||
|
|
||
| /* Aliases (same behavior) */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /* Aliases (same behavior) */ | |
| /* Aliases (same behavior) */ |
| * @brief I2C Analog-to-digital converter with programmable gain amplifier. | ||
| * The device has four input channels: AIN0, AIN1, AIN2, and AIN3, | ||
| * that can be selected by using @ref ads1115_set_ain_ch_input. | ||
| * |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
|
||
| #ifdef __cplusplus | ||
| } | ||
| #endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| #endif | |
| #endif | |
| /** @} */ /* close @defgroup drivers_ads1115 */ |
I'm sorry, I didn't know that it sends you an email on every change, it's my first open-source contribution. I'm going to pay attention for the future |
|
I just searched for something else in the RIOT code and discovered that there already is a driver that supports the ADC111x ADCs 🤔 😅 Your driver seems to have the nicer code and more documentation, but having two drivers for the same purpose is a bit redundant. Perhaps you could take a look at the |
That's a shame..., in fact the integrated driver supports ads111x despite its lack of documentation. What do you suggest ? Should I abandon this driver or update the existant code to document it better? |
|
I'll have to check what the best approach is. |
I'm sorry to be the bearer of bad news. Yes we should not have duplicate drivers, without some compelling reason. We would very much appreciate if you would open a PR to port any improvements in code and documentation over to the existing driver from your work here though. |
Enoch247
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This driver duplicates an existing one already in RIOT. See here.
|
@Enoch247 @crasbe The problem is that the existing driver seems to be made for ADS101x and it not fully support the ADS111x family. For example, the data rate configuration description of the ADS101x is not the same as the ADS111x here. For me, it would make sense to write a dedicated driver for the ADS111x even if it means writing a common file for commons functions ( |
|
It is not uncommon to write drivers to have a shared common part in one module along with device specific parts that are compiled on demand as separate modules or pseudomodules. The ST77xx is one example. Is this the approach you have in mind? |
|
Yes, exactly! We could use a public header |
|
I'm not familiar with the devices, but seems like a reasonable approach. |

Contribution description
This PR adds initial support for the [Texas Instruments ADS1115 analog-to-digital converter (ADC)] (https://www.ti.com/lit/ds/symlink/ads1115.pdf?ts=1752581207840&ref_url=https%253A%252F%252Fro.mouser.com%252F) by introducing a dedicated RIOT driver.
The driver enables:
This implementation lays the groundwork for further extensions such the other ADS111x, comparator handling etc...
Testing procedure
Issues/PRs references
#21612