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

HubCentral / Portview based on AppData and StartUserProgram slot parameter in protocol v1.4.0 #2315

Open
wants to merge 30 commits into
base: master
Choose a base branch
from

Conversation

afarago
Copy link
Contributor

@afarago afarago commented Sep 28, 2024

Implementation and first merge of HubCentral/PortView feature based on the discussion pybricks/support#1708

Slightly modified hubcentral protocol, included _builtin_port_view.py as well for reference.

image

@afarago
Copy link
Contributor Author

afarago commented Sep 28, 2024

Issues/topics to clarify:

NOW:

  • Sporadic error especially with special characters (unicode issue?) e.g. with char “°”
  • Is the current flow acceptable to activate the portview app (via showDialog / hideDialog)?
    or should we need more secure and synchronous flow?
    If yes, not clear how to solve.
  • This layout supports 6-port hub only; guess I need to check the essential and the technic hub as well. (No move hub support, right?). Any best practice to easily detect hub type?

NEXT:

  • Is using slot_id = 129 OK?
    Which command to use: StartUserProgram = 1 OR StartRepl = 2 (PBIO_PYBRICKS_COMMAND_START_BUILTIN_PROGRAM).
    Are there any differences or builtin/repl will get deprecated?

LATER:

  • icons and visual design following up on Laurens's virtual hub design
  • handle unknown device icons/visuals
  • mode change (app->hub)

@laurensvalk
Copy link
Member

Thanks for taking the time to rework this with the firmware changes!

Sporadic error especially with special characters

The hub port view script is still breaking down messages to 19 byte chunks, so we could be seeing this?

We could look into using the negotiated MTU on the firmware side so we can send whole messages in one go.

I was initially thinking we could send pre-formatted strings, in order to limit the device specific stuff we need to add to Pybricks Code. But if make device specific visualizations anyway, we can look at packing a single port value into a 19 byte message, which should be feasible.

Is using slot_id = 129 OK?

Yes.

Which command to use: StartUserProgram = 1 OR StartRepl = 2

StartUserProgram with new one byte payload

Are there any differences or builtin/repl will get deprecated?

The old command PBIO_PYBRICKS_COMMAND_START_REPL is internally mapped to StartUserProgram(128) for backwards compatibility with older versions of Pybricks Code.

No move hub support, right?

Correct. There is a new feature flag PBIO_PYBRICKS_FEATURE_FLAG_BUILTIN_USER_PROGRAM_PORT_VIEW in the hub features you can check, instead of coding any hub checks in Pybricks Code. Compare with the existing and unchanged PBIO_PYBRICKS_FEATURE_FLAG_BUILTIN_USER_PROGRAM_REPL feature flag to see how this is used to disable the REPL button on Move Hub.

This layout supports 6-port hub only; guess I need to check the essential and the technic hub as well. (...) Any best practice to easily detect hub type?

Since we're providing everything for the device visuals from the Python script, we can include hub type also.

I think we'll want to selectively include some of these modules in the firmware if we end up making fancy things for Prime Hub (Like remote-control interfaces), so that we don't increase the other firmware sizes unnecessarily.

feat: hub center improved visuals
fix: boost color and distance sensor fix and improvement
@afarago
Copy link
Contributor Author

afarago commented Oct 1, 2024

  • Sporadic error fixed - thanks for the hint
  • Visuals Improved.

{FD85C330-ADD1-48D3-B02C-C19444B3CC74}

  • PBIO_PYBRICKS_FEATURE_FLAG_BUILTIN_USER_PROGRAM_PORT_VIEW is in the firmware code, I don't see how I could leverage it from user-python or from pybricks-code. Btw: at the moment I can use the version (from usys import version) or the number of ports returned in pybricks-code.

@laurensvalk
Copy link
Member

PBIO_PYBRICKS_FEATURE_FLAG_BUILTIN_USER_PROGRAM_PORT_VIEW is in the firmware code, I don't see how I could leverage it from user-python or from pybricks-code.

See how pbio_pybricks_feature_flags_t with PBIO_PYBRICKS_FEATURE_FLAG_BUILTIN_USER_PROGRAM_REPL is equivalent to HubCapabilityFlag with HasRepl :

export enum HubCapabilityFlag {

This is used to enable the button in ReplButton.tsx.

@afarago
Copy link
Contributor Author

afarago commented Oct 2, 2024

  1. Added the feature flag and if does not exist, reverted back to the old info popup.

  2. Changed color display to hsv (hsb) as you suggested

  3. There is a number of devices and hubs to be rendered, yet as a next step, please help me understand what are the MVP next steps to get this PR merged. I am quite enthusiastic to make it happen:

  • is the current (portview<->pybricks-code) protocol acceptable or do you want to change it? I would be OK with the current protocol: [Port.<A-F>|battery|buttons|imu]\t[port type|field0]\t[fields separated by tab]
    • pro: the current protocol is versatile and quite fault tolerant (display as is, if needed consume some fields), still if that fails there is a graceful degradation (e.g. shaft is not moving, color is not showing up)
    • con: slightly more data over appdata channel
  • if yes, would it be possible to finalize --> I would adopt and implement this on both sides

{A933A5C5-C8DF-45AE-8AF0-2619B6509CB2}

@laurensvalk
Copy link
Member

There is a number of devices and hubs to be rendered, yet as a next step, please help me understand what are the MVP next steps to get this PR merged. I am quite enthusiastic to make it happen:

Thank you!

I am currently working on releasing the current state of things as beta with all the recent firmware updates. The most relevant firmware issues have now been fixed. Then I plan to turn my attention to the block coding backlog and Pybricks Code.

please help me understand what are the MVP next steps

  • I think this is a good time to review exactly how we want this to look. I think this is a good start. We can make some visual mockups on how we might want to scale the device images and display and lay out the measurements.
  • We can work on the encoding protocol, see [Feature] Hub state encoding support#1859 opened a few days ago.
  • Finalize and document the hub protocols added so far and suggest any changes if we expect to need any.
  • Update Pybricks Code to work with the new protocol, without port view just yet. So all the new commands, feature flags, etc. Then everything will be prepared for this new port view feature.

@dlech
Copy link
Member

dlech commented Oct 5, 2024

To make it easier on me to review, could we first split out the Pybricks Profile 1.4 changes to a separate pull request?

Copy link
Contributor

@BertLindeman BertLindeman left a comment

Choose a reason for hiding this comment

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

A naive remark and may be out of order.

  1. I think I miss the Technic-move motor.

  2. How does the Color-Distance sensor show the distance.
    All I see is it is getting mode = 0.

Tell me to shut-up if this is all irrelevant.

What a great project this is!

Bert

@afarago
Copy link
Contributor Author

afarago commented Oct 9, 2024

A naive remark and may be out of order.

  1. I think I miss the Technic-move motor.
  2. How does the Color-Distance sensor show the distance.
    All I see is it is getting mode = 0.

Tell me to shut-up if this is all irrelevant.

What a great project this is!

Bert

All ideas are welcome here!

This is a sneak peak of my work-in-progress version.
{E581F8DF-F79E-4EFF-9239-8FAE23C436E7}

Keep the excitement coming, we have some UI design and review work to be done...

@BertLindeman
Copy link
Contributor

BertLindeman commented Oct 9, 2024

All ideas are welcome here!

I was referring to the Technic move hub built-in motors:

ID (hex) Description
86 (0x56) Technic Move hub built-in drive motor
87 (0x57) Technic Move hub built-in steering motor
  • You simply change modes...

I miss something, then. I plug the sensor into the hub, how to change the mode?
[EDIT] You just showed me, sorry a UI thing I did not see in the python.

snipped the picture

Keep the excitement coming, we have some UI design and review work to be done...

Yes, please.

Bert

@BertLindeman
Copy link
Contributor

Hi Attila,

It took me quite some fiddling to see why CityHub and TechnicHub only showed pup devices.
In line 29 in _builtin_port_view.py

hub = ThisHub

the parens are missing, should be:

hub = ThisHub()

With the result that hub.battery.voltage() is refused. And other things.

Bert

@BertLindeman
Copy link
Contributor

I think the focus for now is mostly on the PrimeHub, but all hubs except the move hub should be supported.
Het program is already too large for the movehub.

Small addition:
This snippet near line 260:

def battery_task():
    if not hub.battery: return

    count = 0
    while True:
        count += 1
        if count % 100:
            yield None
        else:
            # skip cc 10 seconds before sending an update 
            percentage = round(min(100,(hub.battery.voltage()-6000)/(8300-6000)*100))
            voltage = hub.battery.voltage()
            status = hub.charger.status()
            data = f"pct={percentage}%\tv={voltage}mV\ts={status}"
            yield f"battery\t{data}"

Not all hubs that support hub.battery also have a charger.
But status = hub.charger.status() prevents to report all available battery data.

@afarago
Copy link
Contributor Author

afarago commented Oct 12, 2024

To make it easier on me to review, could we first split out the Pybricks Profile 1.4 changes to a separate pull request?

Hi @dlech , would the granularity of the #2317 be OK for a review size and for us to move forward?
I also corrected some tests and added new ones for the new protocol functions.

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