Memotech-Bill / PicoBB

BBC BASIC for Raspberry Pi Pico
zlib License
32 stars 4 forks source link

Problems with using cursor position to determine screen width #20

Open colin-repos opened 9 months ago

colin-repos commented 9 months ago

When the pico powers up with your uf2 tinyusb makes your program output wait until DTR is asserted. The problem is the time between DTR being asserted and you expecting a reply from your terminal width enquiry is critical.

When Hearsay - the terminal program I'm modifying on riscos - starts up a terminal it asserts DTR early for modem connection - yes old school - and as a result misses your cursor position enquiry reply window which it handles when it is ready. As a result of not getting the cursor position enquiry early enough you default to a 1 column row. This isn't a problem if I reset the pico usb then reconnect from the terminal window

If I type MODE whatever the screen resizes with the esc[t command you send with a mode change.

Can't you start your program with a MODE whatever instead of inquiring the screen size? Then you are not dependent on a timely reply.

rtrussell commented 9 months ago

Can't you start your program with a MODE whatever instead of inquiring the screen size? Then you are not dependent on a timely reply.

The MODE statement (or VDU 22) relies on the terminal accepting the CSI 8 ... t escape sequence, which is an xterm extension and not a standard VT100/ANSI command. Some common terminals don't accept it, so I would not want BBC BASIC to be dependent on it being supported.

colin-repos commented 9 months ago

Ok how about a sanity check on the reply you use to set the width. It seems to me that I am out of sync with the pico and you are receiving the response to the start of line cursor position request and using it to set the screen width. Either that or you are defaulting to a 1 col screen.

rtrussell commented 9 months ago

Ok how about a sanity check on the reply you use to set the width.

The existing code (common to all the Console Mode editions, not just the Pico) assumes that a working bi-directional link to the terminal has been established before BBC BASIC is even started; if it hasn't all bets are off. With platforms other than the Pico that can usually be relied upon.

I would prefer that the initialisation phase which is specific to the Pico somehow ensures that this is the case before the 'common' code is entered. I don't know what that would involve, perhaps Bill can comment.

Memotech-Bill commented 9 months ago

When the pico powers up with your uf2 tinyusb makes your program output wait until DTR is asserted. The problem is the time between DTR being asserted and you expecting a reply from your terminal width enquiry is critical.

Not true. For the UART, only GND, TX and RX are used. On startup, PicoBB waits for either a USB connection, or a CR received from the TX line.

colin-repos commented 9 months ago

Over USB the pico waits for DTR. If I disable setting DTR in the terminal I receive nothing. No amount of key presses help - the key presses are being sent to pico I can see them from the serialusb drivers.

It's common to all the other uf2's I've tried

colin-repos commented 9 months ago

I should add that a while later when I set DTR manually output appears from the pico but is not the startup banner as you would expect it starts at the command prompt. However the terminal width is correct so I received the terminal width query.

Memotech-Bill commented 9 months ago

Over USB the pico waits for DTR. If I disable setting DTR in the terminal I receive nothing. No amount of key presses help - the key presses are being sent to pico I can see them from the serialusb drivers.

For USB I use a call to stdio_usb_connected () to determine whether or not a connection is established. That routine is provided by the SDK, and will rely upon routines within the TinuUSB library. I am not sure whether there is even any way of testing the value of USB DTR,

I would not wish to change this as it seems to be reliable on other platforms.

Memotech-Bill commented 9 months ago

Looking at the SDK code

bool stdio_usb_connected(void) {
#if PICO_STDIO_USB_CONNECTION_WITHOUT_DTR
    return tud_ready();
#else
    // this actually checks DTR
    return tud_cdc_connected();
#endif
}

So an option might be to define PICO_STDIO_USB_CONNECTION_WITHOUT_DTR for your build.

You will either have to edit the Makefile, or perhaps create a custom board definition file which defines this.

colin-repos commented 9 months ago

No I wouldn't change it it is necessary for your program to work otherwise your program would be outputting text when the pico was first plugged in and nothing was listening. Its a shame that toggling dtr doesn't restart like it does on an arduino.

Just for your information - it may help diagnose someone elses problem - I can pause a running basic program at any time by unsetting dtr and start it again by setting dtr so if they are not getting any output it could be dtr related

From my point of view it looks like the pico isn't starting paused - whether that is true or not I don't know. I clear DTR as early as possible in the serialusb drivers when the pico is plugged in - or reset - but I intermittently lose the banner info and start at the prompt.

rtrussell commented 9 months ago

I can pause a running basic program at any time by unsetting dtr and start it again by setting dtr

I assume that if you were very unlucky, and you paused it just at the moment when a cursor-position query had been sent but the reply not yet received, then it would misbehave.

But that's unavoidable in any situation when a timeout is involved - and a timeout is essential when needing to distinguish between pressing the Escape key and receiving an escape sequence.

I clear DTR as early as possible in the serialusb drivers when the pico is plugged in - or reset - but I intermittently lose the banner info and start at the prompt.

Why do you think Hearsay behaves this way when other terminals on other platforms don't, as far as I am aware? Are we hitting a fundamental weakness in RISC OS?

colin-repos commented 9 months ago

Are we hitting a fundamental weakness in RISC OS?

I haven't given up yet :-) I need to find out where the missing banners go. Its only a problem because of the screen size issue. I had to jump through hoops to stop that happening - far more complicated than it should be. Other uf2s don't have this problem is there some tinyusb intialisation missing by any chance - wishful thinking.

Hearsay works well as a serial terminal connected to linux over serial, usbserial and telnet - don't want to write an ssh module would you :-)

Memotech-Bill commented 9 months ago

I have added a make option USB_CON. You can build the program using the command:

make USB_CON=n

where n has the value 0-3:

0 = As previous versions (also the default if USB_CON is omitted). 1 = Require a CR on the USB connection. 2 = Define PICO_STDIO_USB_CONNECTION_WITHOUT_DTR=1. 3 = Define PICO_STDIO_USB_CONNECTION_WITHOUT_DTR=1 and require a CR on the USB connection.

I have to say that testing using picocom on an RPi, options 2 and 3 give strange results. However you may find that they work for your case.

colin-repos commented 9 months ago

They didn't really help 1 and 3 were the same. they worked sometimes but at least I didn't get the single column text but pressing a key is no real solution. 2 just gives single column text - even in putty which I think is expected.

I wouldn't make any changes. The problem is obviously at my end. Typing mode 3 fixes everything then things work normally

colin-repos commented 9 months ago

I have a solution. In waitconsole() remove "Waiting for connection" and everything works normally

#ifndef PICO_GUI
    //puts_raw("Waiting for connection\r\n");

The only explanation I have as to why other OS work is that they may suspend the devices until the app uses them so they may startup the device later. Riscos doesn't suspend usbdevices.

Note the putc_raw('.') never happens so presumably the device is already connected.

Memotech-Bill commented 9 months ago

Very well. I have redefined USB_CON. Valid values are now 0-7:

So you will require USB_CON=2.

colin-repos commented 9 months ago

I have a better solution for you, remove the '\n' in "Waiting for connection\r\n"

Note puts_raw outputs its own '\n' but its anyone's guess why the extra \n is a problem.

--- a/src/bbpico.c
+++ b/src/bbpico.c
@@ -2380,7 +2380,7 @@ static int waitdone=0;
 void waitconsole(void){
        if(waitdone) return;
 #ifndef PICO_GUI
-       puts_raw("Waiting for connection\r\n");
+       puts_raw("Waiting for connection\r");
     led_init ();
     int iLED = 0;
        while (true)
Memotech-Bill commented 9 months ago

From the pico/stdio.h:

/*! \brief puts variant that skips any CR/LF conversion if enabled
 * \ingroup pico_stdio
 */
int puts_raw(const char *s);

Are you sure it is not your USB connection which is doing the CR to CR+NL translation?

colin-repos commented 9 months ago

https://github.com/raspberrypi/pico-sdk/issues/1335

Yes I'm not receiving any data on USB. puts_raw and putchar_raw are writing to the UART thats why USB connections aren't seeing 'Waiting for connection' and the dots.

The linefeed is added even when there is no CRLF or CR, it's always output. If you remove the CR so you just have "Waiting for connection" the dots start at the end of the text on the line below - ie there is no CR so it doesn't return to the beginning of the line - unless you terminal treats LF as CRLF

rtrussell commented 9 months ago

Are you sure it is not your USB connection which is doing the CR to CR+NL translation?

puts() is specified as adding a LF: "puts() writes the string s and a trailing newline to stdout". In pico_stdio/stdio.c this is achieved by setting the third parameter newline of stdio_put_string() to true:

int puts_raw(const char *s) {
    int len = (int)strlen(s);
    stdio_put_string(s, len, true, true);
    stdio_flush();
    return len;
}

Everywhere you call puts_raw() in bbpico.c (and there are a lot) you append your own additional LF, for example:

    puts_raw ("\r\n");
    puts_raw("Waiting for connection\r\n");

The result will be for the output to be terminated with CR LF LF. Was that your intention? It seems not entirely in keeping with BBC BASIC generally using CRLF as the line termination.

If it also fixes the OP's issue I'd be in favour of changing all those puts_raw() calls to use either "\r" for a single line termination or "\r\n\r" if an extra line space is wanted.

Memotech-Bill commented 9 months ago

There are exactly two calls to puts_raw in my code, the two you identified,

Until a couple of commits ago, these were calls to printf, which required the \n. I changed them to puts_raw as part of the attempt to resolve the RISCOS connect issue.

I have removed the now spurious \n.

rtrussell commented 9 months ago

I have a better solution for you, remove the '\n' in "Waiting for connection\r\n"

Now Bill has made that change, where does it leave us? Naturally I want to make available for download as 'universal' a UF2 as I can; as I've said before, the great majority of users of BBC BASIC on a Pico will neither want to, nor have the means to, build it from source. Compile-time options like USB_CON= aren't helpful to them.

Memotech-Bill commented 9 months ago

Probably the most universal option is to build with USB_CON=2.

The "Waiting for connection" message and repeating stops are not essential for Windows or Linux users. They just provide a bit of "user friendlyness" when connecting by UART.

However, I am not inclined to change the default.

colin-repos commented 9 months ago

I just want to confirm that the latest version is working - it's obvious it should but I just wanted to confirm it.

Regarding the USB_CON=2 issue. The waiting for connection and dots is not an issue for USB connections. They are ignored before usb isn't connected and when usb is connected usb breaks out of the loop.

On a side note the waiting for connection message gave me the wrong idea when I tried out the uart. I thought it was going to connect itself - Maybe 'Press key to start' may be better.

One problem I do envisage with the waiting for connection and dots is if you have the pico powered up and connect the uart some time later you'll miss the message because it is outside the loop - or maybe I'm the only one who hotswaps uart connections. One solution may be to write the message inside the loop and keep overwriting it in the same position. Maybe it ends with a space and a dot which alternates with the led flash instead of increasing dots

Thanks for all your efforts. It's been very useful for debugging my end.