lancaster-university / codal-microbit-v2

CODAL target for the micro:bit v2.x series of devices
MIT License
44 stars 52 forks source link

Design sidechannel comms approach we're happy to live with for some time (mailbox) #271

Open jaustin opened 1 year ago

jaustin commented 1 year ago

for the MakeCode 2023 release we’re planning a feature that allows MakeCode to fetch the data out of the micro:bit data log over web USB and Bluetooth (but in different ways for each) (#157 and #270)

Given the performance limitations of the micro:bit interface chip, we don’t want Jacdac, this data logging feature and web USB serial to be fighting each other.

As I understand it, Jacdac’s mailbox is implemented in the MakeCode C++, but we need to implement the data logging mailbox/sidechannel in CODAL so that it’s available directly from the MY_DATA.html page (no matter which editor was used) as well as from MakeCode.

Furthermore, we have a couple of other very useful features we’d like to possibly allow over such a channel:

… or maybe in the long run even more...

So, I’d like to make sure we don’t end up with these things clobbering each other.

jaustin commented 1 year ago

Current proposal is to use the Jacdac mailbox and pull it down into CODAL:

https://github.com/microsoft/pxt-jacdac/blob/master/mbbridge.cpp

We can then implement the sidechannel operations we want via Jacdac (this doesn't require any other parts of Jacdac, we're really just using the mailbox and the Jacdac frame layout)

Looking at https://microsoft.github.io/jacdac-docs/reference/protocol/ and reading sentence "The responsibility of the physical layer is to send/receive a byte buffer (representing a Jacdac frame from the transport layer over a particular media)” I expect that what goes into the mailbox is defined here: https://microsoft.github.io/jacdac-docs/reference/protocol/#jacdac-frames-packets-and-service-indices, but have emailed MMoskal to check.

@finneyj please confirm you think this is still a viable approach after looking at the work Daniel did and the mailbox/protocol specifications above

finneyj commented 1 year ago

Thanks @jaustin

I took a look at the Jacdac mailbox code Michal shared. The code itself is pretty simple, but there are a few implications I think we need to consider before committing.

  1. We need to ratify the two statements in this thread. "Given the performance limitations of the micro:bit interface chip, we don’t want Jacdac, this data logging feature and web USB serial to be fighting each other" and "Current proposal is to use the Jacdac mailbox and pull it down into CODAL. We can then implement the sidechannel operations we want via Jacdac".

Are we sure these two things are orthogonal? Reviewing the mailbox code, and from memory, there's a nice IRQ trigger for micro:bit side code to pick up messages dropped into the mailbox in a timely fashion. I think delivering data in the opposite direction is polling based. i.e. the web page will be polling that memory frequently via DapJS. Won't this create exactly the same contention issues on the Daplink chip that we're trying to avoid?

  1. If we implement the services we want as Jacdac services (compliant with the existing jacdac protocol), then we'll need to either a) reimplement some of the core jacdac protocol stack inside CODAL (e.g. addressing, advertising, service enumeration etc), which would replicate the stack in MakeCode. This is bad for flash usage and consistency. b) Write the services (data logger etc) using TypeScript on the real Jacdac stack. This minimizes replication, but breaks our statement above " we don’t want Jacdac", and means we can only support one platform with this API (no easy support for micropython IDE etc), and we presumably end up wit a lot of Jacdac in every micro:bit makecode build (bringing additional overhead).

Can I get thoughts on the above from folks that have interest in this before I proceed?

I'm particularly interested to here if there are in fact efficient non-polling based ways to do a kind-of "JS callback when micro:bit memory changed" type of behaviour with DapJS.

@jaustin @microbit-carlos @abchatra

finneyj commented 1 year ago

Update - spoke briefly with @mmoskal. JS side of the comms is indeed polling based, so any implementation based on this architecture is very likely going to create the same bottleneck on the DapLink chip.

jaustin commented 1 year ago

I think worth paging @thegecko here, specifically on about this point:

I'm particularly interested to here if there are in fact efficient non-polling based ways to do a kind-of "JS callback when micro:bit memory changed" type of behaviour with DapJS.

Do we still have to poll for breakpoints!? What about watchpoints. How much disruption are we prepared to take? I imagine if we used actual breakpoints we wouldn't end up with display flicker at least. Not sure about watchpoints?

For your other two questions, @finneyj

Are we sure these two things are orthogonal? Reviewing the mailbox code, and from memory, there's a nice IRQ trigger for micro:bit side code to pick up messages dropped into the mailbox in a timely fashion. I think delivering data in the opposite direction is polling based. i.e. the web page will be polling that memory frequently via DapJS. Won't this create exactly the same contention issues on the Daplink chip that we're trying to avoid?

I think in our original discussion, @mmoskal pointed out he thought the polling rate could be significantly dropped for the kind of use-cases we have in mind here and then increased again if "real" jacdac services were encountered.

If we implement the services we want as Jacdac services (compliant with the existing jacdac protocol), then we'll need to either a) reimplement some of the core jacdac protocol stack inside CODAL (e.g. addressing, advertising, service enumeration etc), which would replicate the stack in MakeCode. This is bad for flash usage and consistency. b) Write the services (data logger etc) using TypeScript on the real Jacdac stack. This minimizes replication, but breaks our statement above " we don’t want Jacdac", and means we can only support one platform with this API (no easy support for micropython IDE etc), and we presumably end up wit a lot of Jacdac in every micro:bit makecode build (bringing additional overhead).

I think this is more 'all in' on Jacdac than we'd originally envisaged, but we'd definitely discussed it since. IIRC what was on the table after the original call was using the Jacdac framing format and mailbox but not fully implementing them as services. IE allowing Jacdac and these new side-channel packets to coexist in the same box.

(However, if we want Jacdac in MicroPython and other places like Scratch then we'll need it outside of MakeCode TS in the long run anyway)

finneyj commented 1 year ago

Thanks for sharing your thoughts @jaustin. Yes, I agree this is what we last discussed.

My concerns are just growing that this design approach is being built on shaky foundations due to the additional load we're placing on the DAPLink interface chip. We know this is already a bottleneck, and we see contention on that chip when MakeCode is in normal use (which typically presents itself as slow/lossy I2C control comms, and has historically caused long running issues with data logging).

Even at reduced / scalable polling rates, it feels like we're walking eyes open into a known problem. e.g. Imagine a common scenario, where a kid writes a data logging program in MakeCode. It's been out in the field for a while, and then the kids brings it back and plugs it in so MakeCode can download and visualize the data. When plugged in:

1) The kids app will still be logging new data over I2C (to the DAPLink chip) 2) MakeCode will use DAPJs to starting polling the mailbox (also via DAPLink) 3) Kids code may also have the established serial streaming of log data enabled over uart for live updates (also via DAPLink) 4) MakeCode writes commands to mailbox to download the full log (also via DAPLink) 5) MakeCode likely increases the polling rate of the mailbox temporarily to get better UX (also via DAPLink) 6) CODAL downloads up to 124K of log data over the I2C bus in ~128 bytes chunk (up to 992 transactions also via DAPLink) 7) MakeCode reads the mailbox for each block (up to 992 transactions via dapink) 8) MakeCode and writes back a "ready for more data status" for each of those blocks (up to 992 transactions via DapLink)

