[bugfix] Modify dlog output code #191

songgot · 2023-10-18T05:22:34Z

When outputting dlog using dlogutil in Tizen, garbage values are output in the format starting with %

taos-ci · 2023-10-18T05:22:37Z

📝 TAOS-CI Version: 1.5.20200925. Thank you for submitting PR #191. Please a submit 1commit/1PR (one commit per one PR) policy to get comments quickly from reviewers. Your PR must pass all verificiation processes of cibot before starting a review process from reviewers. If you are new member to join this project, please read manuals in documentation folder and wiki page. In order to monitor a progress status of your PR in more detail, visit http://ci.nnstreamer.ai/.

taos-ci · 2023-10-18T05:29:09Z

cibot: @songgot, A builder checker could not be completed because one of the checkers is not completed. In order to find out a reason, please go to http://ci.nnstreamer.ai/nnstreamer-edge/ci/repo-workers/pr-checker/191-202310181422380.47395205497742-d1ee31d0e679a1c478a49fdec65d8ddb30dca408/.

taos-ci

@songgot, 💯 All CI checkers are successfully verified. Thanks.

myungjoo · 2023-10-20T00:56:22Z

src/libnnstreamer-edge/nnstreamer-edge-aitt.h

@@ -14,6 +14,7 @@
 #define __NNSTREAMER_EDGE_AITT_H__

 #include "nnstreamer-edge.h"
+#include "nnstreamer-edge-log.h"


I don't see any symbols defined in nnstreamer-edge-log.h in this header file.

If there are .c files using nnstreamer-edge-log.h symbols, add include in .c files, not in .h file.

Thank you.
#include <aitt_c.h>, line 26 in this file
The two values below must be redefined and used. This value is defined in <dlog.h> in nnstreamer-edge-log.h.

[ 41s] In file included from /usr/include/aitt/aitt_c.h:24, [ 41s] from /home/abuild/rpmbuild/BUILD/nnstreamer-edge-0.2.5/src/libnnstreamer-edge/nnstreamer-edge-aitt.h:25, [ 41s] from /home/abuild/rpmbuild/BUILD/nnstreamer-edge-0.2.5/src/libnnstreamer-edge/nnstreamer-edge-aitt.c:19: [ 41s] /usr/include/aitt/AittTypes.h:71:26: error: expected identifier before numeric constant [ 41s] 71 | #define TIZEN_ERROR_NONE 0 [ 41s] | ^ [ 41s] In file included from /home/abuild/rpmbuild/BUILD/nnstreamer-edge-0.2.5/src/libnnstreamer-edge/nnstreamer-edge-log.h:26, [ 41s] from /home/abuild/rpmbuild/BUILD/nnstreamer-edge-0.2.5/src/libnnstreamer-edge/nnstreamer-edge-aitt.c:23: [ 41s] /usr/include/dlog/dlog.h:72:29: error: 'TIZEN_ERROR_NOT_PERMITTED' undeclared here (not in a function); did you mean 'TIZEN_ERROR_NOT_SUPPORTED'? [ 41s] 72 | DLOG_ERROR_NOT_PERMITTED = TIZEN_ERROR_NOT_PERMITTED /**< Operation not permitted */ [ 41s] | ^~~~~~~~~~~~~~~~~~~~~~~~~ [ 41s] | TIZEN_ERROR_NOT_SUPPORTED [ 41s] make[2]: *** [src/CMakeFiles/nnstreamer-edge.dir/build.make:191: src/CMakeFiles/nnstreamer-edge.dir/libnnstreamer-edge/nnstreamer-edge-aitt.c.o] Error 1

Do you mean that there is a symbol in nnstreamer-edge-aitt.h that refers to aitt_c.h or not?

If yes: include aitt_c.h
If no: don't include aitt_c.h.

Or do you mean that there is a symbol in aitt_c.h that requires including another header before including aitt_c.h?

If yes: fix aitt_c.h or do workaround right before aitt_c.h with proper comments so that we can remove it later. And report to AITT or submit a PR to AITT to fix it.
If no: ?

okay, thank you.

The AITT representative asked me to use a pc file or add -DTIZEN. We are not using aitt pc in CMakeList.txt.
so I will add -DTIZEN

