Skip to content

Conversation

@schodet
Copy link
Contributor

@schodet schodet commented Dec 21, 2025

This migrates the LCD driver from nxos to pbio model. Handling is now done in pbio thread context and the driver is made compatible with pbio/image.

The nxos display functions are dropped, and the remaining code using them are updated to use pbio/image. If decision is taken to drop them, it can be done later.

Bring LCD driver into a dedicated pbio driver.

No feature change yet, only apply code formatting.

Refs: pybricks/support#2425
This is needed on NXT to avoid damaging the display. Call it in
pbdrv_deinit.

Refs: pybricks/support#2425
Use a PBIO thread to handle the screen refresh instead of doing it in
ISR.

The driver provides the frame buffer which is used by the user. On
refresh, format is converted to match the one used by the LCD driver.

In the future, the user frame buffer may use the LCD driver format in
order to improve memory usage. This will require pbio/image to handle
this format.

Refs: pybricks/support#2425
Remove unused debug display code. Use pbio image and driver instead of
nxos display functions when appropriate.

Refs: pybricks/support#2425
This is needed to allocate memory for images, without using the
micropython allocator.

Refs: pybricks/support#2425
@coveralls
Copy link

coveralls commented Dec 21, 2025

Coverage Status

coverage: 55.799% (-0.02%) from 55.816%
when pulling b955b07 on schodet:nxt-display
into 22c6e47 on pybricks:master.

@laurensvalk
Copy link
Member

Fantastic! Can't wait to try this out.

static void spi_write_command_byte(uint8_t command) {
spi_set_tx_mode(SPI_MODE_COMMAND);

// Wait for the transmit register to empty.
Copy link
Member

Choose a reason for hiding this comment

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

If I understand correctly, this blocking loop is only used during setup and the synchronous panic handler?

If so, then we should be OK, but it's worth adding a note about this to justify the blocking loop.

But if we are dropping the panic handler (see chat conversation), we could just as well make this non-blocking. We can use await_once + request_poll in the loop in that case so it is still very quick.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, your understanding is right. The loop should take something like 192 cycles (48 MHz / 2 MHz * 8).

await_once + request_poll seems a good solution, I was afraid to need an interrupt to do it right.

I also have a blocking 20 ms wait in deinit, is it correct?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, we'd want to make that non-blocking too.

We can use this pattern like we do in a few other places, so no additional processes are needed.

// this doesn't do deinit right away, but it ask the display process to gracefully exit at a useful spot, and then do deinit. The busy count is used so all processes can await deinit before actual power off.

void pbdrv_display_deinit(void) {
    pbio_busy_count_up();
    pbio_os_process_make_request(&pbdrv_display_nxt_process, PBIO_OS_PROCESS_REQUEST_TYPE_CANCEL);
}

And then replace:

    // Update the display with the user frame buffer, if changed.
    for (;;) {
        ...
    }

    PBIO_OS_ASYNC_END(PBIO_SUCCESS);
}

with

    // Update the display with the user frame buffer, if changed.
    while (pbdrv_display_nxt_process.request != PBIO_OS_PROCESS_REQUEST_TYPE_CANCEL) {
        ...
    }

    // ...
    *AT91C_SPI_IDR = ~0;
    *AT91C_SPI_PTCR = AT91C_PDC_TXTDIS;
    spi_write_command_byte(RESET());  // could also be non blocking as discussed separately

    PBIO_OS_AWAIT_MS(state, timer, 20); // instead of     nx_systick_wait_ms(20);

    pbio_busy_count_down();

    PBIO_OS_ASYNC_END(PBIO_ERROR_CANCELLED);
}

Use asynchronous code to avoid blocking.

Thanks: Laurens for the code to do it.

Refs: pybricks/support#2425
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.

3 participants