Skip to content

Conversation

@baptleduc
Copy link
Contributor

Contribution description

This PR renames the existing ads101x driver to ads1x1x.
The purpose of this change is to prepare the codebase and to simplify the review process for supporting both the ADS101x and ADS111x families in a unified driver.

No functional changes are introduced in this PR.
Only file names, Kconfig symbols, defines macros, and related references have been updated to reflect the new driver name.

This rename will simplify the review process for upcoming PR that extend the driver to handle the ADS111x family.

Testing procedure

Build and run existing tests/drivers/ads1x1x (now renamed accordingly) to confirm no regressions.

Issues/PRs references

A follow-up PR will introduce ADS111x support and internal handling of family-specific differences.

@github-actions github-actions bot added Area: tests Area: tests and testing framework Area: drivers Area: Device drivers Area: Kconfig Area: Kconfig integration labels Sep 20, 2025
@baptleduc baptleduc force-pushed the rename-ads101x-driver branch from 464f63b to 43e3aad Compare September 20, 2025 10:21
@crasbe crasbe added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Sep 21, 2025
@riot-ci
Copy link

riot-ci commented Sep 21, 2025

Murdock results

✔️ PASSED

9dfd1d1 drivers/ads101x: rename driver to ads1x1x

Success Failures Total Runtime
10949 0 10950 10m:18s

Artifacts

@baptleduc
Copy link
Contributor Author

The macro defines weren’t documented before I renamed them, which caused the CI to fail after the renaming.
What I don’t understand is how the CI passed before, since it shouldn’t have.

@leandrolanzieri
Copy link
Contributor

The failure seems to be related to moving the Kconfig file:

kconfiglib.KconfigError: /tmp/dwq.0.1912543697952842/44aa831c8a8590889923f0e364609cb7/drivers/Kconfig:32: '/tmp/dwq.0.1912543697952842/44aa831c8a8590889923f0e364609cb7/drivers/ads101x/Kconfig' not found (in 'rsource "ads101x/Kconfig"'). Check that environment variables are set correctly (e.g. $srctree, which is unset or blank). Also note that unset environment variables expand to the empty string.

You probably need to also update this line:

rsource "ads101x/Kconfig"

@baptleduc
Copy link
Contributor Author

Ok it passed Murdock CI. But the static tests always failed because I didn't touch the documentation during the renaming process, and it was missing from the start.

@leandrolanzieri leandrolanzieri added the Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. label Sep 25, 2025
@baptleduc
Copy link
Contributor Author

So do I need to comment the macros for the review ?

@maribu
Copy link
Member

maribu commented Sep 30, 2025

The alternative to adding Doxygen comments documenting the macros would be to update the list of expected Doxygen errors. That list will contain exceptions for the old file, but is missing them for the new file.

@github-actions github-actions bot added the Area: tools Area: Supplementary tools label Oct 1, 2025
@baptleduc baptleduc force-pushed the rename-ads101x-driver branch from 3fcc23a to 5ea2ea5 Compare November 24, 2025 14:01
@github-actions github-actions bot added the Area: SAUL Area: Sensor/Actuator Uber Layer label Nov 24, 2025
@baptleduc baptleduc force-pushed the rename-ads101x-driver branch from 5ea2ea5 to de7e40f Compare November 24, 2025 15:07
@Teufelchen1
Copy link
Contributor

Nice! Almost there.
You overlooked a name change for the dependency resolution and I would like to see an error message for the users of the old name, instructing them to switch to the new name. (Okay, that will be super short lived and obsolete once the followup PR is merged but eh, better do it right.)

I would propose this change to address both issues:

--- a/drivers/Makefile.dep
+++ b/drivers/Makefile.dep
@@ -4,8 +4,11 @@ ifneq (,$(filter adc%1c,$(USEMODULE)))
   USEMODULE += adcxx1c
 endif
 
-ifneq (,$(filter ads101%,$(USEMODULE)))
-  USEMODULE += ads101x
+ifneq (,$(filter ads101x,$(USEMODULE)))
+  $(error "Module ads101x got renamed to ads1x1x.")
+endif
+ifneq (,$(filter ads1x1%,$(USEMODULE)))
+  USEMODULE += ads1x1x
 endif
 
 ifneq (,$(filter apds99%,$(USEMODULE)))

@baptleduc baptleduc force-pushed the rename-ads101x-driver branch from de7e40f to 5e86311 Compare November 26, 2025 02:37
@github-actions github-actions bot added the Area: examples Area: Example Applications label Nov 26, 2025
@baptleduc
Copy link
Contributor Author

baptleduc commented Nov 26, 2025

