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/04/20 16:37:01 UTC
[GitHub] [incubator-nuttx] davids5 opened a new pull request #3580: arch: move up_irq{save|restore} to irq.h
davids5 opened a new pull request #3580:
URL: https://github.com/apache/incubator-nuttx/pull/3580
## Summary
Fixes https://github.com/apache/incubator-nuttx/pull/2566#discussion_r616836971
## Impact
## 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] davids5 commented on pull request #3580: arch: move up_irq{save|restore} to irq.h
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #3580:
URL: https://github.com/apache/incubator-nuttx/pull/3580#issuecomment-823442151
@btashton I agree!
Here I what I can say...
I was working on up-taking NuttX master for PX4 and it showed up. (lost in 1000s of format specifier issues)
We have `-Wfatal-errors` and some driver code in PX4 do include arch.h and I would assume irq.h, but the error does not tell me where the conflicting definition was.
I have several changes to upstream for the format specifier issues, then I will push the PX4 build on NuttX master ish....
What else would help you?
--
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] gustavonihei commented on a change in pull request #3580: arch: move up_irq{save|restore} to irq.h
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3580:
URL: https://github.com/apache/incubator-nuttx/pull/3580#discussion_r616928005
##########
File path: arch/risc-v/include/bl602/irq.h
##########
@@ -224,8 +224,49 @@ extern "C"
* Public Function Prototypes
****************************************************************************/
+/****************************************************************************
+ * Name: up_irq_enable
+ *
+ * Description:
+ * Return the current interrupt state and enable interrupts
+ *
+ ****************************************************************************/
+
EXTERN irqstate_t up_irq_enable(void);
+/****************************************************************************
+ * Name: up_irq_save
+ *
+ * Description:
+ * Save the current interrupt state and disable interrupts.
+ *
+ * Input Parameters:
+ * None
+ *
+ * Returned Value:
+ * Interrupt state prior to disabling interrupts.
+ *
+ ****************************************************************************/
+
+EXTERN irqstate_t up_irq_save(void);
Review comment:
This seems a bit weird.
`up_irq_save` is already declared at `arch/risc-v/include/irq.h`, which is already included in `bl602_irq.c` as `<arch/irq.h>`.
If this patch is really necessary, there might be something wrong with the include paths.
https://github.com/apache/incubator-nuttx/blob/418e11b8b38855eacf55deea8d658c5760b51488/arch/risc-v/include/irq.h#L71
--
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] gustavonihei commented on a change in pull request #3580: arch: move up_irq{save|restore} to irq.h
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #3580:
URL: https://github.com/apache/incubator-nuttx/pull/3580#discussion_r616928005
##########
File path: arch/risc-v/include/bl602/irq.h
##########
@@ -224,8 +224,49 @@ extern "C"
* Public Function Prototypes
****************************************************************************/
+/****************************************************************************
+ * Name: up_irq_enable
+ *
+ * Description:
+ * Return the current interrupt state and enable interrupts
+ *
+ ****************************************************************************/
+
EXTERN irqstate_t up_irq_enable(void);
+/****************************************************************************
+ * Name: up_irq_save
+ *
+ * Description:
+ * Save the current interrupt state and disable interrupts.
+ *
+ * Input Parameters:
+ * None
+ *
+ * Returned Value:
+ * Interrupt state prior to disabling interrupts.
+ *
+ ****************************************************************************/
+
+EXTERN irqstate_t up_irq_save(void);
Review comment:
This seems a bit weird.
`up_irq_save` is already declared at `arch/risc-v/include/irq.h`, which is already included in `bl602_irq.c` as `<arch/irq.h>`.
If this patch is really necessary, there might be something wrong with the include paths.
--
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] gustavonihei commented on pull request #3580: arch: move up_irq{save|restore} to irq.h
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on pull request #3580:
URL: https://github.com/apache/incubator-nuttx/pull/3580#issuecomment-824030006
> @gustavonihei @btashton - how should we proceed?
I believe that those function declarations on bl602/irq.h are not necessary and should be removed. Those function are already declared on the RISC-V generic header, which is shared among other chips.
If removing them triggers another error, then we should fix the root cause.
--
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] davids5 edited a comment on pull request #3580: arch: move up_irq{save|restore} to irq.h
Posted by GitBox <gi...@apache.org>.
davids5 edited a comment on pull request #3580:
URL: https://github.com/apache/incubator-nuttx/pull/3580#issuecomment-823442151
@btashton I agree!
Here is what I can say...
I was working on up-taking NuttX master for PX4 and it showed up. (lost in 1000s of format specifier issues)
We have `-Wfatal-errors` and some driver code in PX4 do include arch.h and I would assume irq.h, but the error does not tell me where the conflicting definition was.
I have several changes to upstream for the format specifier issues, then I will push the PX4 build on NuttX master ish....
What else would help you?
--
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] acassis merged pull request #3580: arch: move up_irq{save|restore} to irq.h
Posted by GitBox <gi...@apache.org>.
acassis merged pull request #3580:
URL: https://github.com/apache/incubator-nuttx/pull/3580
--
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] davids5 commented on a change in pull request #3580: arch: move up_irq{save|restore} to irq.h
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #3580:
URL: https://github.com/apache/incubator-nuttx/pull/3580#discussion_r616956499
##########
File path: arch/risc-v/include/bl602/irq.h
##########
@@ -224,8 +224,49 @@ extern "C"
* Public Function Prototypes
****************************************************************************/
+/****************************************************************************
+ * Name: up_irq_enable
+ *
+ * Description:
+ * Return the current interrupt state and enable interrupts
+ *
+ ****************************************************************************/
+
EXTERN irqstate_t up_irq_enable(void);
+/****************************************************************************
+ * Name: up_irq_save
+ *
+ * Description:
+ * Save the current interrupt state and disable interrupts.
+ *
+ * Input Parameters:
+ * None
+ *
+ * Returned Value:
+ * Interrupt state prior to disabling interrupts.
+ *
+ ****************************************************************************/
+
+EXTERN irqstate_t up_irq_save(void);
Review comment:
@gustavonihei - it may very well be that the added lines in arch/risc-v/include/bl602/irq.h are not needed but the inclusion into arch.h is conflicting with the arch specific deceleration.
--
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] davids5 commented on pull request #3580: arch: move up_irq{save|restore} to irq.h
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #3580:
URL: https://github.com/apache/incubator-nuttx/pull/3580#issuecomment-824011776
@gustavonihei @btashton - how should we proceed?
--
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] davids5 commented on pull request #3580: arch: move up_irq{save|restore} to irq.h
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #3580:
URL: https://github.com/apache/incubator-nuttx/pull/3580#issuecomment-824066329
> > @gustavonihei @btashton - how should we proceed?
>
> I believe that those function declarations on bl602/irq.h are not necessary and should be removed. Those function are already declared on the RISC-V generic header, which is shared among other chips.
> If removing them triggers another error, then we should fix the root cause.
@gustavonihei - thank you. Let's See how CI feels about 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] btashton commented on pull request #3580: arch: move up_irq{save|restore} to irq.h
Posted by GitBox <gi...@apache.org>.
btashton commented on pull request #3580:
URL: https://github.com/apache/incubator-nuttx/pull/3580#issuecomment-823427595
While I agree, I want to get to the bottom of why this caused issues for you and none of the other builds so that we can prevent it from breaking 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