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

pbsys/program_load: Add read/write access to user data. #121

Merged
merged 2 commits into from
Oct 21, 2022

Conversation

laurensvalk
Copy link
Member

This feature has been requested many times (e.g. here, here, or here). Now that we have enabled storage to save programs, we might as well add some storage for user data.

Proposed API:

# To write some data:
hub.system.storage(offset=5, write=b"Hello, world!")

# To read some data:
data = hub.system.storage(offset=12, read=6)
print(data) # This will print b"world!"

Appropriate errors are be raised so read/write access is always safe.

We could also add a storage_view later on for hubs that can support it, but this should cover the basics.

@laurensvalk laurensvalk added the software: pybricks-micropython Issues with Pybricks MicroPython firmware (or EV3 runtime) label Oct 21, 2022
@laurensvalk laurensvalk requested a review from dlech October 21, 2022 11:05
@laurensvalk
Copy link
Member Author

It would be great to have this part of the next beta so it will get some proper testing / feedback.

@laurensvalk
Copy link
Member Author

laurensvalk commented Oct 21, 2022

Demo time!

savedata.mp4
from pybricks.hubs import CityHub
from pybricks.pupdevices import ColorDistanceSensor, ColorLightMatrix
from pybricks.parameters import Port, Color
from pybricks.tools import wait

# Initialize devices.
hub = CityHub()
sensor = ColorDistanceSensor(Port.A)
lights = ColorLightMatrix(Port.B)

# We want to detect only these colors.
scanned_colors = [Color.NONE, Color.BLUE, Color.RED]
sensor.detectable_colors(scanned_colors)

# Waits for a color.
def wait_for_colors():
    wait(500)
    while sensor.color() != Color.NONE:
        wait(10)
    while sensor.color() == Color.NONE:
        wait(10)
    return sensor.color()

# Load previous data. Bytes 0 to 9 represent colors.
saved_colors = bytearray(hub.system.storage(0, 9))

while True:
    # Dipslay the colors.
    lights.on([scanned_colors[c] for c in saved_colors])

    try:
        # Get index of next empty pixel.
        index = bytes(saved_colors).index(bytes([0]))
    except ValueError:
        # Display full, so start over.
        saved_colors = bytearray(9)
        index = 0

    # Wait for the next color and store it.
    saved_colors[index] = scanned_colors.index(wait_for_colors())   
    hub.system.storage(0, write=bytes(saved_colors))

We will need this when setting other data, like user data.
This allows end-users to save small amounts of data on the hub. This
is useful for settings like train speed or counters in great ball
contraption modules.

Fixes pybricks/support#85
@laurensvalk
Copy link
Member Author

We are expediting merging this so users will be able to try it out in the beta.

This API is still open for changes after the beta release; we won't lock it until the full V3.2 release.

return mp_const_none;
}

mp_raise_ValueError(MP_ERROR_TEXT("Must set either read (int) or write (bytes)."));
Copy link
Member

Choose a reason for hiding this comment

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

Many Pythonistas would expect TypeError rather than a ValueError (we get here based on type checks after all).

Comment on lines +126 to +130
if (read_in == mp_const_none && !mp_obj_is_str(write_in) && mp_obj_is_str_or_bytes(write_in)) {
mp_obj_str_t *obj = ((mp_obj_str_t *)MP_OBJ_TO_PTR(write_in));
pbsys_program_load_set_user_data(offset, obj->data, obj->len);
return mp_const_none;
}
Copy link
Member

@dlech dlech Oct 21, 2022

Choose a reason for hiding this comment

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

How about allowing any object that uses the buffer protocol instead of strictly bytes to avoid casting/copying?

Suggested change
if (read_in == mp_const_none && !mp_obj_is_str(write_in) && mp_obj_is_str_or_bytes(write_in)) {
mp_obj_str_t *obj = ((mp_obj_str_t *)MP_OBJ_TO_PTR(write_in));
pbsys_program_load_set_user_data(offset, obj->data, obj->len);
return mp_const_none;
}
if (read_in == mp_const_none) {
mp_buffer_info_t bufinfo;
mp_get_buffer_raise(write_in, &bufinfo, MP_BUFFER_READ);
pb_assert(pbsys_program_load_set_user_data(offset, bufinfo.buf, bufinfo.len));
return mp_const_none;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, this is the one I mentioned I was hoping to fix.

// Handle write.
if (read_in == mp_const_none && !mp_obj_is_str(write_in) && mp_obj_is_str_or_bytes(write_in)) {
mp_obj_str_t *obj = ((mp_obj_str_t *)MP_OBJ_TO_PTR(write_in));
pbsys_program_load_set_user_data(offset, obj->data, obj->len);
Copy link
Member

Choose a reason for hiding this comment

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

missing pb_assert()

@laurensvalk
Copy link
Member Author

Feel free to merge in all three of your suggestions above.

dlech added a commit that referenced this pull request Oct 21, 2022
This updates the storage method to allow any object with the buffer
protocol instead of just bytes.

Also change the missing arg error to a TypeError while we are touching
this.

Issue: #121
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
software: pybricks-micropython Issues with Pybricks MicroPython firmware (or EV3 runtime)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants