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/06/02 19:00:34 UTC
[GitHub] [incubator-nuttx] xiaoxiang781216 opened a new pull request #1180: libc: Remove the up_romgetc support from fgets
xiaoxiang781216 opened a new pull request #1180:
URL: https://github.com/apache/incubator-nuttx/pull/1180
since the fgets isn't decorated by IPTR
Signed-off-by: Xiang Xiao <xi...@xiaomi.com>
Change-Id: I2a83afb4d934a44ad1b56ec6dd72e654f4e112a3
## Summary
## Impact
## Testing
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] btashton commented on a change in pull request #1180: libc: Fix IPTR for puts/fputs/scanf
Posted by GitBox <gi...@apache.org>.
btashton commented on a change in pull request #1180:
URL: https://github.com/apache/incubator-nuttx/pull/1180#discussion_r437854688
##########
File path: libs/libc/stdio/lib_fputs.c
##########
@@ -60,7 +60,7 @@
****************************************************************************/
#if defined(CONFIG_ARCH_ROMGETC)
-int fputs(FAR const char *s, FAR FILE *stream)
+int fputs(FAR const IPTR char *s, FAR FILE *stream)
Review comment:
https://gcc.gnu.org/onlinedocs/gcc/Named-Address-Spaces.html shows `__mem` defined which is why I do not understand why it would not be in a "generic" avr gcc.
----------------------------------------------------------------
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 #1180: libc: Add IPTR for puts/fputs
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1180:
URL: https://github.com/apache/incubator-nuttx/pull/1180#discussion_r434522505
##########
File path: libs/libc/stdio/lib_fputs.c
##########
@@ -60,7 +60,7 @@
****************************************************************************/
#if defined(CONFIG_ARCH_ROMGETC)
-int fputs(FAR const char *s, FAR FILE *stream)
+int fputs(FAR const IPTR char *s, FAR FILE *stream)
Review comment:
I don't know very much about how IPTR works. I had a different solution using up_romgetc and the IPTR changes were contributed later. I have never used it
We should both learn more before we change any code.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #1180: libc: Add IPTR for puts/fputs
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1180:
URL: https://github.com/apache/incubator-nuttx/pull/1180#discussion_r434632495
##########
File path: libs/libc/stdio/lib_fputs.c
##########
@@ -60,7 +60,7 @@
****************************************************************************/
#if defined(CONFIG_ARCH_ROMGETC)
-int fputs(FAR const char *s, FAR FILE *stream)
+int fputs(FAR const IPTR char *s, FAR FILE *stream)
Review comment:
> Ok, I will restore to the previous one.
I apologize. I did not understand all of the issues.
----------------------------------------------------------------
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 #1180: libc: Add IPTR for puts/fputs
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1180:
URL: https://github.com/apache/incubator-nuttx/pull/1180#discussion_r434566698
##########
File path: libs/libc/stdio/lib_fputs.c
##########
@@ -60,7 +60,7 @@
****************************************************************************/
#if defined(CONFIG_ARCH_ROMGETC)
-int fputs(FAR const char *s, FAR FILE *stream)
+int fputs(FAR const IPTR char *s, FAR FILE *stream)
Review comment:
One thing that always bothered me about this whole concept is that it assumes that all strings lie in FLASH. Often, however, strings may be create in RAM via strcat or sprintf. In that case fputs() similar functions would still fail. There really is no good solution for AVR.
----------------------------------------------------------------
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 pull request #1180: libc: Remove the up_romgetc support from fgets
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #1180:
URL: https://github.com/apache/incubator-nuttx/pull/1180#issuecomment-637763581
This function did not need to be decorated with IPTR since it explicitly copied the bytes from ROM. IPTR is a special storage class for Harvard architecture maches that indicates that the pointed to data resides in instruction (ROM) space. In this case, it would instead explicitly copy from instruction space.
This function is older than IPTR. If you are going to remove this function, then could you please add IPTR to the string pointer argument?
----------------------------------------------------------------
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 #1180: libc: Remove the up_romgetc support from fgets
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1180:
URL: https://github.com/apache/incubator-nuttx/pull/1180#discussion_r434282251
##########
File path: libs/libc/stdio/lib_fputs.c
##########
@@ -60,7 +60,7 @@
****************************************************************************/
#if defined(CONFIG_ARCH_ROMGETC)
-int fputs(FAR const char *s, FAR FILE *stream)
+int fputs(FAR const IPTR char *s, FAR FILE *stream)
Review comment:
I think we are miscommunicating. The version of fouts should not be needed if IPTR is added. It is needed only if IPTR is not present. IPTR is a storage class that should automatically force the compiler to generate the special instructions to fetch the data from instruction space.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #1180: libc: Remove the up_romgetc support from fgets
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #1180:
URL: https://github.com/apache/incubator-nuttx/pull/1180#discussion_r434284152
##########
File path: libs/libc/stdio/lib_fputs.c
##########
@@ -60,7 +60,7 @@
****************************************************************************/
#if defined(CONFIG_ARCH_ROMGETC)
-int fputs(FAR const char *s, FAR FILE *stream)
+int fputs(FAR const IPTR char *s, FAR FILE *stream)
Review comment:
Ok, I will remove the CONFIG_ARCH_ROMGETC section, but if the compiler can generate the special instruction to read string from code region by IPTR, why we call up_romgetc for printf series?
----------------------------------------------------------------
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 #1180: libc: Remove the up_romgetc support from fgets
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1180:
URL: https://github.com/apache/incubator-nuttx/pull/1180#discussion_r434131699
##########
File path: libs/libc/stdio/lib_fputs.c
##########
@@ -59,51 +59,6 @@
*
****************************************************************************/
-#if defined(CONFIG_ARCH_ROMGETC)
-int fputs(FAR const char *s, FAR FILE *stream)
-{
- int nput;
- int ret;
- char ch;
-
- /* Make sure that a string was provided. */
-
-#ifdef CONFIG_DEBUG_FEATURES /* Most parameter checking is disabled if DEBUG is off */
- if (!s)
- {
- set_errno(EINVAL);
- return EOF;
- }
-#endif
-
- /* Write the string. Loop until the null terminator is encountered */
-
- for (nput = 0, ch = up_romgetc(s); ch; nput++, s++, ch = up_romgetc(s))
- {
- /* Write the next character to the stream buffer */
-
- ret = lib_fwrite(&ch, 1, stream);
- if (ret <= 0)
- {
- return EOF;
- }
-
- /* Flush the buffer if a newline was written to the buffer */
-
- if (ch == '\n' && (stream->fs_flags & __FS_FLAG_LBF) != 0)
- {
- ret = lib_fflush(stream, true);
- if (ret < 0)
- {
- return EOF;
- }
- }
- }
-
- return nput;
-}
-
-#else
int fputs(FAR const char *s, FAR FILE *stream)
Review comment:
```suggestion
int fputs(FAR const char *s, FAR IPTR FILE *stream)
```
The prototype in stdio.h would need the corresponding change.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #1180: libc: Remove the up_romgetc support from fgets
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #1180:
URL: https://github.com/apache/incubator-nuttx/pull/1180#issuecomment-637763581
This function did not need to be decorated with IPTR since it explicitly copied the bytes from ROM. IPTR is a special storage class for Harvard architecture maches that indicates that the pointed to data resides in instruction (ROM) space. In this case, it would instead explicitly copy from instruction space.
This function is older than IPTR. If you are going to remove this function, then could you please add IPTR to the string pointer argument? Currrently, the copy from ROM, either explicitly as here or implicitly using IPTR is required for all AVR builds.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #1180: libc: Fix IPTR for puts/fputs/scanf
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #1180:
URL: https://github.com/apache/incubator-nuttx/pull/1180#discussion_r437881376
##########
File path: libs/libc/stdio/lib_fputs.c
##########
@@ -60,7 +60,7 @@
****************************************************************************/
#if defined(CONFIG_ARCH_ROMGETC)
-int fputs(FAR const char *s, FAR FILE *stream)
+int fputs(FAR const IPTR char *s, FAR FILE *stream)
Review comment:
I don't know that whether __mem become the standard gcc feature. But if it's true we can create a new patch to remove all up_romgetc stuff later in one patch. At least, we need make the current code ase consistent with each other: it's very strange that printf call up_romgetc but scanf don't.
----------------------------------------------------------------
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 #1180: libc: Remove the up_romgetc support from fgets
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1180:
URL: https://github.com/apache/incubator-nuttx/pull/1180#discussion_r434282251
##########
File path: libs/libc/stdio/lib_fputs.c
##########
@@ -60,7 +60,7 @@
****************************************************************************/
#if defined(CONFIG_ARCH_ROMGETC)
-int fputs(FAR const char *s, FAR FILE *stream)
+int fputs(FAR const IPTR char *s, FAR FILE *stream)
Review comment:
I think we are miscommunicating. The version of fouts should not be needed if IPTR is added. It is needed only if IPTR is not present.
----------------------------------------------------------------
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 a change in pull request #1180: libc: Fix IPTR for puts/fputs/scanf
Posted by GitBox <gi...@apache.org>.
btashton commented on a change in pull request #1180:
URL: https://github.com/apache/incubator-nuttx/pull/1180#discussion_r437854688
##########
File path: libs/libc/stdio/lib_fputs.c
##########
@@ -60,7 +60,7 @@
****************************************************************************/
#if defined(CONFIG_ARCH_ROMGETC)
-int fputs(FAR const char *s, FAR FILE *stream)
+int fputs(FAR const IPTR char *s, FAR FILE *stream)
Review comment:
https://gcc.gnu.org/onlinedocs/gcc/Named-Address-Spaces.html shows `__memx` defined which is why I do not understand why it would not be in a "generic" avr gcc.
----------------------------------------------------------------
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 pull request #1180: libc: Remove the up_romgetc support from fgets
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #1180:
URL: https://github.com/apache/incubator-nuttx/pull/1180#issuecomment-637763581
This function did not need to be decorated with IPTR since it explicitly copied the bytes from ROM. IPTR is a special storage class for Harvard architecture maches that indicates that the pointed to data resides in instruction (ROM) space. In this case, it would instead explicitly copy from instruction space.
This function is older than IPTR. If you are going to remove this function, then could you please add IPTR to the string pointer argument? Currrently, the copy from ROM, either explicitly as here or implicitly using IPTR is required for all AVR builds. Removing the explicit ROM copy requires the addition of IPTR to input string pointer.
When IPTR is present for the AVR, it adds the storage class _memx to the string. The AVR compiler will then fetch all data from instruction space. It is a nice clean solution that does not require the explicit call that you are removing. If you add IPTR to the input string pointer, then full functionality should still be present.
----------------------------------------------------------------
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 #1180: libc: Add IPTR for puts/fputs
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1180:
URL: https://github.com/apache/incubator-nuttx/pull/1180#discussion_r434568001
##########
File path: libs/libc/stdio/lib_fputs.c
##########
@@ -60,7 +60,7 @@
****************************************************************************/
#if defined(CONFIG_ARCH_ROMGETC)
-int fputs(FAR const char *s, FAR FILE *stream)
+int fputs(FAR const IPTR char *s, FAR FILE *stream)
Review comment:
The safest thing to do would be to make no changes now. Or, perhaps as in your previous change, just add IPTR to the function prototype. I think this is one of those subjects that requires and clear understanding of the effected platforms and a plan for exactly how we want them to work. Let's not merge this until we have those things.
----------------------------------------------------------------
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 a change in pull request #1180: libc: Fix IPTR for puts/fputs/scanf
Posted by GitBox <gi...@apache.org>.
btashton commented on a change in pull request #1180:
URL: https://github.com/apache/incubator-nuttx/pull/1180#discussion_r437854321
##########
File path: libs/libc/stdio/lib_fputs.c
##########
@@ -60,7 +60,7 @@
****************************************************************************/
#if defined(CONFIG_ARCH_ROMGETC)
-int fputs(FAR const char *s, FAR FILE *stream)
+int fputs(FAR const IPTR char *s, FAR FILE *stream)
Review comment:
@xiaoxiang781216 Is there anything besides AVR that we support that uses this though? For AVR I am not clear why `__memx` would not be defined.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #1180: libc: Fix IPTR for puts/fputs/scanf
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #1180:
URL: https://github.com/apache/incubator-nuttx/pull/1180#discussion_r436276206
##########
File path: libs/libc/stdio/lib_fputs.c
##########
@@ -60,7 +60,7 @@
****************************************************************************/
#if defined(CONFIG_ARCH_ROMGETC)
-int fputs(FAR const char *s, FAR FILE *stream)
+int fputs(FAR const IPTR char *s, FAR FILE *stream)
Review comment:
@patacongo let's finish this request.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #1180: libc: Fix IPTR for puts/fputs/scanf
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #1180:
URL: https://github.com/apache/incubator-nuttx/pull/1180#issuecomment-641702165
Does anyone can help review this change?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo edited a comment on pull request #1180: libc: Remove the up_romgetc support from fgets
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #1180:
URL: https://github.com/apache/incubator-nuttx/pull/1180#issuecomment-637763581
This function did not need to be decorated with IPTR since it explicitly copied the bytes from ROM. IPTR is a special storage class for Harvard architecture maches that indicates that the pointed to data resides in instruction (ROM) space. In this case, it would instead explicitly copy from instruction space.
This function is older than IPTR. If you are going to remove this function, then could you please add IPTR to the string pointer argument? Currrently, the copy from ROM, either explicitly as here or implicitly using IPTR is required for all AVR builds.
When IPTR is present for the AVR, it adds the storage class _memx to the string. The AVR compiler will then fetch all data from instruction space. It is a nice clean solution that does not require the explicit call that you are removing. If you add IPTR to the input string pointer, then full functionality should still be present.
----------------------------------------------------------------
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 merged pull request #1180: libc: Fix IPTR for puts/fputs/scanf
Posted by GitBox <gi...@apache.org>.
btashton merged pull request #1180:
URL: https://github.com/apache/incubator-nuttx/pull/1180
----------------------------------------------------------------
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 a change in pull request #1180: libc: Fix IPTR for puts/fputs/scanf
Posted by GitBox <gi...@apache.org>.
btashton commented on a change in pull request #1180:
URL: https://github.com/apache/incubator-nuttx/pull/1180#discussion_r437901897
##########
File path: libs/libc/stdio/lib_fputs.c
##########
@@ -60,7 +60,7 @@
****************************************************************************/
#if defined(CONFIG_ARCH_ROMGETC)
-int fputs(FAR const char *s, FAR FILE *stream)
+int fputs(FAR const IPTR char *s, FAR FILE *stream)
Review comment:
Ok. `__memx` looks like it has been part of gcc since 2012.
----------------------------------------------------------------
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 #1180: libc: Remove the up_romgetc support from fgets
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1180:
URL: https://github.com/apache/incubator-nuttx/pull/1180#discussion_r434131699
##########
File path: libs/libc/stdio/lib_fputs.c
##########
@@ -59,51 +59,6 @@
*
****************************************************************************/
-#if defined(CONFIG_ARCH_ROMGETC)
-int fputs(FAR const char *s, FAR FILE *stream)
-{
- int nput;
- int ret;
- char ch;
-
- /* Make sure that a string was provided. */
-
-#ifdef CONFIG_DEBUG_FEATURES /* Most parameter checking is disabled if DEBUG is off */
- if (!s)
- {
- set_errno(EINVAL);
- return EOF;
- }
-#endif
-
- /* Write the string. Loop until the null terminator is encountered */
-
- for (nput = 0, ch = up_romgetc(s); ch; nput++, s++, ch = up_romgetc(s))
- {
- /* Write the next character to the stream buffer */
-
- ret = lib_fwrite(&ch, 1, stream);
- if (ret <= 0)
- {
- return EOF;
- }
-
- /* Flush the buffer if a newline was written to the buffer */
-
- if (ch == '\n' && (stream->fs_flags & __FS_FLAG_LBF) != 0)
- {
- ret = lib_fflush(stream, true);
- if (ret < 0)
- {
- return EOF;
- }
- }
- }
-
- return nput;
-}
-
-#else
int fputs(FAR const char *s, FAR FILE *stream)
Review comment:
```suggestion
int fputs(FAR IPTR const char *s, FAR FILE *stream)
```
The prototype in stdio.h would need the corresponding change.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #1180: libc: Add IPTR for puts/fputs
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #1180:
URL: https://github.com/apache/incubator-nuttx/pull/1180#discussion_r434617588
##########
File path: libs/libc/stdio/lib_fputs.c
##########
@@ -60,7 +60,7 @@
****************************************************************************/
#if defined(CONFIG_ARCH_ROMGETC)
-int fputs(FAR const char *s, FAR FILE *stream)
+int fputs(FAR const IPTR char *s, FAR FILE *stream)
Review comment:
Ok, I will restore to the previous 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
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #1180: libc: Remove the up_romgetc support from fgets
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1180:
URL: https://github.com/apache/incubator-nuttx/pull/1180#discussion_r434131699
##########
File path: libs/libc/stdio/lib_fputs.c
##########
@@ -59,51 +59,6 @@
*
****************************************************************************/
-#if defined(CONFIG_ARCH_ROMGETC)
-int fputs(FAR const char *s, FAR FILE *stream)
-{
- int nput;
- int ret;
- char ch;
-
- /* Make sure that a string was provided. */
-
-#ifdef CONFIG_DEBUG_FEATURES /* Most parameter checking is disabled if DEBUG is off */
- if (!s)
- {
- set_errno(EINVAL);
- return EOF;
- }
-#endif
-
- /* Write the string. Loop until the null terminator is encountered */
-
- for (nput = 0, ch = up_romgetc(s); ch; nput++, s++, ch = up_romgetc(s))
- {
- /* Write the next character to the stream buffer */
-
- ret = lib_fwrite(&ch, 1, stream);
- if (ret <= 0)
- {
- return EOF;
- }
-
- /* Flush the buffer if a newline was written to the buffer */
-
- if (ch == '\n' && (stream->fs_flags & __FS_FLAG_LBF) != 0)
- {
- ret = lib_fflush(stream, true);
- if (ret < 0)
- {
- return EOF;
- }
- }
- }
-
- return nput;
-}
-
-#else
int fputs(FAR const char *s, FAR FILE *stream)
Review comment:
```suggestion
int fputs(FAR const char *s, FAR IPTR FILE *stream)
```
----------------------------------------------------------------
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 #1180: libc: Add IPTR for puts/fputs
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1180:
URL: https://github.com/apache/incubator-nuttx/pull/1180#discussion_r434565450
##########
File path: libs/libc/stdio/lib_fputs.c
##########
@@ -60,7 +60,7 @@
****************************************************************************/
#if defined(CONFIG_ARCH_ROMGETC)
-int fputs(FAR const char *s, FAR FILE *stream)
+int fputs(FAR const IPTR char *s, FAR FILE *stream)
Review comment:
Looking in include/nuttx/compiler.h, I see that IPTR is not necessarily supported for all AVRs:
164 #elif defined(__AVR__)
165 # if defined(CONFIG_AVR_HAS_MEMX_PTR)
166 /* I-space access qualifiers needed by Harvard architecture */
167
168 # define IOBJ __flash
169 # define IPTR __memx
170
171 # else
172 /* No I-space access qualifiers */
173
174 # define IOBJ
175 # define IPTR
176 # endif
So, if __memx were not available, then yes you are right. AVR would have to fall back and use the up_romgetc() functions. Sorry about that.
Also the __memx qualifier is only available in the enhanced Atmel Studeo GCC compiler:
63 config AVR_HAS_MEMX_PTR
64 bool "Enable in-flash static const stings"
65 depends on AVR_ATMEL_AVR_TOOLCHAIN
66 select ARCH_DEBUG_H
67 default y
68 ---help---
69 Enabling this option (recommended) will place all constant
70 strings used for debugging and assertion in program memory
71 and allow the corresponding routines to get the strings
72 directly from there. This will dramatically decrease amount
73 of RAM needed to hold this static data.
If someone is using a generic AVR GCC compiler such as the one built by the NuttX buildroot, then __memx would not be available.
So you are right and I am wrong. The alternative versions with up_romgetc() are needed in those cases.
18 config ARCH_ROMGETC
19 bool "Support for ROM string access"
20 default n
21 ---help---
22 In Harvard architectures, data accesses and instruction accesses
23 occur on different buses, perhaps concurrently. All data accesses
24 are performed on the data bus unless special machine instructions
25 are used to read data from the instruction address space. Also, in
26 the typical MCU, the available SRAM data memory is much smaller that
27 the non-volatile FLASH instruction memory. So if the application
28 requires many constant strings, the only practical solution may be
29 to store those constant strings in FLASH memory where they can only
30 be accessed using architecture-specific machine instructions.
31
32 If ARCH_ROMGETC is defined, then the architecture logic must export
33 the function up_romgetc(). up_romgetc() will simply read one byte
34 of data from the instruction space.
35
36 If ARCH_ROMGETC is selected, certain C stdio functions are effected: (1)
37 All format strings in printf, fprintf, sprintf, etc. are assumed to lie
38 in FLASH (string arguments for %s are still assumed to reside in SRAM).
39 And (2), the string argument to puts and fputs is assumed to reside
40 in FLASH. Clearly, these assumptions may have to modified for the
41 particular needs of your environment. There is no "one-size-fits-all"
42 solution for this problem.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #1180: libc: Remove the up_romgetc support from fgets
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #1180:
URL: https://github.com/apache/incubator-nuttx/pull/1180#discussion_r434284152
##########
File path: libs/libc/stdio/lib_fputs.c
##########
@@ -60,7 +60,7 @@
****************************************************************************/
#if defined(CONFIG_ARCH_ROMGETC)
-int fputs(FAR const char *s, FAR FILE *stream)
+int fputs(FAR const IPTR char *s, FAR FILE *stream)
Review comment:
Ok, I will remove the CONFIG_ARCH_ROMGETC, but if the compiler can generate the special instruction to read string from code region by IPTR, why we call up_romgetc for printf series?
----------------------------------------------------------------
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