Skip to content

stm32/dac: impl. write for gpdma and other cleanups#6513

Merged
xoviat merged 5 commits into
embassy-rs:mainfrom
xoviat:dac-gpdma
Jul 17, 2026
Merged

stm32/dac: impl. write for gpdma and other cleanups#6513
xoviat merged 5 commits into
embassy-rs:mainfrom
xoviat:dac-gpdma

Conversation

@xoviat

@xoviat xoviat commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

closes #6500.

@RaresAndrei0

Copy link
Copy Markdown
Contributor

I have a STM32U545RE and I can't seem to make a successful transfer using GPDMA using this version of write, I might need some guidance.
From my research, the GPDMA transfer on my board won't start unless connected to a hardware timer trigger, and it seems your PR completely ditched the timer trgo select on the control register.
Of course, I can be setting up the transfer the wrong way...

RaresAndrei0 added a commit to RaresAndrei0/embassy that referenced this pull request Jul 15, 2026
@RaresAndrei0

Copy link
Copy Markdown
Contributor

Thanks for the help, seems like the GPDMA works now

@xoviat
xoviat added this pull request to the merge queue Jul 17, 2026
Comment on lines +182 to +185
pub const fn no_packing() -> bool {
false
}

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 think this could be an enum instead

enum Packing {
Disabled,
Enabled
}

Comment on lines +355 to +358
pub const fn no_packing() -> Pam {
Pam::ZeroExtendOrLeftTruncate
}

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.

Any reason we need this and we couldn't use Pam::ZeroExtendOrLeftTruncate directly?

fn new_inner<T: Instance, C: Channel>(
_peri: Peri<'d, T>,
peri: Option<Peri<'d, T>>,
trigger: Option<u8>,

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.

Could you comment what the u8 value is supposed to represent? What valid values would be, etc

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.

Could be worthwhile to comment the other parameters as well, since its non-obvious when something should be None, vs Some, what the values should be, etc

Comment on lines +353 to +358
let _guard = OnDrop::new(|| {
self.info.regs.cr().modify(|w| {
w.set_en(self.idx, false);
w.set_dmaen(self.idx, false);
})
});

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.

As a personal preference, I would think its more obvious if we just add these disable commands at the end of the function, since there is no other place this would get dropped (if I am not missing something)

};

// Initiate the correct type of DMA transfer depending on what data is passed
let tx_f = unsafe { dma.write_raw(W::dma_buf(data), W::dma_ptr(self.info.regs, self.idx), tx_options) };

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.

Safety comment

Merged via the queue into embassy-rs:main with commit 696b630 Jul 17, 2026
7 checks passed
@xoviat
xoviat deleted the dac-gpdma branch July 17, 2026 11:19
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.

3 participants