First line wraps one character early

[rtrussell]

Running this program:

MODE 3
PRINT STRING$(100, "0123456789")

results in the first line wrapping one character early on the Pico, but not when run locally in a native console; I don't understand why there should be a difference. Here is a screenshot from the Pico (PuTTY in Windows, the same thing happens with picoterm in Linux):

and here running locally (Windows):

[Memotech-Bill]

It seems to be related to an anomaly in the Pico SDK.

Presumably for historical reasons, the routine xeqvdu () mixes calls

    fwrite (&vdu, 1, 1, stdout);

and

    printf (...);

The Pico SDK provides its own version of printf (). It appears that the output from these two routines can get reversed. In particular, in the code:

    fwrite (&vdu, 1, 1, stdout) ;
    if ((col == rhs) && ((cmcflg & 1) == 0))
        {
        printf ("\015") ;
        col = 0 ;
        newline (&col, &row) ;
        }

The vdu character is being output after the CR, LF.

I have tried replacing all the calls of the form:

    fwrite (&vdu, 1, 1, stdout);

by

    printf ("%c", vdu);

That solves that particular issue, but now I am missing the ">" prompt after the MODE 3 statement.

I suspect there must also be an issue with the interaction of fflush (stdout) and printf (...), I have yet to resolve this.

[rtrussell]

Presumably for historical reasons, the routine xeqvdu () mixes calls

    fwrite (&vdu, 1, 1, stdout);

and

    printf (...);

It may well be because my experience with BASIC made me assume that using printf() to output a single character was wasteful, and that the fwrite() was more efficient. I didn't realise that mixing them was risky (my thinking was that if they both output to stdout they must cooperate sensibly).

[rtrussell]

I suspect there must also be an issue with the interaction of fflush (stdout) and printf (...)

Google suggests that fflush() should flush the output from printf(), for example this StackOverflow article so I'm suspicious that this might be a bug in the Pico SDK. I'm not sure where one should report that, but it may be worth doing so.

[Memotech-Bill]

All of those assumptions are reasonable in most cases, where all the output goes through a single library. They are almost certainly mandated by the Posix standard.

However, the Pico is anomalous, with fwrite and fflush being provided by newlib, but printf being a Pico custom routine. The two output paths only join somewhere in the I/O drivers. Hence the scope for inconsistency.

If I can create a simple example which illustrates the problem, I will raise an issue on the pico-sdk GitHub.

Edit: Raised an issue for the fwrite vs printf ordering here.

[rtrussell]

However, the Pico is anomalous, with fwrite and fflush being provided by newlib, but printf being a Pico custom routine.

The person who discovered the issue and reported it to me suggests the following fix: replace every printf(...); with fprintf(stdout, ...);. He's tried that and apparently it's resolved the issue for him.

If that works for you too, and you think it's a reasonable solution, I'm happy to go with it.

[Memotech-Bill]

Replacing fwrite by printf works. Replacing fwrite by putchar is perhaps better (although it looks as though the compiler optimises printf ("%c", ...) to putchar (...) anyway).

With regards to the missing ">" prompt after the MODE command, it looks as though it is being output, but in column 999!

00000000: 2e 2e 2e 2e 0d 0a 1b 5b 36 6e 1b 5b 33 30 3b 39  .......[6n.[30;9
00000010: 39 39 48 1b 5b 36 6e 1b 5b 33 30 3b 31 48 42 42  99H.[6n.[30;1HBB
00000020: 43 20 42 41 53 49 43 20 66 6f 72 20 50 69 63 6f  C BASIC for Pico
00000030: 20 43 6f 6e 73 6f 6c 65 20 76 30 2e 34 35 2c 20   Console v0.45, 
00000040: 42 75 69 6c 64 20 53 65 70 20 31 32 20 32 30 32  Build Sep 12 202
00000050: 33 2c 20 55 53 42 20 43 6f 6e 73 6f 6c 65 2c 20  3, USB Console, 
00000060: 55 41 52 54 20 43 6f 6e 73 6f 6c 65 2c 20 46 6c  UART Console, Fl
00000070: 61 73 68 20 46 69 6c 65 73 79 73 74 65 6d 2c 20  ash Filesystem, 
00000080: 63 79 77 34 33 3d 67 70 69 6f 2c 20 53 44 4c 20  cyw43=gpio, SDL 
00000090: 53 6f 75 6e 64 2c 20 2f 64 65 76 2f 75 61 72 74  Sound, /dev/uart
000000a0: 2c 20 4d 69 6e 20 53 74 61 63 6b 2c 20 53 74 61  , Min Stack, Sta
000000b0: 63 6b 20 43 0d 0a 1b 5b 36 6e 68 65 63 6b 20 34  ck C...[6nheck 4
000000c0: 2c 20 52 54 43 0d 0a 1b 5b 36 6e 28 43 29 20 43  , RTC...[6n(C) C
000000d0: 6f 70 79 72 69 67 68 74 20 52 2e 20 54 2e 20 52  opyright R. T. R
000000e0: 75 73 73 65 6c 6c 2c 20 32 30 32 33 0d 0a 1b 5b  ussell, 2023...[
000000f0: 36 6e 63 79 77 34 33 20 69 6e 69 74 69 61 6c 69  6ncyw43 initiali
00000100: 73 61 74 69 6f 6e 20 73 75 63 63 65 64 65 64 0d  sation succeded.
00000110: 0a 1b 5b 36 6e 3e 4d 4f 44 45 20 33 0d 0a 1b 5b  ..[6n>MODE 3...[
00000120: 36 6e 1b 5b 38 3b 32 35 3b 38 30 74 1b 28 42 1b  6n.[8;25;80t.(B.
00000130: 5b 33 37 6d 1b 5b 34 30 6d 1b 5b 48 1b 5b 4a 1b  [37m.[40m.[H.[J.
00000140: 5b 36 6e 1b 5b 36 6e 1b 5b 31 3b 39 39 39 48 1b  [6n.[6n.[1;999H.
00000150: 5b 36 6e 3e 1b 5b 31 3b 31 48 4c 49 53 54 0d 0a  [6n>.[1;1HLIST..
00000160: 1b 5b 36 6e 3e                                   .[6n>

Edit: Resolved this with an additional fflush.

[Memotech-Bill]

Committed required changes.

[rtrussell]

With regards to the missing ">" prompt after the MODE command, it looks as though it is being output, but in column 999!

This is part of the mechanism for determining the terminal width. BBC BASIC sends the terminal a 'move the cursor to column 999' command, to which the terminal responds by moving it to the right-most column. BASIC then interrogates the cursor position and by that means discovers the width in characters.

The cursor is moved back to the left-most column before the '>' is output.

[rtrussell]

The OP has made this comment on the committed changes (he isn't a member of GitHub):

"The solution of using printf is a mistake. The fact that the code needed an fflush to get the prompt back shows that it is unreliable - using fprintf does not require an fflush. You may have solved the problem with the mode 3 prompt but how many other untested display anomalies will require a random fflush to get the data in the correct order".

Perhaps you could comment. I don't know enough about how the Pico-SDK implementation of printf() differs from the standard one to judge what risk, if any, you are taking.

[Memotech-Bill]

The cursor is moved back to the left-most column before the '>' is output.

The problem is that the cursor was not moved back before the '>' is output. If you look at the dump I posted, the '>' is output before the "[1;1H". Another example of output not being in the order of the subroutine calls.

[Memotech-Bill]

Perhaps you could comment. I don't know enough about how the Pico-SDK implementation of printf() differs from the standard one to judge what risk, if any, you are taking.

I am reluctant to use fprintf. It requires more extensive changes, Also, to quote from the top of "pico-sdk/src/pico_printf/printf.c"

// \brief Tiny printf, sprintf and (v)snprintf implementation, optimized for speed on
//        embedded systems with a very limited resources. These routines are thread
//        safe and reentrant!
//        Use this instead of the bloated standard/newlib printf cause these use
//        malloc for printf (and may not be thread safe).````

[rtrussell]

I am reluctant to use fprintf. It requires more extensive changes, Also, to quote from the top of "pico-sdk/src/pico_printf/printf.c"

Fair enough. I'm aware that it would require a lot of changes to bbccos.c too, which would impact on the other Console Mode editions (unless we were to split out the Pico version of that module but that would be a shame). And there's something rather unsettling about replacing printf(..) with fprintf(stdout, ...) which as far as I know should normally act identically.

Perhaps before putting this to bed we should wait for the reaction to your report of the issue because I would hope that this would be treated as quite a serious bug. If they fix it upstream in a timely fashion it should remove the residual concerns that the OP (Colin Granville) has.

[Memotech-Bill]

On further investigation I am more confused about printf. It seams that in at least some cases it is using the newlib version anyway. There seems to be a define LIB_PICO_PRINTF_PICO which is related, but I can't find where.

The problem seems to be caused when the compiler optimises printf calls into putchar, which then bypasses the stdout buffering.

PicoBB already has a custom version of "bbccos.c" in order to provide the device specific star commands. That is the file which contains xeqvdu, which is where I had made the previous changes. Changing printf () to fprintf (stdout, ...) also requuires a significant number of changes in "bbpico.c".

I have resolved the issue by defining the following macro in "bbpico.c" and "bbccos.c":

#define printf(...) fprintf (stdout, __VA_ARGS__)

[rtrussell]

The problem seems to be caused when the compiler optimises printf calls into putchar, which then bypasses the stdout buffering.

That seems less an 'optimisation' and more a 'breaking change'!

PicoBB already has a custom version of "bbccos.c"

Are you sure? There are already #ifdef PICO statements in my 'master' bbccos.c, and I'm pretty sure that's the file I use when I build it here to generate the UF2 on my website: For example:

#ifdef PICO
#include "lfswrap.h"
extern char *szLoadDir ;  // @dir$
extern int dirlen;
#else
#include <dirent.h>
#endif

and

#ifdef PICO
#define COPYBUFLEN 512	// length of buffer used for *COPY command
#else
#define COPYBUFLEN 4096	// length of buffer used for *COPY command
#endif

Perhaps you can check and if necessary I'll incorporate any changes required so my build has the same star commands as yours.

[colin-repos]

Hi - I'm the person who informed Russell of this bug.

Replacing printf with fprintf (stderr. was fairly easy with search and replace. There's 1 case with no space between the name and (. Then you just need to take care of the 4 sprintf cases.

Can I suggest replacing printf with xprintf an the fwrites with xputchar

Then create macros

#define xprintf(...)  fprintf (stdout, __VA__ARGS__)

and

#define xputchar(c) whatever

Then you can change what functions are called easily.

[colin-repos]

Are you sure? There are already #ifdef PICO statements in my 'master' bbccos.c, and I'm pretty sure that's the file I use when I build it here to generate the UF2 on my website:

That may explain the different problems I have. I came across Russells uf2 when someone asked me to get it working with Hearsay on riscos. It turned out that at that time the version on Russells website would only connect to riscos usb intermittently. Memotech-Bills version was ok. I compiled an older version from Russells sources - that was ok and also didn't have a problem I have with both versions now and that is a 1 line width display caused by the response to the 999 tab being too slow despite it happening within 1 cs of receiving the tab. instruction.

[Memotech-Bill]

PicoBB already has a custom version of "bbccos.c"

Are you sure? There are already #ifdef PICO statements in my 'master' bbccos.c, and I'm pretty sure that's the file I use when I build it here to generate the UF2 on my website.

If you are using my Makefile to build, then you are using my version of bbccos.c. The attached file gives the differences.
bbccos.zip

[colin-repos]

re diffs
Nothing that would explain the slight variations I've seen in different builds.

[rtrussell]

If you are using my Makefile to build, then you are using my version of bbccos.c.

To make sure I pick up any changes I've made locally but not yet committed to GitHub, I copy my source files (bb*.c) over the top of yours as part of the build process (when I remember to, anyway, it's not currently automated).

So if your custom version of bbccos.c has the same filename I will be replacing it with mine, but if you've given it a different name (in the same way as you've replaced bbccon.c with bbpico.c) I won't.

The attached file gives the differences. bbccos.zip

That shows the importance of me doing what I do - you've not incorporated in your version the modification which fixes the problem of opening a file with a comma in its name (which is the principal reason for the latest update to the Console Mode editions).

Since there are rather a lot of additions I'm not sure it would be sensible to bloat the 'common' bbccos.c with all that code (even if conditional on PICO). Could you do some refactoring so that more of the Pico-specific code lives in bbpico.c, which is the proper place for it, rather than in bbccos.c? For example I could conditionally vector 'unrecognised' star commands to an external routine for you to handle there.

[rtrussell]

Replacing printf with fprintf (stderr. was fairly easy with search and replace.

Did you notice that bbccos.c would need to be changed as well? Since that's also used by the other Console Mode editions (Windows, MacOS, Linux, Raspberry Pi) it would have implications for bbccon.c too, even though that's not used by the Pico edition. The whole thing risks getting very messy,

It seems to me that by far the best solution is for the upstream bug to be fixed, although of course I don't know how long that may take. But making extensive modifications, which impact on all the Console Mode editions, as a temporary workaround for a Pico-SDK bug isn't very attractive.

[Memotech-Bill]

Since there are rather a lot of additions I'm not sure it would be sensible to bloat the 'common' bbccos.c with all that code (even if conditional on PICO). Could you do some refactoring so that more of the Pico-specific code lives in bbpico.c, which is the proper place for it, rather than in bbccos.c? For example I could conditionally vector 'unrecognised' star commands to an external routine for you to handle there.

I will have a look at this, however I am probably otherwise occupied for the next few days.

[colin-repos]

@rtrussell

Seems reasonable.

It's not the end of the world that the bug isn't fixed for me. What is important for me is an explanation of the problem so I know it isn't mine :-) I just don't like seeing symptoms fixing as opposed to a real fix.

It seems to me - not having any experience programming the pico that printf on a pico is not a complete printf more a means of abstracting output to different devices. Are putchar and printf in different libraries? If they don't work together - well that's fairly fundamental. the designers can't be that stupid it has to be deliberate.

Have you tried making stdout non buffered

[rtrussell]

I will have a look at this, however I am probably otherwise occupied for the next few days.

I understand, but I trust that you will at least update your bbccos.c to track mine, for example to incorporate the fix for opening files with a comma in the name. There will also definitely be more changes when I next update my repository, since I've fixed another issue locally.

At the moment I have the choice of making two builds: one with the comma fix but not the Pico-specific star commands, or one with the star commands but not the comma fix! It also means that a user cannot trust the version number in my Pico UF2 to accurately reflect what features it contains.

I know that sharing source files between projects is fraught, but it's the sort of thing that GitHub should make possible, so long as we're careful with where code specific to just one project (the Pico build in this case) goes.

[Memotech-Bill]

Since there are rather a lot of additions I'm not sure it would be sensible to bloat the 'common' bbccos.c with all that code (even if conditional on PICO). Could you do some refactoring so that more of the Pico-specific code lives in bbpico.c, which is the proper place for it, rather than in bbccos.c?

I have now refactored my code. The differences between my version of bbccos.c and yours are now:

15a16
> #include <picocos.h>
220c221
< 		fwrite (&vdu, 1, 1, stdout) ;
---
>         printf ("%c", vdu);
257c258
< 			fwrite (&vdu, 1, 1, stdout) ;
---
>             printf ("%c", vdu);
273c274
< 				fwrite (&vdu, 1, 1, stdout) ;
---
>                 printf ("%c", vdu);
308c309
< 			fwrite (&vdu, 1, 1, stdout) ;
---
>             printf ("%c", vdu);
369c370
< 			fwrite (&code, 1, 1, stdout) ;
---
>             printf ("%c", code);
408c409
< 				fwrite (&vdu, 1, 1, stdout) ;
---
>                 printf ("%c", vdu);

Full version attached. If you are able to accept this version, then I will remove my copy.

To make sure I pick up any changes I've made locally but not yet committed to GitHub, I copy my source files (bb*.c) over the top of yours as part of the build process (when I remember to, anyway, it's not currently automated).

Which folder do you copy your sources to? My makefile takes your sources from the folder PicoBB/BBCSDL/src, whereas my copy of bbccos.c is in folder PicoBB/src.
bbccos.zip

[rtrussell]

Full version attached. If you are able to accept this version, then I will remove my copy.

The only thing I'd say is that by redefining oscli in picocos.h it means that any changes that I make to that routine - and there is already a pending change that I've not committed to GitHub - are not automatically going to get picked up in the Pico edition.

To that extent what you've done is a bit of a cheat, because there is still a significant chunk of bbccos.c which you have effectively replaced by custom Pico code, just not as explicitly as having your own version of bbccos.c!

I do of course appreciate that this has made the 'refactoring' much more straightforward, so I can certainly see the attraction from your point of view. It's just that I have a residual concern that you might fail to spot (for example) bug fixes to the oscli routine and carry them across to yours.

But I don't want to rock the boat too much so I'll merge your bbccos.c with mine (there are other pending changes that aren't in your version) and we can always revisit the potential issues in the future.

Which folder do you copy your sources to? My makefile takes your sources from the folder PicoBB/BBCSDL/src, whereas my copy of bbccos.c is in folder PicoBB/src

Since it's (currently) a manual process I can't remember, but most likely (because I'm not familiar with the directory structure of the Pico build) I use find to locate all the bb*.c files and overwrite any that I find.

Should the BBC_SRC variable still work in your build? That ought to be an easier and safer way of forcing it to use my latest local copies of the 'core' source files, but I stopped using it because it seemed not always to work.

[Memotech-Bill]

The only thing I'd say is that by redefining oscli in picocos.h it means that any changes that I make to that routine - and there is already a pending change that I've not committed to GitHub - are not automatically going to get picked up in the Pico edition.

To that extent what you've done is a bit of a cheat, because there is still a significant chunk of bbccos.c which you have effectively replaced by custom Pico code, just not as explicitly as having your own version of bbccos.c!

Not really: My version of oscli ends by calling org_oscli. So your code handles all the star commands except those that are specific to the Pico plus four (display, output, refresh and screensave) for which the Pico has different implementations,

Should the BBC_SRC variable still work in your build? That ought to be an easier and safer way of forcing it to use my latest local copies of the 'core' source files, but I stopped using it because it seemed not always to work.

Yes, it shoud work.

[rtrussell]

Yes, it should work.

I bet, with what we know now, I concluded it was broken because it failed to replace bbccos.c with my local version!