sys/irq: Add C++ wrapper using RAII by jenswet · Pull Request #17066 · RIOT-OS/RIOT

jenswet · 2021-10-27T14:52:36Z

Contribution description

This adds a C++ wrapper around the irq.h API. The wrapper uses RAII to accomplish a convenient and bug resistent use.

A little background: I'm currently writing my master thesis on using C++ for embedded development, at the working group that @maribu is part of. For that I will try to add better C++ support to several parts of RIOT and then do some benchmarking and metrics to compare it with the C implementation. For example, I also plan to add a wrapper around i2c, a std::cout drop-in replacement and probably some more about networks or threads.

Testing procedure

I've added a unit test to verify that the IRQ wrapper calls the original irq functions as expected. As C++ and wrapper testing isn't done much so far in this project, I've added two additional headers to ease testing:

pkg/fff: Add fake functions framework package #17076 - fake functions framework, already merged
As there is no framework for C++ unit tests yet, I've added something for this too. Unfortunately the existing frameworks like GoogleTest, CppUTest or CppUnit don't easily compile for embedded or are difficult to integrate in to the RIOT build process. That's why I wrote some (simple) helper functions and macros inspired by the above frameworks. That allows to create C++ tests based on a fixture class with set up and tear down methods. It also allows some simple assertions and is easily extendable for other use cases. It wraps some of the fff functionality too.

Both of this is obviously not required for the initial reason of this PR. But I'd like to provide unit tests for the features that I suggest to introduce where possible. So I'd appreciate some feedback on that too. If you'd prefer a PR without or different tests please let me know.

You can run the test irq_cpp locally or on the CI to test the implementation.

Please feel free to give feedback or suggest improvements!

maribu

Thanks for your PR, this looks pretty good to me. I added some comments inline.

sys/include/cppunit/fff.h

dist/tools/doccheck/exclude_patterns

core/include/irq.hpp

maribu · 2021-10-28T08:03:52Z

core/include/irq.hpp

+        irq_restore(state);
+    }
+
+    irq_lock(irq_lock const& irq) = delete;


Note to myself: Check if it is possible to get rid of the bogus "keyword 'delete' not followed by a single space" warning of the CI.

sys/include/cppunit/cpp_test.hpp

tests/irq_cpp/irq.h

core/include/irq.hpp

pkg/fff/Kconfig

tests/irq_cpp/main.cpp

tests/irq_cpp/tests/01-run.py

aabadie · 2021-10-28T09:33:36Z

This PR also introduces the fff package. I'd like to also see a test application in C, just for it.

jenswet · 2021-10-28T13:48:29Z

This PR also introduces the fff package. I'd like to also see a test application in C, just for it.

@aabadie I've added a test fff_test that shows a basic test with an irq mock in C language. Is that enough or would you like to see more?

aabadie · 2021-10-28T13:56:56Z

I've added a test fff_test that shows a basic test with an irq mock in C language. Is that enough or would you like to see more?

Thanks! I think the addition of the fff package and the test application could into their own separate PR to simplify the review.

aabadie · 2021-10-28T13:59:51Z

Also make sure that you read our CONTRIBUTING guidelines and especially the section about commit conventions and about fixup commits.

benpicco · 2023-01-13T00:55:32Z

This needs a squashing, right?

maribu · 2023-01-13T07:23:32Z

Yes. I fear that squashing done by anyone else except the author results in the PR being closes (likely a security feature to avoid sneaking in unrelated code while obfuscating the commit history / authorship). At least this was what happened the last two times a maintainer squashed a PR she/he didn't open.

But then again, I could just open a new PR if that happens and keep the correct authorship in the commit meta data.

benpicco · 2023-01-13T07:49:20Z

it worked for #12353 😇

jenswet · 2023-01-13T07:52:37Z

Hey guys

I am sorry for the long delay in communication. I was a few months abroad without regular access to my computer. I will make the PR ready for merging asap in the next weeks.

maribu · 2023-01-13T12:59:27Z

@jenswet: Thx :) I the squashing (and the disabling of ESP8266) was the only thing missing. Let's see if we can get this in now.

bors merge

bors · 2023-01-13T12:59:30Z

