search

Memotech-Bill / PicoBB

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

Support for Raspberry Pi Pico W? #8

Closed rtrussell closed 2 years ago

rtrussell commented 2 years ago

Are you planning to support the Raspberry Pi Pico W? I'm assuming that all this would involve is adding to the list of functions callable by SYS those necessary to access the network stack (and then somebody writing a library to make use of them, ideally compatible with the existing BBC BASIC 'socklib' libraries).

Memotech-Bill commented 2 years ago

At he moment I don't have a Pico W on which to perform any development.

I do have another project which could use a Pico W for networking. If that goes ahead I will consider what is involved.

I have been looking at networking on a standard Pico, using Ethernet over USB. With not much success https://forums.raspberrypi.com/viewtopic.php?t=336605

rtrussell commented 2 years ago

At the moment I don't have a Pico W on which to perform any development.

Pimoroni have them in stock, but it's a maximum of one per customer!

rtrussell commented 2 years ago

I now have a Pico W here, so if you want anything tested I can do it for you. I wouldn't be surprised if the WiFi code in the kernel needs some additional stack to be allocated, meaning we might need to eat into BBC BASIC's memory map yet again. :-(

But I can see whole new range of applications for BBC BASIC opening up if WiFi can be supported.

Memotech-Bill commented 2 years ago

I still don't have a Pico W, but I took a quick look at the LWIP API.

LWIP has over 100 header files. I did not count how many function definitions. I don't think it is practical to include all of these in SYS.

Do you know which functions you need to call?

rtrussell commented 2 years ago

Do you know which functions you need to call?

The functions I call from the nearest equivalent existing library (the Windows one) are as follows; they may not all be essential:

"getpeername", "getprotobyname", "gethostbyname","gethostbyaddr", "getservbyname", "getservbyport"
"gethostname", "getsockopt", "setsockopt", "socket", "ioctl" , "close"
"listen", "accept", "select", "connect", "htonl", "htons", "bind"
"recv", "send", "recvfrom", "sendto", "inet_addr", "inet_ntoa"
Memotech-Bill commented 2 years ago

LWIP does (optionally) provide support for many of these functions. However in order to implement them it requires multi-threading support from the underlying operating environment. We do not have that on the Pico unless BBC BASIC is re-written yet again to run on top of FreeRTOS.

It may be that only two threads are required, one for BBC BASIC and one for LWIP. It is possible that this can be implemented using the two cores of the Pico. It would mean that the second core can no longer be used for sound generation. MUCH more research is required to implement this.

The other option is to use the "Raw TCP" API functions as documented here. (I have to say that I am finding the LWIP documentation fairly impenetrable). Do you think you could work with these?

rtrussell commented 2 years ago

The other option is to use the "Raw TCP" API functions

That would certainly be the more attractive solution, if it can be made to work, especially as I imagine there could be benefits in terms of stack usage etc. compared with using the higher-level functions.

What about UDP (which is supported by BBC BASIC on other platforms, and used by the netchat.bbc demo program)? Is there a similar set of 'Raw UDP' functions available?

What I would ideally like to see is some example code, using the raw API functions, to perform simple connect-send-receive-disconnect transactions so I can get a feel for exactly what is involved.

Memotech-Bill commented 2 years ago

Yes there are also "Raw UDP" API functions.

What I would ideally like to see is some example code, using the raw API functions, to perform simple connect-send-receive-disconnect transactions so I can get a feel for exactly what is involved.

Me too.

There are a couple of very basic examples in pico-examples.

Memotech-Bill commented 2 years ago

The problem I see with the Raw API functions is they work by making callbacks to user-supplied C routines. It would be necessary to fine some way of turning these to calls to BBC BASIC routines.

rtrussell commented 2 years ago

The problem I see with the Raw API functions is they work by making callbacks to user-supplied C routines.

Whether this is an issue depends on how the callbacks are used. If they are purely 'informative' (so typically declared as void) it's probably quite straightforward, but if they have to return something 'in a timely fashion' (so maybe declared as int) that could be more involved.

However I wouldn't be too concerned either way, Callbacks are widely used in BBC BASIC for Windows (there's a library to support them: callback.bbc) and in LB Booster (which uses the BB4W engine under the hood) and I've often been surprised at how trouble-free and reliable they can be.

