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

pbio/drv/uart: Refactor async read and write #275

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

Conversation

laurensvalk
Copy link
Member

Motivation

I was working on a UART driver for a new device (not part of this PR) and wanted to take on some of the open tasks before adding another driver with the same open tasks/issues:

Get rid of the autostart for the UART process

Changing the initialization was easy enough, but then I realized that this wasn't actually a process like we have in the other drivers. The UART drivers are only used as part of other drivers, like the legodev driver (for LUMP devices), which would be running its own event-driven process.

The UART processes were there for a different reason, which was to postpone handling of IRQ data to the event loop, to update the transmission state. Since the UART drivers are only used as part of other processes, we can drive the poll handling from there. In practice, this amounts to just updating the transmission state before the completion check so no explicit separate poll step is necessary. (Which also fixes the subtle bug mentioned below.)

Get rid of event broadcasts

The UART poll handlers were broadcasting events to every other process even though only the processes that use UARTS need polling. This was a longstanding TODO in the drivers. This is now replaced with configurable callback. There is one callback per driver since in the general case different device drivers may use UARTS (not just legodev).

Potentially this is a change to watch out for, though in a good way. If there were any processes that were inadvertently relying on these unrelated broadcast events to move things along, these may now reveal themselves.

Better understanding of read and write tasks

We used to have a read_begin function and a read_end function we kept calling in a loop from within a protothread:

#define PBIO_PT_WAIT_READY(pt, expr) PT_WAIT_UNTIL((pt), (expr) != PBIO_ERROR_AGAIN)

    PBIO_PT_WAIT_READY(&ludev->pt, ludev->err = pbdrv_uart_read_begin(ludev->uart, ludev->rx_msg, 1, 10));
    if (ludev->err != PBIO_SUCCESS) {
        DBG_ERR(ludev->last_err = "UART Rx error during baud");
        PT_EXIT(&ludev->pt);
    }

    PBIO_PT_WAIT_READY(&ludev->pt, ludev->err = pbdrv_uart_read_end(ludev->uart));
    // ludev->err is then processed below

This separation made the implementation of pbdrv_uart_read_begin and pbdrv_uart_read_end somewhat hard to follow. For this PR I have combined these into a single protothread that we can spawn.

PT_SPAWN(&ludev->pt, &ludev->read_pt, pbdrv_uart_read(&ludev->read_pt, ludev->uart, ludev->rx_msg, 1, 10, &ludev->err));
    // ludev->err is then processed below

Fixing a subtle bug with single-byte reads

This also fixes a subtle bug where using begin/end to read a single byte always required a new byte, even if that was not the byte that was going to be read. I noticed this when I used the existing implementation for a MicroPython REPL. If more than one byte was pasted, it would be received into the ringbuffer by the IRQ but reading from it would be (N-1) steps behind, so you needed to type more characters before the old ones would be processed by pbdrv_uart_read_begin/end

Now, reading a byte will read an existing byte if it is available without requiring additional polling. Flush is now implemented on all platforms and can be called if a fresh byte is needed, so there is no loss of generality.

I almost don't see how this could have worked before, but perhaps the event broadcasting (and slightly more than necessary, one too many for each write) was providing enough ticks to move things along.

Build size
This also saves 360 bytes on Move Hub, which is a nice bonus.


This squashes the following intermediate commits:


pbio/drv/uart: Don't broadcast uart events to all processes.

Instead poll subscribers, which is just the legodev
process for now. This can be expanded to poll the
specific uart instead of all of them.

This also drops the uart processes, which were not
doing anything, saving some code size.


pbio/drv/uart: Replace begin/end with single protothread.

UART read and write always used begin/end sequentially, each wrapped
in a wait for err != PBIO_ERROR_AGAIN. This replaces the begin/end
pattern with one awaitable protothread.

This makes the code easier to follow and reduces code size.


pbio/drv/uart: Set callback per device.

Not all devices may be used by the legodev driver.

@coveralls
Copy link

coveralls commented Dec 2, 2024

Coverage Status

coverage: 56.349% (-0.04%) from 56.385%
when pulling 4f2ed8e on uarts
into 77eb752 on master.

This squashes the following intermediate commits:

pbio/drv/uart: Don't broadcast uart events to all processes.

Instead poll subscribers, which is just the legodev
process for now. This can be expanded to poll the
specific uart instead of all of them.

This also drops the uart processes, which were not
doing anything, saving some code size.

pbio/drv/uart: Replace begin/end with single protothread.

UART read and write always used begin/end sequentially, each wrapped
in a wait for err != PBIO_ERROR_AGAIN. This replaces the begin/end
pattern with one awaitable protothread.

This makes the code easier to follow and reduces code size.

pbio/drv/uart: Set callback per device.

Not all devices may be used by the legodev driver.
This can be used to debug non-blocking protothreads like the Bluetooth driver. We need to explicitly call back to the relevant process since the uart drivers are no longer broadcasting to every process.
This was a longstanding chore that we have done gradually over time.

The ADC driver was the last to be tranformed to the manual start format.

To ensure that this isn't a potentially breaking change in itself, we move the call to initialize the ADC driver to the end of pbdrv_init, which is where the process was previously auto-started.
This completes the transition started in the previous commit. The ADC does not depend on anything other than clocks, so we can move it along with the other drivers.
@laurensvalk
Copy link
Member Author

We we are doing this, let's also drop the autostart dependency for the one remaining driver that used it: ADC.

@laurensvalk
Copy link
Member Author

Move Hub savings: -400 bytes
Technic Hub savings: -180 bytes

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.

Nice one. Just a few suggestions.

Comment on lines +2277 to +2278
etimer_set(&timer, 100);
PROCESS_WAIT_EVENT_UNTIL(ev == PROCESS_EVENT_TIMER && etimer_expired(&timer));
Copy link
Member

Choose a reason for hiding this comment

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

Why delay boot for 100 ms?

It should be fine to just yield here and skip the timer.

Comment on lines +16 to +19
extern char debug_buffer[];
extern struct pt debug_printf_thread;
extern pbdrv_uart_dev_t *debug_uart;
extern pbio_error_t debug_err;
Copy link
Member

Choose a reason for hiding this comment

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

Please add a namespace prefix to public symbols.

@@ -343,6 +349,7 @@ static PT_THREAD(pbdrv_legodev_pup_thread(ext_dev_t * dev)) {
while (true) {

// Initially assume nothing is connected.
legodev_pup_disable_uart(dev->pins);
Copy link
Member

Choose a reason for hiding this comment

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

I didn't dig into it, but this looks suspect to me. It seems like this could interfere with the device detection code that expects pins to be in a certain state.

@@ -431,7 +447,15 @@ void pbdrv_legodev_init(void) {
pbio_dcmotor_get_dcmotor(legodev, &dcmotor);
legodev->ext_dev->uart_dev = pbdrv_legodev_pup_uart_configure(legodev_data->ioport_index, port_data->uart_driver_index, dcmotor);

// legodev driver is started after all other drivers, so we
// assume that we do not need to wait for this to be ready.
Copy link
Member

Choose a reason for hiding this comment

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

I would add an assert statement here so that we could at least catch this in debug builds to catch future changes that might break this assumption.

* We get notified when the uart driver has completed sending or receiving data.
*/
static void uart_poll_callback(pbdrv_uart_dev_t *uart) {
// REVISIT: Only need to poll the specified uart device.
Copy link
Member

Choose a reason for hiding this comment

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

We could add a flag that gets set here so that we only poke the uart instances that need it in the poll handler.

DBG_ERR(ludev->last_err = "UART Rx error during baud");
PT_EXIT(&ludev->pt);
}
PT_SPAWN(&ludev->pt, &ludev->read_pt, pbdrv_uart_read(&ludev->read_pt, ludev->uart, ludev->rx_msg, 1, 10, &ludev->err));
Copy link
Member

Choose a reason for hiding this comment

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

Add another macro if this timeout is different than EV3_UART_IO_TIMEOUT with a comment that explains the difference.

}
}
}

static void handle_exit(void) {
// Currently not used
Copy link
Member

Choose a reason for hiding this comment

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

Surround it with #if 0 or delete it if not used. Don't make it non-static.


typedef struct {

} pbdrv_uart_dev_t;

typedef void (*pbdrv_uart_poll_callback_t)(pbdrv_uart_dev_t *uart);
Copy link
Member

Choose a reason for hiding this comment

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

We should mention here that the callback can be called in an interrupt context to remind future implementers to be careful.


static void handle_exit(void) {
// Currently not used
void handle_exit(void) {
Copy link
Member

Choose a reason for hiding this comment

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

Prefer #if 0.

@laurensvalk
Copy link
Member Author

Thanks for your review!

I will update it as suggested, but not merge it just yet. I think we can do that after we make the next release so we can avoid breaking anything.

I'm also inclined to maybe drop that (unused) uart debug port interface as it is, and come up with something more useful in a future iteration. I'd like it to work from anywhere instead of from other processes. And also from the first port instead of the last (or any port) so it will work better with EV3.

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