🕐 Waiting for PR status (GitHub check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set.

riot-ci · 2023-01-13T14:32:06Z

Murdock results

✔️ PASSED

a9c5987 core/irq: Add C++ wrapper

Success	Failures	Total	Runtime
6779	0	6779	12m:36s

Artifacts

Documentation preview

maribu · 2023-01-15T17:13:47Z

bors retry

bors · 2023-01-15T17:13:50Z

🕐 Waiting for PR status (GitHub check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set.

maribu · 2023-01-15T17:14:01Z

bors cancel
bors merge

bors · 2023-01-15T17:14:03Z

🕐 Waiting for PR status (GitHub check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set.

maribu · 2023-01-15T18:13:48Z

bors cancel

maribu · 2023-01-15T18:13:55Z

bors merge

maribu · 2023-01-15T18:14:52Z

bors r+

maribu · 2023-01-15T18:14:58Z

bors retry

maribu · 2023-01-15T18:15:24Z

bors ping

bors · 2023-01-15T18:16:44Z

pong

bors · 2023-01-15T18:16:47Z

Already running a review

bors · 2023-01-15T18:16:48Z

Already running a review

bors · 2023-01-15T21:02:20Z

Build succeeded:

maribu · 2023-01-15T21:28:53Z

🎉

@jenswet: Thanks for your patience :) I'm confident that the next PR should get in much quicker :)

jenswet requested review from MichelRottleuthner, aabadie, fjmolinas, kaspar030, leandrolanzieri, miri64 and smlng as code owners October 27, 2021 14:52

github-actions bot added Area: core Area: RIOT kernel. Handle PRs marked with this with care! Area: doc Area: Documentation Area: sys Area: System Area: tests Area: tests and testing framework labels Oct 27, 2021

jenswet mentioned this pull request Oct 27, 2021

github/pr: Fix link to coding conventions #17067

Merged

benpicco requested a review from maribu October 27, 2021 18:33

github-actions bot added the Area: tools Area: Supplementary tools label Oct 28, 2021

maribu reviewed Oct 28, 2021

View reviewed changes

miri64 reviewed Oct 28, 2021

View reviewed changes

core/include/irq.hpp Outdated Show resolved Hide resolved

jenswet requested review from cgundogan and jia200x as code owners October 28, 2021 09:21

github-actions bot added Area: Kconfig Area: Kconfig integration Area: pkg Area: External package ports and removed Area: tools Area: Supplementary tools labels Oct 28, 2021

jenswet commented Oct 28, 2021

View reviewed changes

pkg/fff/Kconfig Outdated Show resolved Hide resolved

aabadie reviewed Oct 28, 2021

View reviewed changes

tests/irq_cpp/main.cpp Show resolved Hide resolved

aabadie reviewed Oct 28, 2021

View reviewed changes

tests/irq_cpp/tests/01-run.py Outdated Show resolved Hide resolved

aabadie reviewed Oct 28, 2021

View reviewed changes

tests/irq_cpp/tests/01-run.py Outdated Show resolved Hide resolved

jenswet requested a review from maribu October 28, 2021 13:53

benpicco removed this from the Release 2022.10 milestone Jan 13, 2023

maribu force-pushed the feature/irq-api-cpp branch from c6f920a to 467d61a Compare January 13, 2023 12:56

core/irq: Add C++ wrapper

a9c5987

maribu force-pushed the feature/irq-api-cpp branch from 467d61a to a9c5987 Compare January 13, 2023 12:58

benpicco added Area: pkg Area: External package ports and removed Area: pkg Area: External package ports labels Jan 15, 2023

bors bot merged commit 1b352d6 into RIOT-OS:master Jan 15, 2023

jia200x added this to the Release 2023.04 milestone Apr 25, 2023

leandrolanzieri mentioned this pull request Jun 13, 2024

Makefile.include: fix override headers from application #20744

Merged

Conversation

jenswet commented Oct 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Uh oh!

maribu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maribu Oct 28, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aabadie commented Oct 28, 2021

Uh oh!

jenswet commented Oct 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aabadie commented Oct 28, 2021

Uh oh!

aabadie commented Oct 28, 2021

Uh oh!

benpicco commented Jan 13, 2023

Uh oh!

maribu commented Jan 13, 2023

Uh oh!

benpicco commented Jan 13, 2023

Uh oh!

jenswet commented Jan 13, 2023

Uh oh!

maribu commented Jan 13, 2023

Uh oh!

bors bot commented Jan 13, 2023

Uh oh!

riot-ci commented Jan 13, 2023

Murdock results

Artifacts

Uh oh!

maribu commented Jan 15, 2023

Uh oh!

bors bot commented Jan 15, 2023

Uh oh!

maribu commented Jan 15, 2023

Uh oh!

bors bot commented Jan 15, 2023

Uh oh!

maribu commented Jan 15, 2023

Uh oh!

maribu commented Jan 15, 2023

Uh oh!

maribu commented Jan 15, 2023

Uh oh!

maribu commented Jan 15, 2023

Uh oh!

maribu commented Jan 15, 2023

Uh oh!

bors bot commented Jan 15, 2023

Uh oh!

bors bot commented Jan 15, 2023

Uh oh!

bors bot commented Jan 15, 2023

Uh oh!

bors bot commented Jan 15, 2023

Uh oh!

maribu commented Jan 15, 2023

Uh oh!

Reviewers

jenswet commented Oct 27, 2021 •

edited

Loading

jenswet commented Oct 28, 2021 •

edited

Loading