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

[riot-ci]

Murdock results

✔️ PASSED

58ffa37 drivers/ads1x1x: add undef params, store bits res in struct

Success	Failures	Total	Runtime
10515	0	10516	12m:06s

Artifacts

• Documentation preview

[crasbe]

Please also note the static errors about whitespaces.

[crasbe]

This might not cover all cases currently. If you specify USEMODULE+=ads101x or USEMODULE+=ads111x in your application, the module will not be automatically initialized.

Also the ads1x1x driver will not be loaded. Typically you'd add the pseudomodule resolution to drivers/Makefile.dep file (keep in mind the alphabetical order).

[crasbe]

However this will cause a name conflict with the current ads101x driver.

[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.