search

Memotech-Bill / PicoBB

BBC BASIC for Raspberry Pi Pico
zlib License
35 stars 5 forks source link

Build missing network interface support routines #26

Open Memotech-Bill opened 4 months ago

Memotech-Bill commented 4 months ago

As per topic on Raspberry Pi forums, Richard Russel's build of PicoBB is missing network support routines.

>PRINT SYS "net_freeall"
         0

However my build has them:

>PRINT SYS "net_freeall"
 268532725

My build sequence is simply:

cd PicoBB/console/pico_w
make

We need to identify what is different about the way Richard builds the program.

Memotech-Bill commented 4 months ago

Looking at the signon messages, Richard's build has:

BBC BASIC for Pico Console v0.46, Build Jul 2 2024, USB Console, UART Console, Flash Filesystem, cyw43=gpio, SDL Sound, /dev/uart, Stack Check 4, RTC

Whereas my build has:

BBC BASIC for Pico W Console v0.46, Build Jul 3 2024, USB Console, UART Console, Flash Filesystem, cyw43=background, SDL Sound, /dev/uart, Min Stack, Stack Check 4, RTC

From this, I guess that Richard is building from the console/pico folder not the console/pico_w folder. Specifying BOARD=pico+w is not sufficient to enable WiFi support.

rtrussell commented 4 months ago

From this, I guess that Richard is building from the console/pico folder not the console/pico_w folder.

That's correct. My understanding has always been that the various build options are achieved by specifying them in the make command line. Indeed, referring back to the issue where this was previously discussed, these were the instructions you gave for building a Pico W version:

Then build the experimental version:

cd console/pico
make BOARD=pico_w CYW43=Y

Apart from the change to BOARD=pico+w that's what I am still doing.

Memotech-Bill commented 4 months ago

That's correct. My understanding has always been that the various build options are achieved by specifying them in the make command line. Indeed, referring back to the issue where this was previously discussed, these were the instructions you gave for building a Pico W version:

That is correct in principle. Doing make in the different folders just selects different default values.

However, as noted later in that thread:

* The CYW43 switch now supports a number of options to select the different SDK libraries:

  * CYW43=GPIO - Only support the GPIO functions (the onboard LED and power options). This version has the same amount of BASIC memory as the standard Pico build. It could become the default build, replacing the version for the standard Pico.
  * CYW43=POLL Use the cyw43_arch_poll library. Note that this does not eliminate callback functions. The callbacks are triggered when you call cyw43_arch_poll(), rather thai in response to a timer interrupt.
  * CYW43=BACKGROUND. Use my fixed version of the cyw43_arch_threadsafe_background library. I believe that this is the most convenient version to use with BASIC.

Looking at the CMakeLists.txt file, CYW43=Y should be equivalent to CYW43=BACKGROUND, so I am not quite sure why your build does not include network support. Are you still doing git checkout lwip_raw_tcp? That was an experimental branch for initial development of networking, and does not contain all the latest revisions.

Building in the console/pico_w folder was documented at the end of that thread.

rtrussell commented 4 months ago

However, as noted later in that thread:

Hmm, "later in the thread" is only useful if there was a reason for me to read it, and there probably wasn't!

Are you still doing git checkout lwip_raw_tcp?

I don't think so..

Most of my BBCSDL builds are as 'idiot proof' as I can make them (using a bog-standard makefile). Unfortunately there are two that aren't - Android and Pico - because in each case I'm reliant on a build process that I'm not familiar with: Android Studio in the first case and CMake in the second.

So in those cases I just blindly follow instructions that I've written down from a previous occasion and seem to work. I don't know why I'm doing what I'm doing, and unless some error message or obvious malfunction results, I am none the wiser.

To cut to the chase, what should I be doing to build the most 'universal' version, with support for the Pico W but with the minimum degree of impact (e.g. in respect of available memory) when it's running on a non-W Pico?

Memotech-Bill commented 4 months ago

To cut to the chase, what should I be doing to build the most 'universal' version, with support for the Pico W but with the minimum degree of impact (e.g. in respect of available memory) when it's running on a non-W Pico?

That depends upon whether or not you want network support. For a version that will run on either Pico or Pico W, but without network support:

cd PicoBB
git pull --recurse-submodules
git checkout master
cd console/pico
make BBC_SRC=<path to your source>

For a version that will run on both Pico and Pico W, and has network support on the Pico W, at the cost of reduced memory available to BASIC on either host, replace cd console/pico with cd console/pico_w.

A few words of explanation:

git pull will download from GitHub any changes I have made to my repository. The --recurse-submodules means that any changes to your repository, or the LittleFS repository, will also be downloaded.

get checkout master ensures that you are on the master branch. You probably will be anyway, this is just a precaution.

make builds the program. The option BBC_SRC=<path to your source> is only necessary if you have a local copy of your source that contains changes that you have not yet committed to GitHub.

As a check, after building the program, you can use the command:

picotool info <filename.uf2>

to show the signon message without having to load the program into a Pico. The significant part is the cyw43=... string. cyw43=gpio indicates support for the Pico W LED, while cyw43=background indicates network support (as well as the Pico W LED).

rtrussell commented 4 months ago

That depends upon whether or not you want network support.

I did say 'universal' version, so naturally that means it needs to support the key feature of the Pico W, i.e. WiFi (which I assume is what the W stands for). Other differences between the non-W and W versions are largely incidental as far as I'm concerned, and I would expect that they can be handled within, for example, a GPIO library.

How far did you get in making the memory costs of networking at least partly conditional on actually using the socklib library? The sort of thing I would expect is that calling PROC_initsockets would rejig the memory map, moving HIMEM down to make space for the networking buffers, before enabling the 'background' functionality.

Ideally (but less important, because one can always restart the Pico) calling PROC_exitsockets should disable the background functionality and then restore the memory map. But if that's not practical or too difficult so be it.

cd PicoBB git pull --recurse-submodules git checkout master cd console/pico make BBC_SRC=

That's similar to what I do, except that I think I use git clone rather than git pull, I don't know if that is significant. I don't use the BBC_SRC= option because at one point it didn't work properly, so my build includes a step in which I replace the source files in your cloned repository with mine. It may well be that I could use that option now.

I also explicitly replace the example programs and libraries with my versions, because commonly I will have made recent changes that aren't reflected in your repository.

Memotech-Bill commented 4 months ago

That depends upon whether or not you want network support.

I did say 'universal' version, so naturally that means it needs to support the key feature of the Pico W, i.e. WiFi (which I assume is what the W stands for). Other differences between the non-W and W versions are largely incidental as far as I'm concerned, and I would expect that they can be handled within, for example, a GPIO library.

The LED on the Pico W is controlled by a GPIO on the CYW43 chip, not a GPIO on the RP2040. It requires a different set of SDK routines to control this. If these are not present, no BBC BASIC library could control the Pico W LED.

How far did you get in making the memory costs of networking at least partly conditional on actually using the socklib library? The sort of thing I would expect is that calling PROC_initsockets would rejig the memory map, moving HIMEM down to make space for the networking buffers, before enabling the 'background' functionality.

All such memory allocation is deep within the lwip library. It is not obvious how one would donate some memory from that available to BBC BASIC.

cd PicoBB git pull --recurse-submodules git checkout master cd console/pico make BBC_SRC=

That's similar to what I do, except that I think I use git clone rather than git pull, I don't know if that is significant. I don't use the BBC_SRC= option because at one point it didn't work properly, so my build includes a step in which I replace the source files in your cloned repository with mine. It may well be that I could use that option now.

git clone downloads a copy of the entire repository, whereas git pull only downloads any changes sice the previous clone or pull.

rtrussell commented 4 months ago

The LED on the Pico W is controlled by a GPIO on the CYW43 chip, not a GPIO on the RP2040. It requires a different set of SDK routines to control this. If these are not present, no BBC BASIC library could control the Pico W LED.

Is that not very similar to the difference between the GPIO on the RPi 1-4 and the GPIO on the RPi 5? On the earlier models the GPIO is driven by the main SoC chip, but on the RPi 5 it's driven by a separate chip which communicates with the SoC using the I²C bus. The software interfaces are completely different, but I don't need to build different versions of BBC BASIC for the different models, it's entirely handled within the gpiolib.bbc library.

If you're saying that it's impossible for a single UF2 to be able to drive the GPIO on both the original Pico and the Pico W, I don't understand why that would be the case. Surely a UF2 could contain the drivers for both and a (notional) GPIO library use whichever interface is appropriate for the model on which it is running, similar to how it works on the Raspberry Pi itself?

If it really is impossible, it makes the whole idea of a 'universal' UF2 moot.

All such memory allocation is deep within the lwip library. It is not obvious how one would donate some memory from that available to BBC BASIC.

I thought that the CPU in the Pico was a relatively simple 'microcontroller' type device, lacking the kind of hardware memory-management which could enforce such partitioning. In its absence, I don't see how anything in the system could 'know' that BASIC is sharing the 'network' memory, so long as any background processes also accessing that memory are disabled.

If you examine the memory map of the WiFi-enabled version, is the memory allocated to networking contiguous with the memory that BASIC uses? If it is, or it can be persuaded to be (e.g. by controlling the order in which various modules are initialised) can't you disable the background processes and simply allow BASIC to extend its memory into that region. How could anything know you'd done that?

Memotech-Bill commented 4 months ago

The LED on the Pico W is controlled by a GPIO on the CYW43 chip, not a GPIO on the RP2040. It requires a different set of SDK routines to control this. If these are not present, no BBC BASIC library could control the Pico W LED.

Is that not very similar to the difference between the GPIO on the RPi 1-4 and the GPIO on the RPi 5? On the earlier models the GPIO is driven by the main SoC chip, but on the RPi 5 it's driven by a separate chip which communicates with the SoC using the I²C bus. The software interfaces are completely different, but I don't need to build different versions of BBC BASIC for the different models, it's entirely handled within the gpiolib.bbc library.

If you're saying that it's impossible for a single UF2 to be able to drive the GPIO on both the original Pico and the Pico W, I don't understand why that would be the case. Surely a UF2 could contain the drivers for both and a (notional) GPIO library use whichever interface is appropriate for the model on which it is running, similar to how it works on the Raspberry Pi itself?

I think we are talking at cross purposes. On the Pico, the LED is controlled by one of the RP2040 GPIO's. On the Pico W, that GPIO is repurposed to control the CYW43 chip. The LED is then controlled by one of the extra GPIOs on the CYW43 chip, which do not exist on the Pico. The option CYW43=GPIO (or CYW43=BACKGROUND) selects including the drivers for the CYW43 GPIO, necessary to control these additional GPIO, in order to provide a "Universal" UF2.

All such memory allocation is deep within the lwip library. It is not obvious how one would donate some memory from that available to BBC BASIC.

I thought that the CPU in the Pico was a relatively simple 'microcontroller' type device, lacking the kind of hardware memory-management which could enforce such partitioning. In its absence, I don't see how anything in the system could 'know' that BASIC is sharing the 'network' memory, so long as any background processes also accessing that memory are disabled.

If you examine the memory map of the WiFi-enabled version, is the memory allocated to networking contiguous with the memory that BASIC uses? If it is, or it can be persuaded to be (e.g. by controlling the order in which various modules are initialised) can't you disable the background processes and simply allow BASIC to extend its memory into that region. How could anything know you'd done that?

Moving HIMEM down makes space available right in the middle of the RAM used by BASIC, between the program and CPU stack.

I am not sure without looking whether the lwip network buffer allocation is static or dynamic.

