Fixes to: #31

drensber · 2024-03-06T17:06:22Z

 - 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 <[email protected]>

* 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 <[email protected]> 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 <[email protected]> * Initial changes * Add hashing and lock pool * Updates Co-authored-by: Jose Antonio Moral <[email protected]> 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 <[email protected]> (cherry picked from commit 1176652) Co-authored-by: Pablo Garrido <[email protected]> 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 <[email protected]> Add fork checker for humble Signed-off-by: Pablo Garrido <[email protected]> Update to iron (micro-ROS#27) Signed-off-by: acuadros95 <[email protected]>

(cherry picked from commit 55de2e1) Co-authored-by: Błażej Sowa <[email protected]>

- 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 <[email protected]>

src/logging.c

src/strcasecmp.c

pablogs9

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.

pablogs9 · 2024-03-12T06:27:28Z

src/error_handling_helpers.h

+#endif
+
+#if !defined(RCUTILS_AVOID_DYNAMIC_ALLOCATION)


Suggested change

#endif

#if !defined(RCUTILS_AVOID_DYNAMIC_ALLOCATION)

Irrelevant please remove

pablogs9 · 2024-03-12T06:29:14Z

src/logging.c

+  (void)one_semantic;
+  (void)zero_semantic;


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

Remember that this is a fork

pablogs9 · 2024-03-12T06:29:56Z

src/logging.c

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


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.

src/logging.c

…t cooresponds with the #endif

… repo instead

drensber · 2024-04-02T21:15:05Z

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 <[email protected]>

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 <[email protected]>

pablogs9 reviewed Mar 7, 2024

View reviewed changes

src/logging.c Outdated Show resolved Hide resolved

src/strcasecmp.c Outdated Show resolved Hide resolved

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes to: #31

Fixes to: #31

drensber commented Mar 6, 2024

pablogs9 left a comment

pablogs9 Mar 12, 2024

pablogs9 Mar 12, 2024

pablogs9 Mar 12, 2024

drensber Mar 12, 2024

pablogs9 Mar 12, 2024

drensber Mar 12, 2024

drensber Mar 19, 2024

drensber Mar 19, 2024

pablogs9 Mar 20, 2024

drensber Mar 21, 2024

drensber Mar 26, 2024

drensber commented Apr 2, 2024

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

Fixes to: #31

Are you sure you want to change the base?

Fixes to: #31

Conversation

drensber commented Mar 6, 2024

pablogs9 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drensber commented Apr 2, 2024