You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2021/12/14 09:16:08 UTC
[GitHub] [incubator-nuttx] OctopusHu0105 opened a new pull request #4999: sched_note: change uint32_t nbi_module into uint8_t array
OctopusHu0105 opened a new pull request #4999:
URL: https://github.com/apache/incubator-nuttx/pull/4999
sched_note: change uint32_t nbi_module into uint8_t array, because variables keep the same style that use uint8_t array
sched_note: add note dump module tag definitions
NOTE_MODULE(a, b, c, d)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #4999: sched_note: change uint32_t nbi_module into uint8_t array
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #4999:
URL: https://github.com/apache/incubator-nuttx/pull/4999#discussion_r768458332
##########
File path: include/nuttx/sched_note.h
##########
@@ -99,6 +99,15 @@
memset((s), 0, sizeof(struct note_filter_irq_s))
#endif
+/* Note dump module tag definitions */
+
+#ifdef CONFIG_SCHED_INSTRUMENTATION_DUMP
+#define NOTE_MODULE(a, b, c, d) \
Review comment:
```suggestion
# define NOTE_MODULE(a, b, c, d) \
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] OctopusHu0105 commented on a change in pull request #4999: sched_note: change uint32_t nbi_module into uint8_t array
Posted by GitBox <gi...@apache.org>.
OctopusHu0105 commented on a change in pull request #4999:
URL: https://github.com/apache/incubator-nuttx/pull/4999#discussion_r770283136
##########
File path: include/nuttx/sched_note.h
##########
@@ -99,6 +99,15 @@
memset((s), 0, sizeof(struct note_filter_irq_s))
#endif
+/* Note dump module tag definitions */
+
+#ifdef CONFIG_SCHED_INSTRUMENTATION_DUMP
+# define NOTE_MODULE(a, b, c, d) \
+ ((unsigned)(a) | ((unsigned)(b) << 8) | ((unsigned)(c) << 16) | ((d) << 24))
Review comment:
@ you are right
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #4999: sched_note: change uint32_t nbi_module into uint8_t array
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #4999:
URL: https://github.com/apache/incubator-nuttx/pull/4999#discussion_r768534482
##########
File path: include/nuttx/sched_note.h
##########
@@ -99,6 +99,15 @@
memset((s), 0, sizeof(struct note_filter_irq_s))
#endif
+/* Note dump module tag definitions */
+
+#ifdef CONFIG_SCHED_INSTRUMENTATION_DUMP
+#define NOTE_MODULE(a, b, c, d) \
Review comment:
So that is from `apps` repo right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #4999: sched_note: change uint32_t nbi_module into uint8_t array
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #4999:
URL: https://github.com/apache/incubator-nuttx/pull/4999#discussion_r769387932
##########
File path: include/nuttx/sched_note.h
##########
@@ -336,7 +345,7 @@ struct note_string_s
struct note_binary_s
{
struct note_common_s nbi_cmn; /* Common note parameters */
- uint32_t nbi_module; /* Module number */
+ uint8_t nbi_module[4]; /* Module number */
Review comment:
Yes, but there is not benefit of doing it. Both `void sched_note_dump(uint32_t module, uint8_t event, FAR const void *buf, size_t len)` and `void sched_note_vbprintf(uint32_t module, uint8_t event, FAR const char *fmt, va_list va)` take `uint32_t module` as a parameter and it is naturally to store it into a same type placeholder. Even `NOTE_MODULE` macro creates an integer value. So why do we need this particular change?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #4999: sched_note: change uint32_t nbi_module into uint8_t array
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #4999:
URL: https://github.com/apache/incubator-nuttx/pull/4999#discussion_r768460202
##########
File path: include/nuttx/sched_note.h
##########
@@ -99,6 +99,15 @@
memset((s), 0, sizeof(struct note_filter_irq_s))
#endif
+/* Note dump module tag definitions */
+
+#ifdef CONFIG_SCHED_INSTRUMENTATION_DUMP
+#define NOTE_MODULE(a, b, c, d) \
Review comment:
Where `NOTE_MODULE` macro is used?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #4999: sched_note: change uint32_t nbi_module into uint8_t array
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #4999:
URL: https://github.com/apache/incubator-nuttx/pull/4999#discussion_r770264057
##########
File path: include/nuttx/sched_note.h
##########
@@ -99,6 +99,15 @@
memset((s), 0, sizeof(struct note_filter_irq_s))
#endif
+/* Note dump module tag definitions */
+
+#ifdef CONFIG_SCHED_INSTRUMENTATION_DUMP
+# define NOTE_MODULE(a, b, c, d) \
+ ((unsigned)(a) | ((unsigned)(b) << 8) | ((unsigned)(c) << 16) | ((d) << 24))
Review comment:
```suggestion
# define NOTE_MODULE(a, b, c, d) \
(((uint32_t)(a) & 0xff)| (((uint32_t)(b) & 0xff) << 8) | (((uint32_t)(c) & 0xff) << 16) | ((uint32_t)(d) << 24))
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] OctopusHu0105 commented on a change in pull request #4999: sched_note: change uint32_t nbi_module into uint8_t array
Posted by GitBox <gi...@apache.org>.
OctopusHu0105 commented on a change in pull request #4999:
URL: https://github.com/apache/incubator-nuttx/pull/4999#discussion_r770326109
##########
File path: include/nuttx/sched_note.h
##########
@@ -99,6 +99,18 @@
memset((s), 0, sizeof(struct note_filter_irq_s))
#endif
+/* Note dump module tag definitions */
+
+#ifdef CONFIG_SCHED_INSTRUMENTATION_DUMP
+# define NOTE_MODULE(a, b, c, d) \
+ ((uint32_t)(a & 0xff) | \
+ ((uint32_t)(b & 0xff) << 8) | \
+ ((uint32_t)(c & 0xff) << 16) | \
+ ((uint32_t)(d & 0xff) << 24))
Review comment:
great
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] OctopusHu0105 commented on a change in pull request #4999: sched_note: change uint32_t nbi_module into uint8_t array
Posted by GitBox <gi...@apache.org>.
OctopusHu0105 commented on a change in pull request #4999:
URL: https://github.com/apache/incubator-nuttx/pull/4999#discussion_r768606535
##########
File path: include/nuttx/sched_note.h
##########
@@ -99,6 +99,15 @@
memset((s), 0, sizeof(struct note_filter_irq_s))
#endif
+/* Note dump module tag definitions */
+
+#ifdef CONFIG_SCHED_INSTRUMENTATION_DUMP
+#define NOTE_MODULE(a, b, c, d) \
Review comment:
yes, i will have a new PR for "NOTE_MODULE" in apps.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 merged pull request #4999: sched_note: change uint32_t nbi_module into uint8_t array
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #4999:
URL: https://github.com/apache/incubator-nuttx/pull/4999
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4999: sched_note: change uint32_t nbi_module into uint8_t array
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4999:
URL: https://github.com/apache/incubator-nuttx/pull/4999#discussion_r769604209
##########
File path: include/nuttx/sched_note.h
##########
@@ -336,7 +345,7 @@ struct note_string_s
struct note_binary_s
{
struct note_common_s nbi_cmn; /* Common note parameters */
- uint32_t nbi_module; /* Module number */
+ uint8_t nbi_module[4]; /* Module number */
Review comment:
Another reason is to ensure that the saved data can be parsed by the machine with the different endian.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] OctopusHu0105 commented on a change in pull request #4999: sched_note: change uint32_t nbi_module into uint8_t array
Posted by GitBox <gi...@apache.org>.
OctopusHu0105 commented on a change in pull request #4999:
URL: https://github.com/apache/incubator-nuttx/pull/4999#discussion_r768492111
##########
File path: include/nuttx/sched_note.h
##########
@@ -99,6 +99,15 @@
memset((s), 0, sizeof(struct note_filter_irq_s))
#endif
+/* Note dump module tag definitions */
+
+#ifdef CONFIG_SCHED_INSTRUMENTATION_DUMP
+#define NOTE_MODULE(a, b, c, d) \
Review comment:
@pkarashchenko
#define TEST_MODULE NOTE_MODULE('t', 'e', 's', 't')
sched_note_dump(TEST_MODULE, event, data, len);
#define AUDIO_MODULE NOTE_MODULE('a', 'u', 'd', 'i')
sched_note_dump(AUDIO_MODULE, event, data, len);
or
sched_note_dump(NOTE_MODULE('a', 'u', 'd', 'i'), event, data, len);
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #4999: sched_note: change uint32_t nbi_module into uint8_t array
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #4999:
URL: https://github.com/apache/incubator-nuttx/pull/4999#discussion_r769666090
##########
File path: include/nuttx/sched_note.h
##########
@@ -336,7 +345,7 @@ struct note_string_s
struct note_binary_s
{
struct note_common_s nbi_cmn; /* Common note parameters */
- uint32_t nbi_module; /* Module number */
+ uint8_t nbi_module[4]; /* Module number */
Review comment:
@xiaoxiang781216 agree! That is 100% valid. This item can be resoled and only fix of `NOTE_MODULE` is remaining
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] OctopusHu0105 commented on a change in pull request #4999: sched_note: change uint32_t nbi_module into uint8_t array
Posted by GitBox <gi...@apache.org>.
OctopusHu0105 commented on a change in pull request #4999:
URL: https://github.com/apache/incubator-nuttx/pull/4999#discussion_r768604993
##########
File path: include/nuttx/sched_note.h
##########
@@ -99,6 +99,15 @@
memset((s), 0, sizeof(struct note_filter_irq_s))
#endif
+/* Note dump module tag definitions */
+
+#ifdef CONFIG_SCHED_INSTRUMENTATION_DUMP
+# define NOTE_MODULE(a, b, c, d) \
+ ((unsigned)(a) | ((unsigned)(b) << 8) | ((unsigned)(c) << 16) | ((d) << 24))
Review comment:
@pkarashchenko , Integer promotions
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #4999: sched_note: change uint32_t nbi_module into uint8_t array
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #4999:
URL: https://github.com/apache/incubator-nuttx/pull/4999#discussion_r770262566
##########
File path: include/nuttx/sched_note.h
##########
@@ -99,6 +99,15 @@
memset((s), 0, sizeof(struct note_filter_irq_s))
#endif
+/* Note dump module tag definitions */
+
+#ifdef CONFIG_SCHED_INSTRUMENTATION_DUMP
+# define NOTE_MODULE(a, b, c, d) \
+ ((unsigned)(a) | ((unsigned)(b) << 8) | ((unsigned)(c) << 16) | ((d) << 24))
Review comment:
Actually if any of `a`, `b` or `c` are not byte size them result value will be wrong.
If you want to write reliable macro you need to add `& 0xff` to each argument.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #4999: sched_note: change uint32_t nbi_module into uint8_t array
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #4999:
URL: https://github.com/apache/incubator-nuttx/pull/4999#discussion_r769359176
##########
File path: include/nuttx/sched_note.h
##########
@@ -99,6 +99,15 @@
memset((s), 0, sizeof(struct note_filter_irq_s))
#endif
+/* Note dump module tag definitions */
+
+#ifdef CONFIG_SCHED_INSTRUMENTATION_DUMP
+# define NOTE_MODULE(a, b, c, d) \
+ ((unsigned)(a) | ((unsigned)(b) << 8) | ((unsigned)(c) << 16) | ((d) << 24))
Review comment:
yes. current variant has a bug when `(unsigned)(a)` is promoted (that is not needed), but `((d) << 24)` is not.
additionally in `void sched_note_vbprintf(uint32_t module, uint8_t event, FAR const char *fmt, va_list va)` the `module` is `uint32_t`, so there is not reason to use `unsigned` for promotion
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] OctopusHu0105 commented on a change in pull request #4999: sched_note: change uint32_t nbi_module into uint8_t array
Posted by GitBox <gi...@apache.org>.
OctopusHu0105 commented on a change in pull request #4999:
URL: https://github.com/apache/incubator-nuttx/pull/4999#discussion_r770283418
##########
File path: include/nuttx/sched_note.h
##########
@@ -99,6 +99,15 @@
memset((s), 0, sizeof(struct note_filter_irq_s))
#endif
+/* Note dump module tag definitions */
+
+#ifdef CONFIG_SCHED_INSTRUMENTATION_DUMP
+# define NOTE_MODULE(a, b, c, d) \
+ ((unsigned)(a) | ((unsigned)(b) << 8) | ((unsigned)(c) << 16) | ((d) << 24))
Review comment:
@pkarashchenko good idea
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] OctopusHu0105 commented on a change in pull request #4999: sched_note: change uint32_t nbi_module into uint8_t array
Posted by GitBox <gi...@apache.org>.
OctopusHu0105 commented on a change in pull request #4999:
URL: https://github.com/apache/incubator-nuttx/pull/4999#discussion_r768586138
##########
File path: include/nuttx/sched_note.h
##########
@@ -336,7 +345,7 @@ struct note_string_s
struct note_binary_s
{
struct note_common_s nbi_cmn; /* Common note parameters */
- uint32_t nbi_module; /* Module number */
+ uint8_t nbi_module[4]; /* Module number */
Review comment:
@pkarashchenko variables keep the same style, only use uint8_t in struct of sched_note.h.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] OctopusHu0105 commented on a change in pull request #4999: sched_note: change uint32_t nbi_module into uint8_t array
Posted by GitBox <gi...@apache.org>.
OctopusHu0105 commented on a change in pull request #4999:
URL: https://github.com/apache/incubator-nuttx/pull/4999#discussion_r770185981
##########
File path: include/nuttx/sched_note.h
##########
@@ -99,6 +99,15 @@
memset((s), 0, sizeof(struct note_filter_irq_s))
#endif
+/* Note dump module tag definitions */
+
+#ifdef CONFIG_SCHED_INSTRUMENTATION_DUMP
+# define NOTE_MODULE(a, b, c, d) \
+ ((unsigned)(a) | ((unsigned)(b) << 8) | ((unsigned)(c) << 16) | ((d) << 24))
Review comment:
@pkarashchenko If `(a)` is negative number，`((a) | *********) ` will promote wrong data.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #4999: sched_note: change uint32_t nbi_module into uint8_t array
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #4999:
URL: https://github.com/apache/incubator-nuttx/pull/4999#discussion_r770286764
##########
File path: include/nuttx/sched_note.h
##########
@@ -99,6 +99,18 @@
memset((s), 0, sizeof(struct note_filter_irq_s))
#endif
+/* Note dump module tag definitions */
+
+#ifdef CONFIG_SCHED_INSTRUMENTATION_DUMP
+# define NOTE_MODULE(a, b, c, d) \
+ ((uint32_t)(a & 0xff) | \
+ ((uint32_t)(b & 0xff) << 8) | \
+ ((uint32_t)(c & 0xff) << 16) | \
+ ((uint32_t)(d & 0xff) << 24))
Review comment:
```suggestion
((uint32_t)((a) & 0xff) | \
((uint32_t)((b) & 0xff) << 8) | \
((uint32_t)((c) & 0xff) << 16) | \
((uint32_t)((d) & 0xff) << 24))
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #4999: sched_note: change uint32_t nbi_module into uint8_t array
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #4999:
URL: https://github.com/apache/incubator-nuttx/pull/4999#discussion_r768565390
##########
File path: include/nuttx/sched_note.h
##########
@@ -336,7 +345,7 @@ struct note_string_s
struct note_binary_s
{
struct note_common_s nbi_cmn; /* Common note parameters */
- uint32_t nbi_module; /* Module number */
+ uint8_t nbi_module[4]; /* Module number */
Review comment:
Based on provided use case description I do not see why we need change `uint32_t nbi_module;` to `uint8_t nbi_module[4];`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #4999: sched_note: change uint32_t nbi_module into uint8_t array
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #4999:
URL: https://github.com/apache/incubator-nuttx/pull/4999#discussion_r768563199
##########
File path: include/nuttx/sched_note.h
##########
@@ -99,6 +99,15 @@
memset((s), 0, sizeof(struct note_filter_irq_s))
#endif
+/* Note dump module tag definitions */
+
+#ifdef CONFIG_SCHED_INSTRUMENTATION_DUMP
+# define NOTE_MODULE(a, b, c, d) \
+ ((unsigned)(a) | ((unsigned)(b) << 8) | ((unsigned)(c) << 16) | ((d) << 24))
Review comment:
```suggestion
((a) | ((uint32_t)(b) << 8) | ((uint32_t)(c) << 16) | ((uint32_t)(d) << 24))
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org