If it is static, then the compiler will locate the buffers within the .bss region. Even if it could be arranged that the buffers are at the end of the region, and that this region was adjacent to the BASIC region, then in order to move this memory into or out of that available to BASIC, it would be necessary to dynamically adjust PAGE, not HIMEM. I don't think that can be done while a program is running.

If they are dynamically allocated, then they will be on the C heap. Either the heap is located below BASIC RAM, in which case the same issue as for static buffers arises, or the entire heap is located at HIMEM. Even then, moving HIMEM down would introduce space below the existing heap, but the heap grows upwards, not down. Also, the socklib library would be sitting between the original C heap and the freed space.

The only way I can see to make what you suggest work would be to modify the lwip code to use some form of custom memory allocation for the network buffers, making use of space either freed at HIMEM or perhaps allocated as BASIC variables. I am not currently inclined to undertake that.

Memotech-Bill commented 4 months ago

The current RAM memory map for the console/pico_w build is:

rtrussell commented 4 months ago

I think we are talking at cross purposes.

Let me try to clarify things with a couple of simple questions:

  1. Is it possible to create a 'universal' UF2 which contains code for driving both the Pico W GPIO (including the LED) and the non-W Pico GPIO (selected at runtime if necessary)?
  2. If that is possible, why make the inclusion of the Pico W driver code controllable by the CYW43=GPIO switch? Surely it's easier and less confusing to include it in every build as standard, rather than making it optional.

The only way I can see to make what you suggest work would be to modify the lwip code to use some form of custom memory allocation for the network buffers,

That would be the best way, certainly; other approaches are likely to involve some nasty assembly-language tricks to move the CPU's stack around without it noticing!

Obviously I've not seen the code itself, but I'm surprised that you don't think it's worth attempting. If we are still talking in terns of 40K of memory, which I think is what you originally suggested, that's a large amount to lose 'permanently' when it's only actually needed whilst socklib is in use.

Memotech-Bill commented 4 months ago
  1. Is it possible to create a 'universal' UF2 which contains code for driving both the Pico W GPIO (including the LED) and the non-W Pico GPIO (selected at runtime if necessary)?

Yes

  1. If that is possible, why make the inclusion of the Pico W driver code controllable by the CYW43=GPIO switch? Surely it's easier and less confusing to include it in every build as standard, rather than making it optional.

The makefiles specify CYW43=GPIO or CYW43=BACKGROUND (which includes GPIO) by default. It is necessary to explicitly specify CYW43=NONE to omit the driver.

Obviously I've not seen the code itself, but I'm surprised that you don't think it's worth attempting. If we are still talking in terns of 40K of memory, which I think is what you originally suggested, that's a large amount to lose 'permanently' when it's only actually needed whilst socklib is in use.

lwip is a large and complex library. It will take some research to even work out what and how memory is allocated. Depending on how the code is structured it may be necessary to fork the entire library in order to change the memory allocation, with subsequent support issues.

Also it is not clear that the network buffers are the main issue. It may be that much of the memory loss is due to the background processing of the CYW43 full driver.

I will do a bit more research.

rtrussell commented 4 months ago

It is necessary to explicitly specify CYW43=NONE to omit the driver.

OK. But - just so I'm absolutely clear - is that only if built in the console/pico_w directory or is it also included by default in the console/pico directory? I still haven't entirely got my head around the reason for having two different directories rather than a simple build option to enable networking, like all the other options.

lwip is a large and complex library. It will take some research to even work out what and how memory is allocated.

I don't doubt it, but even a complex library may well allocate the majority of its static and dynamic buffers quite near the start, and thereafter only refer to them symbolically (e.g. as a pointer), so it might not be complicated.

Also it is not clear that the network buffers are the main issue. It may be that much of the memory loss is due to the background processing of the CYW43 full driver.

Indeed, but if the background processing is completely disabled except for when socklib is active, they can hopefully all be lumped together.

Memotech-Bill commented 4 months ago

It is necessary to explicitly specify CYW43=NONE to omit the driver.

OK. But - just so I'm absolutely clear - is that only if built in the console/pico_w directory or is it also included by default in the console/pico directory? I still haven't entirely got my head around the reason for having two different directories rather than a simple build option to enable networking, like all the other options.

I currently maintain four builds:

For me it is most convenient to have separate folders for these. I simply have to do make without any parameters in each of console/pico, console/pico_w, bin/pico and bin/pico_w.

The "No Network" builds are retained because they do have more BASIC memory available than the "With Network" builds.

If you look at the Makefile in each of these folders, they are quite simple, they simply set the default value for each of the required parameters and then invoke src/pico/Makefile.

rtrussell commented 4 months ago

I currently maintain four builds:

Fair enough, but I only distribute a single 'as universal as possible' build for the Pico (just as I do for all the other platforms) and it's the build most people are going to come across - it seems to be what Paul Porcelijn is using.

The "No Network" builds are retained because they do have more BASIC memory available than the "With Network" builds.

Which of course is exactly what I am trying to avoid. It's not really viable to have to load a different UF2 into the Pico in order to run a BASIC program that uses network access and then revert back to the original UF2 in order to run a program that has a large memory usage.

In an ideal world it should be possible to write, for example, an application which initially accesses the network in order to download some data files and then performs some processing on that data which needs a lot of memory.

Memotech-Bill commented 4 months ago

I currently maintain four builds:

Fair enough, but I only distribute a single 'as universal as possible' build for the Pico (just as I do for all the other platforms) and it's the build most people are going to come across - it seems to be what Paul Porcelijn is using.

Which is currently the build in the console/pico_w folder. Although I have had support requests for the other builds.

The "No Network" builds are retained because they do have more BASIC memory available than the "With Network" builds.

Which of course is exactly what I am trying to avoid. It's not really viable to have to load a different UF2 into the Pico in order to run a BASIC program that uses network access and then revert back to the original UF2 in order to run a program that has a large memory usage.

I have identified in the memory map a fairly large static allocation associated with network buffers:

 COMMON         0x200059a8     0x98c7 CMakeFiles/bbcbasic.dir/home/pi/pico/pico-sdk/lib/lwip/src/core/memp.c.obj
                0x200059a8                memp_memory_RAW_PCB_base
                0x20005a1c                memp_memory_TCP_SEG_base
                0x20005c20                memp_memory_PBUF_POOL_base
                0x2000ebc4                memp_memory_PBUF_base
                0x2000ecc8                memp_memory_TCP_PCB_LISTEN_base
                0x2000edac                memp_memory_REASSDATA_base
                0x2000ee50                memp_memory_UDP_PCB_base
                0x2000eed4                memp_memory_TCP_PCB_base
                0x2000f20c                memp_memory_SYS_TIMEOUT_base

At present I have not worked out how to replace this.

Even if I do manage to modify the lwip code it is not clear where within the BASIC memory I can allocate storage,

rtrussell commented 4 months ago
  • socklib subroutines cannot change HIMEM.

Subroutines can change HIMEM but what they can't (easily and safely) do is change BASIC's stack pointer whilst it is in use!

But the key thing is easily and safely, libraries are allowed to (and the ones I write often do) break the normal rules of good coding practice, because they are 'black boxes' that end users shouldn't be fiddling with.

Here's a procedure that does succeed in lowering HIMEM, and BASIC's stack pointer, to free up some memory (here 10,000 bytes) which could be used for networking buffers:

      DEF PROCtest
      LOCAL b%%, d%%, s%%
      DIM s%% LOCAL TRUE, b%% LOCAL 10000, d%% LOCAL TRUE
      WHILE s%% < HIMEM
        ?d%% = ?s%%
        d%% += 1 : s%% += 1
      ENDWHILE
      HIMEM = d%%
      ENDPROC

Which commands removes data from the heap? CLEAR, NEW, any others?

CHAIN.

Memotech-Bill commented 4 months ago

Here's a procedure that does succeed in lowering HIMEM, and BASIC's stack pointer, to free up some memory (here 10,000 bytes) which could be used for networking buffers:

And suppose some intermediate subroutine has taken the address of a LOCAL variable?

rtrussell commented 4 months ago

And suppose some intermediate subroutine has taken the address of a LOCAL variable?

Give me an example, I can't see how it could fail so long as that code is the first thing in the procedure.

rtrussell commented 4 months ago

It could be useful to return the original value of HIMEM to the caller, for example so you can restore it later:

      DEF PROCtest(RETURN s%%)
      LOCAL b%%, d%%
      DIM s%% LOCAL TRUE, b%% LOCAL 10000, d%% LOCAL TRUE
      WHILE s%% < HIMEM
        ?d%% = ?s%%
        d%% += 1 : s%% += 1
      ENDWHILE
      HIMEM = d%%
      ENDPROC

In case it isn't obvious, the reserved memory for the networking buffer is at (the new value of) HIMEM, not at b%%.

Memotech-Bill commented 4 months ago

I have managed to find a way to reclaim 38KB of memory for BASIC if networking is available but not used.

In order to make it work, it requires the following code added to routine clear() in file bbmain.c in your repository:

#ifdef PICO
    pico_clear ();
#endif

With my code modification, buffers used for networking are allocated on the BASIC heap in a similar way to BASIC strings. Unused buffers are recycled, but the allocated space is not freed until an explicit or implicit CLEAR. As a consequence of this, CLEAR also closes all network connections.

Looking at the revised memory map it is not clear that any further memory can be reclaimed.

During development it was a nasty shock to find that the value of the BASIC stack pointer (esp) needed to check for available space was not valid in routines called back from LWIP. This was caused by the variable being declared as a register variable in BBC.h. This was eventually resolved by adding the compiler switches -ffixed-r10 -ffixed-r11 so that the LWIP code did not use these registers.

More testing is needed. Should you wish to test, then git clone or git pull the latest version of my repository, then in folder PicoBB/console/pico_w run the command make NET_HEAP=1.

rtrussell commented 4 months ago

I have managed to find a way to reclaim 38KB of memory for BASIC if networking is available but not used.

Excellent, I was hopeful that should be possible.

With my code modification, buffers used for networking are allocated on the BASIC heap

Unfortunately I don't think that will work, because one needs to be able to free the memory used for the buffers by calling PROC_exitsockets, so that for example the rest of the program can use that memory for processing data that it has previously downloaded from the network. If the buffers are allocated 'midway' through the heap there's no way of freeing it other than using CLEAR, which will likely throw away the very data that has been downloaded!

The sort of program structure that ideally needs to be supported is as follows:

PROC_initsockets : REM Initialise network access, including allocating buffers
REM. Download data from the net, for example into an array created on the heap
PROC_exitsockets : REM Free network buffers and close down network access
REM. Process the downloaded data, if necessary using the freed buffer memory.

It's a classic problem when your only options are to allocate buffers either on the heap or the stack, which is why I suggested allocating them by lowering HIMEM (for example using the code I recently published, and now documented at the Wiki). Memory above HIMEM is also better protected against accidental corruption (it's only too easy to splat the heap in BBC BASIC) and it would avoid the necessity to change bbmain.c.

This does of course still impose the limitation that PROC_initsockets and PROC_exitsockets can only safely be called when the stack is empty, i.e. from the 'main program', but that's not an uncommon limitation for libraries.

Another potential advantage is that it might allow for sockets to be kept open during a CHAIN (which calls CLEAR), in exactly the same way as files are kept open during CHAIN. This can allow multiple modules to access an open file or socket by passing its handle to the CHAINed module in a static integer variable.

Hopefully it will be as easy to allocate the buffers by lowering HIMEM as on the heap,

