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/21 09:34:00 UTC
[GitHub] [incubator-nuttx] jerpelea opened a new pull request #836: audio nxstyle fixes and audio drivers cleanup
jerpelea opened a new pull request #836:
URL: https://github.com/apache/incubator-nuttx/pull/836
# Pull Request Template
## Title guidelines
Following the guidelines for writting good commit messages (https://chris.beams.io/posts/git-commit/) and creating a meaningful title is key in effective team communication.
**do's**
- stm32h7: Add SDMMC Support
- nsh: Separate `source` and `sh` for POSIX compliance
- nxstyle: Fixed Camel case detection
- drivers/serial: Fixed style violation
**dont's**
- fixed bug
- nxstyle
- PR #12
## Description
**Describe problem solved by the PR**
A clear and concise description of the problem, if any, this PR will solve. Please also include relevant motivation and context: E.g. The current nsh sh command violates the POSIX ...
List any dependencies that are required for this change.
**Describe your solution**
A clear and concise description of what you have implemented.
**Describe possible alternatives**
A clear and concise description of alternative solutions you've considered.
**Additional context**
Add any other context or screenshots for the pr.
Fixes # (issue)
## Type of change
Delete options that are not relevant.
- [ ] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
- [ ] This change requires a documentation update
## How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
- [ ] Test A
- [ ] Test B
**Test Configuration**:
* Nuttx board/config:
* Hardware:
* Toolchain:
## Checklist:
- [ ] My code follows the style guidelines of this project (NEED link to how to run checkpatch)
- [ ] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have made corresponding changes to the documentation
- [ ] My changes generate no new warnings
- [ ] I have added tests that prove my fix is effective or that my feature works
- [ ] New and existing unit tests pass locally with my changes
- [ ] Any dependent changes have been merged and published in downstream modules
- [ ] I have checked my code and corrected any misspellings (NEED link to how to run checkpatch spelling)
----------------------------------------------------------------
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] patacongo commented on issue #836: audio nxstyle fixes and audio drivers cleanup
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #836:
URL: https://github.com/apache/incubator-nuttx/pull/836#issuecomment-617462135
> I have a CI job I tested a bit back that lets you target a PR in apps with a label "ci/use-app/{PR#}" let me see if I can bring to mainline.
I think we need to do something. We can reduce the risk considerably as we did in this case by reviewing and testing so that that likelihood of anything bad happening is not so big. But I can imagine cases where there could be a real mess and the checks could be down for a long time if we had to fix compile errors or revert 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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on issue #836: audio nxstyle fixes and audio drivers cleanup
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on issue #836:
URL: https://github.com/apache/incubator-nuttx/pull/836#issuecomment-617504364
> I have a CI job I tested a bit back that lets you target a PR in apps with a label "ci/use-app/{PR#}" let me see if I can bring to mainline.
Yes, this is a common solution, it is also very useful to sync nuttx/apps during the release.
----------------------------------------------------------------
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] patacongo commented on issue #836: audio nxstyle fixes and audio drivers cleanup
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #836:
URL: https://github.com/apache/incubator-nuttx/pull/836#issuecomment-617451198
I have merved apps 197. That breaks the PR checks for a little while. I have re-started checks on this PR. If everything builds as it should, then we should be okay and things will be fixed in around 30 minutes.
If there is a problem, I will try to fix. Sorry about the inconvenience.
----------------------------------------------------------------
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] jerpelea commented on issue #836: audio nxstyle fixes and audio drivers cleanup
Posted by GitBox <gi...@apache.org>.
jerpelea commented on issue #836:
URL: https://github.com/apache/incubator-nuttx/pull/836#issuecomment-617246951
that was my comment that i have to fix the apps repository before we can merge this PR
----------------------------------------------------------------
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] patacongo commented on issue #836: audio nxstyle fixes and audio drivers cleanup
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #836:
URL: https://github.com/apache/incubator-nuttx/pull/836#issuecomment-617237203
Looks like it should compiler to me. it is complaiing:
i2sloop_main.c: In function 'i2sloop_main':
i2sloop_main.c:123:10: error: 'union <anonymous>' has no member named 'ppBuffer'; did you mean 'pbuffer'?
123 | desc.u.ppBuffer = &apb;
| ^~~~~~~~
| pbuffer
include/nuttx/audio/audio.h:
460 struct audio_buf_desc_s
461 {
462 #ifdef CONFIG_AUDIO_MULTI_SESSION
463 FAR void *session; /* Associated channel */
464 #endif
465 uint16_t numbytes; /* Number of bytes to allocate */
466 union
467 {
468 FAR struct ap_buffer_s *pBuffer; /* Buffer to free / enqueue */
469 FAR struct ap_buffer_s **ppBuffer; /* Pointer to receive allocated buffer */
470 } u;
471 };
I don't see why the built check is complaining.
----------------------------------------------------------------
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] patacongo edited a comment on issue #836: audio nxstyle fixes and audio drivers cleanup
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on issue #836:
URL: https://github.com/apache/incubator-nuttx/pull/836#issuecomment-617462135
> I have a CI job I tested a bit back that lets you target a PR in apps with a label "ci/use-app/{PR#}" let me see if I can bring to mainline.
I think we need to do something. We can reduce the risk considerably as we did in this case by reviewing and testing so that that likelihood of anything bad happening is not so big. But I can imagine cases where there could be a real mess and the checks could be down for a long time if we had to fix compile errors or revert changes.
Fortunately, in this case, I also have confidence in Alin who does very thorough work.
----------------------------------------------------------------
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] patacongo commented on issue #836: audio nxstyle fixes and audio drivers cleanup
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #836:
URL: https://github.com/apache/incubator-nuttx/pull/836#issuecomment-617243835
No, your PR really does break the build. You cannot change audio.h or any other common header file without also changing ALL of the files that include that header files. You can either:
1. Back out the changes that you mad to common header files, or,
2. Change all places where those common header files are included.
The first would be easiest. The second would be the better contribution to the OS.
----------------------------------------------------------------
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 issue #836: audio nxstyle fixes and audio drivers cleanup
Posted by GitBox <gi...@apache.org>.
btashton commented on issue #836:
URL: https://github.com/apache/incubator-nuttx/pull/836#issuecomment-617365209
I have a CI job I tested a bit back that lets you target a PR in apps with a label "ci/use-app/{PR#}" let me see if I can bring to mainline.
----------------------------------------------------------------
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] jerpelea commented on a change in pull request #836: audio nxstyle fixes and audio drivers cleanup
Posted by GitBox <gi...@apache.org>.
jerpelea commented on a change in pull request #836:
URL: https://github.com/apache/incubator-nuttx/pull/836#discussion_r412275342
##########
File path: include/nuttx/audio/audio.h
##########
@@ -465,8 +480,8 @@ struct audio_buf_desc_s
uint16_t numbytes; /* Number of bytes to allocate */
union
{
- FAR struct ap_buffer_s *pBuffer; /* Buffer to free / enqueue */
- FAR struct ap_buffer_s **ppBuffer; /* Pointer to receive allocated buffer */
+ FAR struct ap_buffer_s *buffer; /* Buffer to free / enqueue */
+ FAR struct ap_buffer_s **pbuffer; /* Pointer to receive allocated buffer */
} u;
};
Review comment:
thanks for pointing that the apps are failing
In the NuttX code I changed all files that depend on it and fixed the violation but i did not check the apps
I will fix it now and open another PR for apps
Thanks
----------------------------------------------------------------
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] patacongo edited a comment on issue #836: audio nxstyle fixes and audio drivers cleanup
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on issue #836:
URL: https://github.com/apache/incubator-nuttx/pull/836#issuecomment-617246009
> You cannot change audio.h or any other common header file without also changing ALL of the files that include that header files. You can either:
>
> 1. Back out the changes that you mad to common header files, or,
> 2. Change all places where those common header files are included.
it looks like you already did (2) in the nuttx/ repository. But you will also have to do that in the apps/ repository as well.
They you will have the situation where you must have both PRs in place in order for either of them to pass the build checks. So the PR checks will still fail.
----------------------------------------------------------------
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] jerpelea commented on issue #836: audio nxstyle fixes and audio drivers cleanup
Posted by GitBox <gi...@apache.org>.
jerpelea commented on issue #836:
URL: https://github.com/apache/incubator-nuttx/pull/836#issuecomment-617316442
@patacongo thanks for pointing it out and I think I fixed the apps and NuttX complains.
I built locally both nxplayer and nxrecorder but I would be grateful for a double check since the automated build is failing .
----------------------------------------------------------------
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] patacongo commented on issue #836: audio nxstyle fixes and audio drivers cleanup
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #836:
URL: https://github.com/apache/incubator-nuttx/pull/836#issuecomment-617248224
> that was my comment that i have to fix the apps repository before we can merge this PR
Okay.. then you already have the answer. I am just trying to respond to
> @patacongo can you please look at the failure
> I can not replicate the failure locally on an ARM platform
----------------------------------------------------------------
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] patacongo edited a comment on issue #836: audio nxstyle fixes and audio drivers cleanup
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on issue #836:
URL: https://github.com/apache/incubator-nuttx/pull/836#issuecomment-617243835
No, your PR really does break the build. You changed the name of the field ppBuffer in include/nuttx/audio/audio.h to ppbuffer. That will break every file that uses audio buffers.
You cannot change audio.h or any other common header file without also changing ALL of the files that include that header files. You can either:
1. Back out the changes that you mad to common header files, or,
2. Change all places where those common header files are included.
The first would be easiest. The second would be the better contribution to the OS.
----------------------------------------------------------------
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] patacongo edited a comment on issue #836: audio nxstyle fixes and audio drivers cleanup
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on issue #836:
URL: https://github.com/apache/incubator-nuttx/pull/836#issuecomment-617246009
> You cannot change audio.h or any other common header file without also changing ALL of the files that include that header files. You can either:
>
> 1. Back out the changes that you mad to common header files, or,
> 2. Change all places where those common header files are included.
it looks like you already did (2) in the nuttx/ repository. But you will also have to do that in the apps/ repository as well.
They you will have the situation where you must have both PRs in place in order for either of them to pass the build checks.
----------------------------------------------------------------
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] patacongo commented on issue #836: audio nxstyle fixes and audio drivers cleanup
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #836:
URL: https://github.com/apache/incubator-nuttx/pull/836#issuecomment-617246009
> You cannot change audio.h or any other common header file without also changing ALL of the files that include that header files. You can either:
>
> 1. Back out the changes that you mad to common header files, or,
> 2. Change all places where those common header files are included.
it looks like you already did (2) in the nuttx/ repository. But you will also have to do that in the apps/ repository as well.
----------------------------------------------------------------
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] patacongo commented on issue #836: audio nxstyle fixes and audio drivers cleanup
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #836:
URL: https://github.com/apache/incubator-nuttx/pull/836#issuecomment-617355693
The automated build is failing in the same way that it was before:
i2sloop_main.c: In function 'i2sloop_main':
i2sloop_main.c:123:10: error: 'union <anonymous>' has no member named 'ppBuffer'; did you mean 'pbuffer'?
123 | desc.u.ppBuffer = &apb;
| ^~~~~~~~
| pbuffer
nxplayer does, indeed, need to be changed but also apps/examples/i2sloop.
I find these references to ppBuffer in apps/:
examples/i2schar/i2schar_receiver.c: desc.u.ppBuffer = &apb;
examples/i2schar/i2schar_transmitter.c: desc.u.ppBuffer = &apb;
examples/i2sloop/i2sloop_main.c: desc.u.ppBuffer = &apb;
system/nxplayer/nxplayer.c: buf_desc.u.ppBuffer = &pbuffers[x];
system/nxrecorder/nxrecorder.c: buf_desc.u.ppBuffer = &pbuffers[x];
These files also refer to pBuffer: All need to be fixed.
----------------------------------------------------------------
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] jerpelea commented on issue #836: audio nxstyle fixes and audio drivers cleanup
Posted by GitBox <gi...@apache.org>.
jerpelea commented on issue #836:
URL: https://github.com/apache/incubator-nuttx/pull/836#issuecomment-617228932
@patacongo can you please look at the failure
I can not replicate the failure locally on an ARM platform
----------------------------------------------------------------
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] patacongo commented on issue #836: audio nxstyle fixes and audio drivers cleanup
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #836:
URL: https://github.com/apache/incubator-nuttx/pull/836#issuecomment-617357418
You understand that is it a limitation of the build system that if there is are PRs in apps/ and nuttx/ that mutually depend on each other to get through the PR checks, then there is no way that you will ever pass the PR checks.
In this case, you really have to merge both and just wait and see what breaks. I suppose you could merge one, then restart the test on the other. But then the respositories are 100% guaranteed to be broken. I think that you just have to "bite the bullet" and merge both when you are confident that you have made all of the necessary changes.
@xiaoxiang781216 do you have any better advice in this case?
----------------------------------------------------------------
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] patacongo commented on a change in pull request #836: audio nxstyle fixes and audio drivers cleanup
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #836:
URL: https://github.com/apache/incubator-nuttx/pull/836#discussion_r412272807
##########
File path: include/nuttx/audio/audio.h
##########
@@ -465,8 +480,8 @@ struct audio_buf_desc_s
uint16_t numbytes; /* Number of bytes to allocate */
union
{
- FAR struct ap_buffer_s *pBuffer; /* Buffer to free / enqueue */
- FAR struct ap_buffer_s **ppBuffer; /* Pointer to receive allocated buffer */
+ FAR struct ap_buffer_s *buffer; /* Buffer to free / enqueue */
+ FAR struct ap_buffer_s **pbuffer; /* Pointer to receive allocated buffer */
} u;
};
Review comment:
audio.h cannot be changed! You cannot chanage audio.h without also changing all of the files that include audio.h.
You will either have to remove the changes to this (and any other common header files), or modify all places where the header file is included. That includes apps/examples/i2sloop/i2sloop_main.c
----------------------------------------------------------------
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