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.

Sign up for GitHub

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

Jump to bottom

cpu: efm32: provide non-standard UART modes #9127

Merged
merged 3 commits into from
Jun 4, 2018
Merged

cpu: efm32: provide non-standard UART modes #9127

merged 3 commits into from
Jun 4, 2018

Conversation

basilfx
Copy link
Member

@basilfx basilfx commented May 13, 2018 •
edited
Loading

Contribution description

I need non-standard UART modes for some (KNX) transceivers that want 8E1. This PR adds them as an option to the uart_config struct, similar to how it was done for some Kinetis boards. The default remains 8N1, for both USART and LEUART.

Conversion methods were added to Gecko-SDK (see this commit), which minimizes the changes in the RIOT code and provides a clean abstraction for both USART and LEUART.

Issues/PRs references

#5899, #7165

@basilfx basilfx added Platform: ARM Platform: This PR/issue effects ARM-based platforms CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Area: boards Area: Board ports labels May 13, 2018
@basilfx basilfx requested a review from kYc0o May 14, 2018 06:12
@basilfx
Copy link
Member Author

basilfx commented May 25, 2018

Ping @kYc0o

kYc0o
kYc0o reviewed May 25, 2018
View reviewed changes
cpu/efm32/periph/uart.c Outdated
@@ -104,6 +109,9 @@ int uart_init(uart_t dev, uint32_t baudrate, uart_rx_cb_t rx_cb, void *arg)

init.enable = leuartDisable;
init.baudrate = baudrate;
init.databits = LEUART_DataBits2Def((uart_config[dev].mode >> 0) & 0xf);
init.stopbits = LEUART_StopBits2Def((uart_config[dev].mode >> 4) & 0xf);
init.parity = LEUART_Parity2Def((uart_config[dev].mode >> 8) & 0xf);
Copy link
Contributor

Choose a reason for hiding this comment

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

So, the baud rate will only change on init, then it stays like that as I see? Which means once the UART is attached to a device it cannot be detached is it?

Copy link
Member Author

@basilfx basilfx May 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, that's true. #5899 proposes a runtime option, but it is not merged/fixed/proposed.

STM32 Kinetis does the same, iirc.

kYc0o
kYc0o reviewed May 25, 2018
View reviewed changes
pkg/gecko_sdk/Makefile Outdated
@@ -1,6 +1,6 @@
PKG_NAME=gecko_sdk
PKG_URL=https:/basilfx/RIOT-gecko-sdk
PKG_VERSION=d381e526d68a2d0c951f37040c1c2e168ac66cd6
PKG_VERSION=ee01ab57eaaa8805916601dd444d1b63f03037de
Copy link
Contributor

Choose a reason for hiding this comment

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

I see there's an update to the gecko sdk. Is this intended here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, the update is intended, as they provide the utility methods to convert bits to enums.

@kYc0o
Copy link
Contributor

kYc0o commented May 25, 2018

I also wonder how is this configurable at compile time. The only way to change the behaviour is to modify the source code, which seems not super practical. Maybe you can add an #ifdef on the headers (which would be also a kind of API change, but only as a parameter) and then pass the config through CFLAGS? Otherwise even if there are reasons to have this PR in, it doesn't really give the option and the behaviour stays unchanged (with some additional bytes to the final binary because of the new operations).

@basilfx
Copy link
Member Author

basilfx commented May 25, 2018 •
edited
Loading

@kYc0o As we discussed on IRC, I have updated this PR to make this feature optional (in favor of code size, because the current boards/configuration doesn't make use of this feature). I did not make it configurable for each UART interface, since I think that if this is a desired feature, we should fix #5899.

I opted for a solution that I can reuse to make other parts of EFM32 optional, like disabling support for low-power peripherals (if not used). So the last two commits may seem like a lot, I want to re-use them and provide a uniform interface/place for EFM32 specific features/toggles.

Note that Murdock won't test the code, but if you manually test it and provide EFM32_UART_MODES=1 on the command line, you will see that it increases the code size.

@kaibeckmann kaibeckmann mentioned this pull request May 28, 2018
Closed
12 tasks
@basilfx basilfx force-pushed the feature/efm32_uart_modes branch 5 times, most recently from c6fc3c7 to a4734cd Compare June 4, 2018 16:01
@kYc0o
Copy link
Contributor

kYc0o commented Jun 4, 2018

Ok, I think we're good here. You may squash.

@basilfx
Copy link
Member Author

basilfx commented Jun 4, 2018

Squashed and rebased.

kYc0o
kYc0o approved these changes Jun 4, 2018
View reviewed changes
Copy link
Contributor

@kYc0o kYc0o left a comment

Choose a reason for hiding this comment

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

ACK and go!

@kYc0o kYc0o merged commit 188886d into RIOT-OS:master Jun 4, 2018
This was referenced Jun 11, 2018
Closed
Merged
Merged
@cladmi cladmi added this to the Release 2018.07 milestone Jul 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kYc0o kYc0o kYc0o approved these changes

Assignees
No one assigned
Labels
Area: boards Area: Board ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms
Projects
None yet
Milestone
Release 2018.07
Development

Successfully merging this pull request may close these issues.
3 participants
@basilfx @kYc0o @cladmi