Fix UartMux to be able to use multiple muxes #3216

alexandruradovici · 2022-09-13T12:05:57Z

Pull Request Overview

This pull request fixes the way in which UartMux is being instantiated. Before this PR:

the new function from the UartMuxComponent would instantiate the UartMux using a static_init! macro. Due to this, instantiating two UartMux structures would result in the second instance overwriting the first one.
the RX_BUF was defined within the virtual_uart capsule, meaning that two instances of UartMux would use the same buffer.

This PR:

moves the instantiating of the UartMux in the uart_mux_component_helper macro.
moves the instantiating of the RX_BUF in the uart_mux_component_helper macro.

Example

let uart_mux1 = components::console::UartMuxComponent::new(
    &peripherals.usart1,
    115200,
    dynamic_deferred_caller,
)
.finalize(components::uart_component_helper!(64));

let uart_mux2 = components::console::UartMuxComponent::new(
    &peripherals.usart2,
    115200,
    dynamic_deferred_caller,
)
.finalize(components::uart_component_helper!(64));

Without this PR uart_mux2 overwrites uart_mux1, as the structure is allocated in the same memory space.

This PR enables the usart2 for the STM32F3Discovery board.

This is similar to what the I2C and SPI components do.

Testing Strategy

This pull request was tested using an STM32F3 Discovery board with two serial ports. One serial port was used for the kernel messages and process console, while the second one was used for applications.

TODO or Help Wanted

N/A

Documentation Updated

Updated the relevant files in /docs, or no updates are required.

Formatting

Ran make prepush.

bradjc · 2022-09-13T12:31:19Z

Does this conflict or overlap #3046?

alexandruradovici · 2022-09-13T12:36:33Z

Does this conflict or overlap #3046?

I don't think it does, #3046 does not seem to change anything for the mux initialization or the uart receive buffer. The boards/components/src/console.rs file is not changed.

hudson-ayers

Looks good to me -- it was always a problem that the receive buffer was defined in the capsule.

I am adding a new label for this PR to track changes that modify the board interface in a way that an out-of-tree board maintainer would need to replicate, just so we can track these types of changes between releases.

bradjc

I see that this is inline with the I2C component, but I'm not sure about this change. static_ini_half! was created for a very specific purpose: to support sharing components when the actual object is templated on a chip-specific type. Here there is no chip-specific type in the virtual uart object, and as such the console component didn't use static_init_half.

I don't think we should continue to use static_init_half for a different purpose (to allow the same component to be used multiple times). Also, there is currently no indication of which components can be used multiple times. If components are going to be maintainable and usable, I think we need something more clear about how they can be used.

hudson-ayers · 2022-09-13T23:14:55Z

Ideally the policy should be that every component is reusable; but that would require a high-effort overhaul of all components to get there, and we have seen with #1618 that there isn't anyone motivated for a whole-scale components overhaul. IMO it seems like a shame to not at least make usability improvements on an as-needed basis, or to block those improvements on a very high-effort complete overhaul.

alexandruradovici · 2022-09-14T03:16:35Z

@bradjc are you suggesting moving the arguments of new to the macro?

The UartMux needs to be initialized in a macro, otherwise it will be overwritten every time we call new.

bradjc · 2022-09-14T03:36:11Z

I'm not suggesting we do a full change to components, or that we cannot upgrade the console component independently of all other components. What I'm saying is that static_init_half!() is a hack I added to allow us to share components across microcontrollers. According to our own documentation, we aren't supposed to be using static_init_half anymore:

tock/kernel/src/utilities/static_init.rs