@jaeyun-jung posted a related PR(#196). so I will remove -DTIZEN

src/libnnstreamer-edge/nnstreamer-edge-log.h

taos-ci

@songgot, 💯 All CI checkers are successfully verified. Thanks.

jaeyun-jung · 2023-11-06T06:15:18Z

Suggestion: nns_edge_set_log_level() and nns_edge_log_level_e in nnstreamer-edge.h

When the macro for log message is moved to header (for tizen and android), set-log-level and the enumeration would be deleted because we cannot set the logging level.

I suggest deleting logging function in nnstreamer-edge.h, if this PR is necessary.

songgot · 2023-11-06T09:12:44Z

Okay, thank you.

taos-ci

@songgot, 💯 All CI checkers are successfully verified. Thanks.

songgot · 2023-11-10T02:19:41Z

Do you have any comments about this PR?

gichan-jang · 2023-11-17T06:51:02Z

include/nnstreamer-edge.h

- * @brief Set the logging level. Default value is NNS_EDGE_LOG_INFO.
- * @param[in] level The log level to print out.
- */
-void nns_edge_set_log_level (nns_edge_log_level_e level);


Just remove this function? How can we set log level after this?

Operation is possible without nns_edge_set_log_level().

gichan-jang · 2023-11-17T06:57:18Z

To be honest, I don't know the intention of this change..
Can you give me brief explanation?

wooksong · 2023-11-20T06:57:56Z

src/libnnstreamer-edge/nnstreamer-edge-util.c

@@ -170,7 +169,7 @@ nns_edge_malloc (nns_size_t size)
    mem = malloc (size);

  if (!mem)
-    nns_edge_loge ("Failed to allocate memory (%llu).", size);
+    nns_edge_loge ("Failed to allocate memory");


Is there any reason to change this line?

%lu or %llu is required when building in 32bit and 64bit with gbs.
I tried to solve it with glib API, but nnstreamer-edge does not have glib dependency.
There is also a way to use the C library.
Since there was only one log, I agreed with @gichan-jang to delete it for now.

nnstreamer-edge/include/nnstreamer-edge.h

Line 28 in ded7b09

typedef uint64_t nns_size_t;

If it was size_t, you may use %zu, but that's just uint64_t. You may refer to https://stackoverflow.com/questions/30092872/how-to-print-uint64-t-in-gcc too.

A major NNStreamer VoC is that error messages do not give you enough info. Please do not reduce info in error messages. Please add more info.

Okay, thank you.
https://github.com/openbsd/src/blob/master/include/inttypes.h

wooksong · 2023-11-20T07:07:13Z

src/CMakeLists.txt

    ${NNS_EDGE_SRC_DIR}/nnstreamer-edge-util.c
    ${NNS_EDGE_SRC_DIR}/nnstreamer-edge-queue.c
 )
+IF (NOT ENABLE_TIZEN)
+    SET(NNS_EDGE_SRCS ${NNS_EDGE_SRCS} ${NNS_EDGE_SRC_DIR}/nnstreamer-edge-log.c)


Why is the nnstreamer-edge-log.c excluded when ENABLE_TIZEN is off?

If not using Tizen or Android, nns_edge_print_log() in nnstreamer-edge-log.c is used.

taos-ci

@songgot, 💯 All CI checkers are successfully verified. Thanks.

taos-ci · 2023-12-04T09:51:55Z

cibot: @songgot, A builder checker could not be completed because one of the checkers is not completed. In order to find out a reason, please go to http://ci.nnstreamer.ai/nnstreamer-edge/ci/repo-workers/pr-checker/191-202312041844230.5980179309845-862b7fc85ee6e680ae95e6cce9f01305cc957d30/.

taos-ci

@songgot, 💯 All CI checkers are successfully verified. Thanks.

When outputting dlog using dlogutil in Tizen, garbage values are output in the format starting with % Signed-off-by: hyunil park <[email protected]>

taos-ci

@songgot, 💯 All CI checkers are successfully verified. Thanks.

songgot requested review from myungjoo, jijoongmoon, again4you, jaeyun-jung, anyj0527 and gichan-jang as code owners October 18, 2023 05:22

songgot force-pushed the fix_dlog branch 2 times, most recently from 60ef94d to 41c4382 Compare October 18, 2023 07:07

taos-ci approved these changes Oct 18, 2023

View reviewed changes

myungjoo reviewed Oct 20, 2023

View reviewed changes

jaeyun-jung reviewed Oct 30, 2023

View reviewed changes