...and this is in addition to any of the normal WebUSB / WebSerial interaction with DAPLink that MakeCode does as part of its normal operation. i.e. This design strategically targets the weakest link we have in the data logging subsystem, and one we can't mitigate any more than we already have.

I don't want to sound like I'm scaremongering here, but it does feel a little bit like we're ignoring all the warning signs!!

Thanks for clarification re: Jacdac compliance. To summarize so that it's clear to everyone that we're proposing:

1) A joint micro:bit/Jacdac mailbox implemented in CODAL, that should provide equivalent functionality to the one from michal cited above. Some very minor changes on Jacdac side may be required to support both jacdac enabled/disabled operation but I'll keep it as transparent as I can.

2) We create a Jacdac frame compatible but probably NOT PROTOCOL COMPATIBLE set of operations to enable access to the new DataLogging functions we need in the browser. This may mean that on the MakeCode side, the top level Jacdac APIs might NOT be sufficient to access these new operations. i.e. the resulting implementation will NOT look like a normal Jacdac service to the MakeCode web app. Again, I'll try to keep it transparent as similar/transparent as I can, so that some of the low-level API code from Jacdac TS will likely help the MakeCode dev team.

3) Regarding future xplat support for Jacdac, I fully agree... there's already a C implementation we can integrate. However if we're not ready to adopt Jacdac yet, any non-compliant work we do here runs the risk of being deprecated when we do adopt it later. At which point I reserve the right to make breaking changes here, and not maintain a legacy of non-compliance :)

@abchatra @jwunderl - would be great to get a perspective on this from the MakeCode side :)

Summarizing, this just feels like a rather scrappy, risky approach.

I'll build one, but Prof Joe says: "There be dragons here"...

finneyj commented 1 year ago

p.s. Regarding DapJS async callbacks, I had some students look into this a few months ago when building an in-browser debugger... As far as we could tell, no support in there currently for async callbacks on watchpoint or breakpoints yet.

Please do let me know if we missed anything though @thegecko!

thegecko commented 1 year ago

I'm particularly interested to here if there are in fact efficient non-polling based ways to do a kind-of "JS callback when micro:bit memory changed" type of behaviour with DapJS.

In your case, I believe you are using WebUSB as the transport in DapJS which, at the lowest level offers a transferIn() command for the device. This is initiated by the host, so I think this will need to be polled, unfortunately.

If implementing this, I recommend any polling is kept as close to this call in the code as possible (maybe expose some sort of event and loop in the transport layer), however it will be quite blocking and I would consider implementing a system-wide access mutex for the device or the ability to pause polling during other operations somehow.

If you aren't tied to WebUSB, a new transport could be added to support WebHID, similar to transport/hid which does offer inputReport events which wouldn't require polling. This approach will likely require work in other areas, though (hid endpoint on the device, selecting a WebHID device as well as WebUSB device in the webpage as they aren't related, etc.)

If we are thinking in this way, I can't help thinking events via webbluetooth characteristics may also work...

HTH

finneyj commented 1 year ago

Many thanks @thegecko!

I'm not familiar with the detail, but I believe MakeCode is using WebUSB as the transport, yes.

WebHID sounds an interesting option, but I don't think(?) we expose that as an endpoint from our DapLink interface chip at the moment, and we have a lot of legacy. Changes to DAPLink firmware are difficult for our users to swallow, so are best avoided if at all possible!

The ability to have custom events would be extremely useful though...

abchatra commented 1 year ago

May be this needs a call. I agree with @finneyj on the scenario outline.

finneyj commented 1 year ago

OK folks - I've drafted up a first implementation of this. You can find it on a branch called codal-mailbox in this repo.

The branch currently includes:

I've included some CODAL C++ code at the end of this post that I used for testing, as it both logs data and simulates the behaviours of what an attached PC should do to access the log via the mailbox. The delays in there are just to slow things down for testing, but go gentle on it. I haven't done extensive testing or tests under heavy load. This should be enough to give an indication of how this will perform in the wild though.

@abchatra - A few things to note for the MakeCode side:

I'm actually on holiday all this week, so support I can give will be a bit sporadic, but do let me know if you have questions or issues, and I'll see what I can do to help. I'll be back on campus next week...

p.s. Can you work with the codal-mailbox branch for your work @abchatra? or do you need a tag?

Example C++ test code:

#include "MicroBit.h"
#include "DataLogMailboxHandler.h"

MicroBit uBit;

bool requestLog = false;

void onA(Event)
{
    requestLog = true;
}

void doSomeLogging()
{
    int score = 1;

    while(1)
    {
        DMESG("LOGGING DATA...");
        uBit.log.beginRow();
        uBit.log.logData("Joe", score);
        uBit.log.logData("Jamie", score-1);
        uBit.log.endRow();

        score++;
        uBit.sleep(5000);
    }
}

int 
main()
{
    uBit.init();

    uBit.messageBus.listen(MICROBIT_ID_BUTTON_A, MICROBIT_BUTTON_EVT_CLICK, onA);

    create_fiber(doSomeLogging);

    // Configure a CSV request packet.
    ManagedBuffer requestPacket(256);
    jd_packet_t *p = (jd_packet_t *)&requestPacket[0];

    p->crc = 0;
    p->_size = 16;
    p->flags = 0x01;
    p->device_identifier = MICROBIT_MAILBOX_DEVICE_ID;
    p->service_size = 4;
    p->service_number = JACDAC_MAILBOX_SERVICE_ID_DATALOG;
    p->service_command = DATALOG_MAILBOX_COMMAND_GET_CSV_DATA;

    *((uint32_t *)p->data) = 0;

    // Simulate PC comms

    // First, ensure the mailbox is ready.
    // This is indicated by a value of 0xFF in the length field of the transmite buffer.

    JacdacMailboxBuffer *mailboxBuffer = (JacdacMailboxBuffer *)microbit_no_init_memory_region.mailboxBaseAddress;

    while(1)
    {
        DMESG("WAITING FOR EXCHANGE BUFFER TO INITIALIZE");
        if (mailboxBuffer->outputBuffer[2] == 0xFF)
        {
            // Indicate that we are now listening on the buffer
            mailboxBuffer->outputBuffer[2] = 0x00;
            break;
        }

        uBit.sleep(1000);
    }

    DMESG("EXCHANGE BUFFER INITIALIZED");
    while(1)
    {
        if (requestLog)
        {
            requestLog = false;
            DMESG("ISSUING LOG REQUEST");

            // Assert that the buffer is empty (it should be).
            if (mailboxBuffer->inputBuffer[2] != 0x00)
            {
                DMESG("   WARNING: INPUT BUFFER CONTENTION");
                continue;
            }

            memcpy((void *) mailboxBuffer->inputBuffer, &requestPacket[0], requestPacket.length());
            uBit.mailbox.processQueues();

            while (mailboxBuffer->inputBuffer[2] != 0x00)
            {
                DMESG("   WAITING FOR INPUT BUFFER TO PROCESS");
                uBit.sleep(500);
            }

            DMESG("   INPUT BUFFER PROCESSED");

            while (mailboxBuffer->outputBuffer[2] == 0x00)
            {
                DMESG("   WAITING FOR RESPONSE PACKET");
                uBit.sleep(500);
            }

            DMESG("   RESPONSE RECEIVED.");
            jd_packet_t *resultPacket = (jd_packet_t *) &mailboxBuffer->outputBuffer;
            int length = resultPacket->_size - 4;

            for (int i=0; i < length; i++)
            {
                DMESGN("%c", resultPacket->data[i]);
            }

            mailboxBuffer->outputBuffer[2] = 0x00;
            DMESG("   DONE.");
        }
        uBit.sleep(1000);
    }
}
abchatra commented 1 year ago

@finneyj can you please produce a tag. It is bit easier for us to build on top of it. @jwunderl is looking at this.

jaustin commented 1 year ago

@abchatra do you just mean a git tag, or do you want an actual release? I think @finneyj's out for a bit now but @microbit-carlos could make the tag if that's what you need (though, in general being able to test MakeCode on arbitrary codal branches/forks etc is super valuable, so would be nice to smooth that path too)

finneyj commented 1 year ago

Thanks guys.

@abchatra - tagged "v0.2.50-mailbox.0"