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

Starting to flesh out a uart API #1

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 83 additions & 3 deletions hal.zig
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,91 @@ pub const Pin = enum(u8) { // or whatever fits for the HAL

/// Runtime lookup of a pin by name.
pub fn resolve(name: []const u8) ?Pin;

/// The corresponding UART instance this pin can be configured to.
/// - However... What happens when multiple Uarts can be routed to the same pin?
/// - There's an argument this function shouldn't exist, and it's on the user
/// to correctly configure the corresponding pins before using the UART driver,
/// and specify the correct Uart instance themselves.
pub fn to_uart_instance(self: Pin) !uart.Instance;
Comment on lines +11 to +17
Copy link

Choose a reason for hiding this comment

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

Imho, "Pin to peripherial conversion" is the wrong way, and we should either go the other way round or just provide a generic purpose function selector or something like that

};

pub const UART = struct {
pub fn write_blockingly(uart: UART, chunk: []const u8) !usize;
pub fn read_blockingly(uart: UART, chunk: []const u8) !usize;
// Pretending we're in uart.zig
pub const uart = struct {

// Seems like a useful abstraction instead of limiting via a "uN" integer type,
// there's usually a very reasonable number of UARTs on any one chip so this
// enum shouldn't be too tedious to populate by hand.
pub const Instance = enum {
uart0,
uart1,
uart2,
};
Comment on lines +23 to +30
Copy link

Choose a reason for hiding this comment

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

How could we handle different properties of different uarts like "uart0 and uart2 are of type a, but uart1 is completely different" (LPC1768, or even worse: AVR)

Copy link
Author

Choose a reason for hiding this comment

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

Hmmm little confused what you mean, like the UART peripheral itself varies by instance (USART vs UART on ST for instance), or you're talking to different devices on each instance?


/// Configuration parameters only relevant to operating the Uart in "dma" mode
pub const DmaConfig = struct {
dma_channel: u8,
// Imagine other relevant params
};

/// Configuration parameters only relevant to some other weird Uart mode I haven't thought of
pub const OtherSpecialConfig = struct {};

/// I don't love the name, but one possible way to allow extendability
/// for special configuration modes that go beyond "vanilla" uart usage
pub const ExtendedConfig = union(enum) {
dma: DmaConfig,
other: OtherSpecialConfig,
};

pub const Parity = enum {
none,
odd,
even,
};
Comment on lines +48 to +52
Copy link

Choose a reason for hiding this comment

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

all legal values:

  • none
  • even
  • odd
  • mark
  • space

Copy link
Author

Choose a reason for hiding this comment

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

Oh yeah, this isn't an exhaustive list, was more just noodling on a "rough" structure for what a peripheral driver looks like


/// Common configuration parameters to all Uart use cases
pub const Configuration = struct {
baud_rate: u32,
parity: Parity,
stop_bit: bool,
Copy link

Choose a reason for hiding this comment

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

enum { one, two }

// Use your imagination for other params :)

// Allows specific configurations for something like DMA, etc.
mode_specific: ?ExtendedConfig,
Comment on lines +61 to +62
Copy link

Choose a reason for hiding this comment

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

i would just "inline" them. if they are available, it works, otherwise it won't

Copy link
Author

Choose a reason for hiding this comment

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

I think that makes sense. What I was trying to avoid here was the ST HAL style massive configuration structs that contain any and every possible option and it's up to you to pick through the code to see which fields are used when. A tagged enum lets you be a little more explicit that "I'm providing configuration for this specific mode". Food for thought.

};

pub const UART = struct {
instance: Instance,

/// What is yall's opinion on this piece of boilerplate?
/// Pros:
/// - blocks users from calling write_blockingly/read_blockingly with an error
/// if they try to use the HAL without calling init()
/// Cons:
/// - There are some edge cases where some users might want to do their own low level config
/// themselves at register level, and skip calling init() but still use write_blockingly/read_blockingly
/// - But if this is the case, would they be using the HAL to begin with...?
initialized: bool,
Comment on lines +68 to +76
Copy link

Choose a reason for hiding this comment

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

I would not define fields, because it migh the totally reasonable to implement a UART as opaque {} or enum(…) {}

Copy link
Author

Choose a reason for hiding this comment

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

Hmmmmmm.... This one is interesting... Do we never need fields for any peripheral? That seems like it might be super restrictive if you need to hold some kind of state for a given instance. I'm not necessarily disagreeing I just don't want to code ourselves into a corner.


/// Should put the UART peripheral into a state where it's ready to call methods that actually
/// do something (write some bytes, read some bytes, whatever)
pub fn init(self: *UART, config: Configuration) !void {
Copy link

Choose a reason for hiding this comment

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

naming scheme here would be apply, and i'd remove deinit and implement a reset


// Various Uart register writes
// ...
//
self.initialized = true;
}

pub fn deinit(self: *UART) void {
// Returns UART peripheral registers to "reset" state
self.initialized = false;
}

pub fn write_blockingly(uart: UART, chunk: []const u8) !usize;
pub fn read_blockingly(uart: UART, chunk: []const u8) !usize;
};
};

pub const I2C_Host = struct {
Expand Down