Allow for multiple builtin program including Port View

[laurensvalk]

This is a PR against hostbuffer to make reviewing easier. These are separate features, but it builds on that one.

---

This adds an optional 32-bit little endian identifier payload to the START_USER_PROGRAM and START_BUILTIN_PROGRAM (formerly START_REPL).

The REPL becomes (remains) builtin program 0. Port View will be builtin program 1. These are defined at the application level (micropython.c) rather than at the pbio level. The main purpose for now is to enable Port View support. Future builtin programs can be gyro calibration routines (#252), etc.

User programs get an identifier payload as well, so we can properly start distinct user programs if we ever have them. Currently only index 0 is allowed for user programs.

@dlech : Should these preferably be new commands instead, or is it good as above? I don't mind changing it either way.

---

Can be tested with Pybricks Code by modifying createStartReplCommand follows:

export function createStartReplCommand(): Uint8Array {
    const msg = new Uint8Array(5);
    const view = new DataView(msg.buffer);
    view.setUint8(0, CommandType.StartRepl);
    const programID = 1; // HACK: Test to start port view. This should become an optional argument to this function.
    view.setUint32(1, programID, true); 
    return msg;
}

[laurensvalk]

@afarago: This already includes a placeholder for port view in bricks/_common/modules/_builtin_port_view.py and has the AppData (HostBuffer) stuff, so it gets a bit closer to what we'd need for Port View. So in principle you can add your Python code there, and use the additional payload discussed above to start it almost instantly.

If it's OK with you, I'd like to give implementing _builtin_port_view.py a go as well, but I'll stick with your emitted format for now.

I would be happy to leave the Pybricks Code implementation side more to you and David, since that isn't my expertise. I'll post some visual suggestions in an issue, but we can start by combining our existing development and synchronize on a call. Thanks!

[afarago]

@afarago: This already includes a placeholder for port view in bricks/_common/modules/_builtin_port_view.py and has the AppData (HostBuffer) stuff, so it gets a bit closer to what we'd need for Port View. So in principle you can add your Python code there, and use the additional payload discussed above to start it almost instantly.

If it's OK with you, I'd like to give implementing _builtin_port_view.py a go as well, but I'll stick with your emitted format for now.

I would be happy to leave the Pybricks Code implementation side more to you and David, since that isn't my expertise. I'll post some visual suggestions in an issue, but we can start by combining our existing development and synchronize on a call. Thanks!

Sure, glad to see it happening.
I have recently experimented with the AppData, but feel free to implement an optimized version, this week I am anyhow unwell.
As I understand it will reside on the hub so I assume it should be as small as possible and maybe already in mpy?

https://gist.github.com/afarago/350a931992e05c423860f4b25ea5c6a2

[laurensvalk]

As I understand it will reside on the hub so I assume it should be as small as possible and maybe already in mpy?

This PR ensures that the indicated file is built into the firmware as frozen bytecode. Size is a factor, though primarily on City/Technic Hub. Move Hub isn't going to have this feature at all.

The "on-brick" component is needed on Prime/Inventor Hub only, where we have some more space.

[coveralls]

coverage: 55.961% (-0.06%) from 56.025%
when pulling df60572 on builtin-programs
into 53a60f9 on hostbuffer.

[github-actions]

Download the artifacts for this pull request:

• pr_number.zip
• mpy-cross.zip
• pybricks-micropython-build-3512-gita0403a2b.zip
• nxt-firmware-build-3512-gita0403a2b.zip
• movehub-firmware-build-3512-gita0403a2b.zip
• cityhub-firmware-build-3512-gita0403a2b.zip
• essentialhub-firmware-build-3512-gita0403a2b.zip
• technichub-firmware-build-3512-gita0403a2b.zip
• primehub-firmware-build-3512-gita0403a2b.zip

[laurensvalk]

I've added a preliminary port view script. It generates output of the form discussed here.

It uses AppData to send telemetry to Pybricks Code and accepts mode info via AppData too.

Tested on Technic Hub and Prime Hub. Due to an limitation with the Technic Hub, messages are concatenated and split in chunks of at most 19 bytes. They can be put together in Pybricks Code and split at new lines to get the correct information.

---

If you just want to do a sanity check to make sure everything is working before you make any changes to Pybricks Code, just change the following before you build the firmware:

diff --git a/bricks/_common/micropython.c b/bricks/_common/micropython.c
index f43b804cf..785d44d83 100644
--- a/bricks/_common/micropython.c
+++ b/bricks/_common/micropython.c
@@ -302,12 +302,12 @@ typedef enum {
     /**
      * The MicroPython REPL.
      */
-    PYBRICKS_MICROPYTHON_BUILTIN_PROGRAM_ID_REPL = 0,
+    PYBRICKS_MICROPYTHON_BUILTIN_PROGRAM_ID_REPL = 1,
     /**
      * Program that detects attached devices, displays sensor values, and
      * relays sensor data to host if connected.
      */
-    PYBRICKS_MICROPYTHON_BUILTIN_PROGRAM_ID_PORT_VIEW = 1,
+    PYBRICKS_MICROPYTHON_BUILTIN_PROGRAM_ID_PORT_VIEW = 0,
     /**
      * The number of builtin programs.
      */
diff --git a/pybricks/tools/pb_type_app_data.c b/pybricks/tools/pb_type_app_data.c
index 89dcdffbc..b994156a3 100644
--- a/pybricks/tools/pb_type_app_data.c
+++ b/pybricks/tools/pb_type_app_data.c
@@ -115,7 +115,7 @@ STATIC mp_obj_t pb_type_app_data_make_new(const mp_obj_type_t *type, size_t n_ar
     app_data_instance->tx_context.done = NULL;
     app_data_instance->tx_context.connection = PBDRV_BLUETOOTH_CONNECTION_PYBRICKS;
     app_data_instance->tx_context.data = app_data_instance->tx_buffer;
-    app_data_instance->tx_buffer[0] = PBIO_PYBRICKS_EVENT_WRITE_APP_DATA;
+    app_data_instance->tx_buffer[0] = PBIO_PYBRICKS_EVENT_WRITE_STDOUT;
 
     return MP_OBJ_FROM_PTR(app_data_instance);
 }

This makes Port View builtin program 0, so you can just start it with the existing REPL >>> button for now. And it prints the hub data to stdout so you can see that it does in fact work before you begin processing it as app data.

[laurensvalk]

This is working now, and could be used for Pybricks Code experiments 😃

[dlech]

This adds an optional 32-bit little endian identifier payload to the START_USER_PROGRAM and START_BUILTIN_PROGRAM (formerly START_REPL).

Yes, I think that is fine. We will also need to bump the protocol version and document the changes at https://github.com/pybricks/technical-info/blob/master/pybricks-ble-profile.md.

these are defined at the application level (micropython.c) rather than at the pbio level.

Since these are going over the air, they should be part of the protocol and not application-specific. With a 32-bit field, we will never run out of slots anyway.

---

Idea: if we make the index fields 1 byte, we could also send that info back in the periodic status update event so that Pyrbicks Code knows exactly which program is running, even when started by buttons on the hub.

I haven't thought too much about the Pybricks Code side of things yet but we may need something like this to make sure things work correctly.

[laurensvalk]

Since these are going over the air, they should be part of the protocol and not application-specific. With a 32-bit field, we will never run out of slots anyway.

I suppose I was thinking of them similar to variable user programs, where the host just requests to start a particular index and get an error if it isn't available. And to avoid having any kind of MicroPython references in pbio.

But I can see that specifics could be useful and indeed with a 32 bit field we should not run out.

The thinking behind 4 bytes instead of 1 was to keep a few options open since we haven't settled on a UI or multi-file protocol yet. For example, the identifier might encode a 25-bit display icon for a particular program or builtin. Or maybe it could include a checksum, so that the program can only start if its contents correspond to its identifier.

Idea: if we make the index fields 1 byte, we could also send that info back in the periodic status update event so that Pybricks Code knows exactly which program is running, even when started by buttons on the hub.

I like it. So we would include that in pbio_pybricks_status_t?. Yes, that makes a good case for 1 byte.

[afarago]

some

Awesome progress, I see that there is so much going on it is better to wait for things to settle before moving forward with the code on both sides.

As a first remark with _builtin_port_view.py:
Due to the infinite loop within the update_*_sensor subroutines the mode update is not taken into account.
Would suggest a separate function instead of a parameter:

def get_mode(port):
    return app_data.get_values()[ports.index(port)]

...

def update_color_and_distance_sensor(port, type_id):
   ...
    while True:
        mode = get_mode(port)

[laurensvalk]

Thank you, I'll fix that.

Feel free to experiment, but we can definitely try to schedule a chat between all of us to catch up.

I've not done much exploring on the Pybricks Code side, so I'll be curious to see how that might work.

[dlech]

So we would include that in pbio_pybricks_status_t?

No. We would add a new "program ID" parameter to pbio_pybricks_event_status_report(). (side note, it could be nice to add a battery percent to this while we are touching it)

Also, we should probably rename PBIO_PYBRICKS_STATUS_USER_PROGRAM_RUNNING to PBIO_PYBRICKS_STATUS_PROGRAM_RUNNING because it really means user or builtin program running (and changing that meaning would break things).

Then we likely also need a new flag in pbio_pybricks_status_t to differentiate between user and builtin programs running if we just add one "program ID" parameter to pbio_pybricks_event_status_report().

---

Another option could be to just keep calling everything a "user program" and have, e.g. the msb of the program ID byte be a flag indicating if it is a user "slot" or a builtin program. In this case, we could deprecate START_REPL and just add the extra arg to START_USER_PROGRAM.

[laurensvalk]

Another option could be to just keep calling everything a "user program" and have, e.g. the msb of the program ID byte be a flag indicating if it is a user "slot" or a builtin program. In this case, we could deprecate START_REPL and just add the extra arg to START_USER_PROGRAM.

👍

Then we can keep the START_REPL command working as is for backwards compatibility, which will just be cast to builtin program 0.

[laurensvalk]

Another option could be to just keep calling everything a "user program" and have, e.g. the msb of the program ID byte be a flag indicating if it is a user "slot" or a builtin program.

Right. By this terminology, builtin programs are "builtin user programs".

[dlech]

Idea: if we make the index fields 1 byte, we could also send that info back in the periodic status update event so that Pyrbicks Code knows exactly which program is running, even when started by buttons on the hub.

In addition to knowing which program is running, we will also want to know what programs are available (e.g. so that the Pybricks UI can be smart about only enabling buttons if a feature is actually available). I just remembered that we already have pbio_pybricks_feature_flags_t for this. It already has a flag for the REPL. So we should add a new flag for the port view program to this.

[laurensvalk]

This PR currently addresses that by generalizing the meaning of that flag to supporting builtin programs or not.

But flags per program would indeed give a bite more refined control. At the moment, it returns an error if the selected builtin is not available.

[dlech]

But flags per program would indeed give a bite more refined control. At the moment, it returns an error if the selected builtin is not available.

Yeah, it would be best if we could know before actually trying to run the program.

[laurensvalk]

This was accidentally closed due to the opposing branch being removed. I'll re-submit when the above suggestions are incorporated.