Memotech-Bill commented 4 months ago

What is wrong with the following?

Program 1:
    Download data from network
    Write data to file
    Chain to program 2

Program 2:
    Read data from file
    Process data

Since CHAIN clears variables, all memory is available for the analysis in program 2.

The problem is that the network code actually performs multiple small allocations while reading and writing network sockets.

Admittedly, the standard Pico SDK implementation (when using background processing) reserves a fixed static pool of memory, with size determined by constants in lwipopts.h, and then allocates memory out of this pool. This has two disadvantages:

If memory is allocated by moving HIMEM there are two options:

For a LOCAL structure, what is stored on the BASIC stack?

If either the format or data block, while these will be moved when the stack is moved, the pointers in the name block will not be updated with the new position.

rtrussell commented 4 months ago

What is wrong with the following?

I wouldn't say there is anything "wrong" with it, it's just messy and not something that a BBC BASIC programmer would expect to be necessary.

Admittedly, the standard Pico SDK implementation (when using background processing) reserves a fixed static pool of memory, with size determined by constants in lwipopts.h, and then allocates memory out of this pool. This has two disadvantages...

But are we really in the business of trying to 'improve' on what the "standard Pico SDK implementation" does (which is presumably what currently happens in the Pico W build)?

  • Allocate a fixed memory pool in PROCinitsockets. This has the disadvantages listed above.

It has those disadvantages, but it also has the advantages I described. You may take a different view, but it seems to me that the advantages outweigh the disadvantages. For example it doesn't matter too much if the static allocation is a little bigger than the total of the dynamic ones, if it can be freed for other purposes by calling PROC_exitsockets.

  • Move HIMEM and the BASIC stack down a bit more every time the network code needs more memory.

I entirely agree that's not acceptable, not least because of all the stack copying involved. One would want to allocate it all in one go, preferably at a time when the stack isn't in use, which is likely to be the case when PROC_initsockets is called. This is commonly the code that would be used, near the beginning of the program: 

INSTALL @lib$ + "socklib"
PROC_initsockets

which pretty much guarantees that the stack won't be in use at the time.

If you really can't do it any other way I'll live with the heap allocation, but I'm not prepared to change bbmain.c, not least because in no version of BBC BASIC, right back to the original in 1981, has CLEAR ever had any 'side effects'. It would be breaking new ground, which is just the sort of thing I'm keen to avoid.

Therefore it would have to be calling PROC_exitsockets, not CLEAR, that closes down the network and releases, but doesn't free, the buffers. This is really no more dangerous than any of the other existing libraries that will leave the system in an unstable state if the associated 'cleanup' function isn't called before the program is terminated.

I would just want to add, though, that the interpreter makes assumptions about when the heap can and can't grow. This is particularly the case when 'long strings' (and in the case of the Pico version that's not very long!) are stored temporarily in the unallocated part of the heap (that is, the pointer to free memory isn't raised). This would rule out your code allocating heap memory in an asynchronous callback.

Memotech-Bill commented 4 months ago

Your code PROCtest copies down the contents of the BASIC stack, but as far as I can see it does nothing to actually move the stack pointer to point to the new stack location.

I have tried doing this from within C code. Testing with lanchat which calls PROC_initsockets at the start, and with my version of PROC_initsockets beginning:

REM Initialise the BBCSDL Sockets interface
DEF PROC_initsockets(N%)
DEF PROC_initsockets
LOCAL chan%, err%
SYS "net_init"

I find that when net_init() is entered, himem and esp differ by 56 bytes, so even in the case where PROC_initsockets is called from the start of the main program there is a non-trivial BASIC stack.

In C routine net_init(), I can move himem, copy the BASIC stack to the new location and update esp. However in routine xeq(): case TSYS (in bbexec.c) the value of esp is remembered in a local variable on entry, and restored after return from my net_init() routine. So, by the time execution returns to BASIC, the stack pointer is pointing to the original copy of the stack, not the moved one.

The only way I can see of making this work is if the code for xeq(): case THIMEML was modified to relocate the BASIC stack whenever HIMEM is changed. Or make the variable oldesp in xeq () a global variable, rather than a local one, so that my net_init() code can set the value of esp that is eventually returned.

As a side issue, I think the C variable esp is confusingly / incorrectly typed in BBC.h. It is currently typed as heapptr *esr;, which implies double indirection, so that *esr points to the basic stack. I think it should be typed void *esr; or char *esr; since the value of esr is the pointer to the BASIC stack.

rtrussell commented 4 months ago

Your code PROCtest copies down the contents of the BASIC stack, but as far as I can see it does nothing to actually move the stack pointer to point to the new stack location.

"As far as you can see" is not far enough, because it works! The stack pointer is indeed moved to the new location, as can easily be demonstrated:

      PRINT "On entry:"
      PRINT "HIMEM = &"; STR$~HIMEM
      DIM s%% LOCAL -1
      PRINT "Stack pointer = &"; ~s%%
      PROCtest
      PRINT "On exit:"
      PRINT "HIMEM = &"; STR$~HIMEM
      DIM s%% LOCAL -1
      PRINT "Stack pointer = &"; ~s%%
      END

      DEF PROCtest
      LOCAL b%%, d%%, s%%
      DIM s%% LOCAL TRUE, b%% LOCAL 10000, d%% LOCAL TRUE
      WHILE s%% < HIMEM
        ?d%% = ?s%%
        d%% += 1 : s%% += 1
      ENDWHILE
      HIMEM = d%%
      ENDPROC

I have tried doing this from within C code.

Don't (you've already been caught by trying to manipulate the stack pointer external to the interpreter, which you are not supposed to do)! Do it in BASIC code in PROC_initsockets and then pass the new value of HIMEM into your C code using SYS, then everything will work correctly.

As a side issue, I think the C variable esp is confusingly / incorrectly typed in BBC.h.