src/libnnstreamer-edge/nnstreamer-edge-log.h Show resolved Hide resolved

songgot force-pushed the fix_dlog branch from 41c4382 to 6c51afe Compare November 2, 2023 04:48

taos-ci approved these changes Nov 2, 2023

View reviewed changes

songgot force-pushed the fix_dlog branch from 6c51afe to 400dd58 Compare November 7, 2023 02:31

taos-ci approved these changes Nov 7, 2023

View reviewed changes

gichan-jang reviewed Nov 17, 2023

View reviewed changes

wooksong reviewed Nov 20, 2023

View reviewed changes

songgot force-pushed the fix_dlog branch from 400dd58 to 9b36f8e Compare December 4, 2023 08:18

taos-ci approved these changes Dec 4, 2023

View reviewed changes

songgot force-pushed the fix_dlog branch from 9b36f8e to 862b7fc Compare December 4, 2023 09:44

songgot changed the title ~~[bugfix] Modify dlog output code~~ [Wait for #196][bugfix] Modify dlog output code Dec 4, 2023

songgot force-pushed the fix_dlog branch from 862b7fc to 118bf4c Compare December 6, 2023 04:05

taos-ci approved these changes Dec 6, 2023

View reviewed changes

songgot changed the title ~~[Wait for #196][bugfix] Modify dlog output code~~ [bugfix] Modify dlog output code Dec 6, 2023

jaeyun-jung approved these changes Dec 6, 2023

View reviewed changes

songgot force-pushed the fix_dlog branch from 118bf4c to 7feb594 Compare December 6, 2023 08:19

[bugfix] Modify dlog output code

690ee08

When outputting dlog using dlogutil in Tizen, garbage values are output in the format starting with % Signed-off-by: hyunil park <[email protected]>

songgot force-pushed the fix_dlog branch from 7feb594 to 690ee08 Compare December 7, 2023 01:25

taos-ci approved these changes Dec 7, 2023

View reviewed changes

myungjoo approved these changes Dec 13, 2023

View reviewed changes

jaeyun-jung merged commit b71723c into nnstreamer:main Dec 14, 2023
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[bugfix] Modify dlog output code #191

[bugfix] Modify dlog output code #191

songgot commented Oct 18, 2023

taos-ci commented Oct 18, 2023

taos-ci commented Oct 18, 2023

taos-ci left a comment

myungjoo Oct 20, 2023

songgot Oct 23, 2023 •

edited

Loading

myungjoo Nov 6, 2023

songgot Nov 17, 2023

songgot Dec 4, 2023

songgot Dec 4, 2023 •

edited

Loading

taos-ci left a comment

jaeyun-jung commented Nov 6, 2023

songgot commented Nov 6, 2023

taos-ci left a comment

songgot commented Nov 10, 2023

gichan-jang Nov 17, 2023

songgot Dec 4, 2023

gichan-jang commented Nov 17, 2023

wooksong Nov 20, 2023

songgot Dec 4, 2023

myungjoo Dec 6, 2023

myungjoo Dec 6, 2023

songgot Dec 6, 2023 •

edited

Loading

wooksong Nov 20, 2023

songgot Dec 4, 2023

taos-ci left a comment

taos-ci commented Dec 4, 2023

taos-ci left a comment

taos-ci left a comment

[bugfix] Modify dlog output code #191

[bugfix] Modify dlog output code #191

Conversation

songgot commented Oct 18, 2023

taos-ci commented Oct 18, 2023

taos-ci commented Oct 18, 2023

taos-ci left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

songgot Oct 23, 2023 • edited Loading

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

songgot Dec 4, 2023 • edited Loading

Choose a reason for hiding this comment

taos-ci left a comment

Choose a reason for hiding this comment

jaeyun-jung commented Nov 6, 2023

songgot commented Nov 6, 2023

taos-ci left a comment

Choose a reason for hiding this comment

songgot commented Nov 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gichan-jang commented Nov 17, 2023

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

songgot Dec 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taos-ci left a comment

Choose a reason for hiding this comment

taos-ci commented Dec 4, 2023

taos-ci left a comment

Choose a reason for hiding this comment

taos-ci left a comment

Choose a reason for hiding this comment

songgot Oct 23, 2023 •

edited

Loading

songgot Dec 4, 2023 •

edited

Loading

songgot Dec 6, 2023 •

edited

Loading