iox-#1982 FreeRTOS platform implementation #1983

JakubSosnovec · 2023-05-10T08:20:18Z

Pre-Review Checklist for the PR Author

Notes for Reviewer

Changes

Add freertos iceoryx platform implementation.
Add iceoryx_dust cmake configuration for constants in NamedPipe, for further memory consumption optimization.
Fix a few inconsistencies with integer types that appeared as warnings when building for 32-bit systems:
- int32_t -> int for file descriptors
- size_t -> uint64_t for vector sizes/indices
- Use std::thread::native_handle_type instead of pthread types, for the cases when the native handle has different type.

Explanation

Adding this new platform implementation is unfortunately a bit more complicated than with the normal operating systems such as Linux. We tried to make this implementation is general and generic as possible, however, it just wasnt possible always.

Apart from the standard FreeRTOS kernel (https://freertos.org/RTOS.html), this implementation depends on the official FreeRTOS+POSIX extension (https://freertos.org/FreeRTOS-Plus/FreeRTOS_Plus_POSIX/index.html) and additionally on a little third-party C++ threading library for FreeRTOS (https://github.com/grygorek/FreeRTOS_cpp11).

In order to test this implementation easily, we made this "integration" repository: https://github.com/NXPHoverGames/iceoryx_freertos It basically brings all of the dependencies as well as iceoryx together, provides the CMake build system and compiler toolchain (mostly taken from FreeRTOS_cpp11) in order to be able to build for a particular platform and execute it in an emulator environment (QEMU). Moreover, all of these steps can be easily reproduced because they are wrapped in Docker container image.

For further information and background, I recommend to read the README in our integration repo: https://github.com/NXPHoverGames/iceoryx_freertos/blob/main/README.md as well as the README in the C++ threading lib: https://github.com/grygorek/FreeRTOS_cpp11/blob/master/README.md

Checklist for the PR Reviewer

Commits are properly organized and messages are according to the guideline
Code according to our coding style and naming conventions
Unit tests have been written for new behavior
Public API changes are documented via doxygen
Copyright owner are updated in the changed files
All touched (C/C++) source code files from iceoryx_hoofs have been added to ./clang-tidy-diff-scans.txt
PR title describes the changes

Post-review Checklist for the PR Author

All open points are addressed and tracked via issues

References

Closes Implement iceoryx platform for FreeRTOS #1982

JakubSosnovec · 2023-05-10T08:25:49Z

Hi all! I have now finally got the green light to open this long-promised FreeRTOS platform implementation.

Please do review carefully and ask any and all questions that you have! Also, if you think it could be beneficial, I would be happy to jump on a call with you guys (perhaps the bi-weekly iox call?) in order to properly explain everything and answer any more complex and involved things and questions.

elfenpiff · 2023-05-10T09:32:58Z

@JakubSosnovec awesome! As soon as I have time I will look at it!

elBoberido

Just an initial brief look at the code. Did not yet look at all the platform files

iceoryx_dust/BUILD.bazel

iceoryx_dust/include/iceoryx_dust/internal/cxx/convert.inl

...yx_hoofs/include/iceoryx_hoofs/internal/posix_wrapper/shared_memory_object/shared_memory.hpp

iceoryx_hoofs/include/iceoryx_hoofs/posix_wrapper/thread.hpp

iceoryx_hoofs/posix/design/include/iox/file_management_interface.hpp

iceoryx_hoofs/primitives/include/iox/algorithm.hpp

iceoryx_hoofs/source/posix_wrapper/posix_access_rights.cpp

elBoberido · 2023-05-11T12:53:09Z

@JakubSosnovec regarding the iceoryx developer meetup today. All of us have an important meeting at that time and we will only be available from 17:30. So if you want to join today keep in mind that it starts later

JakubSosnovec · 2023-05-11T13:02:31Z

@JakubSosnovec regarding the iceoryx developer meetup today. All of us have an important meeting at that time and we will only be available from 17:30. So if you want to join today keep in mind that it starts later

@elBoberido Unfortunately today I wont be able to join at all.

elBoberido · 2023-05-11T13:21:28Z

@JakubSosnovec no problem. Is the regular meeting in two weeks soon enough for you or would you like to have something sooner?

JakubSosnovec · 2023-05-11T13:24:19Z

@JakubSosnovec no problem. Is the regular meeting in two weeks soon enough for you or would you like to have something sooner?

I think that is fine for me :)

elBoberido · 2023-06-05T10:06:32Z

@JakubSosnovec can you please run clang-format on the changed files?

elBoberido · 2023-06-14T16:24:22Z

@JakubSosnovec okay, there is still an issue left. Could you please add csignal to tools/used-headers.txt? I have the feeling we should get rid of that file but that's something for an other PR.

elBoberido

From my POV just a few minor things. Sorry for the slow review :)

...yx_hoofs/include/iceoryx_hoofs/internal/posix_wrapper/shared_memory_object/shared_memory.hpp

iceoryx_hoofs/source/posix_wrapper/posix_access_rights.cpp

iceoryx_hoofs/include/iceoryx_hoofs/posix_wrapper/thread.hpp

iceoryx_hoofs/primitives/include/iox/algorithm.hpp

JakubSosnovec · 2023-07-07T07:53:09Z

@JakubSosnovec okay, there is still an issue left. Could you please add csignal to tools/used-headers.txt? I have the feeling we should get rid of that file but that's something for an other PR.

Its already there :( https://github.com/eclipse-iceoryx/iceoryx/blob/master/tools/scripts/used-headers.txt#L37

Edit: Oh, I guess I need to add it to the # QNX platform / libc headers category. I will try that.
Edit2: Hmm, now its still failing, even though I updated the used-headers.txt file. The script output doesnt match the new file. Not sure whats going on.

elBoberido · 2023-07-10T17:53:53Z

@JakubSosnovec oh, my bad. Please remove all csignal from tools/used-headers.txt. You removed that header from process_manager.cpp and it seems it was not used elsewhere.

elBoberido · 2023-07-26T11:12:23Z

@JakubSosnovec I hope our slow response has not discouraged you :)

codecov · 2023-07-26T14:45:54Z

Codecov Report

Merging #1983 (190291a) into master (90996dc) will increase coverage by 0.26%.
Report is 114 commits behind head on master.
The diff coverage is 84.61%.

@@            Coverage Diff             @@
##           master    #1983      +/-   ##
==========================================
+ Coverage   74.05%   74.31%   +0.26%     
==========================================
  Files         404      414      +10     
  Lines       15924    16086     +162     
  Branches     2243     2249       +6     
==========================================
+ Hits        11793    11955     +162     
+ Misses       3422     3416       -6     
- Partials      709      715       +6

Flag	Coverage Δ
unittests	`74.10% <84.61%> (+0.37%)`	⬆️
unittests_timing	`14.89% <23.07%> (+0.14%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
...dust/include/iceoryx_dust/internal/cxx/convert.inl	`41.42% <ø> (ø)`
.../include/iceoryx_dust/posix_wrapper/named_pipe.hpp	`100.00% <ø> (ø)`
...ceoryx_hoofs/internal/posix_wrapper/posix_call.inl	`100.00% <ø> (ø)`
...fs/internal/posix_wrapper/shared_memory_object.hpp	`87.50% <ø> (ø)`
...six_wrapper/shared_memory_object/shared_memory.hpp	`100.00% <ø> (ø)`
...oofs/internal/posix_wrapper/unix_domain_socket.hpp	`100.00% <ø> (ø)`
...ofs/include/iceoryx_hoofs/posix_wrapper/thread.hpp	`100.00% <ø> (ø)`
iceoryx_hoofs/primitives/include/iox/algorithm.hpp	`92.85% <ø> (ø)`
iceoryx_hoofs/source/concurrent/loffli.cpp	`87.87% <ø> (+3.03%)`	⬆️
..._hoofs/source/posix_wrapper/unix_domain_socket.cpp	`61.66% <ø> (ø)`
... and 11 more

... and 37 files with indirect coverage changes

elfenpiff

Lets fix the comma and get it merged.

elfenpiff · 2023-07-27T07:44:09Z

iceoryx_dust/BUILD.bazel


 cc_library(
    name = "iceoryx_dust",
    srcs = glob([
        "source/**/*.cpp",
        "source/**/*.hpp",
    ]),
-    hdrs = glob(["include/**"] + glob(["vocabulary/**"])),
+    hdrs = glob(["include/**"] + glob(["vocabulary/**"])) + [,


I think you have to remove the comma here to get the bazel CI running again.

The coma in the next line probably also :)

@elBoberido coma ... sounds like you made a @budrus and this is not allowed under the Cobra License ;) ... Please correct your typo to something like cmoma

elfenpiff · 2023-07-27T08:01:08Z

@JakubSosnovec you did a great job here. Now we have freertos support that is awesome, we only require a CI job for it and we are done.
I will try to take care of it after my 3 weeks vacation.

elBoberido

Almost done. Thanks for your effort

elBoberido · 2023-07-27T10:08:57Z

iceoryx_dust/BUILD.bazel


 cc_library(
    name = "iceoryx_dust",
    srcs = glob([
        "source/**/*.cpp",
        "source/**/*.hpp",
    ]),
-    hdrs = glob(["include/**"] + glob(["vocabulary/**"])),
+    hdrs = glob(["include/**"] + glob(["vocabulary/**"])) + [,


The coma in the next line probably also :)

elBoberido

Looks good. Updating the release notes would be nice but that can also be done in a follow up. Could be merged from my point of view

iox-eclipse-iceoryx#1982 FreeRTOS platform implementation

562e6e3

JakubSosnovec marked this pull request as ready for review May 10, 2023 08:24

elfenpiff requested review from elfenpiff and elBoberido May 10, 2023 09:33

mossmaurice assigned JakubSosnovec May 10, 2023

mossmaurice added the enhancement New feature label May 10, 2023

elBoberido reviewed May 10, 2023

View reviewed changes

JakubSosnovec added 2 commits May 12, 2023 11:35

iox-eclipse-iceoryx#1982 Fix bazel dust build configuration defaults

324e613

iox-eclipse-iceoryx#1982 Introduce sharedMemoryHandle_t

9b7dcd2

iox-eclipse-iceoryx#1982 Run clang-format over FreeRTOS platform changes

b47a677

JakubSosnovec force-pushed the iox-1982-freertos-platform branch from 03d2c52 to b47a677 Compare June 9, 2023 08:15

elBoberido reviewed Jun 30, 2023

View reviewed changes

iox-eclipse-iceoryx#1982 Fix alias naming and used-headers.txt

d1a3f58

iox-eclipse-iceoryx#1982 Remove csignal from used-headers.txt

0f0ace6

elfenpiff reviewed Jul 27, 2023

View reviewed changes

elBoberido reviewed Jul 27, 2023

View reviewed changes

iox-eclipse-iceoryx#1982 Fix dust BAZEL build

190291a

elfenpiff approved these changes Jul 27, 2023

View reviewed changes

elBoberido approved these changes Jul 27, 2023

View reviewed changes

elfenpiff merged commit 7a5d86b into eclipse-iceoryx:master Jul 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

iox-#1982 FreeRTOS platform implementation #1983

iox-#1982 FreeRTOS platform implementation #1983

JakubSosnovec commented May 10, 2023 •

edited by elfenpiff

Loading

JakubSosnovec commented May 10, 2023

elfenpiff commented May 10, 2023

elBoberido left a comment

elBoberido commented May 11, 2023

JakubSosnovec commented May 11, 2023

elBoberido commented May 11, 2023

JakubSosnovec commented May 11, 2023

elBoberido commented Jun 5, 2023

elBoberido commented Jun 14, 2023

elBoberido left a comment

JakubSosnovec commented Jul 7, 2023 •

edited

Loading

elBoberido commented Jul 10, 2023

elBoberido commented Jul 26, 2023

codecov bot commented Jul 26, 2023 •

edited

Loading

elfenpiff left a comment

elfenpiff Jul 27, 2023

elBoberido Jul 27, 2023

elfenpiff Jul 27, 2023 •

edited

Loading

elfenpiff commented Jul 27, 2023

elBoberido left a comment

elBoberido Jul 27, 2023

elBoberido left a comment

iox-#1982 FreeRTOS platform implementation #1983

iox-#1982 FreeRTOS platform implementation #1983

Conversation

JakubSosnovec commented May 10, 2023 • edited by elfenpiff Loading

Pre-Review Checklist for the PR Author

Notes for Reviewer

Changes

Explanation

Checklist for the PR Reviewer

Post-review Checklist for the PR Author

References

JakubSosnovec commented May 10, 2023

elfenpiff commented May 10, 2023

elBoberido left a comment

Choose a reason for hiding this comment

elBoberido commented May 11, 2023

JakubSosnovec commented May 11, 2023

elBoberido commented May 11, 2023

JakubSosnovec commented May 11, 2023

elBoberido commented Jun 5, 2023

elBoberido commented Jun 14, 2023

elBoberido left a comment

Choose a reason for hiding this comment

JakubSosnovec commented Jul 7, 2023 • edited Loading

elBoberido commented Jul 10, 2023

elBoberido commented Jul 26, 2023

codecov bot commented Jul 26, 2023 • edited Loading

Codecov Report

elfenpiff left a comment

Choose a reason for hiding this comment

elfenpiff Jul 27, 2023

Choose a reason for hiding this comment

elBoberido Jul 27, 2023

Choose a reason for hiding this comment

elfenpiff Jul 27, 2023 • edited Loading

Choose a reason for hiding this comment

elfenpiff commented Jul 27, 2023

elBoberido left a comment

Choose a reason for hiding this comment

elBoberido Jul 27, 2023

Choose a reason for hiding this comment

elBoberido left a comment

Choose a reason for hiding this comment

JakubSosnovec commented May 10, 2023 •

edited by elfenpiff

Loading

JakubSosnovec commented Jul 7, 2023 •

edited

Loading

codecov bot commented Jul 26, 2023 •

edited

Loading

elfenpiff Jul 27, 2023 •

edited

Loading