Refactor src/uartdev for async and code size #169
Conversation
|
Todos:
Originally posted by @dlech in #169 (comment)
Originally posted by @dlech in #169 (comment)
Originally posted by @dlech in #169 (comment) Done by getting format first, which asserts number of modes.
Originally posted by @dlech in #169 (comment) Had this initially, but that still requires casting to
Originally posted by @dlech in #169 (comment) |
|
There was a note on naming in one of the review comments. Since
We could drop the Names like We could defer this until we actually get to #159 though. |
One limitation of the previous version of the code and carried over now, is that we only assert the device ID when instantiating the user device class. In theory you could instantiate the class, unplug the sensor, plug in something else, and then do a read/write. Now that everything is consolidated to just a few generic get/set calls, it may be a bit cheaper than before to assert not only the mode but also the ID. |
|
To make it safer, instead of pbio_error_t pbio_iodev_get_data(pbio_iodev_t *iodev, uint8_t mode, void **data)We could do: pbio_error_t pbio_iodev_get_data(pbio_iodev_t *iodev, uint8_t mode, void *data)And let it explicitly unpack the data based on its type in the mode info. |
|
It is currently probably in a shape where I can start completing the (separate) async PR. Would you be interested in taking this pull request further if you think that is better? |
These were no-ops since pbdrv_uart_write_cancel was still "//TODO" on all platforms except on Move Hub. Considering that LEGO UART sensors work fine without it, we can make the higher level code easier and smaller by by not calling these no-ops on cancel.
Only set_data is used.
The current iodev and uartdev code will only ever be used for LEGO uart devices, so we can reduce the level of abstraction and complexity considerably. Combined with the previous and next commits, this reduces code size by about 580 bytes and helps prepare for async functionality later on, which is the main reason for this refactoring. The goal it to keep src/uartdev.c completely in charge of uart devices, instead of relying on user modules correctly calling operations like set_mode_end.
This further simplifies handling waits/awaits when writing data, so fewer hacks are needed in the high-level modules, saving code size.
This is more stable and will be cheaper in the async case, since this does not require both a mode switch and a data setter.
See also dd322df for portions that were previously removed.
This lets pbio/uartdev handle setting data on completing the mode change. When we get to async later on, this means the async handlers don't need class-specific data buffers or states to remember what phase of mode/data setting they are in.
Based on review in #169.
This way it completes even if the higher level modules never check the ready state.
|
Returning void* from pb_pup_device_get_data() instead of passing **data as arg save a bit on code size. Using void* instead of uint8_t* eliminates the need for explicit casts. Also always use array access of the returned value for consistency.
Some functions were already moved the from the iodev module to the uartdev module. This moves the declarations to the correct header and fixes the namespace prefix in the function names.
This saves some CONTAINER_OF calls by passing the uartdev port_data pointer directly to private methods instead of passing the iodev pointer.
The virtualhub doesn't use uartdev currently so was failing when trying to initialize motors. Code is disable on virtualhub until we can make a more general solution.
Sounds good. I've been working on exactly this, let's see what we can save here... EDIT: Pushed some drafts to https://github.com/pybricks/pybricks-micropython/tree/iodev-explore This saves a further 200 bytes and goes some way to address several old FIXMEs about not using the port to access things. I'm not merging that into this PR yet as it may not be needed for now and isn't fully finished (it would be nice to clean up how motor IDs work). |
|
Thanks for the review and additional improvements! Since several other PRs hinge on this, I've gone ahead and merged it. We can continue to polish in a follow up PR as needed. |
The current iodev and uartdev code will only ever be used for LEGO uart devices, so we can reduce the level of abstraction and complexity considerably. More generic devices would be handled by a port interface like proposed in #159.
The main reason for doing is to prepare for handling sensors in an async way compatible with #166. Now, we can make a single synchronous call to set the mode/data (and raise a relevant error as needed), and await data to be ready in the same way.
This also reduces the code size by about 570 bytes despite adding new functionality like buffered lpf2 writes, which should further reduce the (otherwise needed) size to implement async for sensors.
The final commit with buffered data writes is perhaps the more controversial one, as it gets a bit complicated. We could consider doing without that one and check if it's needed when we get to the async part.