Sorry if it's confusing, but it's correct. Declaring it as void would break the pointer arithmetic, because sizeof(void) is 1 (in GCC and Clang) whereas sizeof(heapptr) is 4, which is what we want. Doing it this way ensures that the stack pointer is always correctly aligned, whereas declaring it as void would make it possible to adjust the stack pointer by non-multiples of 4 bytes.

Memotech-Bill commented 4 months ago

I can only assume that it works because the stack space reserved by b%% LOCAL 10000 is not freed when the routine exits, even though it is defined as LOCAL.

In that case, how do I free that space in PROC_exitsockets and move the stack pointer back up?

rtrussell commented 4 months ago

In that case, how do I free that space in PROC_exitsockets and move the stack pointer back up?

This is one way (it's possible there is a better way, but I haven't given it much thought). In an ideal world one would probably want to pass the 'amount to reserve' or 'amount to free' as parameters rather than as embedded constants (10000) as here:

      PRINT "On entry:"
      PRINT "HIMEM = &"; STR$~HIMEM
      DIM s%% LOCAL -1
      PRINT "Stack pointer = &"; ~s%%
      PROCtest
      PRINT "After PROCtest:"
      PRINT "HIMEM = &"; STR$~HIMEM
      DIM s%% LOCAL -1
      PRINT "Stack pointer = &"; ~s%%
      PROCfree
      PRINT "After PROCfree:"
      PRINT "HIMEM = &"; STR$~HIMEM
      DIM s%% LOCAL -1
      PRINT "Stack pointer = &"; ~s%%
      END

      DEF PROCtest
      LOCAL b%%, d%%, s%%
      DIM s%% LOCAL TRUE, b%% LOCAL 10000, d%% LOCAL TRUE
      WHILE s%% < HIMEM
        ?d%% = ?s%%
        d%% += 1 : s%% += 1
      ENDWHILE
      HIMEM = d%%
      ENDPROC

      DEF PROCfree
      LOCAL b%%, d%%, s%%
      DIM s%% LOCAL TRUE, b%% LOCAL 7, d%% LOCAL TRUE
      b%% = 10000 + s%% - d%% - 4
      s%%!-12 += b%% : d%% = s%% + b%%
      WHILE s%% < HIMEM
        ?d%% = ?s%%
        d%% += 1 : s%% += 1
      ENDWHILE
      HIMEM = d%%
      ENDPROC
Memotech-Bill commented 4 months ago

Thanks for that. My version is:

 1 WHILE TRUE
 2 INPUT "Space to allocate", space%
 3 PRINT "Space requested = &";~space%
 4 PRINT "On entry:"
 5 PRINT "HIMEM = &"; STR$~HIMEM
 6 DIM s%% LOCAL -1
 7 PRINT "Stack pointer = &"; ~s%%
 8 PROC__socklib_rsrv(space%)
 9 PRINT "After PROC__socklib_rsrv:"
10 PRINT "HIMEM = &"; STR$~HIMEM
11 DIM s%% LOCAL -1
12 PRINT "Stack pointer = &"; ~s%%
13 PROC__socklib_free(space%)
14 PRINT "After PROC__socklib_free:"
15 PRINT "HIMEM = &"; STR$~HIMEM
16 DIM s%% LOCAL -1
17 PRINT "Stack pointer = &"; ~s%%
18 ENDWHILE
19 END
20 
21 REM Move HIMEM and BASIC stack down to make room for network heap
22 DEF PROC__socklib_rsrv(size%)
23 LOCAL b%%, d%%, s%%
24 DIM s%% LOCAL TRUE, b%% LOCAL size%-&20, d%% LOCAL TRUE
25 WHILE s%% < HIMEM
26 ?d%% = ?s%%
27 d%% += 1 : s%% += 1
28 ENDWHILE
29 HIMEM = d%%
30 ENDPROC
31 
32 REM Move HIMEM and BASIC stack up recovering space used by network heap
33 DEF PROC__socklib_free(size%)
34 LOCAL b%%, d%%, s%%
35 DIM s%% LOCAL TRUE, b%% LOCAL 7, d%% LOCAL TRUE
36 b%% = size% + s%% - d%% - &20
37 s%%!-12 += b%% : d%% = s%% + b%%
38 WHILE s%% < HIMEM
39 ?d%% = ?s%%
40 d%% += 1 : s%% += 1
41 ENDWHILE
42 HIMEM = d%%
43 ENDPROC

And test results:

Space to allocate? 8192
Space requested = &2000
On entry:
HIMEM = &2002A800
Stack pointer = &2002A7F8
After PROC__socklib_rsrv:
HIMEM = &20028800
Stack pointer = &200287F8
After PROC__socklib_free:
HIMEM = &2002A800
Stack pointer = &2002A7F8
Space to allocate? 4096
Space requested = &1000
On entry:
HIMEM = &2002A800
Stack pointer = &2002A7F8
After PROC__socklib_rsrv:
HIMEM = &20029800
Stack pointer = &200297F8
After PROC__socklib_free:
HIMEM = &2002A800
Stack pointer = &2002A7F8
Space to allocate? 2048
Space requested = &800
On entry:
HIMEM = &2002A800
Stack pointer = &2002A7F8
After PROC__socklib_rsrv:
HIMEM = &2002A000
Stack pointer = &20029FF8
After PROC__socklib_free:
HIMEM = &2002A800
Stack pointer = &2002A7F8
Space to allocate? 1024
Space requested = &400
On entry:
HIMEM = &2002A800
Stack pointer = &2002A7F8
After PROC__socklib_rsrv:
HIMEM = &2002A400
Stack pointer = &2002A3F8
After PROC__socklib_free:
HIMEM = &2002A800
Stack pointer = &2002A7F8
Space to allocate? 512
Space requested = &200
On entry:
HIMEM = &2002A800
Stack pointer = &2002A7F8
After PROC__socklib_rsrv:
HIMEM = &2002A600
Stack pointer = &2002A5F8

