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

[uart] Refactor STM32 USART HAL and make TX/RX buffer optional #564

Merged
merged 5 commits into from
Feb 23, 2021

Conversation

salkinium
Copy link
Member

@salkinium salkinium commented Feb 19, 2021

  1. Refactors the UsartHal class for STM32 for clarity and less boilerplate and fixes an issue with word length selection.
  2. Removes the :platform:uart:N:buffered option for STM32, and instead allows the buffer.tx and buffer.rx options to be set to 0, which removes the atomic queue and IRQ code separately for both.
  3. Removes :platform:uart:N:buffered option for AVRs as well, as it was never used.
  4. Sets the default UART buffer sizes to 0, to not waste code/memory unnecessarily. This is breaking change, so in some examples the buffer.tx option needs to be manually set. All of our BSPs with a logger output already set this option

@chris-durand
Copy link
Member

Nice work, but are you aware this is a silent breaking change for users on F1/F4 that have been using even/odd parity? With the refactored version initialize(Parity::Odd) will result in 7 bit data + parity whereas it has been 8 bit + parity before. This also applies to the higher level Uart interface that calls UartHal::initialize().

It would be possible to add an additional initialize() overload with the word length instead of the defaulted argument to preserve the current behaviour.

@chris-durand
Copy link
Member

setParity() was setting the M flag for 9 bit word length once parity was enabled resulting in 8 data bits. That should have worked for controllers that did not have M1 and M0.
Old code from setParity() before refactoring:

if (parity != Parity::Disabled) {
    // Parity Bit counts as 9th bit -> enable 9 data bits
    flags |= USART_CR1_M;
} 

@salkinium salkinium force-pushed the feature/stm32-uart-hal-buffers branch from 417e273 to 5d9c2a7 Compare February 19, 2021 23:17
@salkinium
Copy link
Member Author

salkinium commented Feb 19, 2021

I made the Hal::initialize() method non-default argumented, so that it will definitely not silently fail. I think a breaking change in the Hal class is ok, but I made a second Uart::initialize() method to preserve existing behavior in the interface class.

@salkinium salkinium force-pushed the feature/stm32-uart-hal-buffers branch from 0e423af to 13a2f3d Compare February 20, 2021 20:03
@salkinium
Copy link
Member Author

I would want to merge this before the SCons thing, so do a review on this first.

@rleh rleh requested review from rleh and chris-durand February 21, 2021 15:48
@salkinium salkinium force-pushed the feature/stm32-uart-hal-buffers branch from b43d014 to ab6d025 Compare February 21, 2021 16:55
src/modm/platform/uart/sam/uart_hal.hpp.in Outdated Show resolved Hide resolved
src/modm/platform/uart/sam/uart_hal.hpp.in Outdated Show resolved Hide resolved
template<class SystemClock, modm::baudrate_t baudrate>
void modm_always_inline
modm::platform::{{ name }}::initialize(Parity parity)
template<class SystemClock, modm::baudrate_t baudrate, modm::percent_t tolerance>
Copy link
Member

Choose a reason for hiding this comment

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

If tolerance is added here, shouldn't we actually check it? Adding the baudrate calculation with compile-time checking would be quite straight forward, it's pretty similar to STM32.

There are also 8x and 16x oversampling modes, selected by setting {{ peripheral }}->USART.CTRLA.bit.SAMPR (16x: 0x1, 8x: 0x3), see SAMD5/E5 manual, page 830,854,862. The prescaler has a 3 bit fractional and 13 bit integral part for both x8 and x16 oversampling (configured in line 87, 88 of this file).

It looks like there is a copy-paste bug in these lines anyway, they always configure the instance SERCOM0->USART, not {{ peripheral }}->USART. That should definitively be fixed.

Copy link
Member Author

Choose a reason for hiding this comment

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

I fixed this, but I still need to test this in hardware, since it may be off by a factor or 2.

@salkinium salkinium force-pushed the feature/stm32-uart-hal-buffers branch 2 times, most recently from a278eff to 5c208aa Compare February 22, 2021 02:58
src/modm/platform/uart/sam/uart_hal.hpp.in Show resolved Hide resolved
Comment on lines 78 to 79
static inline void
initialize({% if buffered %}uint32_t interruptPriority, {% endif %}Parity parity, WordLength length)
Copy link
Member

Choose a reason for hiding this comment

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

Can we please change the interface here to make code portable between buffered and unbuffered Uart?

Suggested change
static inline void
initialize({% if buffered %}uint32_t interruptPriority, {% endif %}Parity parity, WordLength length)
static inline void
initialize(Parity parity, WordLength length{% if buffered %}, uint32_t interruptPriority, {% endif %})

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, please. I was uncertain about breaking backwards compatibility.
I'm not at all in favor of bundling NVIC settings with the interfaces, mostly because of the issue with shared interrupts, which completely breaks the API promises (ie. effectively showing the same shared IRQ as different IRQs).

I would perhaps leave it like that for now, since it's backwards compatible and instead add a new driver call NVIC, that perhaps exposes IRQ handling in a better way. (perhaps with a connect-like API or something like that).

Copy link
Member Author

Choose a reason for hiding this comment

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

Well… actually, we're already breaking the API with the lbuild option changes, so it makes sense to fix this now!

Copy link
Member

Choose a reason for hiding this comment

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

If I were a user not closely monitoring modm development, I would generally prefer to have some changelogs telling me if and how my code gets broken. Could we get the xpcc changelog back without much effort, could we generate it semi-automatically? People are putting modm into real products, so we should care somehow ...

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I need to bring the changelog back… Ideally as part of the PR approval somehow, so that the mountain of work doesn't dull my motivation to do quarterly releases.
https://twitter.com/modm_embedded/status/1345396075813859328

Copy link
Member Author

@salkinium salkinium Mar 31, 2021

Choose a reason for hiding this comment

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

I'm bringing Sexy Changelog back
Them other boys libs don't know how to act

Comment on lines 93 to 95
template< class SystemClock, baudrate_t baudrate, percent_t tolerance=pct(1) >
static inline void
initialize({% if buffered %}uint32_t interruptPriority = 12, {% endif %}Parity parity = Parity::Disabled)
Copy link
Member

Choose a reason for hiding this comment

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

Same as above

@salkinium salkinium force-pushed the feature/stm32-uart-hal-buffers branch 2 times, most recently from 9ad2c8d to 8b34cf0 Compare February 23, 2021 01:05
@salkinium
Copy link
Member Author

I broke the initialize API and removed the automatic enable/disable Operation calls. Also collapsed a few HAL functions into one place, like the UART baudrate computation and the setSpiClock.

@salkinium salkinium force-pushed the feature/stm32-uart-hal-buffers branch from 8b34cf0 to 2c9d925 Compare February 23, 2021 14:44
@salkinium salkinium merged commit 2c9d925 into modm-io:develop Feb 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants