Fixes to: by drensber · Pull Request #31 · micro-ROS/rcutils · GitHub

 - avoid compiler warnings
 - avoid unnecessary stack allocations in every log message call.
 - avoid unnecessary reference to isatty() when building in Zephyr.

Signed-off-by: David M. Rensberger <davidr@beechwoods.com>

* micro-ROS changes over dashing Feature/add security directory (micro-ROS#1) * Added security directory * Updated security directory Feature/avoid filesystem and allocation (micro-ROS#2) * Included RCUTILS_NO_FILESYSTEM and RCUTILS_AVOID_DYNAMIC_ALLOCATION * Added no filesystem options * Default allocators write access * Avoid dynamic allocation and no filesytem on error handling * Typo * New flags for filesystem and avoid dynamic * Error handling template * New allocator approach Add test_security_directory test from rcl. (micro-ROS#3) Merge pull request micro-ROS#4 from micro-ROS/feature/zephyr_fixes Feature/zephyr fixes CMake refactor (micro-ROS#5) Update approach (micro-ROS#6) * Update approach * Remove target_compile_definitions and refactor flags install * Added RCUTILS_NO_FILESYSTEM on new functions * Added RCUTILS_NO_FILESYSTEM on new functions Co-authored-by: Pablo Garrido <pablogs9@gmail.com> Updates 17092020 Fix atomics 64bits (micro-ROS#9) * micro-ROS changes over dashing Feature/add security directory (micro-ROS#1) * Added security directory * Updated security directory Feature/avoid filesystem and allocation (micro-ROS#2) * Included RCUTILS_NO_FILESYSTEM and RCUTILS_AVOID_DYNAMIC_ALLOCATION * Added no filesystem options * Default allocators write access * Avoid dynamic allocation and no filesytem on error handling * Typo * New flags for filesystem and avoid dynamic * Error handling template * New allocator approach Add test_security_directory test from rcl. (micro-ROS#3) Merge pull request micro-ROS#4 from micro-ROS/feature/zephyr_fixes Feature/zephyr fixes CMake refactor (micro-ROS#5) Update approach (micro-ROS#6) * Update approach * Remove target_compile_definitions and refactor flags install * Added RCUTILS_NO_FILESYSTEM on new functions * Added RCUTILS_NO_FILESYSTEM on new functions Co-authored-by: Pablo Garrido <pablogs9@gmail.com> * Initial changes * Add hashing and lock pool * Updates Co-authored-by: Jose Antonio Moral <joseantoniomoralparras@gmail.com> Fix atomics 64bits (micro-ROS#9) Updates 09102020 * Release micro-ROS Foxy (micro-ROS#8) Update Cleaning Update Update filesystem Updates Adjust logger level Remove build warning (micro-ROS#10) * Avoid not used warnings * Update Reduce error handling static size (micro-ROS#14) (micro-ROS#15) This reverts commit befc608. Reduce error handling static size (micro-ROS#14) (micro-ROS#15) Signed-off-by: Pablo Garrido <pablogs9@gmail.com> (cherry picked from commit 1176652) Co-authored-by: Pablo Garrido <pablogs9@gmail.com> Revert "Revert "Install headers to include\${PROJECT_NAME} (ros2#351)"" This reverts commit 4546892. Fix atomic 64 b description (micro-ROS#17) (micro-ROS#18) (cherry picked from commit 85efa4a) Co-authored-by: Pablo Garrido <pablogs9@gmail.com> Add fork checker for humble Signed-off-by: Pablo Garrido <pablogs9@gmail.com> Update to iron (micro-ROS#27) Signed-off-by: acuadros95 <acuadros1995@gmail.com>

(cherry picked from commit 55de2e1) Co-authored-by: Błażej Sowa <bsowa123@gmail.com>

- avoid compiler warnings - avoid unnecessary stack allocations in every log message call. - avoid unnecessary reference to isatty() when building in Zephyr. Signed-off-by: David M. Rensberger <davidr@beechwoods.com>

Hello @drensber I have further review your changes and beyond ones in src/error_handling_helpers.h I guess that the others shall be done in the rcutils original repo, then they will be backported to micro-ROS automatically.

Suggested change

      
            #endif
          
            #if !defined(RCUTILS_AVOID_DYNAMIC_ALLOCATION)

Irrelevant please remove

These changes shall be done in the original repo: https://github.com/ros2/rcutils

Remember that this is a fork

These changes shall be done in the original repo: https://github.com/ros2/rcutils

Remember that this is a fork

I could try, but this is a change that really mostly makes sense in the context of small embedded systems. I'm not sure that the main ros2 project would see much reason for a change of this nature.

You are changing the variable from stack to static section, right?

Yes, that's what I'm doing. I could try submitting it to the ros2 repo. There shouldn't technically be anything /wrong/ with doing it that way even on more capable hardware that can handle the overhead.

I tried submitting this to the ros2 repository. They said they can't accept the change that makes the buffer a global (static) variable, because that function is called by multiple threads in a ros2 instance. Does micro-ROS potentially have the same problem, or is micro-ROS guaranteed to be single-threaded with respect to callers of rcutils_logging_console_output_handler()?

They can accept the other change (removing the dependency on isatty() for Zephyr builds), so I'll go ahead and push that to the upstream ros2 repo.

micro-ROS potentially can be used in a threaded environment in some cases with some configuration, so we have the very same problem.

OK, then I think the only safe way to do it is dynamic heap allocation (malloc). Could possibly also use locking to prevent more than one thread from calling the function at a time. Do you have any idea if this is a Zephyr-specific issue?

For now, I think I'll leave the buffer allocation changes out of my PR, since no one else seems to have a problem with stack allocation. We'll continue to patch for our own purposes. Doyon have any comments on any of the other changes? It looks like they passed CI this time.

…t cooresponds with the #endif

… repo instead

Is there anything else I need to do here?

pablogs9 and others added 3 commits February 16, 2024 08:44

Don't link dl library when not needed (micro-ROS#28 ) (micro-ROS#29 )

a292fc9

(cherry picked from commit 55de2e1) Co-authored-by: Błażej Sowa <bsowa123@gmail.com>

Fixes to:

008022a

- avoid compiler warnings - avoid unnecessary stack allocations in every log message call. - avoid unnecessary reference to isatty() when building in Zephyr. Signed-off-by: David M. Rensberger <davidr@beechwoods.com>

pablogs9 reviewed Mar 7, 2024

View reviewed changes

Comment thread src/logging.c Outdated

Comment thread src/strcasecmp.c Outdated

reverting 'strings.h' inclusion

2740e70

pablogs9 requested changes Mar 12, 2024

View reviewed changes

drensber added 2 commits March 12, 2024 15:42

no reason to #endif immediately followed by #if of the same thing tha…

f41591a

…t cooresponds with the #endif

Reverting these changes so that they can be made in the upstream ros2…

fe55987

… repo instead

pablogs9 force-pushed the iron branch from a292fc9 to d3f0889 Compare April 22, 2024 05:35

		static char msg_buf[1024] = "";
		static char output_buf[1024] = "";

		(void)one_semantic;
		(void)zero_semantic;

Fixes to: by drensber · Pull Request #31 · micro-ROS/rcutils · GitHub

Fixes to: - #31

Fixes to:#31drensber wants to merge 6 commits intomicro-ROS:ironmicro-ROS/rcutils:ironfrom drensber:bws_patches_2drensber/rcutils:bws_patches_2Copy head branch name to clipboard

Conversation

drensber commented Mar 6, 2024

Uh oh!

Uh oh!

Uh oh!

pablogs9 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

drensber commented Apr 2, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Fixes to:#31
drensber wants to merge 6 commits into
micro-ROS:ironfrom
drensber:bws_patches_2