Skip to content

ch32v: Revamp the board-level usart setup design - #1021

Open
Grazfather wants to merge 2 commits into
mainfrom
ch32_uart_2
Open

ch32v: Revamp the board-level usart setup design#1021
Grazfather wants to merge 2 commits into
mainfrom
ch32_uart_2

Conversation

@Grazfather

Copy link
Copy Markdown
Collaborator

Following up on discussion on discord, addressing some concerns.

  1. 'Config' vs. 'Setup'
  2. Stuff like baud should not be in the board file. It is an 'application level' configuration.
  3. The setup function is clunky. Put it into a method.
  4. The @hasDecl is clunky. Move it into the method.

Comment thread port/wch/ch32v/src/hals/usart.zig Outdated
@compileError("board does not provide a default uart_setup");

/// Apply 'setup' and 'config'
pub fn setup(comptime self: UartSetup, comptime config: Config) void {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should this be "apply" then? I like the distinction of setup vs config, but "apply up the setup and config" makes more sense than "setup the setup and config"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I agree apply is a better name

Other than that, looks good to me

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I should have thought of that! The convo mentioned setup and I didn't think beyond that :)

pub const uart_config: ch32v.usart.UartConfig = .{};
pub const uart_setup: ch32v.usart.UartSetup = .{
.instance = .USART1,
.tx_pin = ch32v.gpio.Pin.init(0, 9), // PA9

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

will make this optional, support rx_pin, but I don't have access to hardware to test the rx functionality.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Okay let me know once that's tested and we'll merge.

@tact1m4n3 tact1m4n3 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good!

Following up on  discussion on discord, addressing some concerns.

1. 'Config' vs. 'Setup'
2. Stuff like baud should not be in the board file. It is an
   'application level' configuration.
3. The setup function is clunky. Put it into a method.
4. The `@hasDecl` is clunky. Move it into the method.
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.

4 participants