R0 = 00000043  R8  = FFFFFFFF
R1 = 00000000  R9  = 00000000
R2 = 00000000  R10 = 2000AB7F
R3 = 00000000  R11 = 2002A560
R4 = 2002A788  R12 = 200413D8
R5 = 00000043  SP  = 200415F8
R6 = 00000001  LR  = 100110E1
R7 = 00000001  PC  = 1000EEF4
SG = 2002A640  PSP = 61000000

Hard fault at line 39

So clearly a minimum that can be reclaimed.

rtrussell commented 4 months ago

So clearly a minimum that can be reclaimed.

It's the old memcpy vs memmove issue of the source and destination regions overlapping. It's highly unlikely to bite you, since you'll be freeing around 40 Kbytes, but if you really care you could call memmove rather than using the BASIC copy loop:

   32 REM Move HIMEM and BASIC stack up recovering space used by network heap
   33 DEF PROC__socklib_free(size%)
   34 LOCAL b%%, d%%, s%%
   35 DIM s%% LOCAL TRUE, b%% LOCAL 7, d%% LOCAL TRUE
   36 b%% = size% + s%% - d%% - &20
   37 s%%!-12 += b%% : d%% = s%% + b%%
   38 SYS "memmove", d%%, s%%, HIMEM - s%%
   39 HIMEM = d%% + HIMEM - s%%
   40 ENDPROC

Incidentally, it looks like you are trying to tweak the amount by which HIMEM is lowered/raised to be exactly equal to the passed parameter, by means of the - &20 adjustment in both PROC__socklib_rsrv() and PROC__socklib_free(). Just a heads-up that this won't work in general because the required adjustment varies between 32-bit and 64-bit versions, and between assembler and C versions, of the interpreter.

It's safer to accept that HIMEM will be moved a little more than the parameter you pass.

Memotech-Bill commented 4 months ago

Incidentally, it looks like you are trying to tweak the amount by which HIMEM is lowered/raised to be exactly equal to the passed parameter, by means of the - &20 adjustment in both PROC__socklib_rsrv() and PROC__socklib_free(). Just a heads-up that this won't work in general because the required adjustment varies between 32-bit and 64-bit versions, and between assembler and C versions, of the interpreter.

It's safer to accept that HIMEM will be moved a little more than the parameter you pass.

Since this code is only used in the Pico version of socklib I don't think that matters. The constant in the free routine had to be adjusted anyway, since, once I added the size% parameter to your routines, the amount of memory freed did not match that allocated.

I have now pushed my latest code to the repository. For now it still defaults to the original static allocation method. I will change this after further testing.

Should you wish to test, then git clone or git pull the latest version of my repository, then in folder PicoBB/console/pico_w run the command make NET_HEAP=1.

rtrussell commented 4 months ago

Since this code is only used in the Pico version of socklib I don't think that matters.

That's probably true, but I was thinking about publishing the routines at the BBC BASIC Wiki, since they are potentially of much wider application, and then they would need to be 'universal'.

Presumably if you are only allocating the memory in PROC_initsockets and freeing it in PROC_exitsockets you don't really need separate routines anyway, the necessary code could be included in-line, then nobody would be tempted to use it elsewhere.

The constant in the free routine had to be adjusted anyway, since, once I added the size% parameter to your routines, the amount of memory freed did not match that allocated.

The routines, as I supplied them, didn't always match anyway - they sometimes freed 4 bytes less than they allocated. This was deliberate, again for compatibility across the multiple interpreters; you'd have to call them a large number of times for it to matter.

Memotech-Bill commented 4 months ago

Presumably if you are only allocating the memory in PROC_initsockets and freeing it in PROC_exitsockets you don't really need separate routines anyway, the necessary code could be included in-line, then nobody would be tempted to use it elsewhere.

I thought about that, but since I don't really understand exactly how these routines work I was not sure what the effect of other variables (or code) would have on the allocation. And if people read the library source to discover these routines then they could also find and copy any inline source.

The routines, as I supplied them, didn't always match anyway - they sometimes freed 4 bytes less than they allocated. This was deliberate, again for compatibility across the multiple interpreters; you'd have to call them a large number of times for it to matter.

Providing you are confident that the free routine always releases less than (or equal to) that allocated. Freeing more than allocated would result in eating into the bottom of any libraries.

rtrussell commented 4 months ago

Providing you are confident that the free routine always releases less than (or equal to) that allocated. Freeing more than allocated would result in eating into the bottom of any libraries.

That's exactly my point. If you run your versions of the routines in BBC BASIC for Windows (coded in assembler) it will indeed try to free four bytes more than it allocates! Hence the -4 in my version which you removed.

The consequences could be even worse than corrupting libraries, for example BBC BASIC (z80) v5 doesn't support libraries and there's no guarantee there will be any memory at all above HIMEM!

Memotech-Bill commented 4 months ago

That's exactly my point. If you run your versions of the routines in BBC BASIC for Windows (coded in assembler) it will indeed try to free four bytes more than it allocates! Hence the -4 in my version which you removed.

So completely generic routines should probably identify the platform they are running on, and adjust behaviour appropriately.

rtrussell commented 4 months ago

So completely generic routines should probably identify the platform they are running on, and adjust behaviour appropriately.

I disagree, but I'm too tired to argue the point.

rtrussell commented 1 month ago

Should you wish to test, then git clone or git pull the latest version of my repository, then in folder PicoBB/console/pico_w run the command make NET_HEAP=1.

Having just got around to trying that, I'm falling at the first hurdle because my build process expects there to be a lib subdirectory, which there is in PicoBB/console/pico but there isn't in PicoBB/console/pico_w.