Lines 136 to 142 in d21666d

    
           /// This macro is deprecated. You should migrate to using `static_buf!` 
        
           /// followed by a call to `StaticUninitializedBuffer::initialize()`. 
        
           /// 
        
           /// Same as `static_init!()` but without actually creating the static buffer. 
        
           /// The static buffer must be passed in. 
        
           #[macro_export] 
        
           macro_rules! static_init_half {

But what I want to avoid is adding a new hack using an old hack to get re-usable components. I assume there must be some mechanism we can add or use that makes it clear that the console component is now reusable.

alexandruradovici · 2022-09-14T04:24:24Z

So using static_buf instead would work?

alexandruradovici · 2022-09-14T12:04:15Z

I updated it to use the static_buf macro.

capsules/src/virtual_uart.rs

boards/stm32f3discovery/src/main.rs

boards/litex/arty/src/main.rs

alexandruradovici · 2022-09-26T10:07:44Z

I rebased it to use the new static_buf with MaybeUninit. I think this is good to go.

boards/components/src/console.rs

bradjc · 2022-09-28T14:47:01Z

bors r+

3216: Fix UartMux to be able to use multiple muxes r=bradjc a=alexandruradovici ### Pull Request Overview This pull request fixes the way in which `UartMux` is being instantiated. Before this PR: - the `new` function from the `UartMuxComponent` would instantiate the `UartMux` using a `static_init!` macro. Due to this, instantiating two `UartMux` structures would result in the second instance overwriting the first one. - the `RX_BUF` was defined within the `virtual_uart` capsule, meaning that two instances of `UartMux` would use the same buffer. This PR: - moves the instantiating of the `UartMux` in the `uart_mux_component_helper` macro. - moves the instantiating of the `RX_BUF` in the `uart_mux_component_helper` macro. #### Example ```rust let uart_mux1 = components::console::UartMuxComponent::new( &peripherals.usart1, 115200, dynamic_deferred_caller, ) .finalize(components::uart_component_helper!(64)); let uart_mux2 = components::console::UartMuxComponent::new( &peripherals.usart2, 1 8000 15200, dynamic_deferred_caller, ) .finalize(components::uart_component_helper!(64)); ``` Without this PR `uart_mux2` overwrites `uart_mux1`, as the structure is allocated in the same memory space. This PR enables the `usart2` for the STM32F3Discovery board. This is similar to what the I2C and SPI components do. ### Testing Strategy This pull request was tested using an STM32F3 Discovery board with two serial ports. One serial port was used for the kernel messages and process console, while the second one was used for applications. ### TODO or Help Wanted N/A ### Documentation Updated - [x] Updated the relevant files in `/docs`, or no updates are required. ### Formatting - [x] Ran `make prepush`. Co-authored-by: Alexandru RADOVICI <alexandru.radovici@wyliodrin.com> Co-authored-by: Alexandru Radovici <msg4alex@gmail.com>

bors · 2022-09-28T14:48:45Z

Build failed:

ci-build (ubuntu-latest)

this does duplicate code, but without this change, users may use the uart_mux_component_helper with an expression that is not a constant, which will not compile

alexandruradovici · 2022-09-30T11:52:19Z

Rebased

boards/components/src/console.rs

Co-authored-by: Brad Campbell <bradjc5@gmail.com>

bradjc · 2022-09-30T18:34:16Z

bors r+

bors · 2022-09-30T19:07:29Z

Build succeeded:

github-actions bot added the WG-OpenTitan In the purview of the OpenTitan working group. label Sep 13, 2022

alexandruradovici force-pushed the uart_mux_fix branch from 8d245f2 to 904672c Compare September 13, 2022 12:30

hudson-ayers previously approved these changes Sep 13, 2022

View reviewed changes

hudson-ayers added the board-interface-change Changes the interface between boards and the kernel; may need to be replicated in out-of-tree boards label Sep 13, 2022

bradjc requested changes Sep 13, 2022

View reviewed changes

alexandruradovici dismissed hudson-ayers’s stale review via 6110d5f September 14, 2022 11:24

github-actions bot added the stm32 Change pertains to the stm32 family of MCUSs label Sep 14, 2022

bradjc reviewed Sep 14, 2022

View reviewed changes

capsules/src/virtual_uart.rs Show resolved Hide resolved

alexandruradovici force-pushed the uart_mux_fix branch from 3566a55 to 30b24b7 Compare September 15, 2022 12:13

alistair23 reviewed Sep 15, 2022

View reviewed changes

boards/stm32f3discovery/src/main.rs Outdated Show resolved Hide resolved

boards/litex/arty/src/main.rs Show resolved Hide resolved

hudson-ayers previously approved these changes Sep 19, 2022

View reviewed changes

alistair23 previously approved these changes Sep 20, 2022

View reviewed changes

bradjc added the blocked Waiting on something, like a different PR or a dependency. label Sep 22, 2022

alexandruradovici dismissed stale reviews from alistair23 and hudson-ayers via 814faa5 September 26, 2022 10:07

alexandruradovici force-pushed the uart_mux_fix branch from bc0a8cf to 814faa5 Compare September 26, 2022 10:07

bradjc reviewed Sep 26, 2022

View reviewed changes

boards/components/src/console.rs Outdated Show resolved Hide resolved

8000

boards/components/src/console.rs Outdated Show resolved Hide resolved

bradjc removed the blocked Waiting on something, like a different PR or a dependency. label Sep 26, 2022

bradjc previously approved these changes Sep 27, 2022

View reviewed changes

bradjc added the last-call Final review period for a pull request. label Sep 27, 2022

alistair23 previously approved these changes Sep 27, 2022

View reviewed changes

bradjc added the needs-rebase label Sep 28, 2022

alexandruradovici added 8 commits September 30, 2022 14:51

use of multiple uart mux

23afcae

update to static_buf and handle interrupts

3150470

use buffer len from virtual_uart

7e2e62f

make sure that macro parameter will be a literal

356ad9f

this does duplicate code, but without this change, users may use the uart_mux_component_helper with an expression that is not a constant, which will not compile

rebase

a548b29

enable adc channels for stm32f3discovery

fdcb209

format arty/src/main.rs

1a88d11

rebase to use static_buf with MaybeUninit

58eb86c

alexandruradovici dismissed stale reviews from alistair23 and bradjc via 58eb86c September 30, 2022 11:52

alexandruradovici force-pushed the uart_mux_fix branch from 015a623 to 58eb86c Compare September 30, 2022 11:52

update sifive

61e5d9b

bradjc requested changes Sep 30, 2022

View reviewed changes

boards/components/src/console.rs Outdated Show resolved Hide resolved

boards/components/src/console.rs Outdated Show resolved Hide resolved

alexandruradovici and others added 2 commits September 30, 2022 19:51

Update boards/components/src/console.rs

83e58c2

Co-authored-by: Brad Campbell <bradjc5@gmail.com>

Update boards/components/src/console.rs

2093664

Co-authored-by: Brad Campbell <bradjc5@gmail.com>

bradjc removed the needs-rebase label Sep 30, 2022

bradjc approved these changes Sep 30, 2022

View reviewed changes

bors bot merged commit 77b46df into tock:master Sep 30, 2022

mateibarbu19 added a commit to WyliodrinEmbeddedIoT/tock that referenced this pull request Oct 8, 2022

redboard_redv: add fix from merge tock#3216

6e970e7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix UartMux to be able to use multiple muxes #3216

Fix UartMux to be able to use multiple muxes #3216

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Fix UartMux to be able to use multiple muxes #3216

Fix UartMux to be able to use multiple muxes #3216

Uh oh!

Conversation

Pull Request Overview

Example

Testing Strategy

TODO or Help Wanted

Documentation Updated

Formatting

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!