search

rtrussell / BBCSDL

BBC BASIC for SDL 2.0: for Windows, Linux (86), MacOS, Raspberry Pi, Android and iOS.
zlib License
215 stars 28 forks source link

Code modularity #42

Open Andy18650 opened 6 days ago

Andy18650 commented 6 days ago

Hi,

I'm in the process of porting BBC BASIC to a microcontroller-based development board. During this process, I found out that the current structure of BBCSDL code does not lend itself well to being ported over to multiple platforms. It seems several odd choices has been made during the writing of the code. For example, the use of system APIs such as unistd and dirent instead of stdio for stuff like file access, largely omitting the use of header files, etc. I see it as potential for improvement in terms of code modularity and I would like to contribute by cleaning up some code. However, before I commit myself to it, I would like to know whether there are specific reasons for those choices so that I don't go down the wrong path.

Best wishes, Andy Hu

rtrussell commented 6 days ago

The code as it stands has been successfully compiled on a wide range of platforms (including Windows, MacOS, various Linux flavours, Android, iOS, Emscripten and the Raspberry Pi Pico microcontroller) so I don't think there can be any major non-portable features, other than being very specific to GCC/Clang (it relies on several non-C-standard extensions and intrinsics provided by those compilers).

Since you are targetting a microcontroller I wonder if it would be helpful to liaise with Memotech-Bill who ported BBC BASIC to the Raspberry Pi Pico. It's possible that he encountered similar issues and found satisfactory workarounds that may also be applicable to your use case.

Edit: I should perhaps add, if you are not already aware, that much of the code was semi-mechanically translated from the assembly language source code of BBC BASIC for Windows. This 'translation' will have inevitably resulted in a large number of code 'peculiarities'!

Andy18650 commented 6 days ago

The microcontroller I'm working with is risc-v based. There is a port to risc-v but it is heavily modified for the Tube interface. I'm creating a standalone version. The toolchain is gcc based so most things works, but there are some issues:

  1. Some odd things are missing from the risc-v gcc, namely intptr_t. My x86 gcc toolchain (mingw) seem to define that as a singed integer, 32 or 64 bit depending on the architecture, which seems odd considering the name suggests that it's a pointer.
  2. IO functions seems to heavily relies on file-mapped IO, which the SDK of the microcontroller does not support. So I probably need to throw out the existing code and re-implement those functions but:
  3. There seems to be multiple functions doing the same thing, for example, there is one queue for input and one for keyboard, and stdin_handler seems to get characters from the input queue and put it in the keyboard queue? And there are osrdch and oswrch. Same for file access, there are osload, ossave, but also osopen, readb and osbget, writeb and osbput.
  4. The code within those functions are not easy to understand, I kind of guessed the assembly origin ofthe code just by the oddly large number of variables named after i386 registers, but there are also name one-character variables with no explanation for purpose, and they are often reused to save space on stack. I guess I'm just confused... Given enough time I'll probably figure it out myself, but I definitely appreciate getting in touch with someone who has managed to port this to a microcontroller.
rtrussell commented 6 days ago

Some odd things are missing from the risc-v gcc, namely intptr_t.

If the compiler doesn't have intptr_t (declared in stdint.h) it's not C99 compliant. This is the correct type for the use it has in the code; as a last resort you could always declare it yourself.

And there are osrdch and oswrch. Same for file access, there are osload, ossave, but also osopen, readb and osbget, writeb and osbput.

You seem to be criticising features of the BBC Micro and BBC BASIC, not this particular implementation. I suggest you find yourself a time machine and take this up with the designers back in 1981! :-)

It's perfectly true that all versions of BBC BASIC are constrained by the requirement (or at least desirability) of compatibility with a 43-year-old computer. In a few cases hindsight might suggest an alternative (or better) way of doing things, but we don't have that luxury.

Andy18650 commented 4 days ago

If the compiler doesn't have intptr_t (declared in stdint.h) it's not C99 compliant. This is the correct type for the use it has in the code; as a last resort you could always declare it yourself.