Memotech-Bill commented 2 years ago

I have created a new (experimental) branch, which adds the following functions to SYS:

cyw43_arch_init, cyw43_arch_init_with_country, cyw43_arch_enable_sta_mode, cyw43_arch_enable_ap_mode, cyw43_arch_deinit, cyw43_arch_wifi_connect_blocking, cyw43_arch_wifi_connect_timeout_ms, cyw43_arch_wifi_connect_async, cyw43_arch_get_country_code, cyw43_arch_gpio_put, cyw43_arch_gpio_get, cyw43_arch_poll, cyw43_arch_lwip_begin, cyw43_arch_lwip_end, cyw43_arch_lwip_protect, tcp_arg, tcp_poll, tcp_sent, tcp_recv, tcp_err, tcp_new, tcp_new_ip_type, tcp_connect, tcp_close, tcp_shutdown, tcp_abort, tcp_listen_with_backlog, tcp_bind, tcp_recved, tcp_write, tcp_output, tcp_accept, tcp_poll, tcp_tcp_get_tcp_addrinfo, pbuf_alloc, pbuf_copy, pbuf_copy_partial, pbuf_free

To try this out, first ensure you have the latest Pico SDK

cd pico-sdk
git pull
git submodule init
git submodule update

Then build the experimental version:

cd ../PicoBB
git pull
git checkout lwip_raw_tcp
cd console/pico
make BOARD=pico_w CYW43=Y

This version reduces the space available to BASIC by 40kB (used by .bss, see map).

It builds, I can make no promises beyond that. Good luck.

Edit: Corrected name of branch.

rtrussell commented 2 years ago

This version reduces the space available to BASIC by 40kB (used by .bss, see map).

I was afraid the networking would be expensive in memory usage, but that's a lot! If one never calls cyw43_arch_init() can any of that memory be recovered for use by BASIC, or is it lost 'permanently' in that build whether or not the networking is actually used?

Memotech-Bill commented 2 years ago

can any of that memory be recovered for use by BASIC, or is it lost 'permanently' in that build whether or not the networking is actually used?

It is lost 'permanently'. It has to be allocated to .bss at link stage. I think much of it is for statically allocated buffers. It would be necessary to compare the link maps with and without networking to learn more details.

rtrussell commented 2 years ago

cd ../PicoBB git pull git checkout lwip_tcp_raw

Got this far, but it tells me:

"error: pathspec 'lwip_tcp_raw' did not match any file(s) known to git"

What did I do wrong?

Memotech-Bill commented 2 years ago

My error, a typo. It should be:

git checkout lwip_raw_tcp

I will edit the previous comment for the benefit of others.

rtrussell commented 2 years ago

My error, a typo. It should be: git checkout lwip_raw_tcp

Thanks, it creates the UF2 now. I'm getting this error, but I'm assuming it's safe to ignore it as I don't need a merged UF2:

make: ../../src/lfsutil/uf2merge: command not found

Attempting to test it on my Pico W I'm finding that it hangs at line 50 here, any thoughts why that might be?

   10 led_pin=0
   20 SYS "cyw43_arch_init" TO res%
   30 IF res% PRINT "WiFi init failed": STOP
   40 REPEAT
   50   SYS "cyw43_arch_gpio_put", led_pin, 1
   60   WAIT 25
   70   SYS "cyw43_arch_gpio_put", led_pin, 0
   80   WAIT 25
   90 UNTIL FALSE
Memotech-Bill commented 2 years ago

No immediate thoughts.

I am not likely to have any time to look at this until next week. And I still don't have a Pico W.

rtrussell commented 2 years ago

