Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow for multiple builtin program including Port View, take 2. #264

Merged
merged 10 commits into from
Sep 3, 2024

Conversation

laurensvalk
Copy link
Member

@laurensvalk laurensvalk commented Aug 28, 2024

I closed #254 accidentally. This is a follow up. It is conceptually similar, but incorporates David's feedback given there.

There is now a one byte program identifier instead of (4 bytes + 1 bool for the type). Instead of modifying the START_REPL and START_PROGRAM protocol commands with an identifier payload as before, this introduces a single new command PBIO_PYBRICKS_COMMAND_START_USER_OR_BUILTIN_PROGRAM with a one byte payload.

It also adds this byte to the hub status report, but I'm open to implementing this differently. The available builtin programs are now individually included in the feature flags.

I haven't tried making this work with any particular port view visualization yet. We can do that after this is finalized.

Making this work required some refactoring in the way we wait for programs to start. This is a bit simpler now, which makes this an overal reduction in code size of about 100 bytes on Move Hub, despite already making some progress towards out-of-the-box multi-program support on the bigger hubs.

@coveralls
Copy link

coveralls commented Aug 28, 2024

Coverage Status

coverage: 55.947% (-0.01%) from 55.961%
when pulling f41537c on builtin-programs
into 6d49992 on master.

laurensvalk and others added 9 commits September 2, 2024 14:15
This brings the waiting process for the main program to start to the
main.c loop, which makes it easier to follow.

This also generalized the commands to start builtin and user programs
with an optional identifier. For now, this will be used to start either the
REPL or Port View. In the future, it can be used to start programs on
different slots.

This also adds a hook for verifying the program at the application level
so we can verify MicroPython-specific validity and avoid hardcoding such
checks at the pbio level.

pbio/sys/config: Consolidate app config.

The separate placement of these settings was a bit contrived. We can have it
at the pbsys config level since the definitions exist at the pbio protocol level anyway.

This lets us use them as defined feature guards.
The City/Technic Hub currently do not support messages longer than 19 bytes.
This makes it clearer that things such as "user program running" also
applies to builtin programs, which are also user programs.
This will let us include it in the hub status for
monitoring by Pybricks Code.

Use it for both downloaded and builtin user
programs.

Also restore the original START_REPL and START_PROGRAMS, and instead
introduce a new command for starting both builtin and downloaded user
programs. The previous modifications had not been released yet.
This keeps the protocol complete.

Also fix outdated changelog update.
Add optional payload to existing command instead.
Copy link
Member

@dlech dlech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably good enough for now.

One thing I not so sure about though is how to deal with multiple "user" slots in Pybricks Code.

It would probably be a good idea to add some kind of way to read which slots have a valid program in them.

With WebBluetooth, we can't get the actual error code for failures. So if we attempt to run a program in a slot with a bad program, there is no way to know why the error occurred. Was it out of range? Was it a bad program? Did the hub battery die? Right now, we don't have any "expected" errors, so we kind of ignore this problem.

So that's why I'm thinking it would be better to have something where we can "ask first" before trying something that would fail, so that we don't actually ever try to do something that might fail under normal circumstances.

@laurensvalk laurensvalk merged commit f41537c into master Sep 3, 2024
38 checks passed
@laurensvalk laurensvalk deleted the builtin-programs branch September 3, 2024 07:46
@laurensvalk
Copy link
Member Author

@afarago, as requested: here is how you can easily hack/modify the existing createStartReplCommand in Pybricks Code to trigger Port View.

/**
 */
export function createStartReplCommand(): Uint8Array {
    const msg = new Uint8Array(2);
    const view = new DataView(msg.buffer);
    view.setUint8(0, 1);      // < ---- start program command
    view.setUint8(1, 129);  // <---- program ID, 129 is port view
    return msg;
}

@laurensvalk laurensvalk mentioned this pull request Sep 6, 2024
2 tasks
laurensvalk added a commit that referenced this pull request Sep 16, 2024
This was missed in refactoring #264
laurensvalk added a commit that referenced this pull request Sep 16, 2024
This was missed in refactoring #264
laurensvalk added a commit that referenced this pull request Sep 17, 2024
This was missed in refactoring #264
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants