feat(hal): implement PWM with example by NanaHigh · Pull Request #18 · rustsbi/artinchip-hal

NanaHigh · 2026-03-23T07:21:58Z

pbp-pwm example tests on Hengshan pi.

Copilot

Pull request overview

Adds a new PWM HAL implementation for the D13x SoC, wires it into artinchip-rt peripherals/pin-mux, and provides a PBP example + CI build coverage for the example.

Changes:

Introduce artinchip-hal::pwm module (register definitions, config, channel driver, pad traits, extension trait).
Expose PWM in artinchip-rt D13x Peripherals + add PWM pin-mux macros/mappings.
Add pbp-pwm example crate and include it in the workspace and CI example build matrix.

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
examples/pbp/pbp-pwm/src/main.rs	New PBP example that configures PWM ch3 A/B and prints computed settings over UART
examples/pbp/pbp-pwm/build.rs	Links the example with `artinchip-rt.ld` like other PBP examples
examples/pbp/pbp-pwm/README.md	Build/pack instructions for generating the `.pbp` image
examples/pbp/pbp-pwm/Cargo.toml	New example crate manifest + feature wiring to `d13x`
artinchip-rt/src/soc/d13x.rs	Adds PWM peripheral base address, `Peripherals.pwm`, and D13x PWM pin mux mappings
artinchip-rt/src/macros/pin_mux.rs	Adds `pwm_a!` / `pwm_b!` pin-mux macros generating `into_pwm_ch*_a/b()` helpers
artinchip-hal/src/pwm/register.rs	New PWM register block + typed bitfield helpers + layout/unit tests
artinchip-hal/src/pwm/pwm_ext.rs	Adds `PwmExt` trait to create channel drivers
artinchip-hal/src/pwm/pad.rs	Adds pad marker traits and `PwmPads` tuple impl
artinchip-hal/src/pwm/instance.rs	Adds `PwmChannel`/`PwmChannels` instances and `PwmExt` impl
artinchip-hal/src/pwm/driver.rs	Adds the PWM channel driver (clock init, period/duty APIs, enable/disable/free)
artinchip-hal/src/pwm/config.rs	Adds `PwmConfig` + default action qualifiers
artinchip-hal/src/pwm.rs	New PWM module root and re-exports
artinchip-hal/src/pad.rs	Implements PWM pad marker traits for `NoPad`
artinchip-hal/src/lib.rs	Exposes `pwm` module and exports `PwmExt`/instances in prelude/instances
Cargo.toml	Adds `examples/pbp/pbp-pwm` to workspace members
.github/workflows/Cargo.yml	Adds `pbp-pwm` to RV32 example build matrix

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-23T09:01:01Z

+    /// Counts down and `TBCTR` = `CMPB` action.
+    pub cbd: ActionMode,
+    /// Counts up and `TBCTR` = `CMPA` action.
+    pub cbu: ActionMode,
+    /// Counts down and `TBCTR` = `CMPA` action.


Field doc for cbu is incorrect: it says "Counts up and TBCTR = CMPA" but CBU corresponds to the CMPB-up event (see AqControl::set_cbu_mode docs). Please fix the comment to avoid confusing users configuring actions.

Copilot · 2026-03-23T09:01:01Z

+            });
+
+            // Set clock div.
+            let ch_tb_div = (Self::DEFAULT_PWM_CLK.saturating_div(config.tb_clk_rate.0) - 1) as u16;


set_clk_div calculation can underflow or produce an invalid divider. If config.tb_clk_rate.0 is 0, saturating_div returns u32::MAX; if it’s > DEFAULT_PWM_CLK, the division yields 0 and the subsequent - 1 wraps. Consider validating tb_clk_rate (non-zero, <= source clock) and using checked_div/saturating_sub(1) with a sensible clamp (e.g., 0..=0x0FFF).

Suggested change

let ch_tb_div = (Self::DEFAULT_PWM_CLK.saturating_div(config.tb_clk_rate.0) - 1) as u16;

let src_clk_hz = Self::DEFAULT_PWM_CLK;

// Clamp requested tb_clk_rate to a safe, non-zero range within the source clock.

let requested_tb_clk_hz = config.tb_clk_rate.0;

let clamped_tb_clk_hz =

core::cmp::max(1, core::cmp::min(requested_tb_clk_hz, src_clk_hz));

// Compute divider: src_clk / tb_clk - 1, with saturation and hardware-range clamp.

let mut div = src_clk_hz

.checked_div(clamped_tb_clk_hz)

.unwrap_or(1)

.saturating_sub(1);

// Hardware divider is typically limited; clamp to 0x0FFF as a safe upper bound.

div = core::cmp::min(div, 0x0FFF);

let ch_tb_div = div as u16;

Copilot · 2026-03-23T09:01:01Z

+    pub fn set_period_and_duty(&mut self, period: u32, duty_a: u32, duty_b: u32) {
+        let channel = &self.reg.channels[I as usize];
+        self.config.freq = self.period2hertz(period);
+        self.config.period = period;
+        self.config.duty_a = duty_a;


set_period_and_duty calls period2hertz(period) without guarding period == 0, which will panic due to division by zero. Since this is a public API, either return a Result/Option for invalid inputs or explicitly assert!(period > 0) with a clear message before doing any math.

Copilot · 2026-03-23T09:01:02Z

+        let tb_clk = self.config.tb_clk_rate.0;
+        let freq = self.config.freq.0;
+        let prd_reg_val = if self.config.cnt_mode == CntMode::CountUpAndDown {
+            ((tb_clk / freq) / 2).min(u16::MAX as u32) as u16
+        } else {
+            ((tb_clk / freq) - 1).min(u16::MAX as u32) as u16


prd_reg_val computation can divide by zero / underflow: freq can become 0 (e.g., large period causing 1_000_000_000 / period == 0), and ((tb_clk / freq) - 1) will also underflow when tb_clk < freq. Consider computing period ticks directly from period and tb_clk_rate, or at least clamp freq to >= 1 and use saturating_sub(1)/checked_div to avoid wraparound.

Suggested change

let tb_clk = self.config.tb_clk_rate.0;

let freq = self.config.freq.0;

let prd_reg_val = if self.config.cnt_mode == CntMode::CountUpAndDown {

((tb_clk / freq) / 2).min(u16::MAX as u32) as u16

} else {

((tb_clk / freq) - 1).min(u16::MAX as u32) as u16

let tb_clk = self.config.tb_clk_rate.0 as u64;

let period_ns = period as u64;

let ticks = tb_clk

.saturating_mul(period_ns)

/ 1_000_000_000u64;

let prd_reg_val = if self.config.cnt_mode == CntMode::CountUpAndDown {

(ticks / 2).min(u16::MAX as u64) as u16

} else {

ticks

.saturating_sub(1)

.min(u16::MAX as u64) as u16

Copilot · 2026-03-23T09:01:02Z

+        let period_ns = (1_000_000_000 / freq.0).max(1);
+        let duty_ns_a = ((period_ns as f32 * ratio_a / 100.0) + 0.5) as u32;
+        let duty_ns_b = ((period_ns as f32 * ratio_b / 100.0) + 0.5) as u32;
+        let duty_ns_a = duty_ns_a.min(period_ns);
+        let duty_ns_b = duty_ns_b.min(period_ns);
+        self.config.freq = freq;


set_freq_and_ratio documents freq must be > 0, but period_ns is computed with 1_000_000_000 / freq.0 before any validation. If freq.0 == 0, this will panic (division by zero). Add an early check (e.g., return Result or clamp to 1 Hz) before performing the division.

Suggested change

let period_ns = (1_000_000_000 / freq.0).max(1);

let duty_ns_a = ((period_ns as f32 * ratio_a / 100.0) + 0.5) as u32;

let duty_ns_b = ((period_ns as f32 * ratio_b / 100.0) + 0.5) as u32;

let duty_ns_a = duty_ns_a.min(period_ns);

let duty_ns_b = duty_ns_b.min(period_ns);

self.config.freq = freq;

let freq_hz = freq.0.max(1);

let period_ns = (1_000_000_000 / freq_hz).max(1);

let duty_ns_a = ((period_ns as f32 * ratio_a / 100.0) + 0.5) as u32;

let duty_ns_b = ((period_ns as f32 * ratio_b / 100.0) + 0.5) as u32;

let duty_ns_a = duty_ns_a.min(period_ns);

let duty_ns_b = duty_ns_b.min(period_ns);

self.config.freq = Hertz(freq_hz);

Copilot · 2026-03-23T09:01:02Z

+    /// Free the PWM channel and return PwmChannel instance, port A and B pads.
+    pub fn free(self, cmu: &Cmu) -> (PwmChannel<I>, PAD) {
+        let mut reset_is_needed = true;
+        // Check if any channel is still working.
+        for ch in 0..4 {
+            if self.reg.ck_ctrl.read().is_ch_clk_enabled(ch) {
+                reset_is_needed = false;
+            }
+        }
+
+        if reset_is_needed {


free() never disables this channel’s clock (ck_ctrl) or module enable bit (m_ctrl) before checking whether any channels are still active. As written, reset_is_needed will almost always become false because the current channel clock remains enabled, so the PWM module may never be reset/clock-gated. Consider disabling channel I (module + clock) first, then checking remaining channels, and optionally disabling reg.ctrl when the last channel is released.

Copilot · 2026-03-23T09:01:02Z

+//! DMA instance.
+
+use super::config::PwmConfig;
+use super::driver::PwmChDriver;
+use super::pad::PwmPads;
+use super::pwm_ext::PwmExt;


Module-level doc comment says "DMA instance" but this file defines PWM types (PwmChannel, PwmChannels). Please correct the docstring to avoid misleading generated documentation and searches.

luojia65

LGTM with minor modifications :)

luojia65 · 2026-03-23T11:00:33Z

+    /// Unit: nanoseconds.
+    pub duty_a: u32,
+    /// Unit: nanoseconds.
+    pub duty_b: u32,
+    /// Unit: nanoseconds.
+    pub period: u32,


Can we use embedded_time::duration::Nanoseconds here? (where TimeInt = u32, which means Nanoseconds<u32>)

Signed-off-by: Chongbing Yu <nanahigh@openatom.club>

NanaHigh · 2026-03-23T13:23:46Z

LGTM with minor modifications :)

Modifications are completed.

NanaHigh assigned luojia65 Mar 23, 2026

luojia65 requested a review from Copilot March 23, 2026 08:56

Copilot started reviewing on behalf of luojia65 March 23, 2026 08:56 View session

Copilot AI reviewed Mar 23, 2026

View reviewed changes

luojia65 approved these changes Mar 23, 2026

View reviewed changes

feat(hal): implement PWM with example

8c6a8cb

Signed-off-by: Chongbing Yu <nanahigh@openatom.club>

NanaHigh force-pushed the feat/hal branch from 7d130b3 to 8c6a8cb Compare March 23, 2026 13:19

luojia65 approved these changes Mar 23, 2026

View reviewed changes

luojia65 merged commit 43ed195 into rustsbi:main Mar 23, 2026
27 checks passed

NanaHigh deleted the feat/hal branch March 24, 2026 01:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(hal): implement PWM with example#18

feat(hal): implement PWM with example#18
luojia65 merged 1 commit into
rustsbi:mainfrom
NanaHigh:feat/hal

NanaHigh commented Mar 23, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 23, 2026

Uh oh!

Copilot AI Mar 23, 2026

Uh oh!

Copilot AI Mar 23, 2026

Uh oh!

Copilot AI Mar 23, 2026

Uh oh!

Copilot AI Mar 23, 2026

Uh oh!

Copilot AI Mar 23, 2026

Uh oh!

Copilot AI Mar 23, 2026

Uh oh!

luojia65 left a comment

Uh oh!

luojia65 Mar 23, 2026

Uh oh!

NanaHigh commented Mar 23, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

-            let ch_tb_div = (Self::DEFAULT_PWM_CLK.saturating_div(config.tb_clk_rate.0) - 1) as u16;
+            let src_clk_hz = Self::DEFAULT_PWM_CLK;
+            // Clamp requested tb_clk_rate to a safe, non-zero range within the source clock.
+            let requested_tb_clk_hz = config.tb_clk_rate.0;
+            let clamped_tb_clk_hz =
+                core::cmp::max(1, core::cmp::min(requested_tb_clk_hz, src_clk_hz));
+            // Compute divider: src_clk / tb_clk - 1, with saturation and hardware-range clamp.
+            let mut div = src_clk_hz
+                .checked_div(clamped_tb_clk_hz)
+                .unwrap_or(1)
+                .saturating_sub(1);
+            // Hardware divider is typically limited; clamp to 0x0FFF as a safe upper bound.
+            div = core::cmp::min(div, 0x0FFF);
+            let ch_tb_div = div as u16;

Uh oh!

Conversation

NanaHigh commented Mar 23, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

luojia65 left a comment

Choose a reason for hiding this comment

Uh oh!

luojia65 Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

NanaHigh commented Mar 23, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants