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

macros/utils: add LIMIT() and ABS() macros #20352

Merged
merged 5 commits into from
Feb 9, 2024

Conversation

Enoch247
Copy link
Contributor

@Enoch247 Enoch247 commented Feb 7, 2024

Contribution description

This patch adds a macro that can be used to limit a value to a given range. It also adds the classic ABS() macro to go along with the existing MAX() and MIN() macros.

Testing procedure

It's fairly trivial. I have used it and it works in my application code.

Issues/PRs references

  • none known

@Enoch247 Enoch247 requested a review from kaspar030 as a code owner February 7, 2024 14:56
@github-actions github-actions bot added the Area: core Area: RIOT kernel. Handle PRs marked with this with care! label Feb 7, 2024
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 7, 2024
@riot-ci
Copy link

riot-ci commented Feb 7, 2024

Murdock results

✔️ PASSED

fa7a577 tests/unittests: add test for ABS() macro

Success Failures Total Runtime
10016 0 10016 09m:34s

Artifacts

@Enoch247
Copy link
Contributor Author

Enoch247 commented Feb 7, 2024

Looks like there is a name collision with a macro defined in a esp32 package. I am open to other names for the new macro. LIMIT_TO_RANGE() perhaps? Something shorter?

@benpicco
Copy link
Contributor

benpicco commented Feb 7, 2024

If you use the same definition as ESP32, would it still be a redefinition? 🤔

@Enoch247
Copy link
Contributor Author

Enoch247 commented Feb 7, 2024

If you use the same definition as ESP32, would it still be a redefinition? 🤔

I tested the following and did not get an error or warning:

#define FOO 1
#define FOO 1

I don't know if that is promised by the language or just allowed by gcc. Do you know off the top of your head before I go looking? It does feel a bit precarious thought to rely on matching a macro defined outside the source tree.

I though about moving it to macros/math.h and calling it MATH_LIMIT(). That would require including utils.h in math.h. In my opinion MAX() and MIN()should be moved to math.h, but I think that change would affect a lot of people.

@benpicco
Copy link
Contributor

benpicco commented Feb 7, 2024

If it works on CI you can be sure that all supported compilers are covered.

@Enoch247
Copy link
Contributor Author

Enoch247 commented Feb 7, 2024

Ok, I made the change.

Also I added the classic ABS macro to go with MAX/MIN.

@benpicco
Copy link
Contributor

benpicco commented Feb 7, 2024

Please squash!

This patch adds a macro that can be used to limit a value to a given
range.
This patch adds the classic ABS() macro to exist along side of the
MAX/MIN macros.
@Enoch247
Copy link
Contributor Author

Enoch247 commented Feb 8, 2024

squashed

@benpicco benpicco enabled auto-merge February 8, 2024 14:56
@Enoch247
Copy link
Contributor Author

Enoch247 commented Feb 8, 2024

Please wait to merge. I will added unit tests for these macros.

@Enoch247 Enoch247 changed the title macros/utils: add LIMIT() macro macros/utils: add LIMIT() and ABS() macros Feb 8, 2024
@benpicco benpicco disabled auto-merge February 8, 2024 15:47
@Enoch247 Enoch247 requested a review from miri64 as a code owner February 8, 2024 15:56
@github-actions github-actions bot added the Area: tests Area: tests and testing framework label Feb 8, 2024
@Enoch247
Copy link
Contributor Author

Enoch247 commented Feb 8, 2024

Ok, unit tests are added now for new and some existing macros.

@benpicco benpicco added this pull request to the merge queue Feb 8, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 8, 2024
@benpicco benpicco added this pull request to the merge queue Feb 8, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 9, 2024
@maribu maribu added this pull request to the merge queue Feb 9, 2024
Merged via the queue into RIOT-OS:master with commit 2dbc80f Feb 9, 2024
25 checks passed
@MrKevinWeiss MrKevinWeiss added this to the Release 2024.04 milestone Apr 30, 2024
@Enoch247 Enoch247 deleted the add-limit-macro branch October 21, 2024 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: core Area: RIOT kernel. Handle PRs marked with this with care! Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants