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

Add function to enable multiple PWM slices at the same time #668

Merged

Conversation

jlpettersson
Copy link
Contributor

This is needed to be able to have e.g. 3 PWM slices that are in phase, but may use different duty cycles.

Example usage: pwm_slices.enable_simultaneous(0b0000_0111);

Demo

Phases between slices before this fix, using

    pwm0.enable();
    pwm1.enable();

IMG-6377

Phases between slices after this fix, using

pwm_slices.enable_simultaneous(0b0000_0111);

IMG-6378

This is needed to be able to have e.g. 3 PWM slices that are in phase,
but may use different duty cycles.

Example usage: `pwm_slices.enable_simultaneous(0b0000_0111);`
Copy link
Member

@ithinuel ithinuel left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution.

I would've expected to find this as a type-level slice group operation.
This would save one to have to figure the right bit pattern.

Also, don't we need a way do synchronously disable them as well?

@jannic
Copy link
Member

jannic commented Aug 9, 2023

I would've expected to find this as a type-level slice group operation. This would save one to have to figure the right bit pattern.

I agree that the API could be more user friendly. However, as I have no idea how much effort such a better API would be, I wouldn't reject this simple API.

Also, don't we need a way do synchronously disable them as well?

Not necessarily. The reason for synchronously enabling the signals is not to avoid a few cycles of delay between them becoming active, but to make sure the pulses are in phase. That's not an issue when turning the signals off.

@jannic
Copy link
Member

jannic commented Aug 9, 2023

It is possible to implement a function without a bit pattern parameter like this:

    pub fn enable_simultaneously(&mut self, slices: &[&dyn SliceBitmask]) {
        // Enable all slices at the same time
        let bits: u32 = slices.iter().fold(0u32, |a, b| a | b.bitmask());
        unsafe {
            let reg = self._pwm.en.as_ptr();
            write_bitmask_set(reg, bits);
        }
    }

with

impl<I, M> Sealed for Slice<I, M>
where
    I: SliceId,
    M: SliceMode + ValidSliceMode<I>,
 {}
 
pub trait SliceBitmask
where Self: Sealed
{
    fn bitmask(&self) -> u32;
}

impl<I, M> SliceBitmask for Slice<I, M>
where
    I: SliceId,
    M: SliceMode + ValidSliceMode<I>,
{
    fn bitmask(&self) -> u32 {
        self.bitmask()
    }
}

However I think that it's still not a nice API, because you need references to both Slices and to the individual Slice instances which are also part of Slices. This easily leads to borrow checker issues like "error[E0502]: cannot borrow pwm_slices as mutable because it is also borrowed as immutable".

@thejpster
Copy link
Member

I can't tell the difference between the two photos, but the diff looks OK to me.

@jlpettersson
Copy link
Contributor Author

jlpettersson commented Aug 22, 2023

@thejpster ah, I should have explained the pictures better.

The purple square wave is not centered (exactly in phase) within the cyan square wave. In the first picture, the purple is a little bit to the right, but in the second image, it is perfectly in the center of the cyan square wave. (because in the second picture, this feature was used to start the square waves in an atomic operation.

Perhaps this part of the first image explains it best, that the purple square wave are not in phase.
zoomed

@jannic jannic added this to the 0.10.0 milestone Aug 24, 2023
@jannic
Copy link
Member

jannic commented Aug 24, 2023

I placed this in milestone 0.10.0 for now, just to make it obvious that 0.9.0 doesn't need to wait for this PR. It's not a breaking change and can be applied at any time.

@jannic jannic modified the milestones: 0.10.0, 0.9.1 Oct 27, 2023
@jannic jannic added enhancement New feature or request non-breaking change This pull requests contains a minor, not SemVer breaking, change labels Oct 27, 2023
@jannic jannic merged commit 357f4d6 into rp-rs:main Nov 10, 2023
8 checks passed
@jlpettersson jlpettersson deleted the enable_multiple_pwm_slices_atomically branch November 10, 2023 23:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request non-breaking change This pull requests contains a minor, not SemVer breaking, change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants