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/usart: Changing baud rate #3512

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

EnmanuelParache
Copy link

As discussed in #1289 adding support to changing the baud rate for stm32 usart.

Ok(())
}

fn set_usart_baudrate(info: &Info, kernel_clock: Hertz, baudrate: u32) -> Result<(), ConfigError> {
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 not duplicate this chunk of code? for example making configure() call here.

Copy link
Author

Choose a reason for hiding this comment

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

Sure I can, but my only concern is that calling configure would require the developer to pass the Config struct again to avoid overwriting any previous configuration made on Usart instance construction when perhaps all it wants is just to change the baudrate all this since Config is not copied in Usart instance to reuse its values.

I noticed I can reduce code duplication by moving static DIVS to the outer scope, and wrap Usart kind matching and brr search in two separate functions to be used by both configure and set_usart_baudrate if you think this would be better.

Copy link
Member

Choose a reason for hiding this comment

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

configure does more things, so I meant make configure() call set_usart_baudrate() then do the other stuff.

Copy link
Author

Choose a reason for hiding this comment

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

Ah, sorry I misunderstood you. But yeah that makes total sense. I just pushed changes addressing this. Calling find_and_set_brr in both configure and set_baud_rate reducing code duplication. Please take a look and let me know what you think.

@EnmanuelParache EnmanuelParache force-pushed the stm32_usart_set_baudrate branch 3 times, most recently from 23b929d to 8e570da Compare November 7, 2024 19:17
@MabezDev
Copy link
Contributor

MabezDev commented Nov 8, 2024

Where does this stop if we accept this change, will there be an set/get for every item in the config? On top of that, you then have to forward many of those methods to the sub structs UartTx, UartRx, RingBufferedRx etc etc.

Imo adding multiple ways to do things can get a bit messy. I saw in the original issue the concern was not overwriting the current config I think there are two better ways to solve this:

  • Store the current config in the driver itself
    • Depending on the driver, this can be costly in memory footprint, some analysis might need to be done here
    • At least in the case of UART, the config is quite cheap
  • Add a current_config() which reads back and constructs the config
    • This might not be possible for all registers? or costly (CPU time) to reverse the calculation to get back to the source values

There is also a third:

  • Make the user in charge of storing the config and only changing what they need to (AKA reject this PR and document this)

Just some opinion of a user, feel free to ignore :)

@@ -1010,6 +1015,11 @@ impl<'d, M: Mode> UartRx<'d, M> {
}
Ok(())
}

/// Set baudrate
pub fn set_baudrate(&self, baudrate: u32) -> Result<(), ConfigError> {
Copy link
Contributor

Choose a reason for hiding this comment

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

These impls should also be extended to Buffered* and RingBuffered* structs

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the feedback, just did that. Please take a look

@vongaisberg
Copy link

This change is required to generate a DMX signal. Changing the baudrate using set_config is too slow, the pins go into floaing for ~100µs.

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.

4 participants