Making sure that the correct libraries are included with my build has always been an issue, and copying them into the lib subdirectory has been the way I do it. I'm not sure what to do if there isn't one!

Memotech-Bill commented 1 month ago

See the folder PicoBB/console/pico_w/examples/lib.

That includes the Pico version of socklib.

rtrussell commented 1 month ago

See the folder PicoBB/console/pico_w/examples/lib.

Oh. It seems strange (and confusing) that the directory structure is different for the WiFi version compared with the non-WiFi version. In addition, the two files that are in PicoBB/console/pico/lib/ are not in PicoBB/console/pico_w/examples/lib/!

Do we really need two versions any more, now you've managed to reduce the memory penalty if socklib isn't used?

Memotech-Bill commented 1 month ago

Oh. It seems strange (and confusing) that the directory structure is different for the WiFi version compared with the non-WiFi version.

Yes, I agree it is slightly inconsistent. I will probably fix at some point. I think examples/lib is slightly more logical as:

In addition, the two files that are in PicoBB/console/pico/lib/ are not in PicoBB/console/pico_w/examples/lib/!

I don't duplicate files in multiple source folders. Instead PicoBB/src/lfsutil/pico_examples.py assembles the complete filesystem in build/tree from multiple source folders under the control of PicoBB/pico_examples.cfg.

Do we really need two versions any more, now you've managed to reduce the memory penalty if socklib isn't used?

Perhaps not. It will still be possible to produce builds without network support by specifying the appropriate options. I may remove the Non-W folders when I produce a build for the Pico 2. I have some trial code, but currently no hardware to test it on.

rtrussell commented 1 month ago

I agree it is slightly inconsistent. I will probably fix at some point. I think examples/lib is slightly more logical

I think this probably arose because BBCSDL (the GUI version) has a huge number of examples (and associated resource files), more than 800 in all! So they're organised in BBCSDL/examples which sits alongside BBCSDL/lib. The console editions, by contrast, only have about 20 example files so it didn't seem worth the effort of putting them in a subdirectory where a user might not even notice them.

I don't duplicate files in multiple source folders. Instead PicoBB/src/lfsutil/pico_examples.py assembles the complete filesystem in build/tree from multiple source folders under the control of PicoBB/pico_examples.cfg.

Ah, so you're saying I could copy library files into either PicoBB/console/pico/lib or PicoBB/console/pico_w/examples/lib and the end result will be the same? In that case I should probably put them where I have before.

Memotech-Bill commented 1 month ago

Ah, so you're saying I could copy library files into either PicoBB/console/pico/lib or PicoBB/console/pico_w/examples/lib and the end result will be the same? In that case I should probably put them where I have before.

Providing you copy the files before make, and do the build in console/pico_w then yes. If you do the build in console/pico, then files in console/pico_w/examples/lib will not be copied.

rtrussell commented 1 month ago

Next problem (sorry):

CMake Error at symbols/CMakeLists.txt:11 (if):
  if given arguments:

    "STREQUAL" "rp2350"

  Unknown arguments specified

Do I perhaps need to update the Pico SDK and/or my other tools (I'm running quite an old PiOS)?

Memotech-Bill commented 1 month ago

I am using v2.0 of the SDK. In fact, owing to an issue with some of the static_assert statements in v2.0 of the SDK I am using the develop branch rather than the default master branch of the SDK.

I have just pushed a small change to my CMakeLists.txt file which will hopefully allow compilation with v1.x of the SDK.

I am still using the older Bulseye version of RPi OS, not the latest Bookworm.

rtrussell commented 1 month ago

I have just pushed a small change to my CMakeLists.txt file which will hopefully allow compilation with v1.x of the SDK.

Unfortunately there still seems to be an incompatibility with the version of the SDK I have:

*** ERROR: Header file /home/pi/pico/pico-sdk/src/common/pico_base_headers/include/pico/config.h not found

(there's no pico_base_headers directory in common).

If I were to update the SDK (using pico_setup.sh) would that cause other problems?

Memotech-Bill commented 1 month ago

I forgot that v2.0 of the SDK moved the location of a few of the header files.

As I mentioned, the default master branch of v2.0 of the SDK has issues with static_assert if building with older versions of GCC, see here and here. I can't remember whether this affects building for Pico_W or just Pico_2.

The moved header files only cause a problem because I parse them to get the function names to include for SYS. Let me think whether I can work around this.

Memotech-Bill commented 1 month ago

OK, I have pushed a change which makes the location of some header files depend upon SDK version.

Tested with SDK v2.0.

Hopefully I have also correctly recovered the paths for SDK v1.x, but not tested.

rtrussell commented 1 month ago

OK, I have pushed a change which makes the location of some header files depend upon SDK version.

That's an improvement, and it now proceeds much further, but I'm eventually getting this error:

cc1: fatal error: pico/platform/compiler.h: No such file or directory

As that's a relative directory I'm not too sure where it's looking. I've confirmed that an earlier version of PicoBB still builds successfully so I don't think my toolchain is broken.

Memotech-Bill commented 1 month ago

That was caused by a work-around for another SDK v2.0 bug.

My latest push makes the work-around SDK version specific.

rtrussell commented 1 month ago

My latest push makes the work-around SDK version specific.

Nearly there. Now it builds right up to the very last step, when uf2merge fails because filesystem_pwc.uf2 is empty:

ls -al *.uf2
-rw-r--r-- 1 pi pi 1217024 Oct 15 18:24 bbcbasic_pwc.uf2
-rw-r--r-- 1 pi pi       0 Oct 15 18:23 filesystem_pwc.uf2

Looks like the reason is that it runs out of space in the filesystem (I have copied extra files into /lib and /examples):

../../littlefs/lfs.c:691:error: No more free space 0xdd

although this doesn't abort the build and I had to search a long way back to find it.

Is it easy to tell by how much it overflowed, so I can try to find something to delete? Everything fitted with the non-w build.