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 stm32 uart set_config #2105

Merged
merged 6 commits into from
Oct 24, 2023
Merged

Conversation

andresv
Copy link
Contributor

@andresv andresv commented Oct 23, 2023

  • CR1 UE must be false when setting word length and parity bits.
  • use type ConfigError = ConfigError instead of type ConfigError = ()
  • In mod.rs there is fn configure() which uses write to set CR1. This disables RXNEIE and IDLEIE in CR1 and those must be re-enabled again. Otherwise receive does not work anymore.

TODO (done)

  • What to do with fifoen, which one is correct?

#[cfg(usart_v4)]
w.set_fifoen(true);

vs
#[cfg(lpuart_v2)]
w.set_fifoen(true);

Edit: usart_v4 is correct.

TEST

Tested with stm32g081 by reconfiguring baudrate, parity and stop bits.

@xoviat xoviat added this pull request to the merge queue Oct 23, 2023
@Dirbaio Dirbaio removed this pull request from the merge queue due to a manual request Oct 23, 2023
reconfigure::<T>(config)
reconfigure::<T>(config)?;

T::regs().cr1().modify(|w| {
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 put this reg write in reconfigure instead of pasting it 3 time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reconfigure is universally used for all uart flavors. For example when buffered uart is created it calls configure and then sets other registers that are needed for this driver:

configure(r, &config, T::frequency(), T::KIND, true, true)?;
r.cr1().modify(|w| {
#[cfg(lpuart_v2)]
w.set_fifoen(true);
w.set_rxneie(true);
w.set_idleie(true);
});

So putting it to confgure is not a good place. I'll check if I can refactor it a little to remove copy-pasting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I cleaned it up a little by removing redundant w.set_fifoen(true); which is set in configure anyway.
There isn't a good place where to put those copy pasted lines without introducing more complexity.
configure configures all the basic stuff which is used in all uart flavours and then extra configuration is done in buffered.rs which is needed for this specific impl.

I would keep it as is without adding extra stuff to basic configuration.
Although I was thinking that maybe we can remove those self.set_config functions altogether and only rely on SetConfig trait as in here.

Then all the configuration related stuff is neatly together and there is less chance that somebody forgets to update something. However then again SetConfig trait must be imported by the user which is inconvenient.

Copy link
Member

Choose a reason for hiding this comment

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

oh okay! I had missed that, sounds good then! thanks :)

In reconfigure() cr1 register is initialised with write (not modify) which means rxneie and idleneie are disabled after reconfiguration.
@andresv andresv force-pushed the fix-stm32-uart-set-config branch from ca27590 to 7f72dbd Compare October 24, 2023 06:09
@andresv andresv requested review from xoviat and Dirbaio October 24, 2023 07:25
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 24, 2023
Merged via the queue into embassy-rs:main with commit b3879ec Oct 24, 2023
@andresv andresv deleted the fix-stm32-uart-set-config branch January 19, 2024 13:25
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