drivers: add a common driver for ADS1X1X

[baptleduc]

Contribution description

While attempting to create a driver for my ADS1115 (this PR), I realized that a driver already exists here. However, the existing driver does not fully support the ADS111x family (datasheet here), which differs from the ADS101x series (datasheet here in several aspects such as bit resolution and maximum sample rate (see page 3 of the datasheet).

I have therefore implemented a common driver for the ADS1x1x family, better documented and inspired by the existing ADS101x driver. This unified driver supports both ADS101x and ADS111x variants, handling their differences internally.

Testing procedure

I adapted the existing test tests/drivers/ads101x to create `tests/drivers/ads111x and compared the outputs to ensure identical behavior and no regressions.

It may be worth adding a helper function to verify the configuration register after each write, to ensure it contains the expected value.

Open questions

• In a discussion on Element.io, @Enoch247 pointed out that my current family-selection system is exclusive, which prevents supporting a case where both an ADS101x and an ADS111x driver are used in the same application. How should this be handled?
• Is it necessary to check the return value of i2c_read_regs and i2c_write_regs every time?
• I am not entirely clear on the difference between the return codes I2C_NODEV and I2C_NOI2C, so I am not sure I have used them correctly throughout the driver.

Issues/PRs references

Follow-up from PR
See the original issue : #21612
Depends on PR to rename ads101x driver to ads1x1x: #21731

[riot-ci]

Murdock results

✔️ PASSED

10283aa drivers/ads1x1x: add Kconfig support

Success	Failures	Total	Runtime
10931	0	10932	14m:35s

Artifacts

• Documentation preview

[crasbe]

Please also note the static errors about whitespaces.

[Enoch247]

I don't see mention of the author's of the existing driver with the rename. Does this new driver really start from scratch with no influences from the existing one?

[Enoch247]

In a discussion on Element.io, @Enoch247 pointed out that my current family-selection system is exclusive, which prevents supporting a case where both an ADS101x and an ADS111x driver are used in the same application. How should this be handled?

Doing a git grep MODULE_ADS1 only turned up a handful of matches in the code. I can look at them individually and make some suggestions, but I am out of time for today. Generally though it looked like they just changed a few defines that are only used in the params structures. I think you could change these ADS1X1X_* defines to ADS111X_* and ADS101X_* and allow them to co-exist. The params structs are device board specific. So anyone creating a param struct ought to know what kind of device is actually connected to their board and know what set of defines to be using.

[baptleduc]

In a discussion on Element.io, @Enoch247 pointed out that my current family-selection system is exclusive, which prevents supporting a case where both an ADS101x and an ADS111x driver are used in the same application. How should this be handled?

Doing a git grep MODULE_ADS1 only turned up a handful of matches in the code. I can look at them individually and make some suggestions, but I am out of time for today. Generally though it looked like they just changed a few defines that are only used in the params structures. I think you could change these ADS1X1X_* defines to ADS111X_* and ADS101X_* and allow them to co-exist. The params structs are device board specific. So anyone creating a param struct ought to know what kind of device is actually connected to their board and know what set of defines to be using.

In this case, how should I handle defines that are common to both ads101x and ads111x ? Should I keep them as ADS1X1X_* ?

Also, if I rename these defines to be specific, then functions in ads1x1x.c would need to know exactly which module they are handling, probably requiring #if macros…

Finally, I’m not entirely clear on how the user is expected to select which ADS version they want to use.

It would be really helpful if you could provide a small example to illustrate the intended usage, if possible.

[Enoch247]

In this case, how should I handle defines that are common to both ads101x and ads111x ? Should I keep them as ADS1X1X_* ?

I think that would be logical.

Also, if I rename these defines to be specific, then functions in ads1x1x.c would need to know exactly which module they are handling, probably requiring #if macros…

As far as I can tell, the only code in the entire driver that needs to know which hardware flavor it is operating on is the ads1x1x_convert_to_mv(). One possible way of dealing with this could be with a ads101x_convert_to_mv() and ads101x_convert_to_mv() variant.

Finally, I’m not entirely clear on how the user is expected to select which ADS version they want to use.

Referring to my above statement, again, I don't see the driver containing any device specific code, aside from the one conversion function.

[Enoch247]

One architectural issue that I have with this PR, is that it renames the existing driver. The existing driver claims to support both flavors of the hardware, I see no reason to rename it. I prefer the new name you have chosen over the old one, but not enough to justify breaking compatibility with existing users of the driver. The rename also complicates version control, and PR review.

[baptleduc]

In this case, how should I handle defines that are common to both ads101x and ads111x ? Should I keep them as ADS1X1X_* ?

I think that would be logical.

Also, if I rename these defines to be specific, then functions in ads1x1x.c would need to know exactly which module they are handling, probably requiring #if macros…

As far as I can tell, the only code in the entire driver that needs to know which hardware flavor it is operating on is the ads1x1x_convert_to_mv(). One possible way of dealing with this could be with a ads101x_convert_to_mv() and ads101x_convert_to_mv() variant.

Finally, I’m not entirely clear on how the user is expected to select which ADS version they want to use.

Referring to my above statement, again, I don't see the driver containing any device specific code, aside from the one conversion function.

The DATAR and BIT_RES are the only defines macros that differs between the two models. Therefore, the ads_init() function should be specific too, to avoid writing to different init functions, I wrote two default params structure ADS101X_PARAMS and ADS111X_PARAMS and declared statically 2 params structure that the user can chose ads101x_params and ads111x_params.

The problem with that, is the saul's auto_init function, that doesn't know anything about the ADS model we're using.

[Teufelchen1]

@Enoch247 how can we move this further?

[baptleduc]

I fixed the problem by applying @Enoch247 advises.
To make the review process easier, please review this #21731 first.

[Enoch247]

@Enoch247 how can we move this further?

I'm not likely to have time to provide a full review, nor was it ever my intent. I've only looked at the PR from a very high level view of the API and tired to provide feedback on that part only.

[baptleduc]

@Teufelchen1 Hi ! It has been several month since this PR was opened. Should I do anything to make the review more simple ?

[Teufelchen1]

From my POV only this comment is still left open: #21694 (comment)

[baptleduc]

From my POV only this comment is still left open: #21694 (comment)

I've just closed the comment after fixing what you recommended. Everything should be ok now

[Teufelchen1]

Cool! I think it is time to squash the commits down a bit.

I am honestly a bit confused whether or not the renaming PR (#21731) is still needed / still has to be merged beforehand? Maybe my brain melted a bit from all the ads1x1x01x1x0x1 mangling... 🫠 I expected the commits of the other PR to show up here, as it is build on top of it, isn't it?

[baptleduc]

Cool! I think it is time to squash the commits down a bit.

I am honestly a bit confused whether or not the renaming PR (#21731) is still needed / still has to be merged beforehand? Maybe my brain melted a bit from all the ads1x1x01x1x0x1 mangling... 🫠 I expected the commits of the other PR to show up here, as it is build on top of it, isn't it?

Yes you're totally right, unfortunately I've just realized that I didn't have the commits of the other PR and instead of that I had another commit that removed the folder ads101x/. I've just dropped this commit and I'm going to do a rebase on the #21731 branch to include their commits.

Before that I'm going to squash my commits to clean up and to make conflicts resolution easier.

[baptleduc]

@Teufelchen1 I squashed my commits and rebased my work on the #21731 branch so you can see the driver renaming commit in the commit history. I also added a Kconfig support that needs to be review as it's my first time using Kconfig.