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

Fix issue #201: Use NonZeroUsize to specify DMA transfer size #203

Merged
merged 3 commits into from
Jan 12, 2023

Conversation

lonesometraveler
Copy link
Contributor

I wanted to submit a pull request to change the usage of usize to NonZeroUsize in the DMA channel enum. This fixes #201.

This is a breaking change, as NonZeroUsize is a wrapper around usize that ensures the value is non-zero. This change is being made to prevent panic when a zero value is used. Let me know if you have any questions or concerns.

Thank you for considering this change!

Copy link
Member

@MabezDev MabezDev left a comment

Choose a reason for hiding this comment

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

Looks good to me, but I'll get @zRedShift and @ivmarkov's opinions before changing the public API.

@MabezDev MabezDev requested review from ivmarkov and zRedShift January 6, 2023 14:35
Copy link
Collaborator

@zRedShift zRedShift left a comment

Choose a reason for hiding this comment

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

LGTM, but if you want to make panics impossible we could create a newtype wrapping NonZeroU16/NonZeroUsize etc., that would return an Option<Self> when given the transfer size, returning None when zero or non-multiple of 4 and saturating at 4096.

@ivmarkov
Copy link
Collaborator

@lonesometraveler - as @zRedShift mentions above, this PR is only ensuring at compile time that the DMA size is non-zero, while it is still not ensuring - at compile time - that the DMA size is <= 4096 and divisible by 4.

Given that the PR introduces a backwards-incompatible change, I think it would be only worth it if the other two constraints (<= 4096 and divisible by 4) are also ensured compile-time.

So if you would like to enhance the PR with the additional two constraints - I'm all for it. But in its current shape, I don't feel that the PR has enough weight to justify the backwards-incompatible change.

@ivmarkov
Copy link
Collaborator

@lonesometraveler Alternatively, you could implement a backwards-compatibility-preserving PR - along your original suggestion - which checks for dma size <= 4096 and dma size % 4 == 0 at runtime and panicing with a human-readable failure message.

@lonesometraveler
Copy link
Contributor Author

Thank you all for your reviews!

How about something like this? This returns None for zero and rounds up any other values to a multiple of 4, saturating at 4096.

#[derive(Debug, Copy, Clone, Eq, PartialEq)]
pub struct NonZeroTransferSize(usize);

impl NonZeroTransferSize {
    pub fn new(size: usize) -> Option<Self> {
        if size == 0 {
            None
        } else {
            // Round up to a nearest multiple of 4. Saturate at 4096.
            Some(Self(core::cmp::min((size + 3) / 4 * 4, 4096)))
        }
    }
}

#[derive(Debug, Copy, Clone, Eq, PartialEq)]
pub enum Dma {
    Disabled,
    Channel1(NonZeroTransferSize),
    Channel2(NonZeroTransferSize),
    Auto(NonZeroTransferSize),
}

impl Dma {
    pub const fn max_transfer_size(&self) -> usize {
        match self {
            Dma::Disabled => TRANS_LEN,
            Dma::Channel1(size) | Dma::Channel2(size) | Dma::Auto(size) => size.0,
        }
    }
}

I chose to round up instead of returning None for non-multiple of 4 because I wasn't sure how to tell that to users effectively. Naming it NonZeroMultipleOfFourTransferSize seemed too much.

@MabezDev
Copy link
Member

NonZeroTransferSize

I think not being able to use the standard NonZeroUsize negates all the benefits. I think now its best to revert to your original suggestions of an extra check of the length and panicking if it's zero. (Sorry for the extra work!)

@lonesometraveler
Copy link
Contributor Author

Understood. I just made the change. It panics if size is 0.

I am learning a lot from this. Thank you.

@ivmarkov
Copy link
Collaborator

Hey sorry one more request: can you also make the code panic if the size is > 4096? I know I introduced the current asymmetric behavior but in retrospective, that was not a good idea!

@ivmarkov
Copy link
Collaborator

@lonesometraveler ^^^

@lonesometraveler
Copy link
Contributor Author

Ok. It now panic if size > 4096.

@ivmarkov ivmarkov merged commit c53e790 into esp-rs:master Jan 12, 2023
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.

Error handling for DMA max transfer size set to 0
4 participants