+ifneq (,$(filter ads101x,$(USEMODULE)))
+  $(error "Module ads101x got renamed to ads1x1x.")
+endif
+ifneq (,$(filter ads1x1%,$(USEMODULE)))
+  USEMODULE += ads1x1x
 endif

Thanks for pointing that out, it should be fixed now!

I guess I also need to include this change in the branch of the follow-up PR.

However, I'm still a bit unsure about the purpose of this file. You mentioned that the change will be short-lived, but it should still be valid in the next PR, right?

@baptleduc baptleduc force-pushed the rename-ads101x-driver branch from 5e86311 to dc4f6b1 Compare November 26, 2025 08:32
@Teufelchen1
Copy link
Contributor

I guess I also need to include this change in the branch of the follow-up PR.

Yes, you will have to rebase the other PR on this branch.

However, I'm still a bit unsure about the purpose of this file.

That file maps module names as provided by the user to actual drivers. For 1:1 matches that is indeed curious and seems unneccessary. But if you look closely at the old code that you just replaced, you can see a trailing % in the module name. Thats a placeholder telling make to match on any name that has this given prefix. Take a look GNU Make section $(filter pattern…,text).
Here it was used so that USEMODULE += ads1011 and += ads1010 would have triggered that the actual module ads101x would have been included.

ifneq (,$(filter ads101%,$(USEMODULE)))
   USEMODULE += ads101x
endif

(this made me just realise that we might have made a mistake and the error should also match on ads101% instead of ads101x? Can't look into it right now.)

You mentioned that the change will be short-lived, but it should still be valid in the next PR, right?

Yes'ish. I haven't thought about the exact implication for the follow-up driver. Maybe the followup will match on all names, including the old drivers name, in that case the error makes no sense. Maybe as an warning as compilation with old code will not work without adjusting the names? We will see once this is merged and the other PR becomes ready.

@baptleduc
Copy link
Contributor Author

baptleduc commented Nov 26, 2025

I guess I also need to include this change in the branch of the follow-up PR.

Yes, you will have to rebase the other PR on this branch.

However, I'm still a bit unsure about the purpose of this file.

That file maps module names as provided by the user to actual drivers. For 1:1 matches that is indeed curious and seems unneccessary. But if you look closely at the old code that you just replaced, you can see a trailing % in the module name. Thats a placeholder telling make to match on any name that has this given prefix. Take a look GNU Make section $(filter pattern…,text). Here it was used so that USEMODULE += ads1011 and += ads1010 would have triggered that the actual module ads101x would have been included.

ifneq (,$(filter ads101%,$(USEMODULE)))
   USEMODULE += ads101x
endif

Thank you for the explanation.

(this made me just realise that we might have made a mistake and the error should also match on ads101% instead of ads101x? Can't look into it right now.)

Yes you're right, I'm going to fixed this immediately.

You mentioned that the change will be short-lived, but it should still be valid in the next PR, right?

Yes'ish. I haven't thought about the exact implication for the follow-up driver. Maybe the followup will match on all names, including the old drivers name, in that case the error makes no sense. Maybe as an warning as compilation with old code will not work without adjusting the names? We will see once this is merged and the other PR becomes ready.
ok no problem

@baptleduc baptleduc force-pushed the rename-ads101x-driver branch from dc4f6b1 to 5bc8b5a Compare November 26, 2025 13:37
@baptleduc
Copy link
Contributor Author

Hi @Teufelchen1, is it ok for you ? can we move forward by merging this PR ?

Copy link
Contributor

@Teufelchen1 Teufelchen1 left a comment

Choose a reason for hiding this comment

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

Two changes accidentally slipped into this PR and need to be removed.

@baptleduc baptleduc force-pushed the rename-ads101x-driver branch from 5bc8b5a to b5d3404 Compare December 2, 2025 13:50
@baptleduc
Copy link
Contributor Author

Two changes accidentally slipped into this PR and need to be removed.

Sorry it was a mistake, it should be fixed now.

@baptleduc baptleduc force-pushed the rename-ads101x-driver branch from b5d3404 to 9dfd1d1 Compare December 2, 2025 14:11
@github-actions github-actions bot removed the Area: examples Area: Example Applications label Dec 2, 2025
@baptleduc baptleduc requested a review from Teufelchen1 December 2, 2025 14:12
@Teufelchen1 Teufelchen1 added this pull request to the merge queue Dec 3, 2025
Merged via the queue into RIOT-OS:master with commit c5cb0c2 Dec 3, 2025
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: drivers Area: Device drivers Area: Kconfig Area: Kconfig integration Area: SAUL Area: Sensor/Actuator Uber Layer Area: tests Area: tests and testing framework Area: tools Area: Supplementary tools CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants