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/04/03 00:07:30 UTC
[GitHub] [incubator-nuttx] acassis opened a new pull request #699: Check
return of nxsem_wait_uninterruptible
acassis opened a new pull request #699: Check return of nxsem_wait_uninterruptible
URL: https://github.com/apache/incubator-nuttx/pull/699
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on issue #699: Check
return of nxsem_wait_uninterruptible
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on issue #699: Check return of nxsem_wait_uninterruptible
URL: https://github.com/apache/incubator-nuttx/pull/699#issuecomment-608202129
@acassis should we merge the change into one to ensure each patch can build correctly?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] acassis edited a comment on issue #699: Check
return of nxsem_wait_uninterruptible
Posted by GitBox <gi...@apache.org>.
acassis edited a comment on issue #699: Check return of nxsem_wait_uninterruptible
URL: https://github.com/apache/incubator-nuttx/pull/699#issuecomment-608170773
Hi @MasayukiIshikawa could you please review this patch. Mr Greg noticed that it could break the I2S logic because it could exit with the INT enabled. I want to submit an improvement to disable the INT before "return ret", but I need to verify with you first if this solution could you. Do you still having this board to perform the real test?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] acassis commented on issue #699: Check return of
nxsem_wait_uninterruptible
Posted by GitBox <gi...@apache.org>.
acassis commented on issue #699: Check return of nxsem_wait_uninterruptible
URL: https://github.com/apache/incubator-nuttx/pull/699#issuecomment-608365417
> Hi @acassis,
>
> Please fix the build break first and force to push this PR again.
> You can use lc823450-xgevk:audio configuration to check.
Hi @masayuki2009 the style changing were necessary because nxstyle was reporting error. The original lines had more than 78 chars by line. We used to break lines only when it hit 80 chars, but now without these modifications it will not pass on CI validation. I will create a single patch all modifications.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on issue #699: Check
return of nxsem_wait_uninterruptible
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on issue #699: Check return of nxsem_wait_uninterruptible
URL: https://github.com/apache/incubator-nuttx/pull/699#issuecomment-608379860
> > > > Hi @acassis,
> > > > Please fix the build break first and force to push this PR again.
> > > > You can use lc823450-xgevk:audio configuration to check.
> > >
> > >
> > > Hi @masayuki2009 the style changing were necessary because nxstyle was reporting error. The original lines had more than 78 chars by line. We used to break lines only when it hit 80 chars, but now without these modifications it will not pass on CI validation. I will create a single patch all modifications.
> >
> >
> > I think what @masayuki2009 suggesion is split the patch into two:
> > 1.The first one fix the real problem
> > 2.The second one fix nxstyle issue
> > but these two patch sent in one PR.
> > So we can review the real change more carefully.
>
> Ok, I can do it, but I suppose it could fail because nxstyle issues in the file.
No, the precheck will pull all patch in PR before starting. so the nxstyle and build is against the final source code.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] masayuki2009 commented on issue #699: Check
return of nxsem_wait_uninterruptible
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on issue #699: Check return of nxsem_wait_uninterruptible
URL: https://github.com/apache/incubator-nuttx/pull/699#issuecomment-608173483
Hi @acassis,
Please fix the build break first and force to push this PR again.
You can use lc823450-xgevk:audio configuration to check.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on issue #699: Check
return of nxsem_wait_uninterruptible
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on issue #699: Check return of nxsem_wait_uninterruptible
URL: https://github.com/apache/incubator-nuttx/pull/699#issuecomment-608372012
> @xiaoxiang781216 how to merge all these 4 commits in a single one after I submitted a PR? I didn't see a way to modify an already submitted commit. Could you please help me?
in your local branch:
git rebase --interactive
and change pick to squash
git push -f ...
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] jerpelea commented on issue #699: Check return of
nxsem_wait_uninterruptible
Posted by GitBox <gi...@apache.org>.
jerpelea commented on issue #699: Check return of nxsem_wait_uninterruptible
URL: https://github.com/apache/incubator-nuttx/pull/699#issuecomment-608377787
As a best practice I think that we should always submit nxstyle fixes and the actual patch in a PR
This will make our life easier when we review the code
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] acassis commented on issue #699: Check return of
nxsem_wait_uninterruptible
Posted by GitBox <gi...@apache.org>.
acassis commented on issue #699: Check return of nxsem_wait_uninterruptible
URL: https://github.com/apache/incubator-nuttx/pull/699#issuecomment-608380516
> As a best practice I think that we should always submit nxstyle fixes and the actual patch in a PR
> This will make our life easier when we review the code
Yes, everything in a single PR is better and this is the way Greg is doing. See PR688 for reference.
BTW, I will follow Masayuki suggestion for this PR: I will break it in separated commits, but for next PRs I will follow the original way.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on issue #699: Check
return of nxsem_wait_uninterruptible
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on issue #699: Check return of nxsem_wait_uninterruptible
URL: https://github.com/apache/incubator-nuttx/pull/699#issuecomment-608374543
> > Hi @acassis,
> > Please fix the build break first and force to push this PR again.
> > You can use lc823450-xgevk:audio configuration to check.
>
> Hi @masayuki2009 the style changing were necessary because nxstyle was reporting error. The original lines had more than 78 chars by line. We used to break lines only when it hit 80 chars, but now without these modifications it will not pass on CI validation. I will create a single patch all modifications.
I think what @masayuki2009 suggesion is split the patch into two:
1.The first one fix the real problem
2.The second one fix nxstyle issue
but these two patch sent in one PR.
So we can review the real change more carefully.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] acassis commented on issue #699: Check return of
nxsem_wait_uninterruptible
Posted by GitBox <gi...@apache.org>.
acassis commented on issue #699: Check return of nxsem_wait_uninterruptible
URL: https://github.com/apache/incubator-nuttx/pull/699#issuecomment-608376898
> > > Hi @acassis,
> > > Please fix the build break first and force to push this PR again.
> > > You can use lc823450-xgevk:audio configuration to check.
> >
> >
> > Hi @masayuki2009 the style changing were necessary because nxstyle was reporting error. The original lines had more than 78 chars by line. We used to break lines only when it hit 80 chars, but now without these modifications it will not pass on CI validation. I will create a single patch all modifications.
>
> I think what @masayuki2009 suggesion is split the patch into two:
> 1.The first one fix the real problem
> 2.The second one fix nxstyle issue
> but these two patch sent in one PR.
> So we can review the real change more carefully.
Ok, I can do it, but I suppose it could fail because nxstyle issues in the file.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo merged pull request #699: Check return
of nxsem_wait_uninterruptible
Posted by GitBox <gi...@apache.org>.
patacongo merged pull request #699: Check return of nxsem_wait_uninterruptible
URL: https://github.com/apache/incubator-nuttx/pull/699
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] masayuki2009 commented on issue #699: Check
return of nxsem_wait_uninterruptible
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on issue #699: Check return of nxsem_wait_uninterruptible
URL: https://github.com/apache/incubator-nuttx/pull/699#issuecomment-608175036
@acassis
This commit includes several coding style changes.
I prefer to add another commit (or another PR) for the style changes.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] masayuki2009 commented on issue #699: Check
return of nxsem_wait_uninterruptible
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on issue #699: Check return of nxsem_wait_uninterruptible
URL: https://github.com/apache/incubator-nuttx/pull/699#issuecomment-608179087
Hi, @acassis
Please do not add a new commit to fix the compile error but fix the original commit.
Also, as I said the above, please add a new commit for style changes.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on issue #699: Check return
of nxsem_wait_uninterruptible
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #699: Check return of nxsem_wait_uninterruptible
URL: https://github.com/apache/incubator-nuttx/pull/699#issuecomment-608177111
Hi, @masayuki2009
This needs more context. The change will be be the nxsem_wait_uninterruptible will return with the error ECANCELED if the task is canceled. It does not do that now (it hangs).
So when the function receives this error and returns, the task will exist. My concern was that if you enable and interrupt then exit, the interrupt would still fire and set the semaphore. But the task may destroy the semaphore, free the memory and exit. Another task could then allocate the memory and when the semaphore is posted, you would have memory corruption.
Similarly for return after starting the DMA.
My though was that it should simply disable the interrupt (by undoing the modifyreg32) oe stop the DMA is this error occurs. Then it should be save to return and exit.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] masayuki2009 commented on issue #699: Check
return of nxsem_wait_uninterruptible
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on issue #699: Check return of nxsem_wait_uninterruptible
URL: https://github.com/apache/incubator-nuttx/pull/699#issuecomment-608181576
> Hi, @masayuki2009
>
> This needs more context. The change will be be the nxsem_wait_uninterruptible will return with the error ECANCELED if the task is canceled. It does not do that now (it hangs).
>
> So when the function receives this error and returns, the task will exist. My concern was that if you enable and interrupt then exit, the interrupt would still fire and set the semaphore. But the task may destroy the semaphore, free the memory and exit. Another task could then allocate the memory and when the semaphore is posted, you would have memory corruption.
>
> Similarly for return after starting the DMA.
>
> My though was that it should simply disable the interrupt (by undoing the modifyreg32) oe stop the DMA is this error occurs. Then it should be save to return and exit.
Hi, @patacongo
Thanks for the comments.
I think I need to read https://github.com/apache/incubator-nuttx/issues/619 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] acassis commented on issue #699: Check return of
nxsem_wait_uninterruptible
Posted by GitBox <gi...@apache.org>.
acassis commented on issue #699: Check return of nxsem_wait_uninterruptible
URL: https://github.com/apache/incubator-nuttx/pull/699#issuecomment-608382173
> > As a best practice I think that we should always submit nxstyle fixes and the actual patch in a PR
> > This will make our life easier when we review the code
>
> Yes, the actual patch PR can't pass the precheck if nxstyle is on another PR. But it's better to split the nxstyle change into another patch, so the reviewer can focus on the real change.
Yes, it is failing because long lines:
fda56a8afd Check return of nxsem_wait_uninterruptible
../nuttx/tools/checkpatch.sh -g fda56a8afd209ca98a1e91e455c84a7992f27e50
arch/arm/src/lc823450/lc823450_i2s.c:200:80: error: Long line found
arch/arm/src/lc823450/lc823450_i2s.c:202:84: error: Long line found
arch/arm/src/lc823450/lc823450_i2s.c:206:80: error: Long line found
arch/arm/src/lc823450/lc823450_i2s.c:208:81: error: Long line found
arch/arm/src/lc823450/lc823450_i2s.c:331:79: error: Long line found
arch/arm/src/lc823450/lc823450_i2s.c:413:80: error: Long line found
arch/arm/src/lc823450/lc823450_i2s.c:438:87: error: Long line found
arch/arm/src/lc823450/lc823450_i2s.c:499:79: error: Long line found
arch/arm/src/lc823450/lc823450_i2s.c:514:1: error: Too many blank lines
arch/arm/src/lc823450/lc823450_i2s.c:586:79: error: Long line found
##[error]Process completed with exit code 1.
Also the CI is very unstable currently, many errors like this:
Unable to find image 'docker.pkg.github.com/apache/incubator-nuttx-testing/nuttx-ci-linux:latest' locally
/usr/bin/docker: Error response from daemon: received unexpected HTTP status: 504 Gateway Time-out.
Maybe it is the COVID-19 side-effect!
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] acassis commented on issue #699: Check return of
nxsem_wait_uninterruptible
Posted by GitBox <gi...@apache.org>.
acassis commented on issue #699: Check return of nxsem_wait_uninterruptible
URL: https://github.com/apache/incubator-nuttx/pull/699#issuecomment-608170773
Hi @MasayukiIshikawa could you please review this patch. Mr Greg noticed that it could break the I2S logic because it could exist with the INT enabled. I want to submit an improvement to disable the INT before "return ret", but I need to verify with you first if this solution could you. Do you still having this board to perform the real test?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on issue #699: Check
return of nxsem_wait_uninterruptible
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on issue #699: Check return of nxsem_wait_uninterruptible
URL: https://github.com/apache/incubator-nuttx/pull/699#issuecomment-608380809
> As a best practice I think that we should always submit nxstyle fixes and the actual patch in a PR
> This will make our life easier when we review the code
Yes, the actual patch PR can't pass the precheck if nxstyle is on another PR. But it's better to split the nxstyle change into another patch, so the reviewer can focus on the real 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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] acassis commented on issue #699: Check return of
nxsem_wait_uninterruptible
Posted by GitBox <gi...@apache.org>.
acassis commented on issue #699: Check return of nxsem_wait_uninterruptible
URL: https://github.com/apache/incubator-nuttx/pull/699#issuecomment-608373195
> > @xiaoxiang781216 how to merge all these 4 commits in a single one after I submitted a PR? I didn't see a way to modify an already submitted commit. Could you please help me?
>
> in your local branch:
> git rebase --interactive
> and change pick to squash
> git push -f ...
Yes, Abdelatif gave me a hand with it. Thank 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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] acassis commented on issue #699: Check return of
nxsem_wait_uninterruptible
Posted by GitBox <gi...@apache.org>.
acassis commented on issue #699: Check return of nxsem_wait_uninterruptible
URL: https://github.com/apache/incubator-nuttx/pull/699#issuecomment-608365729
> @acassis should we merge the change into one to ensure each patch can build correctly?
Hi @xiaoxiang781216 yes, let to try 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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] acassis commented on issue #699: Check return of
nxsem_wait_uninterruptible
Posted by GitBox <gi...@apache.org>.
acassis commented on issue #699: Check return of nxsem_wait_uninterruptible
URL: https://github.com/apache/incubator-nuttx/pull/699#issuecomment-608369915
@xiaoxiang781216 how to merge all these 4 commits in a single one after I submitted a PR? I didn't see a way to modify an already submitted commit. Could you please help me?
----------------------------------------------------------------
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
With regards,
Apache Git Services