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 2020/07/26 11:01:55 UTC
[GitHub] [incubator-nuttx] xiaoxiang781216 opened a new pull request #1461: arch/sim: Make up_internal.h includable in host environment
xiaoxiang781216 opened a new pull request #1461:
URL: https://github.com/apache/incubator-nuttx/pull/1461
## Summary
to avoid the declaration repetition
## Impact
No functionality change.
## Testing
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #1461: arch/sim: Make up_internal.h includable in host environment
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #1461:
URL: https://github.com/apache/incubator-nuttx/pull/1461#issuecomment-670674928
@yamt do you still insist to split up_internal.h to two header file? if not, please help me merge the 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #1461: arch/sim: Make up_internal.h includable in host environment
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #1461:
URL: https://github.com/apache/incubator-nuttx/pull/1461#issuecomment-667114193
> i think the problem is that sim abuses up_internal.h to share things between host and nuttx.
> i suspect it's better to introduce a dedicated header for the protocol between host and nuttx.
> how do you think?
Both solution is fine. But since it is very easy to make up_internal.h support NuttX/host at the same time, why not select the simple method?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] yamt commented on a change in pull request #1461: arch/sim: Make up_internal.h includable in host environment
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #1461:
URL: https://github.com/apache/incubator-nuttx/pull/1461#discussion_r462719559
##########
File path: arch/sim/src/sim/up_simsmp.c
##########
@@ -90,20 +77,14 @@ static pthread_t g_sim_cputhread[CONFIG_SMP_NCPUS];
* so that it will be ready for the next pause operation.
*/
-volatile spinlock_t g_cpu_wait[CONFIG_SMP_NCPUS];
-volatile spinlock_t g_cpu_paused[CONFIG_SMP_NCPUS];
+volatile bool g_cpu_wait[CONFIG_SMP_NCPUS];
+volatile bool g_cpu_paused[CONFIG_SMP_NCPUS];
Review comment:
up_smpsignal.c seems to treat these spinlock_t.
i guess you should not assume bool and uint8_t are compatible.
##########
File path: arch/sim/src/sim/up_head.c
##########
@@ -89,11 +89,10 @@ int main(int argc, char **argv, char **envp)
/* Start the CPU0 emulation. This should not return. */
sim_cpu0_start();
-#else
+#endif
/* Start the Nuttx emulation. This should not return. */
nx_start();
-#endif
}
Review comment:
how is this related to the rest of the 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] liuguo09 commented on pull request #1461: arch/sim: Make up_internal.h includable in host environment
Posted by GitBox <gi...@apache.org>.
liuguo09 commented on pull request #1461:
URL: https://github.com/apache/incubator-nuttx/pull/1461#issuecomment-678173542
The mixed case letter nxstyle error could be ignored. So I'll help to merge it.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] yamt commented on pull request #1461: arch/sim: Make up_internal.h includable in host environment
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #1461:
URL: https://github.com/apache/incubator-nuttx/pull/1461#issuecomment-666089416
i think the problem is that sim abuses up_internal.h to share things between host and nuttx.
i suspect it's better to introduce a dedicated header for the protocol between host and nuttx.
how do you think?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #1461: arch/sim: Make up_internal.h includable in host environment
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #1461:
URL: https://github.com/apache/incubator-nuttx/pull/1461#issuecomment-675577556
Ping @yamt again.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #1461: arch/sim: Make up_internal.h includable in host environment
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #1461:
URL: https://github.com/apache/incubator-nuttx/pull/1461#discussion_r463537091
##########
File path: arch/sim/src/sim/up_head.c
##########
@@ -89,11 +89,10 @@ int main(int argc, char **argv, char **envp)
/* Start the CPU0 emulation. This should not return. */
sim_cpu0_start();
-#else
+#endif
/* Start the Nuttx emulation. This should not return. */
nx_start();
-#endif
}
Review comment:
Sorry, @yamt this change is related to PR, since sim_cpu0_start is compiled in host environment, we have to delcare nx_start in arch/sim/src/sim/up_simsmp.c. So it's better to avoid call nx_start inside sim_cpu0_start.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] liuguo09 merged pull request #1461: arch/sim: Make up_internal.h includable in host environment
Posted by GitBox <gi...@apache.org>.
liuguo09 merged pull request #1461:
URL: https://github.com/apache/incubator-nuttx/pull/1461
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #1461: arch/sim: Make up_internal.h includable in host environment
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #1461:
URL: https://github.com/apache/incubator-nuttx/pull/1461#discussion_r462740772
##########
File path: arch/sim/src/sim/up_simsmp.c
##########
@@ -90,20 +77,14 @@ static pthread_t g_sim_cputhread[CONFIG_SMP_NCPUS];
* so that it will be ready for the next pause operation.
*/
-volatile spinlock_t g_cpu_wait[CONFIG_SMP_NCPUS];
-volatile spinlock_t g_cpu_paused[CONFIG_SMP_NCPUS];
+volatile bool g_cpu_wait[CONFIG_SMP_NCPUS];
+volatile bool g_cpu_paused[CONFIG_SMP_NCPUS];
Review comment:
Yes, you are right. It's better change the typedef spinlock_t to uint8_t instead of bool to improve the compatibility.
##########
File path: arch/sim/src/sim/up_head.c
##########
@@ -89,11 +89,10 @@ int main(int argc, char **argv, char **envp)
/* Start the CPU0 emulation. This should not return. */
sim_cpu0_start();
-#else
+#endif
/* Start the Nuttx emulation. This should not return. */
nx_start();
-#endif
}
Review comment:
Hmm, git rebase don't catch the confliction. I will remove it.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org