pico-sdk
pico-sdk copied to clipboard
raspberrypi
(WIP) Disable UART when writing to LCR
See #548.
Details are quite messy. Our API supposedly lets you poke the UART settings whilst it's enabled, and doesn't impose restrictions on what state it can be in when you do this (e.g. it might be mid-character).
First problem is the UART requires you to disable it when you change the settings, slightly bigger problem is there is seemingly no way to cleanly halt and restart the UART when it may be mid character, because it doesn't have the flags you would need to poll to do this. This PR attempts to address this by asserting a slightly longer than one character busy wait after disabling the UART.
Another approach would be to allow the UART to be left disabled after uart_init() is called, so the user can poke settings and then enable it. To change settings they would need to reinitialise the UART, which under the hood would just toggle the reset like it does today. This would be a breaking API change, but as the current implementation is quite broken, it doesn't seem like many people are using the uncommon UART formats.
Two options: https://github.com/raspberrypi/pico-sdk/issues/548#issuecomment-915210409
Note would need struct initialization function. Documentation update would be required. However documentation is already needed for delay.
Another option is to hide these functions since they are not really usable. Full hardware module is not supported. Force user to init and deinit to change format and baud rate. Add format to init would be minimal API change.
Deprecate/remove hardware_uart?
Edit: I am not sure that these will really bring anything useful. Could allow for full hardware module support, however it looks like that is not supported here intentionally? Makes more sense to use register interface for other things?
I would add a comment here: https://raspberrypi.github.io/pico-sdk-doxygen/group__hardware__uart.html
Deprecate/remove hardware_uart?
Why on earth would we do that? It provides useful functionality. there is always register access for what it doesn't provide
Deprecate/remove hardware_uart?
Why on earth would we do that? It provides useful functionality. there is always register access for what it doesn't provide
Currently the hardware_uart only works for specific things like printing. It is not UART library. This change has undefined behavior which could be confusing. (Rarely ever a problem.) However this change does restore the API back for most of UART function. One option is to remove the hardware_uart and say that it only works for specific print functions.
Only the CMAKE logic could change the format. Baud rate with fixed format used for everything else. Thus the UART is still possibly under 8/1/N format. However hardware module must be deinit to change baudrate.
Currently register interface is required for certain things in many of the hardware modules. Designing the interface to support these has complexity which may not be desirable. Currently there is some overhead which may not be desirable so it may be recommended to use register access.
Microchip PLIB for example is very clean and basically abstract the register access into C wrappers. However some of them had overhead. However for basic protocols like SPI and UART this usually never a problem. I2C sometimes could add overhead. (Which perhaps is not the PLIB's fault but rather a design issue if this matters.) However that API could be considered somewhat confusing unless you understand the hardware. The SDK attempts to avoid this problem potentially.
Just a suggestion, See my edit in my comment above.
yeah, i mean in general i think i would have preferred if the UART had used a builder pattern as per some of the other h/w. This was something i wanted to look at before launch (the uart code came from an earlier incarnation).
note that all this talk of CMake still makes no sense. i think your complaint is that the hardware_uart packaged is somewhat focused on simple 8 bit transmission/receive, but the fact that pico_stdio_uart uses it is neither here nor there, and that there are some CMake convenience functions for choosing amongst the stdio implementations seems even less germaine.
we should certainly make the existing uart functions behave correctly (as @Wren6991 is looking at) such that you can configure things correctly. it may well be that adding a new init function which is builder based alongside the rest is desirable (and maybe leaving the old way broken if it can't cleanly be fixed).
As for more advanced uart functionality, there doesn't seem to have been a lot of requests for this thus far, so i'm not particularly in favor of adding a whole abstraction. I don't know enough about UART use cases to know what this would look like.
Only the CMAKE logic could change the format
Yes, due to a bug in the implementation. This isn't an API problem.
To be clear, this PR is to make the current API work as advertised. Feel free to open another issue to make the case for API changes.
