pbdrv/legodev: Refactor LEGO device abstraction. #174

laurensvalk · 2023-06-02T13:22:04Z

This is a draft to revise how we deal with lego devices at the low and high level.

This large change is based on findings from the past years. Instead of trying to have an all-encompassing iodev abstraction (which ended up being hard-coded for LEGO devices), we have an interface specifically for LEGO devices, separate from port I/O.

In the future, this may play the role of LEGO-mode on a port (alongside raw UART/I2C modes, etc.).

The interface is platform agnostic, but there will be platform-specific implementations for:

Powered Up (done)
Virtual Hub (done)
ev3dev (in progress)
tests (todo)
nxt (todo)

The following sums it up quite well -- this pull request goes a long way to get rid of technical debt like this:

// Delete me: refactor getter only still used for passive external light.
pbio_iodev_t *pb_pup_device_get_device(pbio_port_id_t port, pbio_iodev_type_id_t valid_id) {

    pbio_iodev_t *iodev;
    pbio_error_t err;
    while ((err = pbdrv_ioport_get_iodev(port, &iodev)) == PBIO_ERROR_AGAIN) {
        mp_hal_delay_ms(50);
    }
    pb_assert(err);

    // Verify the ID or always allow generic LUMP device
    if (iodev->info->type_id != valid_id && valid_id != PBIO_IODEV_TYPE_ID_LUMP_UART) {
        pb_assert(PBIO_ERROR_NO_DEV);
    }

    return iodev;
}

// Revisit: get rid of this abstraction
void pb_pup_device_setup_motor(pbio_port_id_t port, bool is_servo) {
    // HACK: Built-in motors on BOOST Move hub do not have I/O ports associated
    // with them.
    #if PYBRICKS_HUB_MOVEHUB
    if (port == PBIO_PORT_ID_A || port == PBIO_PORT_ID_B) {
        return;
    }
    #endif

    pb_module_tools_assert_blocking();

    // Get the iodevice
    pbio_iodev_t *iodev;
    pbio_error_t err;

    // Set up device
    while ((err = pbdrv_ioport_get_iodev(port, &iodev)) == PBIO_ERROR_AGAIN) {
        mp_hal_delay_ms(50);
    }
    pb_assert(err);

    // Only motors are allowed.
    if (!PBIO_IODEV_IS_DC_OUTPUT(iodev)) {
        pb_assert(PBIO_ERROR_NO_DEV);
    }

    // If it's a DC motor, no further setup is needed.
    if (!PBIO_IODEV_IS_FEEDBACK_MOTOR(iodev)) {
        return;
    }

    // FIXME: there is no guarantee that the iodev is a uartdev
    #if !PYBRICKS_HUB_VIRTUALHUB

    // Choose mode based on device capabilities.
    uint8_t mode_id = PBIO_IODEV_IS_ABS_MOTOR(iodev) ?
        PBIO_IODEV_MODE_PUP_ABS_MOTOR__CALIB:
        PBIO_IODEV_MODE_PUP_REL_MOTOR__POS;

    // Activate mode.
    pb_pupdevices_obj_base_t device = {
        .iodev = iodev,
    };
    pb_pupdevices_get_data_blocking(&device, mode_id);

    #endif // !PYBRICKS_HUB_VIRTUALHUB
}

// REVISIT: Drop pb_device abstraction layer
void pb_device_setup_motor(pbio_port_id_t port, bool is_servo) {
    pb_pup_device_setup_motor(port, is_servo);
}

It also adds an option to exclude some pup_uart functionality to reduce build size on Move Hub by about 500 bytes.

laurensvalk · 2023-06-05T20:07:40Z

Getting somewhere on this one now. Remaining tasks:

finish ev3dev sensors
fix uartdev tests not giving expected results
small cleanups

coveralls · 2023-06-06T11:53:40Z

coverage: 54.468% (+0.02%) from 54.444% when pulling 6905609 on legodev into 3317369 on master.

github-actions · 2023-06-06T11:53:42Z

Download the artifacts for this pull request:

laurensvalk · 2023-06-06T18:36:01Z

All checks are passing now. This will probably need some more testing before we can merge, particularly on more/all hubs.

There was one spurious bug earlier today where a motor would not sync up or start, but I can't seem to reproduce it now.

If nothing is immediately found, my vote would be to merge it on Thursday or Friday and fix any bugs encountered later on.

laurensvalk · 2023-06-07T09:46:16Z

There was one spurious bug earlier today where a motor would not sync up or start, but I can't seem to reproduce it now.

I am occasionally able to reproduce it. It appears that setting the default mode (immediately after syncing up) does not always succeed, and we currently don't retry.

So maybe we should wait briefly before setting the default mode, or retry if a mode switch hasn't succeeded.

dlech · 2023-06-07T12:42:46Z

We should put a logic analyzer on it to see what is actually going over the wire.

laurensvalk · 2023-06-07T19:27:26Z

Nice.

I have an idea that would take care of this issue and also make the general mode setting and data setting code easier to follow and more robust.

Basically, we could modify this existing loop:

while (data->status == PBDRV_LEGODEV_PUP_UART_STATUS_DATA) { 
     // await send keep alive on EV3_UART_IO_TIMEOUT
}

to:

while (data->status == PBDRV_LEGODEV_PUP_UART_STATUS_DATA) { 

     // wait until any of following conditions, then

     // await send keep alive if EV3_UART_IO_TIMEOUT

     // await set mode if requested (unsets request)

     // await set data if requested (unsets request)
}