Apparently intptr_t and uintptr_t are marked as optional in C11 standard. That's small issue though, I just declare it in a ifdef statement.

You seem to be criticising features of the BBC Micro and BBC BASIC, not this particular implementation.

The issue is: I did not grow up with a BBC Micro, nor any BASIC computers. I know a thing or two about MS-BASIC but never looked into the implementation. So I apologize for not being able to distinguish features of BBC Micro/BASIC from features of the implementation.

With that said, I spent two whole days diving deep into this code and BBC BASIC overall, and although I now understand most of it, I still find some parts confusing:

  1. Artifacts from the assembly translation seems to be not constrained to poorly named variables. Some references to static variables seem to be 'spilling over'. For example, I find several references to vduq[10] while vduq only has 10 members. Turns out that they are referring to queue which comes right after vduq[]. This code will only work if vduq[] and queue are structured exactly as they are withou any padding. Also, filbuf[0] is a pointer to file buffers, while filbuf[1] and onwards are a list of file handles.
  2. The code seems to do some things by itself, things that application programs usually don't need to handle. For example, there is a complete terminal emulator with auto wrap-around and command history. I can get that command history is a nice feature but every terminal since the 80s has wrap-around, and terminal sizes are usually not under the control of the application anyway. So why not hand over the work to the terminal emulator when VDU 28 is not defined? Also, there is a complete buffered real/write system for all file access. However, the interface exposed is a byte-based one. This led me to believe that the file buffers are an implementation-specific feature and are not required to be compatible with the original BBC. I know that ADFS uses 256-byte sectors, but that should not be visible to any application software written in BASIC.
  3. In a lot of places compatibility with BBC BASIC for Windows is mentioned. From reading the code I realized that the compatibility is not only referring to commands and file formats, but also internal structure. For example, the bbdata file is kept in assembly and not translated to C, presumably to keep the original variable addresses in place? However, I fail to understand the reason behind this decision. Why go to the length of being compatible with implementation details when those details are not exposed to the user? Do people often PEEK and POKE variables directly into RAM to control stuff?

The above are my ignorant questions about this code. Also, I would love to get in touch with Memotech-Bill to learn more about how to port it to a microcontroller.

rtrussell commented 4 days ago

This code will only work if vduq[] and queue are structured exactly as they are without any padding. Also, filbuf[0] is a pointer to file buffers, while filbuf[1] and onwards are a list of file handles.

Correct.

So why not hand over the work to the terminal emulator

I have no idea how one would do that. I didn't even know that the VT100 had a line-editor built in, I can't remember having seen any reference to it in the ANSI/VT100 protocol docs.

It may be helpful to study the Architecture diagram in the README. It's only the files shown in green (in the central box) that define the BBC BASIC language and shouldn't be changed. You have total freedom in respect of bbccon.c and bbccos.c, you can change them or completely replace them; you may well want to do that in your application. The GUI edition doesn't use them anyway, it uses the files bbcmos.c, bbccli.c, bbcvtx.c and bbcvdu.c.

Of course compatibility with the BBC Micro and other versions of BBC BASIC will be affected by how you choose to implement the features in those files, but they are not strictly 'part of' the BBC BASIC language so the extent to which you want them to be compatible is up to you.

However, I fail to understand the reason behind this decision. Why go to the length of being compatible with implementation details when those details are not exposed to the user? Do people often PEEK and POKE variables directly into RAM to control stuff?

Absolutely they do (not PEEK and POKE, because they don't exist in BBC BASIC, but via indirection)! They are exposed to the user, for example in the Wiki here. It's a crude way of providing access to internal variables, but in keeping with how BBC BASIC worked in the old days.

It's the old hindsight thing again. One wouldn't do it that way if starting from scratch now.

I would love to get in touch with Memotech-Bill to learn more about how to port it to a microcontroller.

I linked to his GitHub repository so you can easily do that.