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/03/06 15:43:21 UTC
[GitHub] [incubator-nuttx] anpaza opened a new pull request #459: stm32h7:
support SDRAM via FMC peripherial
anpaza opened a new pull request #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459
Fixed everything said in my previous merge attempt.
nxstyle passes on new code.
Only FMC-related 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] anpaza commented on issue #459: stm32h7: support
SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
anpaza commented on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-597636050
The text width has grown to 180 chars :)
Other than that, nxstyle is happy.
Also found a small bug in nxstyle.c:
if (len < min)
{
min = len;
lineno_min = lineno_min;
}
should have same fix as was done earlier for lineno_max:
lineno_min = lineno;
----------------------------------------------------------------
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] anpaza commented on issue #459: stm32h7: support
SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
anpaza commented on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-597090058
What changes are required?
I fixed every warning in stm32h7x3xx_rcc.h. That took me about a hour.
Do you want to revert my changes? Are they wrong?
----------------------------------------------------------------
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 #459: stm32h7:
support SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-597145330
I will subit a 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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] johannes-nivus edited a comment on issue #459:
stm32h7: support SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
johannes-nivus edited a comment on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-599914174
@davids5
> For for what you did above without loss of information it would be a ruler of 100
I see no loss of information in my modified stm32h7x3xx_rcc.h. (And it has a ruler of 100.)
My statement concerning loss of information had been ambiguous, I meant:
> Perhaps shorten comments, **only** when possible without loosing important information.
----------------------------------------------------------------
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 edited a comment on issue #459:
stm32h7: support SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-597012524
> I don't understand what's wrong with nxstyle?
Here is the recent nxstyle change which may affect the report:
https://github.com/apache/incubator-nuttx/pull/487
https://github.com/apache/incubator-nuttx/pull/496
> Your link says:
>
> .github:1
>
> Process completed with exit code 1.
>
> what this means?
>
Which means the patch has some style issue need to fix before merging.
> There are warnings in arch/arm/src/stm32h7/stm32h7x7xx_rcc.c and arch/arm/src/stm32h7/hardware/stm32h7x3xx_rcc.h but that's not my fault. Should I fix someone else's warnings?
Good question, you can express your opinion in this email loop:
https://lists.apache.org/thread.html/r6e7b2ed2d5ca1d5b49c5898b591182a8d7475bee2d5de344c026b3f5%40<dev.nuttx.apache.org>
----------------------------------------------------------------
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 edited a comment on issue #459:
stm32h7: support SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-596148082
>
>
> @patacongo recognizing your are not failure with github.
>
> There is a `suggested change` that was requested.
> @patacongo recognizing your are not failure with github.
>
> There is a `suggested change` that was requested.
>
> I have asked you to use that feature and not waste time on adding comments that are not actionable when they can be. The contributor can then merge the suggestion and all our efforts add value not just criticism. Once the review is done the contributor can squash and force push to remove all the bable commits.
@davids5 Go to hell. You have no authority here. You are very offensive.
----------------------------------------------------------------
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] davids5 commented on issue #459: stm32h7: support
SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
davids5 commented on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-600034988
> @davids5
>
> > For for what you did above without loss of information it would be a ruler of 100
>
> I see no loss of information in my modified stm32h7x3xx_rcc.h. (And it has a ruler of 100.)
> My statement concerning loss of information had been ambiguous, I meant:
>
> > Perhaps shorten comments, **only** when possible without loosing important information.
@johannes-nivus
See https://github.com/johannes-nivus/incubator-nuttx/commit/0c6395f7a7ef56e6d634a2c1c933ca6264c77a39#commitcomment-37871019
And yet is does.
----------------------------------------------------------------
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] anpaza commented on issue #459: stm32h7: support
SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
anpaza commented on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-603750575
> @anpaza could you refine your patchset a little bit? now the PR contain 10 patches, and most patch fix the style issue. It's better that:
Never used rebase -i before, so it took a bit of time to read the internets :)
Now it looks a lot cleaner.
But davids5' branch nuttx-to-asf:stm32h7-fmc2_cs_fix should be broken 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.
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 #459: stm32h7:
support SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-597164914
> @anpaza - I think the tools is wrong and flagged things it should not have.
>
> @patacongo would you please advise @anpaza on the rcc file and if we can take it in as is?
This PR pass the style/build check, has it any logic issue to fix, @davids5? if not why don't we merge it? @anpaza already try his best to fix the coding style issue.
----------------------------------------------------------------
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] johannes-nivus edited a comment on issue #459:
stm32h7: support SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
johannes-nivus edited a comment on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-597131641
> The fixed the [real error](https://github.com/apache/incubator-nuttx/pull/488/commits/b27e7d77491d64bb82fe598c90d83f50704961bd#diff-83151d0c6d55ed69fbb71462fe8ae91cR4)
@davids5
Why did you add the double asterisks? Is this recommended by coding style or nxstyle?
----------------------------------------------------------------
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 #459: stm32h7:
support SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-597128263
In general, I don't think that there is any reason why the ruler should be anything different that the standard 77/78 characters for a .c files. But .h files, and especially hardware register definition header files often require much longer rulers to about having to move comments to a different line or dividing into multiple right hand comments. I hope that the extended ruler solution is not used without careful consideration as to when and when it is not appropriate.
----------------------------------------------------------------
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 edited a comment on issue #459:
stm32h7: support SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-596148082
> @patacongo recognizing your are not failure with github.
>
> There is a `suggested change` that was requested.
>
> I have asked you to use that feature and not waste time on adding comments that are not actionable when they can be. The contributor can then merge the suggestion and all our efforts add value not just criticism. Once the review is done the contributor can squash and force push to remove all the bable commits.
@davids5 Go to hell. You have no authority here. You are very offensive.
----------------------------------------------------------------
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 #459: stm32h7:
support SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-595846947
> They are used in stm32_fmc.c (modreg32).
Okay, thanks. I did not see that because Github in all its wisdom collapses large files and so the references were not found in tha search.
----------------------------------------------------------------
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] johannes-nivus commented on issue #459: stm32h7:
support SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
johannes-nivus commented on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-597723928
>
>
> > Also found a small bug in nxstyle.c:
>
> @johannes-nivus Are you going to fix that one?
As already stated this should have been fixed in #489 or do I miss something?
----------------------------------------------------------------
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 edited a comment on issue #459:
stm32h7: support SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-597128263
In general, I don't think that there is any reason why the ruler should be anything different that the standard 77/78 characters for a .c files. But .h files, and especially hardware register definition header files often require much longer rulers to avoid having to move comments to a different line or dividing comments into multiple right hand comments. I hope that the extended ruler solution is not used without careful consideration as to when and when it is not appropriate.
----------------------------------------------------------------
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 a change in pull request
#459: stm32h7: support SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#discussion_r388991575
##########
File path: arch/arm/src/common/up_arch.h
##########
@@ -58,6 +58,12 @@
# define getreg32(a) (*(volatile uint32_t *)(a))
# define putreg32(v,a) (*(volatile uint32_t *)(a) = (v))
+/* Non-atomic, but more effective modification of registers */
+
+# define modreg8(v,m,a) putreg8((getreg8(a) & ~(m)) | ((v) & (m)), a)
+# define modreg16(v,m,a) putreg16((getreg16(a) & ~(m)) | ((v) & (m)), a)
+# define modreg32(v,m,a) putreg32((getreg32(a) & ~(m)) | ((v) & (m)), a)
+
Review comment:
Remove these. They are not used any where. This is just noice.
----------------------------------------------------------------
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 removed a comment on issue #459:
stm32h7: support SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
patacongo removed a comment on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-596148082
> @patacongo recognizing your are not failure with github.
>
> There is a `suggested change` that was requested.
>
> I have asked you to use that feature and not waste time on adding comments that are not actionable when they can be. The contributor can then merge the suggestion and all our efforts add value not just criticism. Once the review is done the contributor can squash and force push to remove all the bable commits.
@davids5 Go to hell. You have no authority here. You are very offensive.
----------------------------------------------------------------
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] johannes-nivus commented on issue #459: stm32h7:
support SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
johannes-nivus commented on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-599254476
@davids5 If you're interested have a look at
https://github.com/johannes-nivus/incubator-nuttx/commit/0c6395f7a7ef56e6d634a2c1c933ca6264c77a39
This would be my solution to have stm32h7x3xx_rcc.h according to coding standard, and with only 100 columns.
I will not file a PR because it interferes with this PR.
The problem in this case is not nxstyle, but many old files do not adhere to coding standard.
And if the policy of "touch it, fix it" is kept, we need to help contributors to do it right.
As rules of thumb:
The checked alignment of right of code comments can be reset by adding a blank line.
If the comments get too long, use multiline right of code, or even a block comment where applicable.
Perhaps shorten comments, when possible without loosing important information.
Adjust block comment length to have longer allowed lines.
----------------------------------------------------------------
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] anpaza commented on issue #459: stm32h7: support
SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
anpaza commented on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-597190237
I see, nxstyle.c has changed in the meanwhile.
I fixed everything he says now, except this:
arch/arm/src/stm32h7/Kconfig: info: No file extension
arch/arm/src/stm32h7/Kconfig: info: Unknown file extension
I hope this will not stop the automatic checks to pass.
P.S. Sidenote: in tools/checkpatch.h you could get the list of files in a commit much simpler:
git show --name-only --pretty="" 94a73a87363557b839b8c1bc032f7c77d02e5044
or even
git show --name-only --pretty="" 94a73a87363557b839b8c1bc032f7c77d02e5044..HEAD
----------------------------------------------------------------
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 #459: stm32h7:
support SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-597101043
I am not sure what the problem is. The only odd thing I see is that long comments were moved to a following line. That is unusal and I would prefer that not be that way in the code because consistency is important.
Why not be merciful and stop anpanza's suffering some way.
In the past, I made all of the nxstyle files myself rather than rely on submitters to do that. Now, I have no idea how to handle situations like this. There is the saying "If you want things done right, do it yourself."
----------------------------------------------------------------
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 #459: stm32h7:
support SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-596999542
@anpaza sorry nxstyle update recently to catch more the style prolem, you have to update the patch a little bit to fix the precheck error reported here:
https://github.com/apache/incubator-nuttx/pull/459/checks?check_run_id=497438972
----------------------------------------------------------------
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] anpaza commented on a change in pull request
#459: stm32h7: support SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
anpaza commented on a change in pull request #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#discussion_r388998837
##########
File path: arch/arm/src/common/up_arch.h
##########
@@ -58,6 +58,12 @@
# define getreg32(a) (*(volatile uint32_t *)(a))
# define putreg32(v,a) (*(volatile uint32_t *)(a) = (v))
+/* Non-atomic, but more effective modification of registers */
+
+# define modreg8(v,m,a) putreg8((getreg8(a) & ~(m)) | ((v) & (m)), a)
+# define modreg16(v,m,a) putreg16((getreg16(a) & ~(m)) | ((v) & (m)), a)
+# define modreg32(v,m,a) putreg32((getreg32(a) & ~(m)) | ((v) & (m)), a)
+
Review comment:
They are used in stm32_fmc.c (modreg32).
I think it is easier to read this:
modreg32 (BOARD_FMC_CLK, RCC_D1CCIPR_FMCSEL_MASK, STM32_RCC_D1CCIPR);
than the old-style equivalent:
regval = getreg32(STM32_RCC_D1CCIPR);
regval = (regval & ~RCC_D1CCIPR_FMCSEL_MASK) | BOARD_FMC_CLK;
putreg32(regval, STM32_RCC_D1CCIPR);
Also I have pending changes to e.g. stm32_ethernet.c using these macros.
I noted that there's a new family of functions modifyreg*() (that were not available on nuttx-8.2 I was working on), but they introduce extra cost for every call, and won't protect anything in my case (and many other cases, as usually modifications to several registers have to be atomic, not individual register 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] patacongo edited a comment on issue #459:
stm32h7: support SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-599788169
No, no meta-data in source files. This has been discussed before. The coding standard prohibits meta data in sources (at least in reference to Deoxygen, but is generalization). Let's not go this way. Let's keep clean C code with no tool-specific meta-shit.
----------------------------------------------------------------
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] davids5 commented on issue #459: stm32h7: support
SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
davids5 commented on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-601165084
@anpaza - did you see my PR to your fork? Shal we get this in? See https://github.com/apache/incubator-nuttx/pull/459#pullrequestreview-376872337
----------------------------------------------------------------
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] davids5 commented on issue #459: stm32h7: support
SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
davids5 commented on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-597614597
@a
> Do you mean:
> a) The rulers in file stm32h7x3xx_rcc.h should be extended to cover the longest right-side comment
> b) All the comments should be right-side of the respective #define
> I can give one more try.
Bravo!
Yes. The way the file is on master, it only needed the blocks extended.
----------------------------------------------------------------
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] davids5 commented on issue #459: stm32h7: support
SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
davids5 commented on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-597081797
>stm32h7x3xx_rcc.h was a nightmare.
I am so sorry!
The trick on h files is to set the comment block out to cover the worst case span. (it is a ruler)
If after that, if the tools give false positives we have to ignore them and fix the tools (@johannes-nivus) has been adding a-lot of value there.
it may be best to revert that file. But I will leave up to 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] anpaza commented on issue #459: stm32h7: support
SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
anpaza commented on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-597077562
I fixed warnings in all files touched by my changes.
stm32h7x3xx_rcc.h was a nightmare.
I think Eclipse would fail on 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] davids5 commented on a change in pull request
#459: stm32h7: support SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#discussion_r389355817
##########
File path: arch/arm/src/stm32h7/stm32_allocateheap.c
##########
@@ -246,6 +249,14 @@ void up_allocate_heap(FAR void **heap_start, size_t *heap_size)
up_heap_color(*heap_start, *heap_size);
#endif
+
+#if defined(CONFIG_DEBUG_FEATURES)
+
+ /* Display memory ranges to help debugging */
+
+ _info("%uKb of SRAM at %p\n", *heap_size / 1024, *heap_start);
Review comment:
@anpaza let me know if you are OK with this change and I will commit and 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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo edited a comment on issue #459:
stm32h7: support SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-599788169
No, no meta-data in source files. This has been discussed before. The coding standard prohibits meta data in sources (at least in reference to Deoxygen, but is generalization). Let's not go this way. Let's keep clean C code with no tool-specific meta-shit.
Furthermore, there is no exlusion from the file lenght in the coding standard. This would essential say that it is okay to violatie the coding standard. That is not a good thing.
----------------------------------------------------------------
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 #459: stm32h7:
support SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-597012524
> I don't understand what's wrong with nxstyle?
Here is the recent nxstyle change which may affect the report:
https://github.com/apache/incubator-nuttx/pull/487
https://github.com/apache/incubator-nuttx/pull/496
> Your link says:
>
> .github:1
>
> Process completed with exit code 1.
>
> what this means?
>
> There are warnings in arch/arm/src/stm32h7/stm32h7x7xx_rcc.c and arch/arm/src/stm32h7/hardware/stm32h7x3xx_rcc.h but that's not my fault. Should I fix someone else's warnings?
Good question, you can express your opinion in this email loop:
https://lists.apache.org/thread.html/r6e7b2ed2d5ca1d5b49c5898b591182a8d7475bee2d5de344c026b3f5%40<dev.nuttx.apache.org>
----------------------------------------------------------------
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 removed a comment on issue #459:
stm32h7: support SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
patacongo removed a comment on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-595846947
> They are used in stm32_fmc.c (modreg32).
Okay, thanks. I did not see that because Github in all its wisdom collapses large files and so the references were not found in tha search.
----------------------------------------------------------------
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 #459: stm32h7:
support SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-595846998
> They are used in stm32_fmc.c (modreg32).
Okay, thanks. I did not see that because Github in all its wisdom collapses large files and so the references were not found in tha search.
----------------------------------------------------------------
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] anpaza commented on issue #459: stm32h7: support
SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
anpaza commented on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-597112831
> > Why not be merciful and stop anpanza's suffering some way.
>
> I agree. @anpaza It id is Ok with you, when I have time this week or on the weekend I will pull it in and fix it.
If you tell me how, I can fix it myself. But I don't get what ruler everyone talks about. If it means you can use lines of unlimited size, it is not too handy for reading. Not everybody uses Helv.8. In my own code I usually move the comment from extra long lines before the #define, e.g:
/* this is something */
#define SOMETHING 1
/* this is something else */
#define SOMETHING_ELSE 2
but with the blank-line-after-comment-rule that becomes way too sparse. So the only solution I have found is to move the comment to next line, and nxstyle didn't object.
It would be good to have an .rc file for GNU indent that would allow to automatically reflow code according to desired rules. But, although indent has zillions of options, I doubt it can handle every nx rule. Maybe fork GNU indent? Don't know.
----------------------------------------------------------------
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 #459: stm32h7:
support SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-597093576
> patacongo would you please advise @anpaza on the rcc file and if we can take it in as is?
I have not been following this PR because you were dealing with it. I have to do a couple of things and I am not up to speed now to make any comments. What is your recommendation?
----------------------------------------------------------------
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 #459: stm32h7:
support SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-597118689
> > "If you want things done right, do it yourself."
>
> You could, for example, bring the changes onto a branch, revert only the rcc.h file, amend the PR, and merge what is left to master. Up to you.
I agree that committer should help contributor make the style change, but contributor shouldn't directly merge the change to mainline, the new PR still need go through the precheck/build process again. committer is human and human will make mistake, only tool can ensure there isn't error happen in this type of modification.
----------------------------------------------------------------
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] anpaza commented on a change in pull request
#459: stm32h7: support SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
anpaza commented on a change in pull request #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#discussion_r388998837
##########
File path: arch/arm/src/common/up_arch.h
##########
@@ -58,6 +58,12 @@
# define getreg32(a) (*(volatile uint32_t *)(a))
# define putreg32(v,a) (*(volatile uint32_t *)(a) = (v))
+/* Non-atomic, but more effective modification of registers */
+
+# define modreg8(v,m,a) putreg8((getreg8(a) & ~(m)) | ((v) & (m)), a)
+# define modreg16(v,m,a) putreg16((getreg16(a) & ~(m)) | ((v) & (m)), a)
+# define modreg32(v,m,a) putreg32((getreg32(a) & ~(m)) | ((v) & (m)), a)
+
Review comment:
They are used in stm32_fmc.c (modreg32).
I think it is easier to read this:
modreg32 (BOARD_FMC_CLK, RCC_D1CCIPR_FMCSEL_MASK, STM32_RCC_D1CCIPR);
than the old-style equivalent:
regval = getreg32(STM32_RCC_D1CCIPR);
regval = (regval & ~RCC_D1CCIPR_FMCSEL_MASK) | BOARD_FMC_CLK;
putreg32(regval, STM32_RCC_D1CCIPR);
Also I have pending changes to e.g. stm32_ethernet.c using these macros.
I noted that there's a new family of functions modifyreg*() (that were not available on nuttx-8.2 I was working on), but they introduce extra cost for every call, and won't protect anything in my case (and many other cases, as usually modifications to several registers have to be atomic, not individual register 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] patacongo commented on issue #459: stm32h7:
support SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-596148082
>
>
> @patacongo recognizing your are not failure with github.
>
> There is a `suggested change` that was requested.
> @patacongo recognizing your are not failure with github.
>
> There is a `suggested change` that was requested.
>
> I have asked you to use that feature and not waste time on adding comments that are not actionable when they can be. The contributor can then merge the suggestion and all our efforts add value not just criticism. Once the review is done the contributor can squash and force push to remove all the bable commits.
@davids5 Go to hell
----------------------------------------------------------------
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] anpaza commented on issue #459: stm32h7: support
SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
anpaza commented on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-597612315
Do you mean:
a) The rulers in file stm32h7x3xx_rcc.h should be extended to cover the longest right-side comment
b) All the comments should be right-side of the respective #define
I can give one more try.
----------------------------------------------------------------
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] anpaza commented on a change in pull request
#459: stm32h7: support SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
anpaza commented on a change in pull request #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#discussion_r390196688
##########
File path: arch/arm/src/stm32h7/stm32_allocateheap.c
##########
@@ -246,6 +249,14 @@ void up_allocate_heap(FAR void **heap_start, size_t *heap_size)
up_heap_color(*heap_start, *heap_size);
#endif
+
+#if defined(CONFIG_DEBUG_FEATURES)
+
+ /* Display memory ranges to help debugging */
+
+ _info("%uKb of SRAM at %p\n", *heap_size / 1024, *heap_start);
Review comment:
Sorry for late answer.
Changed as you suggested.
----------------------------------------------------------------
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] anpaza commented on issue #459: stm32h7: support
SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
anpaza commented on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-595844421
They are used in stm32_fmc.c (modreg32).
I think it is easier to read this:
modreg32 (BOARD_FMC_CLK, RCC_D1CCIPR_FMCSEL_MASK, STM32_RCC_D1CCIPR);
than the old-style equivalent:
regval = getreg32(STM32_RCC_D1CCIPR);
regval = (regval & ~RCC_D1CCIPR_FMCSEL_MASK) | BOARD_FMC_CLK;
putreg32(regval, STM32_RCC_D1CCIPR);
Also I have pending changes to e.g. stm32_ethernet.c using these macros.
I noted that there's a new family of functions modifyreg*() (that were not available on nuttx-8.2 I was working on), but they introduce extra cost for every call, and won't protect anything in my case (and many other cases, as usually modifications to several registers have to be atomic, not individual register 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] patacongo edited a comment on issue #459:
stm32h7: support SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-597145330
I will subit a PR.
...
Please see PR #528
----------------------------------------------------------------
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] davids5 commented on issue #459: stm32h7: support
SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
davids5 commented on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-597178908
>If you tell me how, I can fix it myself.
@anpaza See this commit
b27e7d7
@xiaoxiang781216 I left it up to the contributor @anpaza and he said he would like to fix it. That is what I am waiting on.
----------------------------------------------------------------
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 #459: stm32h7:
support SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-597108706
If long lines are needed and it is inappropriate to break the lines using standard rules, then extending the "ruler" is the correct solution.
----------------------------------------------------------------
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] davids5 commented on issue #459: stm32h7: support
SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
davids5 commented on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-599784780
@johannes-nivus Here is one thing that may make sense if @patacongo is onboard with it. There is the `excess ` control in Nxstyle. What in h files we could have a comment in the title block (like vim settings. ) `excess=N`
Then we would not have to loose infomation in the right side comments.
For for what you did above without loss of information it would be a ruler of 100 and have a tag of
* excess=80
Thoughts?
----------------------------------------------------------------
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 #459: stm32h7:
support SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-597102319
> "If you want things done right, do it yourself."
You could, for example, bring the changes onto a branch, revert only the rcc.h file, amend the PR, and merge what is left to master. Up to 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] davids5 commented on issue #459: stm32h7: support
SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
davids5 commented on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-597119767
>If you tell me how, I can fix it myself.
@anpaza See this commit
https://github.com/apache/incubator-nuttx/pull/488/commits/b27e7d77491d64bb82fe598c90d83f50704961bd
[line 1](https://github.com/apache/incubator-nuttx/pull/488/commits/b27e7d77491d64bb82fe598c90d83f50704961bd#diff-83151d0c6d55ed69fbb71462fe8ae91cR1) extended the "Ruler", then all the block comment were extended to match.
The fixed the [real error](https://github.com/apache/incubator-nuttx/pull/488/commits/b27e7d77491d64bb82fe598c90d83f50704961bd#diff-83151d0c6d55ed69fbb71462fe8ae91cR4)
If you run checkpathc.sh on the before and after of that file. it will be come clear. But I MUST warn you, I has an off by 1 error on one line length and the output of the tools was all wrong. I wasted 40 minutes on that mess. So I feel your pain and very much appreciate your desire to fix the formatting.
----------------------------------------------------------------
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 a change in pull request
#459: stm32h7: support SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#discussion_r390306059
##########
File path: arch/arm/src/stm32h7/stm32_allocateheap.c
##########
@@ -246,6 +249,14 @@ void up_allocate_heap(FAR void **heap_start, size_t *heap_size)
up_heap_color(*heap_start, *heap_size);
#endif
+
+#if defined(CONFIG_DEBUG_FEATURES)
+
+ /* Display memory ranges to help debugging */
+
+ _info("%uKb of SRAM at %p\n", *heap_size / 1024, *heap_start);
Review comment:
In most cases, _info or minfo will not work when called from up_allocateheap(). up_allocateheap() is called earlier in the initialization sequence than is the logic that initialzes the syslog output. Do you actually see output?
----------------------------------------------------------------
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] davids5 commented on a change in pull request
#459: stm32h7: support SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#discussion_r389018630
##########
File path: arch/arm/src/stm32h7/stm32_allocateheap.c
##########
@@ -246,6 +249,14 @@ void up_allocate_heap(FAR void **heap_start, size_t *heap_size)
up_heap_color(*heap_start, *heap_size);
#endif
+
+#if defined(CONFIG_DEBUG_FEATURES)
+
+ /* Display memory ranges to help debugging */
+
+ _info("%uKb of SRAM at %p\n", *heap_size / 1024, *heap_start);
Review comment:
In practice the underscore version is not use directly. see debug.h minfo() would be used here.
```suggestion
minfo("%uKb of SRAM at %p\n", *heap_size / 1024, *heap_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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] johannes-nivus commented on issue #459: stm32h7:
support SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
johannes-nivus commented on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-597131641
> The fixed the [real error](https://github.com/apache/incubator-nuttx/pull/488/commits/b27e7d77491d64bb82fe598c90d83f50704961bd#diff-83151d0c6d55ed69fbb71462fe8ae91cR4)
Why did you add the double asterisks? Is this recommended by coding style or nxstyle?
----------------------------------------------------------------
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 edited a comment on issue #459:
stm32h7: support SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-597118689
> > "If you want things done right, do it yourself."
>
> You could, for example, bring the changes onto a branch, revert only the rcc.h file, amend the PR, and merge what is left to master. Up to you.
I agree that committer could help contributor make the style change, but committer shouldn't directly merge the change to mainline, the new PR still need go through the precheck/build process again. committer is human and human will make mistake, only tool can ensure there isn't error happen in this type of modification.
----------------------------------------------------------------
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] davids5 commented on issue #459: stm32h7: support
SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
davids5 commented on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-597105072
> Why not be merciful and stop anpanza's suffering some way.
I agree. @anpaza It id is Ok with you, when I have time this week or on the weekend I will pull it in and fix 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] davids5 merged pull request #459: stm32h7:
support SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
davids5 merged pull request #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459
----------------------------------------------------------------
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 edited a comment on issue #459:
stm32h7: support SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-599788169
No, no meta-data in source files. This has been discussed before. The coding standard prohibits meta data in sources (at least in reference to Deoxygen, but is generalization). Let's not go this way. Let's keep clean C code with no tool-specific meta-shit.
Furthermore, there is no exlusion from the file lenght in the coding standard. This would essential say that it is okay to violatie the coding standard. That is not a good thing.
You are basically proposing to rub snot into C files so that they can freely violate the coding standard.
----------------------------------------------------------------
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] johannes-nivus commented on issue #459: stm32h7:
support SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
johannes-nivus commented on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-599914174
> For for what you did above without loss of information it would be a ruler of 100
I see no loss of information in my modified stm32h7x3xx_rcc.h. (And it has a ruler of 100.)
My statement concerning loss of information had been ambiguous, I meant:
> Perhaps shorten comments, **only** when possible without loosing important information.
----------------------------------------------------------------
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] davids5 commented on issue #459: stm32h7: support
SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
davids5 commented on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-597073549
@anpaza
> Should I fix someone else's warnings?
I chose to. It makes the code better and in Eclipse with the error parers it is trivial. The set up is a bit tricky. I can share the screen shots here if you like/
----------------------------------------------------------------
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] davids5 commented on issue #459: stm32h7: support
SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
davids5 commented on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-597132496
@johannes-nivus - not it was my mistake!
----------------------------------------------------------------
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] davids5 commented on issue #459: stm32h7: support
SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
davids5 commented on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-597608242
@anpaza - Again I must apologizes to you for our poor tooling, and my inability to communicate the steps to fix it.
If it is OK with you , I will push a commit to this PR that will pass CI on the rcc file by changing the ruler.
The issue I have is is that I have several deadlines I have to meet and will not be abel to get to it right away.
The approach I will take is revert the rcc file and just add your code changes to it. Then lengthen the block comments. This why of fixing the file is 100% OK in a arch .h file.
The way I will push it is documented here:
https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/committing-changes-to-a-pull-request-branch-created-from-a-fork
----------------------------------------------------------------
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] davids5 commented on issue #459: stm32h7: support
SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
davids5 commented on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-596096058
@patacongo recognizing your are not failure with github.
There is a `suggested change` that was requested.
I have asked you to use that feature and not waste time on adding comments that are not actionable when they can be. The contributor can then merge the suggestion and all our efforts add value not just criticism. Once the review is done the contributor can squash and force push to remove all the bable commits.
This PR is waiting on that action by @anpaza
----------------------------------------------------------------
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] davids5 edited a comment on issue #459: stm32h7:
support SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
davids5 edited a comment on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-597614597
@anpaza
> Do you mean:
> a) The rulers in file stm32h7x3xx_rcc.h should be extended to cover the longest right-side comment
> b) All the comments should be right-side of the respective #define
> I can give one more try.
Bravo!
Yes. The way the file is on master, it only needed the blocks extended.
----------------------------------------------------------------
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] davids5 commented on a change in pull request
#459: stm32h7: support SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#discussion_r397833561
##########
File path: arch/arm/src/stm32h7/stm32_allocateheap.c
##########
@@ -60,33 +60,57 @@
#include "hardware/stm32_memorymap.h"
#include "stm32_mpuinit.h"
#include "stm32_dtcm.h"
+#include "stm32_fmc.h"
/****************************************************************************
* Pre-processor Definitions
****************************************************************************/
-/* Internal SRAM is available in all members of the STM32 family. The
- * following definitions must be provided to specify the size and
- * location of internal(system) SRAM:
+/* At startup the kernel will invoke up_addregion() so that platform code
+ * may register available memories for use as part of system heap.
+ * The global configuration option CONFIG_MM_REGIONS defines the maximal
+ * number of non-contiguous memory ranges that may be registered with the
+ * system heap. You must make sure it is large enough to hold all memory
+ * regions you intend to use.
*
- * In addition to internal SRAM, external RAM may also be available through
- * the FMC. In order to use FMC RAM, the following additional things need
- * to be present in the NuttX configuration file:
+ * The following memory types can be used for heap on STM32H7 platform:
*
- * CONFIG_STM32H7_FMC=y : Enables the FMC
- * CONFIG_STM32H7_FMC_S[D]RAM=y : SRAM and/or SDRAM is available via the FMC.
- * Either of these autoselects
- * CONFIG_ARCH_HAVE_HEAP2 which is what we
- * are interested in here.
- * CONFIG_HEAP2_BASE : The base address of the external RAM in
- * the FMC address space
- * CONFIG_HEAP2_SIZE : The size of the external RAM in the FMC
- * address space
- * CONFIG_MM_REGIONS : Must be set to a large enough value to
- * include the FMC external RAM (as determined
- * by the rules provided below)
+ * - AXI SRAM is a 512kb memory area. This will be automatically registered
+ * with the system heap in up_allocate_heap, all the other memory
+ * regions will be registered in up_addregion().
+ * So, CONFIG_MM_REGIONS must be at least 1 to use AXI SRAM.
*
- * CONFIG_STM32H7_DTCMEXCLUDE : Set to exclude the DTCM from heap
+ * - Internal SRAM is available in all members of the STM32 family.
+ * This is always registered with system heap.
+ * There are two contiguous regions of internal SRAM:
+ * SRAM1+SRAM2+SRAM3 and SRAM4 at a separate address.
+ * So, add 2 more to CONFIG_MM_REGIONS.
+ *
+ * - Tightly Coupled Memory (TCM RAM), we can use Data TCM (DTCM) for system
+ * heap. Note that DTCM has a number of limitations, for example DMA
+ * transfers to/from DTCM are impossible.
Review comment:
```suggestion
* transfers to/from DTCM are limited.
```
----------------------------------------------------------------
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] johannes-nivus commented on issue #459: stm32h7:
support SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
johannes-nivus commented on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-597670246
> The text width has grown to 180 chars :)
> Other than that, nxstyle is happy.
>
> Also found a small bug in nxstyle.c:
>
> ```
> if (len < min)
> {
> min = len;
> lineno_min = lineno_min;
> }
> ```
>
> should have same fix as was done earlier for lineno_max:
>
> lineno_min = lineno;
Should have been fixed in #489
----------------------------------------------------------------
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 #459: stm32h7:
support SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-599788169
No, no meta-data in source files.
----------------------------------------------------------------
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] anpaza edited a comment on issue #459: stm32h7:
support SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
anpaza edited a comment on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-603750575
> @anpaza could you refine your patchset a little bit? now the PR contain 10 patches, and most patch fix the style issue. It's better that:
Never used rebase -i before, so it took a bit of time to read the internets :)
Now it looks a lot cleaner.
But davids5' branch nuttx-to-asf:stm32h7-fmc2_cs_fix should be broken 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on issue #459:
stm32h7: support SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-597118689
> > "If you want things done right, do it yourself."
>
> You could, for example, bring the changes onto a branch, revert only the rcc.h file, amend the PR, and merge what is left to master. Up to you.
I agree that committer could help contributor make the style change, but contributor shouldn't directly merge the change to mainline, the new PR still need go through the precheck/build process again. committer is human and human will make mistake, only tool can ensure there isn't error happen in this type of modification.
----------------------------------------------------------------
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 #459: stm32h7:
support SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-603155733
@anpaza could you refine your patchset a little bit? now the PR contain 10 patches, and most patch fix the style issue. It's better that:
1.One or two patch contain the real change
2.One patch contain only nxstyle fix
You can use "git rebase --interactive" to clean up the patch series, and git push -f 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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] davids5 commented on issue #459: stm32h7: support
SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
davids5 commented on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-603827257
> > @anpaza could you refine your patchset a little bit? now the PR contain 10 patches, and most patch fix the style issue. It's better that:
>
> Never used rebase -i before, so it took a bit of time to read the internets :)
> Now it looks a lot cleaner.
> But davids5' branch nuttx-to-asf:stm32h7-fmc2_cs_fix should be broken now :)
@anpaza Thank you!
We do not need to be concerned with the stm32h7-fmc2_cs_fix branch.
Please commit my suggestion and `rebase -i` one more time, the force push and we will merge this.
----------------------------------------------------------------
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 #459: stm32h7:
support SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-597103969
Most of the changes in the rcc.h file appears to be because the "ruler" is not long enough.
----------------------------------------------------------------
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] davids5 commented on issue #459: stm32h7: support
SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
davids5 commented on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-597095946
@patacongo nxstyle misled lead @anpaza to make make changes in the RCC file. I need to know if you will accept them or suggest he reverts the style changes to that 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] davids5 commented on a change in pull request
#459: stm32h7: support SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#discussion_r397833141
##########
File path: arch/arm/src/stm32h7/stm32_allocateheap.c
##########
@@ -60,33 +60,57 @@
#include "hardware/stm32_memorymap.h"
#include "stm32_mpuinit.h"
#include "stm32_dtcm.h"
+#include "stm32_fmc.h"
/****************************************************************************
* Pre-processor Definitions
****************************************************************************/
-/* Internal SRAM is available in all members of the STM32 family. The
- * following definitions must be provided to specify the size and
- * location of internal(system) SRAM:
+/* At startup the kernel will invoke up_addregion() so that platform code
+ * may register available memories for use as part of system heap.
+ * The global configuration option CONFIG_MM_REGIONS defines the maximal
+ * number of non-contiguous memory ranges that may be registered with the
+ * system heap. You must make sure it is large enough to hold all memory
+ * regions you intend to use.
*
- * In addition to internal SRAM, external RAM may also be available through
- * the FMC. In order to use FMC RAM, the following additional things need
- * to be present in the NuttX configuration file:
+ * The following memory types can be used for heap on STM32H7 platform:
*
- * CONFIG_STM32H7_FMC=y : Enables the FMC
- * CONFIG_STM32H7_FMC_S[D]RAM=y : SRAM and/or SDRAM is available via the FMC.
- * Either of these autoselects
- * CONFIG_ARCH_HAVE_HEAP2 which is what we
- * are interested in here.
- * CONFIG_HEAP2_BASE : The base address of the external RAM in
- * the FMC address space
- * CONFIG_HEAP2_SIZE : The size of the external RAM in the FMC
- * address space
- * CONFIG_MM_REGIONS : Must be set to a large enough value to
- * include the FMC external RAM (as determined
- * by the rules provided below)
+ * - AXI SRAM is a 512kb memory area. This will be automatically registered
+ * with the system heap in up_allocate_heap, all the other memory
+ * regions will be registered in up_addregion().
+ * So, CONFIG_MM_REGIONS must be at least 1 to use AXI SRAM.
*
- * CONFIG_STM32H7_DTCMEXCLUDE : Set to exclude the DTCM from heap
+ * - Internal SRAM is available in all members of the STM32 family.
+ * This is always registered with system heap.
+ * There are two contiguous regions of internal SRAM:
+ * SRAM1+SRAM2+SRAM3 and SRAM4 at a separate address.
+ * So, add 2 more to CONFIG_MM_REGIONS.
+ *
+ * - Tightly Coupled Memory (TCM RAM), we can use Data TCM (DTCM) for system
+ * heap. Note that DTCM has a number of limitations, for example DMA
+ * transfers to/from DTCM are impossible.
Review comment:
@anpaza was this copied from the another SoC? The H7 has a multiple DMA engines and DTCM is on the cross bar to this statment is not true.
----------------------------------------------------------------
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 #459: stm32h7:
support SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-597761012
> As already stated this should have been fixed in #489 or do I miss something?
No, it is just that the word "should" is ambiguous. "should" can imply that something done in the past, like "That should do it!" Or it can imply a failure to do something in the past as in "I should have done that". I resolved the ambiguity by assuming the latter. My mistake.
----------------------------------------------------------------
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] davids5 commented on a change in pull request
#459: stm32h7: support SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#discussion_r390278568
##########
File path: arch/arm/src/stm32h7/stm32_allocateheap.c
##########
@@ -246,6 +249,14 @@ void up_allocate_heap(FAR void **heap_start, size_t *heap_size)
up_heap_color(*heap_start, *heap_size);
#endif
+
+#if defined(CONFIG_DEBUG_FEATURES)
+
+ /* Display memory ranges to help debugging */
+
+ _info("%uKb of SRAM at %p\n", *heap_size / 1024, *heap_start);
Review comment:
So the `correct and best` thing to do is to look and see if the HW has an SDRAM in it. Then add a config and update board.h for thoses ones. It if does not then there is no action needed.
The issue is that, if you do not have the HW you can not verify it. But you are the `expert` on this at this point in time and can share that expertise in a board config so that when some does try to use it, they are closer to making it work that staring from scratch.
----------------------------------------------------------------
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] davids5 commented on issue #459: stm32h7: support
SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
davids5 commented on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-597092291
@anpaza - I think the tools is wrong and flagged things it should not have.
@patacongo would you please advise @anpaza on the rcc file and if we can take it in as is?
----------------------------------------------------------------
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] davids5 edited a comment on issue #459: stm32h7:
support SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
davids5 edited a comment on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-597132496
@johannes-nivus - no it was my mistake!
----------------------------------------------------------------
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 #459: stm32h7:
support SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-597195078
> I see, nxstyle.c has changed in the meanwhile.
> I fixed everything he says now, except this:
>
> arch/arm/src/stm32h7/Kconfig: info: No file extension
> arch/arm/src/stm32h7/Kconfig: info: Unknown file extension
>
> I hope this will not stop the automatic checks to pass.
>
This is just an information, don't stop precheck. Actually, a PR already remove this info:
https://github.com/apache/incubator-nuttx/pull/526
> P.S. Sidenote: in tools/checkpatch.h you could get the list of files in a commit much simpler:
> git show --name-only --pretty="" [94a73a8](https://github.com/apache/incubator-nuttx/commit/94a73a87363557b839b8c1bc032f7c77d02e5044)
> or even
> git show --name-only --pretty="" [94a73a8](https://github.com/apache/incubator-nuttx/commit/94a73a87363557b839b8c1bc032f7c77d02e5044)..HEAD
Yes, but checkpatch.sh support to just check the lines you really modify not the whole file. You can try something like this:
checkpatch.sh -r -g HEAD
And compare it with:
checkpatch.sh -g HEAD
----------------------------------------------------------------
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 #459: stm32h7:
support SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-597145174
> no it was my mistake!
Is someone going to fix that? I don't mind doing it but I don't want to collide with anyone.
BTW: The best fix is just to replace the header with the standard Apache 2.0 header.
----------------------------------------------------------------
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 edited a comment on issue #459:
stm32h7: support SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-599788169
No, no meta-data in source files. This has been discussed before. The coding standard prohibits meta data in sources (at least in reference to Deoxygen, but is generalization). Let's not go this way. Let's keep clean C code with no tool-specific meta-shit.
Furthermore, there is no exlusion from the file lenght in the coding standard. This would essential say that it is okay to violatie the coding standard. That is not a good thing.
You are basically proposing to run snot into C files so that they can freely violate the coding standard.
----------------------------------------------------------------
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] anpaza commented on issue #459: stm32h7: support
SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
anpaza commented on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-597006962
I don't understand what's wrong with nxstyle?
Your link says:
.github:1
Process completed with exit code 1.
what this means?
There are warnings in arch/arm/src/stm32h7/stm32h7x7xx_rcc.c and arch/arm/src/stm32h7/hardware/stm32h7x3xx_rcc.h but that's not my fault. Should I fix someone else's warnings?
----------------------------------------------------------------
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] anpaza commented on a change in pull request
#459: stm32h7: support SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
anpaza commented on a change in pull request #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#discussion_r390207163
##########
File path: arch/arm/src/stm32h7/stm32_allocateheap.c
##########
@@ -246,6 +249,14 @@ void up_allocate_heap(FAR void **heap_start, size_t *heap_size)
up_heap_color(*heap_start, *heap_size);
#endif
+
+#if defined(CONFIG_DEBUG_FEATURES)
+
+ /* Display memory ranges to help debugging */
+
+ _info("%uKb of SRAM at %p\n", *heap_size / 1024, *heap_start);
Review comment:
Tried compiling all stm32h7 targets:
stm32h747i-disco:nsh
nucleo-h743zi:nsh
nucleo-h743zi:nxlines_oled
nucleo-h743zi:pwm
without FMC they compile fine, with FMC enable they won't compile because their board.h does not define the respective macros, needed to configure FMC for their board (for example, BOARD_FMC_GPIO_CONFIGS should list the GPIOs used for external memories, without it FMC has no sense).
----------------------------------------------------------------
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 #459: stm32h7:
support SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-597682980
Wow... 180 seems excessive. We need to get you out of the critical path on this one so that you can move forward and have a good life.
But I can't help be believe that there are not just a few lines that are forcing that huge "ruler", just breaking up a few lines probably all that is necessary. But let's not burden you with that.
Many people have a habit of trying to say too much in register definition files. I am guilty of that at times too. It is easier to copy-paste a long bit description from a specificiation than it is to re-write a concise short description. The intention of the register definitions files is simply to provide a proper definition. The comment should not explain the detailed properties of the bit; the user should look in the reference manual for that.
----------------------------------------------------------------
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 #459: stm32h7:
support SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-597687373
> Also found a small bug in nxstyle.c:
@johannes-nivus Are you going to fix that one?
----------------------------------------------------------------
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] johannes-nivus commented on issue #459: stm32h7:
support SDRAM via FMC peripherial
Posted by GitBox <gi...@apache.org>.
johannes-nivus commented on issue #459: stm32h7: support SDRAM via FMC peripherial
URL: https://github.com/apache/incubator-nuttx/pull/459#issuecomment-597105619
Concerning nxstyle I have to comment:
As long as there are #defines below each other my revised nxstyle checks for all right of code comments beeing on the same column. You can set up a new column position by adding a blank line.
So I see no false positives in the output of nxstyle with stm32h7x3xx_rcc.h (of current master).
The nicer way to fix it would have been aligning the comments, and then extend the "ruler" to match the line length. In extreme case (to avoid a too long linelength) use multiline comments.
----------------------------------------------------------------
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