I still don't have a Pico W.

I believe they remain readily available (e.g. from the Pi Hut).

rtrussell commented 2 years ago

No immediate thoughts.

The limited amount of testing I've been able to do in the BBC BASIC environment hasn't shed much light. cyw43_arch_init() returns zero, indicating successful initialisation (although if you call it twice without a cyw43_arch_deinit() in between it crashes). I can call cyw43_arch_poll() without any issues being apparent, it returns OK. But calling either cyw43_arch_gpio_get() or cyw43_arch_gpio_put() hangs, and I have to de-power the Pico W to regain control.

I'm not sure how the various pico_cyw43_arch 'styles' are established, but can you confirm that as far as you are aware BBC BASIC is configured for the pico_cyw43_arch_lwip_poll style in which neither callbacks nor FreeRTOS functionality are required?

rtrussell commented 2 years ago

I've got a real-life application for BBC BASIC running on a Pico W (to control a 'solar battery', using code I already have running on Android) so if there's anything at all I can do to expedite getting the WiFi aspect working please let me know.

Memotech-Bill commented 2 years ago

PicoBB for Pico W is configured using the pico_cyw43_arch_lwip_threadsafe_background library. That is I believe the correct library for automatic servicing of the cyw43_driver and lwIP in background (the bullets and indenting of section 4.4.2 of the SDK documentation do not seem to be consistent).

During startup, PicoBB attempts to flash the onboard LED three times, and then continue to flash while waiting for either a USB connection or a CR on the serial port. For the Pico W build this was being done by calling cyw43_arch_gpio_put() (without having previously called cyw43_arch_init(). Do you see these flashes?

I have now pushed an update which calls cyw43_arch_init() before attempting to flash the LED.

I tried running this build on my Pico (non-W). Interestingly cyw43_arch_init() returns zero even though there is no cyw43 chip. GDB suggests that cyw43_arch_gpio_put() is then hanging with a stuck DMA transfer to SPI while attempting to update registers in the (non-existent) cyw43 chip.

I have now ordered a Pico W for myself, but it will not arrive until later in the week.

Things for you to try:

rtrussell commented 2 years ago

During startup, PicoBB attempts to flash the onboard LED three times, and then continue to flash... .(without having previously called cyw43_arch_init(). Do you see these flashes?

Yes, which is interesting because the published 'blink' program in C on which mine is based does call cyw43_arch_init().

What's even more interesting is that if I delete the call to cyw43_arch_init() in my BASIC program it works, and the LED flashes!

So if cyw43_arch_init() really isn't being called in the version I have, why does the LED flash (both using your startup code and my BASIC code)? And why does calling it (which the published C program suggests is necessary) break things?

Perhaps I should see if any of the other WiFi functionality works without calling cyw43_arch_init(). :)

You should probably restore your code to its previous state, because I doubt that the change you made will have helped, and it may well have stopped it working.

rtrussell commented 2 years ago

My best guess to explain both the documentation and the observed behaviour is that accessing the LED doesn't need cyw43_arch_init() to be called, but it shouldn't mind if it is. If that's correct, something is causing the call of cyw43_arch_init() to fail in such a way that it breaks access to the LED.

Would it perhaps be worth trying pico_cyw43_arch_lwip_poll instead of pico_cyw43_arch_lwip_threadsafe_background? That's the mode I was expecting to be using, because it says "This architecture matches the common use of lwIP on microcontrollers" (although admittedly the caveat that it "provides no multicore safety" could be an issue for video or sound from the second core).

I had expected to need to call cyw43_arch_poll() in order to avoid callbacks, interrupts or multi-threading.

Memotech-Bill commented 2 years ago

I had taken a look at the source of cyw43_arch_init() earlier. One of its main functions is to set up the timer function which provides the background calling of the lwip and cyw43 poll routines. Without this (or regularly calling cyw43_arch_poll()) the network functions are unlikely to work.

It is also clear from some of the GDB back-traces that calling cyw43_arch_gpio_put() does some initialisation of the cyw43 driver if not already initialised. It could be that then calling cyw43_arch_init() after the driver is already at least partially initialised is causing the problem.

rtrussell commented 2 years ago

It could be that then calling cyw43_arch_init() after the driver is already at least partially initialised is causing the problem.

That could be, I suppose, but it would indicate a particularly 'fragile' design in my opinion, bordering on a bug.

If it's the case, how can we proceed? Calling cyw43_arch_init() in your code is undesirable, for example the user might want to call cyw43_arch_init_with_country() instead. In principle you could call cyw43_arch_deinit() afterwards, but I've seen a reference to a bug which means multiple init/deinit calls currently cause problems.

Is the easiest thing simply to remove the LED-flashing from the initialisation, at least temporarily to prove the point one way or the other?

Memotech-Bill commented 2 years ago

There is certainly a deinit / init bug. The cyw43_arch_init() routine calls:

gpio_add_raw_irq_handler_with_order_priority(...)

but cyw43_arch_deinit() fails to call:

gpio_remove_raw_irq_handler(...)

So when cyw43_arch_init() is called a second time, it attempts to install a second handler, causing a panic.

but I've seen a reference to a bug which means multiple init/deinit calls currently cause problems.

It sounds as though the bug has already been reported, and will hopefully be fixed at some point.

Meanwhile, the cyw43_arch_deinit() is not very long. It should be possible to implement our own fixed version, and make the symbol table point to that version rather than the version in the SDK.

Edit: Not so easy. In order to remove the handler you need to know its address. But the handler routine has been declared as static, so the address is not available from another file.

Edit 2: Got past that problem. It is now hanging in the second call to lwip_init() (which is called from cyw43_arch_init())

Memotech-Bill commented 2 years ago

There are a couple of bugs in cyw43_arch_threadsafe_background. I have reported these https://github.com/raspberrypi/pico-sdk/issues/980

I have now pushed a large number of changes accumulated over the past week:

static bool is_cyw43_init = false;

int cyw43_arch_init_safe (void)
    {
    int iErr = 0;
    if ( ! is_cyw43_init )
        {
        iErr = cyw43_arch_init ();
        if ( iErr == 0 ) is_cyw43_init = true;
        }
    return iErr;
    }

int cyw43_arch_init_with_country_safe (uint32_t country)
    {
    if ( is_cyw43_init ) cyw43_arch_deinit ();
    int iErr = cyw43_arch_init_with_country (country);
    if ( iErr == 0 ) is_cyw43_init = true;
    return iErr;
    }

void cyw43_arch_deinit_safe (void)
    {
    if ( is_cyw43_init )
        {
        cyw43_arch_deinit ();
        is_cyw43_init = false;
        }
    }

Your test LED flash program now works (with the cyw43_arch_init() call).

The only thing left to do is to find out whether it is actually possible to establish a network connection!

rtrussell commented 2 years ago

I have now pushed a large number of changes accumulated over the past week:

Sounds like a lot of useful work, thanks.

  • 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.

I haven't yet understood how this works under the hood, but if it does great. I don't have any concerns about calling cyw43_arch_poll() if that proves to be necessary, because I expect calls to the socklib library to be blocking anyway (so long as ON TIME interrupts still run which they should).

The only thing left to do is to find out whether it is actually possible to establish a network connection!

Assuming I can straightforwardly build your new version, I'll have a look at that when more pressing things are out of the way,

Memotech-Bill commented 2 years ago

I was going to have a go at writing a BBC BASIC version of the WiFi scan example.

However I failed at the first hurdle. The cyw43_wifi_scan() routine requires a callback routine. I have found the documentation for the CALLBACK library. However I cannot find the library itself, it does not seem to be in your repro. Is the library specific to BBC BASIC for Windows?

Edit: I found the following statement here: "The following BB4W libraries are not available in BBCSDL: CALLBACK, COMLIB, HQSOUND (not needed), MDILIB, SORTSALIB, SPRITELIB, SUBCLASS, WINLIB*."

Are you willing to let me have a copy of the source of the BB4W CALLBACK library? It might give me some hints as to how to implement for the Pico.

rtrussell commented 2 years ago

However I failed at the first hurdle. The cyw43_wifi_scan() routine requires a callback routine.

I'm not too worried if one can't perform a scan, only today I installed a WiFi-connected piece of kit (an EV charger as it happens) which provided no way of scanning for WiFi services, it simply required the SSID and Password to be entered.

My concern would be if any of the low-level network functions, required to implement the 'socklib' library, need a callback. I've been assuming (at least, hoping) that they don't.

Are you willing to let me have a copy of the source of the BB4W CALLBACK library?

I don't think it would help, since it's dependent on multi-tasking (hopefully that's obvious, because the interpreter must be able to run BASIC code whilst the code which called the callback routine is waiting for it to return). But you don't need me to give you the source, just download and install the free trial version of BBC BASIC for Windows and get it from there.

Memotech-Bill commented 2 years ago

I'm not too worried if one can't perform a scan

I only picked that as it is about the simplest example of using the WiFi available.

My concern would be if any of the low-level network functions, required to implement the 'socklib' library, need a callback. I've been assuming (at least, hoping) that they don't.

The LWIP "Raw" API relies heavily on performing callbacks.

There is a higher level blocking API which does not require callbacks, but as mentioned previously that requires an underlying OS providing multiple threads.

the interpreter must be able to run BASIC code whilst the code which called the callback routine is waiting for it to return

I am thinking of some system where each registered callback routine creates a stub which pushes the current interpreter context onto the stack and then configures a new context to run the BASIC callback routine Return from that routine would then have to restore the previous interpreter context.

However, at present I don't understand the interpreter well enough to know what context needs to be saved, nor how / where to allocate the memory for the stub (probably as an opaque structure).

rtrussell commented 2 years ago

The LWIP "Raw" API relies heavily on performing callbacks.

It's a strange API indeed if heavy use is made of callbacks. In the Win32 API they are almost never used, and similarly in the SDL2 API. They can make programming in any language other then C/C++, not just BASIC, problematical.

Can you point me to the documentation of the 'raw' API? It doesn't seem to be in the main Pico SDK PDF.

rtrussell commented 2 years ago

Can you point me to the documentation of the 'raw' API? It doesn't seem to be in the main Pico SDK PDF.

I found what I assume to be the relevant API here but the only function I can see that requires a callback is raw_recv() which isn't too surprising because received data arrives asynchronously! Am I missing something?.

I would suggest that the easiest way of dealing with this isn't to handle the callback in BASIC code but to write it in C, just as you have your 'safe' functions. Arrange for it to store the packet in a buffer whose address was previously passed from BASIC either in a global variable or by calling a function using SYS.

Memotech-Bill commented 2 years ago

The most relevant part of the API is here. Routines tcp_connect(), tcp_recv(), tcp_sent(), tcp_err(), tcp_accept() and tcp_poll() all have callbacks.

There is a documentation wiki here. There is a document here which is no longer part of the official LWIP documentation, but I have found it to be the easiest to understand.

I am currently considering four options:

  1. Trying to implement BASIC callbacks.
  2. Seeing whether two threads (one on each core) is sufficient to implement the LWIP socket API.
  3. Trying the pico_cyw43_arch_lwip_sys_freertos library from the SDK, which should then support the socket API.
  4. On the lines you suggest, trying to write routines which use the "Raw" API and implement my own single thread socket style blocking API (with embedded poll loops). But why are there not standard LWIP routines for this?
rtrussell commented 2 years ago

The most relevant part of the API is here. Routines tcp_connect(), tcp_recv(), tcp_sent(), tcp_err(), tcp_accept() and tcp_poll() all have callbacks.

The link I gave is to the 'RAW API' whereas yours is to the 'TCP API'.; it does seem to be true that more use of callbacks is made by the TCP API than the RAW API. Would it be possible for BBC BASIC to use the RAW API instead of the TCP API, or is that not practical because of the complexity of the protocols? An API which uses callbacks only for receiving would be easier to work with.

Ideally I'd like to support both UDP and TCP, because the socklib library does on other platforms, so that also might point to the use of the RAW API, if the TCP and UDP protocols could realistically be implemented on top in BASIC code. But perhaps that's overly ambitious!

Memotech-Bill commented 2 years ago

If you look at the documentation tree on the left hand side, the TCP API is shown as a sub-heading of "raw" APIs, and the file name is "grouptcpraw.htm". So what I have referenced is referred to by LWIP as a "raw" API.

There is a similar "raw" UDP API.

might point to the use of the RAW API, if the TCP and UDP protocols could realistically be implemented on top in BASIC code.

I certainly could not do that.

rtrussell commented 2 years ago
  • bool is_pico_w (void)

Might I suggest that setting the @platform% variable to a different value would be more in keeping with how all the other versions report the platform they are running on.

I am currently considering four options:

Sounds like there is no straightforward solution. Do we know what approach MicroPython adopts? That must be faced with a very similar situation.

Memotech-Bill commented 2 years ago

Sounds like there is no straightforward solution. Do we know what approach MicroPython adopts? That must be faced with a very similar situation.

From the configuration file, MicroPython only compiles the LWIP "raw" APIs. Then the Python "socket" module is implemented in C.

It might be possible to steal some code from the latter.

Memotech-Bill commented 2 years ago

Might I suggest that setting the @platform% variable to a different value would be more in keeping with how all the other versions report the platform they are running on.

Routine cyw43_support() has been removed. is_pico_w() now returns an integer:

0 = Standard Pico 1 = Pico W without CYW43 support 2 = Pico W with CYW43 GPIO support 3 = Pico W with CYW43 poll support 4 = Pico W with CYW43 threadsafe background support

The variable @platform% = is_pico_w() + 6.

rtrussell commented 2 years ago

The variable @platform% = is_pico_w() + 6.

Can I request a review of this decision, which was taken without any consultation? I don't think it's very 'friendly' to gobble up 6 values in a 6-bit field (so your Pico variants have taken 10% of all the available values) when similarly minor variations between other platforms aren't allocated different values. For example all Linux platforms (including the Raspberry Pi) are allocated the same value (=1).

I might accept an argument that a Standard Pico and a Pico W are different 'platforms' (although even then I'm not sure that they are as different as, say, a Raspberry Pi from a Desktop PC) but surely the Pico W variants are just different software builds, and therefore should be distinguished (if at all) by different values in the MS 24-bits of @platform% which are reserved to identify the 'operating system'.

Memotech-Bill commented 2 years ago

Can I request a review of this decision, which was taken without any consultation?

My inexperience with BBC BASIC showing. Changed to:

@platform% = 6 + ( is_pico_w () << 8 )
rtrussell commented 2 years ago

@platform% = 6 + ( is_pico_w () << 8 )

That should be fine, although I'd welcome input from others since it's the sort of thing that once 'in the wild' can't easily be changed. So maybe there would be some value in suggesting it at a more 'public' place such as the Raspberry Pi forums or The Distillery. I'll post something to the latter, since Michael McConnell (author of Matrix Brandy) has a legitimate interest.

Memotech-Bill commented 2 years ago

Just because it was simplest, and in order to prove that something works, I have now implemented a version of wifi_scan in my latest commit. To build this:

cd PicoBB
git pull
cd console/pico
rm -rf build
mkdir build
make BOARD=pico_w CYW43=BACKGROUND

The BASIC program is in console/pico/examples/wifi_scan.bas. You can probably improve this, particularly the formatting of the MAC address.

The output seems to list the same access point multiple times, and the channel number seems inconsistent, often the same for all stations. However the C version in pico-examples has exactly the same characteristics.

Making network connections work is going to take a lot more code.

rtrussell commented 2 years ago

You can probably improve this, particularly the formatting of the MAC address.

If you want to ensure that each byte is shown as a 2-digit hex number you can use this method:

  190     FOR i = 0 TO 4
  200       PRINT RIGHT$("0" + STR$~mac&(i), 2);":";
  210     NEXT

Rather than having to pass five 'by address' parameters to net_wifi_scan() have you considered passing a single structure which the function populates? That feels considerably more 'elegant' to me and is more how I would expect a modern API to work. As a free bonus it ensures alignment, which the individual variable approach doesn't.

Here's how a program using that approach might look:

   10 DIM net{rssi%, chan%, auth%, ssid&(32), mac&(5)}
   20 SYS "cyw43_arch_init" TO err%
   30 IF err% <> 0 THEN
   40   PRINT "Error ";err%;" in initialisation"
   50   END
   60 ENDIF
   70 SYS "cyw43_arch_enable_sta_mode"
   80 REPEAT
   90   SYS "net_wifi_scan", net{} TO err%
  100   IF err% = 0 THEN
  110     PRINT net.ssid&()
  120     PRINT "Signal strength: ";net.rssi%
  130     PRINT "Channel: ";net.chan%
  140     PRINT "MAC address: ";
  150     FOR i = 0 TO 4
  160       PRINT RIGHT$("0" + STR$~net.mac&(i), 2);":";
  170     NEXT
  180     PRINT RIGHT$("0" + STR$~net.mac&(5), 2)
  190     PRINT "Authorisation mode: ";net.auth%
  200     PRINT
  210   ENDIF
  220 UNTIL err% <> 0
  230 IF err% = -1 THEN
  240   PRINT "Scan complete"
  250 ELSE
  260   PRINT "Error ";err%
  270 ENDIF
  280 END

It would also make it easy to use an array of structures if you want to store the results of the scan for later study:

   10 MAXNETS = 100
   20 DIM net{(MAXNETS) rssi%, chan%, auth%, ssid&(32), mac&(5)}
   30 SYS "cyw43_arch_init" TO err%
   40 IF err% <> 0 THEN
   50   PRINT "Error ";err%;" in initialisation"
   60   END
   70 ENDIF
   80 SYS "cyw43_arch_enable_sta_mode"
   90 n = 0
  100 REPEAT
  110   SYS "net_wifi_scan", net{(n)} TO err%
  120   IF err% = 0 THEN
  130     PRINT net{(n)}.ssid&()
  140     PRINT "Signal strength: ";net{(n)}.rssi%
  150     PRINT "Channel: ";net{(n)}.chan%
  160     PRINT "MAC address: ";
  170     FOR i = 0 TO 4
  180       PRINT RIGHT$("0" + STR$~net{(n)}.mac&(i), 2);":";
  190     NEXT
  200     PRINT RIGHT$("0" + STR$~net{(n)}.mac&(5), 2)
  210     PRINT "Authorisation mode: ";net{(n)}.auth%
  220     PRINT
  230     n += 1
  240   ENDIF
  250 UNTIL err% <> 0
  260 IF err% = -1 THEN
  270   PRINT "Scan complete"
  280 ELSE
  290   PRINT "Error ";err%
  300 ENDIF
  310 END
Memotech-Bill commented 2 years ago

Rather than having to pass five 'by address' parameters to net_wifi_scan() have you considered passing a single structure which the function populates? That feels considerably more 'elegant' to me and is more how I would expect a modern API to work.

Changed as suggested.

Memotech-Bill commented 2 years ago

Should IPv6 be supported? The documentation for SOCKLIB implies only IPv4, but that may be a function of the age of the documentation.

In LWIP, IPv6 support is a compile time option which changes the definition of the ip_addr_t used for the parameter in a number of routines:

#if LWIP_IPV4 && LWIP_IPV6
    :
    :
typedef struct ip_addr {
  union {
    ip6_addr_t ip6;
    ip4_addr_t ip4;
  } u_addr;
  /** @ref lwip_ip_addr_type */
  u8_t type;
} ip_addr_t;
    :
    :
#else /* LWIP_IPV4 && LWIP_IPV6 */
    :
    :
#if LWIP_IPV4

typedef ip4_addr_t ip_addr_t;
rtrussell commented 2 years ago

Should IPv6 be supported?

I wouldn't have thought so. socklib doesn't because it's a wrapper around SDL2_net, and that's IPv4 only (although I have seen a 'wish list' for a future version of SDL2_net which mentions it, but my guess is that's a long way off).

Maybe I've lived a sheltered existence, but I've never had any exposure to IPv6, either at work or at home.

Memotech-Bill commented 2 years ago

Trying to use the SDK functions using SYS I am repeatedly getting hard faults owing to BASIC variables not being aligned.

I have two alternatives:

rtrussell commented 2 years ago

Trying to use the SDK functions using SYS I am repeatedly getting hard faults owing to BASIC variables not being aligned.

I would say that is perfectly normal and expected. Exactly the same thing happens with other editions when they are running on a machine with alignment requirements. If you look at some of the libraries supplied with BBC BASIC for SDL 2.0 you will find that they commonly contain code to force alignment, for example here is an extract from shaderlib.bbc:

DIM Align@shader{VBO%,code%%}

  • Include my revisions which correctly align BASIC variables.

Don't do that, because it would make the Pico edition different from all the others. Anyway, you could still have misaligned variables in other circumstances such as in structures. And of course alignment requirements aren't always 4 bytes, they are sometimes 16-bytes (such as with some SYS calls on the x86).

  • For every single SDK function that takes an address, write a wrapper that allocates correctly aligned temporary variables and than uses the LOAD and STORE macros to stage the data between BASIC and the SDK.

Again, that's making it different from every other edition. I simply don't consider that alignment requires special handling when there are so many other things you need to be aware of when using SYS. Here is a nice (or horrible, if you prefer) example of the kind of hoops you sometimes have to jump through (here from box2dlib):

      IF __thiscall THEN
        SYS __thiscall, `b2SetAsOrientedBox`, s%%, FN_f4(hw), FN_f4(hh), b2Vec2{}, FN_f4(a)
      ELSE
        IF ARMHF THEN
          SYS `b2SetAsOrientedBox`, s%%, FN_fd2(hw, hh), b2Vec2{}, FN_fd(a)
        ELSE IF @platform% AND &40 THEN;
          SYS `b2SetAsOrientedBox`, s%%, FN_fd(hw), FN_fd(hh), b2Vec2{}, FN_fd(a)
        ELSE
          SYS `b2SetAsOrientedBox`, s%%, FN_f4(hw), FN_f4(hh), b2Vec2{}, FN_f4(a)
        ENDIF
      ENDIF

What this probably is suggesting is that there could usefully be a BBC BASIC library which wraps the SYS calls so that the alignment issues are resolved there rather than in user code.

Memotech-Bill commented 2 years ago

If you look at some of the libraries supplied with BBC BASIC for SDL 2.0 you will find that they commonly contain code to force alignment, for example here is an extract from shaderlib.bbc: DIM Align@shader{VBO%,code%%}

You have previously suggested using a structure for alignment. Does that mean that the start of the data storage of a structure is guaranteed to be at least 4-byte aligned? Is this documented somewhere?

Edit: By experiment, apparently not. As a quick test my program contains the lines:

180 DIM ipaddr{addr%}
185 PRINT ~^ipaddr{}

And prints the value 2001259D.

So how does one know how much padding to add to the start of the structure?

The only way I can see of obtaining alignment from within BASIC is to allocate byte arrays that are three bytes larger than the required data, and then use:

ptr% = (^barray&(0) + 3) AND &FFFFFFFC