This avoids clashes between setting stuff and sending keep alive (theoretically possible since #169 although never happened) by keeping all sending in one place.

But it also lets us simplify the main process loop:

    while (true) {
        for (i = 0; i < PBDRV_CONFIG_LEGODEV_PUP_UART_NUM_DEV; i++) {
            port_data = &dev_data[i];

            pbdrv_legodev_pup_uart_update(port_data);

            if (port_data->status == PBDRV_LEGODEV_PUP_UART_STATUS_DATA) {
                pbdrv_legodev_pup_uart_receive_data(port_data);
            }

            // CAN DROP: this was a hack to handle awaited mode/data sets
            pbdrv_legodev_pup_uart_handle_write_end(port_data);

            // CAN DROP: handled simpler above
            if (port_data->status == PBDRV_LEGODEV_PUP_UART_STATUS_DATA) {
                pbdrv_legodev_pup_uart_handle_data_set_start(port_data);
            }
        }
        PROCESS_WAIT_EVENT();
    }

I've just tried the mode setting part (still need to do data set), and this seems to work well. It also has the benefit of retrying automatically in case of failure.

laurensvalk · 2023-06-08T11:18:30Z

Above issue is fixed, and did some more code consolidation to save on size. This PR saves us another 700 bytes so we have a whole 1KB to spare again which should cover us on bug fixes for a while. 🚀

This is a major overhaul of the lego device interface to clean up many longstanding hacks and simplify maintenance of present and future platforms, including EV3 and NXT. It is also a first step towards generalizing ioports, of which legodev will be one mode.

This handles all writing in one place to avoid clashes and makes the code easier to follow. This also retries setting the mode if it failed.

There were several write_begin and write_end calls used in a similar way. There was already a PT_THREAD for pbdrv_legodev_pup_uart_send_speed_msg, which we can generalize and use for other messages too. This saves on build size. Because write operations are all handled in the same place, we also don't need the tx_busy mutex anymore.

If a device was busy on program exit, the check for power requirements returns ERROR_AGAIN as expected. But this caused the device capabilities not to be read, thus turning off the power to the ultrasonic sensor if the light was being set.

Fixes pybricks/support#1098

dlech mentioned this pull request Jun 2, 2023

drv/ioport_lpf2: add boot power quirk #175

Merged

laurensvalk force-pushed the legodev branch 5 times, most recently from 687e0b3 to c502e89 Compare June 5, 2023 20:05

laurensvalk force-pushed the legodev branch from c502e89 to 3c5d348 Compare June 6, 2023 11:48

laurensvalk force-pushed the legodev branch 2 times, most recently from caef8a4 to 6ab4d3f Compare June 6, 2023 18:27

laurensvalk force-pushed the legodev branch from 6ab4d3f to 698282e Compare June 8, 2023 10:31

laurensvalk changed the title ~~!wip pbdrv/legodev: Refactor LEGO device abstraction.~~ pbdrv/legodev: Refactor LEGO device abstraction. Jun 8, 2023

laurensvalk force-pushed the legodev branch 2 times, most recently from 9b3bc7b to 1116dd2 Compare June 8, 2023 18:48

laurensvalk added 4 commits June 8, 2023 20:51

pbio/drv/legodev/pup_uart: Set mode and data as requests.

f43c4a6

This handles all writing in one place to avoid clashes and makes the code easier to follow. This also retries setting the mode if it failed.

laurensvalk force-pushed the legodev branch from 1116dd2 to 5850509 Compare June 8, 2023 18:52

pybricks.common.LightArray: Fix off method.

6905609

Fixes pybricks/support#1098

laurensvalk merged commit 6905609 into master Jun 8, 2023

laurensvalk deleted the legodev branch June 8, 2023 19:31

tomijah mentioned this pull request Jul 1, 2023

[Bug] TechnicHub with L (88013) Motor pybricks/support#1131

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pbdrv/legodev: Refactor LEGO device abstraction. #174

pbdrv/legodev: Refactor LEGO device abstraction. #174

laurensvalk commented Jun 2, 2023

laurensvalk commented Jun 5, 2023 •

edited

Loading

coveralls commented Jun 6, 2023 •

edited

Loading

github-actions bot commented Jun 6, 2023 •

edited

Loading

laurensvalk commented Jun 6, 2023

laurensvalk commented Jun 7, 2023

dlech commented Jun 7, 2023

laurensvalk commented Jun 7, 2023 •

edited

Loading

laurensvalk commented Jun 8, 2023

pbdrv/legodev: Refactor LEGO device abstraction. #174

pbdrv/legodev: Refactor LEGO device abstraction. #174

Conversation

laurensvalk commented Jun 2, 2023

laurensvalk commented Jun 5, 2023 • edited Loading

coveralls commented Jun 6, 2023 • edited Loading

github-actions bot commented Jun 6, 2023 • edited Loading

laurensvalk commented Jun 6, 2023

laurensvalk commented Jun 7, 2023

dlech commented Jun 7, 2023

laurensvalk commented Jun 7, 2023 • edited Loading

laurensvalk commented Jun 8, 2023

laurensvalk commented Jun 5, 2023 •

edited

Loading

coveralls commented Jun 6, 2023 •

edited

Loading

github-actions bot commented Jun 6, 2023 •

edited

Loading

laurensvalk commented Jun 7, 2023 •

edited

Loading