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 2022/12/07 07:23:20 UTC
[GitHub] [nuttx] PetervdPerk-NXP opened a new pull request, #7809: Systemview make note function weak for co-existance with other note drivers
PetervdPerk-NXP opened a new pull request, #7809:
URL: https://github.com/apache/nuttx/pull/7809
## Summary
When enabling SEGGER Systemview it can conflict with other note drivers trying to hook into `sched_note_xxx`.
This patch makes the `sched_note_xxx` symbols weakly defined and decouples them from the function so that other note drives can nicely co-exist with SEGGER Systemview.
## Impact
Minimal, should have no impact on normal circumstances.
## Testing
Tested on PX4 with their own note driver implementation which can co-exist with SEGGER systemview.
--
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] [nuttx] xiaoxiang781216 commented on pull request #7809: Systemview make note function weak for co-existance with other note drivers
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #7809:
URL: https://github.com/apache/nuttx/pull/7809#issuecomment-1340955236
@PetervdPerk-NXP do you plan to call system_view_ function directly?
--
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] [nuttx] davids5 commented on a diff in pull request #7809: Systemview make note function weak for co-existance with other note drivers
Posted by GitBox <gi...@apache.org>.
davids5 commented on code in PR #7809:
URL: https://github.com/apache/nuttx/pull/7809#discussion_r1043193334
##########
drivers/segger/note_sysview.c:
##########
@@ -275,7 +276,12 @@ static inline int sysview_isenabled_syscall(int nr)
*
****************************************************************************/
-void sched_note_start(FAR struct tcb_s *tcb)
+void weak_function sched_note_start(FAR struct tcb_s *tcb)
+{
+ sysview_note_start(tcb);
+}
+
+void inline sysview_note_start(FAR struct tcb_s *tcb)
Review Comment:
I still think it cleaner to have a KCOFIG option for the sysview module to a prefix
```
#if !defined(CONFIG_SYSTEMVIEW_PREFIX)
# define sysview_prefix
#else
# define sysview_prefix CONFIG_SYSTEMVIEW_PREFIX
// or define sysview_prefix define sysview
#endif
sysview_prefix_sched_note_resume(FAR struct tcb_s *tcb)
```
--
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] [nuttx] davids5 commented on pull request #7809: Systemview add kconfig SEGGER_SYSVIEW_PREFIX for co-existance with other note drivers
Posted by GitBox <gi...@apache.org>.
davids5 commented on PR #7809:
URL: https://github.com/apache/nuttx/pull/7809#issuecomment-1343007512
@pkarashchenko Any further input?
--
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] [nuttx] PetervdPerk-NXP commented on a diff in pull request #7809: Systemview make note function weak for co-existance with other note drivers
Posted by GitBox <gi...@apache.org>.
PetervdPerk-NXP commented on code in PR #7809:
URL: https://github.com/apache/nuttx/pull/7809#discussion_r1043178896
##########
drivers/segger/note_sysview.c:
##########
@@ -275,7 +276,12 @@ static inline int sysview_isenabled_syscall(int nr)
*
****************************************************************************/
-void sched_note_start(FAR struct tcb_s *tcb)
+void weak_function sched_note_start(FAR struct tcb_s *tcb)
+{
+ sysview_note_start(tcb);
+}
+
+void inline sysview_note_start(FAR struct tcb_s *tcb)
Review Comment:
The idea to have the ability to overule the `sched_note_start` function but still be able to use the sysview functionality.
For instance nx_start calls `sched_note_start` e.g.
https://github.com/apache/nuttx/blob/fdc1aa6ae31bd37b232622b8169ef47f100661ee/sched/init/nx_start.c#L634
Normal un-modified behaviour
```mermaid
flowchart LR
A[nx_start] -->|calls| B(sched_note_start)
subgraph note_sysview.c
B -->|inline| C(sysview_note_start)
end
```
Behaviour that hooks into `sched_note_start` does it own handling and calls `sysview_note_start` as well.
```mermaid
flowchart LR
A[nx_start] -->|calls| B(sched_note_start)
subgraph custom_note_driver
B --> C(custom_note_start)
end
subgraph note_sysview.c
C -->|calls| D(sysview_note_start)
end
```
--
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] [nuttx] davids5 commented on a diff in pull request #7809: Systemview make note function weak for co-existance with other note drivers
Posted by GitBox <gi...@apache.org>.
davids5 commented on code in PR #7809:
URL: https://github.com/apache/nuttx/pull/7809#discussion_r1043193334
##########
drivers/segger/note_sysview.c:
##########
@@ -275,7 +276,12 @@ static inline int sysview_isenabled_syscall(int nr)
*
****************************************************************************/
-void sched_note_start(FAR struct tcb_s *tcb)
+void weak_function sched_note_start(FAR struct tcb_s *tcb)
+{
+ sysview_note_start(tcb);
+}
+
+void inline sysview_note_start(FAR struct tcb_s *tcb)
Review Comment:
I still think it cleaner to have a KCOFIG option for the sysview module to a prefix
```
#if !defined(CONFIG_SYSTEMVIEW_PREFIX)
# define sysview_prefix
#else
# define sysview_prefix CONFIG_SYSTEMVIEW_PREFIX
// or define sysview_prefix define sysview
#endif
sysview_prefix_sched_note_resume(FAR struct tcb_s *tcb)
```
--
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] [nuttx] PetervdPerk-NXP commented on pull request #7809: Systemview make note function weak for co-existance with other note drivers
Posted by GitBox <gi...@apache.org>.
PetervdPerk-NXP commented on PR #7809:
URL: https://github.com/apache/nuttx/pull/7809#issuecomment-1340958957
The idea is it too call from the PX4 performance monitoring unit note driver and then hook-in systemview when it's enabled.
https://github.com/PX4/PX4-Autopilot/blob/4267691697a965457bbe4c2fd404f19717f7c0a9/platforms/nuttx/src/px4/common/cpuload.cpp#L141
--
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] [nuttx] xiaoxiang781216 commented on pull request #7809: Systemview make note function weak for co-existance with other note drivers
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #7809:
URL: https://github.com/apache/nuttx/pull/7809#issuecomment-1340951291
The key problem is that sched note doesn't support the multiple implementation like the syslog channel. @Gary-Hobson will improve the sched note like syslog in the next couple week. Before that happen, let's merge your patch first.
--
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] [nuttx] davids5 commented on pull request #7809: Systemview make note function weak for co-existance with other note drivers
Posted by GitBox <gi...@apache.org>.
davids5 commented on PR #7809:
URL: https://github.com/apache/nuttx/pull/7809#issuecomment-1340996121
Can we contrast this to a CONFIG_SYSTEMVIEW_PREFIX approach to avoid the namespace collision and make it callable?
--
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] [nuttx] pkarashchenko commented on a diff in pull request #7809: Systemview make note function weak for co-existance with other note drivers
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #7809:
URL: https://github.com/apache/nuttx/pull/7809#discussion_r1043101284
##########
drivers/segger/note_sysview.h:
##########
@@ -0,0 +1,65 @@
+/****************************************************************************
+ * drivers/segger/note_sysview.h
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership. The
+ * ASF licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the
+ * License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ *
+ ****************************************************************************/
+
+#ifndef __DRIVERS_SEGGER_SYSVIEW_H
+#define __DRIVERS_SEGGER_SYSVIEW_H
+
+/****************************************************************************
+ * Public Function Prototypes
+ ****************************************************************************/
+
+void sysview_note_start(FAR struct tcb_s *tcb);
+
+void sysview_note_stop(FAR struct tcb_s *tcb);
+
+void sysview_note_suspend(FAR struct tcb_s *tcb);
+
+void sysview_note_resume(FAR struct tcb_s *tcb);
+
+#ifdef CONFIG_SCHED_INSTRUMENTATION_IRQHANDLER
+void sysview_note_irqhandler(int irq, FAR void *handler, bool enter);
+#endif
+
+#ifdef CONFIG_SCHED_INSTRUMENTATION_SYSCALL
+void sysview_note_syscall_enter(int nr);
+void sysview_note_syscall_leave(int nr, uintptr_t result);
+#endif
+
+#ifdef CONFIG_SCHED_INSTRUMENTATION_FILTER
+void sysview_note_filter_mode(struct note_filter_mode_s *oldm,
+ struct note_filter_mode_s *newm);
+#endif
+
+#ifdef CONFIG_SCHED_INSTRUMENTATION_IRQHANDLER
+void sysview_note_filter_irq(struct note_filter_irq_s *oldf,
+ struct note_filter_irq_s *newf);
+#endif
+
+#ifdef CONFIG_SCHED_INSTRUMENTATION_IRQHANDLER
+void sysview_note_filter_irq(struct note_filter_irq_s *oldf,
+ struct note_filter_irq_s *newf);
+#endif
+
+#ifdef CONFIG_SCHED_INSTRUMENTATION_SYSCALL
+void sysview_note_filter_syscall(struct note_filter_syscall_s *oldf,
Review Comment:
some places have FAR and some not
##########
drivers/segger/note_sysview.c:
##########
@@ -275,7 +276,12 @@ static inline int sysview_isenabled_syscall(int nr)
*
****************************************************************************/
-void sched_note_start(FAR struct tcb_s *tcb)
+void weak_function sched_note_start(FAR struct tcb_s *tcb)
+{
+ sysview_note_start(tcb);
+}
+
+void inline sysview_note_start(FAR struct tcb_s *tcb)
Review Comment:
will inline have any effect in such usage? I mean that `sysview_note_start` declared after `sched_note_start`.
Also, why do we need `sysview_note_start` at all?
##########
drivers/segger/note_sysview.c:
##########
@@ -624,7 +716,36 @@ void sched_note_filter_irq(struct note_filter_irq_s *oldf,
****************************************************************************/
#ifdef CONFIG_SCHED_INSTRUMENTATION_SYSCALL
-void sched_note_filter_syscall(struct note_filter_syscall_s *oldf,
+void weak_function sched_note_filter_syscall(
+ struct note_filter_syscall_s *oldf,
+ struct note_filter_syscall_s *newf)
+{
+ sysview_note_filter_syscall(oldf, newf);
+}
+#endif
+
+/****************************************************************************
+ * Name: sysview_note_filter_syscall
+ *
+ * Description:
+ * Set and get syscall filter setting
+ * (Same as NOTECTL_GETSYSCALLFILTER / NOTECTL_SETSYSCALLFILTER ioctls)
+ *
+ * Input Parameters:
+ * oldf - A writable pointer to struct note_filter_syscall_s to get
+ * current syscall filter setting
+ * If 0, no data is written.
+ * newf - A read-only pointer to struct note_filter_syscall_s of the
+ * new syscall filter setting
+ * If 0, the setting is not updated.
+ *
+ * Returned Value:
+ * None
+ *
+ ****************************************************************************/
+
+#ifdef CONFIG_SCHED_INSTRUMENTATION_SYSCALL
+void inline sysview_note_filter_syscall(struct note_filter_syscall_s *oldf,
struct note_filter_syscall_s *newf)
Review Comment:
```suggestion
void inline sysview_note_filter_syscall(struct note_filter_syscall_s *oldf,
struct note_filter_syscall_s *newf)
```
--
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] [nuttx] PetervdPerk-NXP commented on a diff in pull request #7809: Systemview make note function weak for co-existance with other note drivers
Posted by GitBox <gi...@apache.org>.
PetervdPerk-NXP commented on code in PR #7809:
URL: https://github.com/apache/nuttx/pull/7809#discussion_r1043225073
##########
drivers/segger/note_sysview.c:
##########
@@ -275,7 +276,12 @@ static inline int sysview_isenabled_syscall(int nr)
*
****************************************************************************/
-void sched_note_start(FAR struct tcb_s *tcb)
+void weak_function sched_note_start(FAR struct tcb_s *tcb)
+{
+ sysview_note_start(tcb);
+}
+
+void inline sysview_note_start(FAR struct tcb_s *tcb)
Review Comment:
Change it to use Kconfig.
@davids5 @pkarashchenko is cleaner/preferrable?
--
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] [nuttx] pkarashchenko commented on a diff in pull request #7809: Systemview make note function weak for co-existance with other note drivers
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #7809:
URL: https://github.com/apache/nuttx/pull/7809#discussion_r1043240284
##########
drivers/segger/note_sysview.c:
##########
@@ -275,7 +276,12 @@ static inline int sysview_isenabled_syscall(int nr)
*
****************************************************************************/
-void sched_note_start(FAR struct tcb_s *tcb)
+void weak_function sched_note_start(FAR struct tcb_s *tcb)
+{
+ sysview_note_start(tcb);
+}
+
+void inline sysview_note_start(FAR struct tcb_s *tcb)
Review Comment:
I see that you already implemented `CONFIG_SYSTEMVIEW_PREFIX` approach.
--
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] [nuttx] PetervdPerk-NXP commented on a diff in pull request #7809: Systemview make note function weak for co-existance with other note drivers
Posted by GitBox <gi...@apache.org>.
PetervdPerk-NXP commented on code in PR #7809:
URL: https://github.com/apache/nuttx/pull/7809#discussion_r1043178896
##########
drivers/segger/note_sysview.c:
##########
@@ -275,7 +276,12 @@ static inline int sysview_isenabled_syscall(int nr)
*
****************************************************************************/
-void sched_note_start(FAR struct tcb_s *tcb)
+void weak_function sched_note_start(FAR struct tcb_s *tcb)
+{
+ sysview_note_start(tcb);
+}
+
+void inline sysview_note_start(FAR struct tcb_s *tcb)
Review Comment:
The idea to have the ability to overule the `sched_note_start` function but still be able to use the sysview functionality.
For instance nx_start calls `sched_note_start` e.g.
https://github.com/apache/nuttx/blob/fdc1aa6ae31bd37b232622b8169ef47f100661ee/sched/init/nx_start.c#L634
```mermaid
flowchart LR
A[nx_start] -->|calls| B(sched_note_start)
subgraph note_sysview.c
B -->|inline| C(sysview_note_start)
end
```
```mermaid
flowchart LR
A[nx_start] -->|calls| B(sched_note_start)
subgraph custom_note_driver
B --> C(custom_note_start)
end
subgraph note_sysview.c
C -->|calls| D(sysview_note_start)
end
```
--
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] [nuttx] pkarashchenko commented on a diff in pull request #7809: Systemview make note function weak for co-existance with other note drivers
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #7809:
URL: https://github.com/apache/nuttx/pull/7809#discussion_r1043239298
##########
drivers/segger/note_sysview.c:
##########
@@ -275,7 +276,12 @@ static inline int sysview_isenabled_syscall(int nr)
*
****************************************************************************/
-void sched_note_start(FAR struct tcb_s *tcb)
+void weak_function sched_note_start(FAR struct tcb_s *tcb)
+{
+ sysview_note_start(tcb);
+}
+
+void inline sysview_note_start(FAR struct tcb_s *tcb)
Review Comment:
@PetervdPerk-NXP Yes, I understood the idea, but need to refresh in my memory how `inline` works in C. I experimented with it long time ago and do not remember correctly. I mean do we need
```
void inline a(void)
{
}
void b(void)
{
a();
}
```
or
```
void b(void)
{
a();
}
void inline a(void)
{
}
```
I mean do we need to have `a` declared before `b` use it.
The `CONFIG_SYSTEMVIEW_PREFIX` seems to be cleaner, but most probably will require some `##` supplementary macros to be added to generate the names.
--
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] [nuttx] pkarashchenko commented on a diff in pull request #7809: Systemview make note function weak for co-existance with other note drivers
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #7809:
URL: https://github.com/apache/nuttx/pull/7809#discussion_r1043240783
##########
drivers/segger/note_sysview.c:
##########
@@ -513,7 +514,7 @@ int sysview_initialize(void)
*
****************************************************************************/
-void sched_note_filter_mode(struct note_filter_mode_s *oldm,
+void PREFIX(sched_note_filter_mode)(struct note_filter_mode_s *oldm,
struct note_filter_mode_s *newm)
Review Comment:
some alignment needs to be adjusted now
--
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] [nuttx] pkarashchenko merged pull request #7809: Systemview add kconfig SEGGER_SYSVIEW_PREFIX for co-existance with other note drivers
Posted by GitBox <gi...@apache.org>.
pkarashchenko merged PR #7809:
URL: https://github.com/apache/nuttx/pull/7809
--
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