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

STM32: combine RccPeripherals reset() and enable() to enable_and_reset() #2035

Merged
merged 4 commits into from
Oct 12, 2023
Merged

STM32: combine RccPeripherals reset() and enable() to enable_and_reset() #2035

merged 4 commits into from
Oct 12, 2023

Conversation

pbert519
Copy link
Contributor

@pbert519 pbert519 commented Oct 10, 2023

Close #2034

Combining the standalone functions reset() and enable() into enable_and_reset().

This are a lot of breaking changes, maybe we should add reset_and_enable(), but keep enable() and reset().
And port only the obvious usages.
I'm not able to test everything on real hardware.

@pbert519 pbert519 marked this pull request as ready for review October 11, 2023 19:06
@pbert519
Copy link
Contributor Author

The DAC peripheral has also shared RCC bits for DAC1 und DAC2 in some h7 controllers.
I tried to remove the workaround, however the build.rs script did not generate impl RccPeripherals for DAC1 for the stm32h742xx-cm7 target, all other targets seems to work.
Not sure how to proceed there

@Dirbaio
Copy link
Member

Dirbaio commented Oct 11, 2023

Thanks for the PR!

The code should first enable the peripherals then reset them. Reset requires the clocks to be running, otherwise peripheral state can't change. Could you change that, and rename the function?

the build.rs script did not generate impl RccPeripherals for DAC1 for the stm32h742xx-cm7 target, all other targets seems to work.

the data comes from stm32-data. see here, DAC1 is missing the rcc field. Compare with this which is on another chip that does have it.

The code that does the matching is here. There's exceptions hardcoded for ADC, seems DAC needs some too.

(if you want to merge this PR now and fix that later that's fine, this is already a big improvement even if it doesn't fix everything. Up to you!)

@pbert519
Copy link
Contributor Author

The code should first enable the peripherals then reset them. Reset requires the clocks to be running, otherwise peripheral state can't change. Could you change that, and rename the function?

Done. It's probably better to assume the reset is synchronous. It's a pity that st is not able to define this.

I'll have a look into the DAC topic, but we should split it into a dedicated PR.

@pbert519 pbert519 changed the title STM32: combine RccPeripherals reset() and enable() to reset_and_enable() STM32: combine RccPeripherals reset() and enable() to enable_and_reset() Oct 11, 2023
@@ -531,10 +531,13 @@ pub struct SubBlock<'d, T: Instance, C: Channel, W: word::Word> {
pub struct SubBlockA {}
pub struct SubBlockB {}

pub struct SubBlockAPeripheral<'d, T>(PeripheralRef<'d, T>);
Copy link
Member

Choose a reason for hiding this comment

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

could you take these unrelated changes out? thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This aren't unrelated changes.
While the SAI is only one Peripheral Block, it contains two complety independent subblocks.
Currently it is possible to create a subblock directly, or frist create a SAI peripheral, which just copies the PeripheralRef, so we can initalise both subblocks.
Question, where do we call enable_and_reset()?

This changes force the user to create a SAI first, which enables clock and reset, and then use the take_sub_block to get a SubBlockXPeripheral to initalise the Subblock

Copy link
Contributor Author

@pbert519 pbert519 Oct 12, 2023

Choose a reason for hiding this comment

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

Or in other words: For some Reason ST did not create a SAI1 and SAI2 Peripheral, but only SAI1 which actually contains two independet Peripherals.

Copy link
Member

Choose a reason for hiding this comment

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

ahh got it, makes sense, thank you!

Copy link
Member

@Dirbaio Dirbaio 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!

@Dirbaio Dirbaio added this pull request to the merge queue Oct 12, 2023
Merged via the queue into embassy-rs:main with commit 66e399b Oct 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.

Not possible to use both ADCs with STM32H7
2 participants