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

drivers/periph: Remove UART from device_enums.h #14916

Merged
merged 7 commits into from
Sep 9, 2020

Conversation

MrKevinWeiss
Copy link
Contributor

@MrKevinWeiss MrKevinWeiss commented Aug 31, 2020

Contribution description

In order to remove the need for the legacy device_enums and clean things up all boards using UART definitions from device_enums have been updated.

This includes:
ek-lm4f120xl - Since only UART0 was implemented, asserts ensure only that uart is implemented.
mbed_lpc1768 - Implementation for all uarts are added assuming pins are together, periph conf was updated for uart 1 to be on p9 and p10 since they were previously inaccessable
msb-430 - Implementation does not require device_enums.h
msb-430h - Implementation does not require device_enums.h
nrf6310 - Implementation does not require device_enums.h
seeeduino_arch-pro - Implementation for all uarts are added assuming pins are together
telosb - Implementation does not require device_enums.h
z1 - Implementation does not require device_enums.h

Some generic cleanup also applied to files as new static checks have shown warnings.

Testing procedure

Use tests/periph_uart to ensure communication (stdio uart) and any additional uarts.

The lpc1768 cpu change was verified with the mbed_lpc1768 by enabling uart 1 and reading loopback messages.

Issues/PRs references

Step forward with #7941

@MrKevinWeiss MrKevinWeiss added Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Aug 31, 2020
@MrKevinWeiss MrKevinWeiss self-assigned this Aug 31, 2020
@MrKevinWeiss
Copy link
Contributor Author

...this has been an effective form of procrastination.

@MrKevinWeiss
Copy link
Contributor Author

sorry for not running static checks... Should be better now, but we will see.

@MrKevinWeiss
Copy link
Contributor Author

I can wait until this doc_check issue is solved as it seems to be more a CI problem.

@MrKevinWeiss
Copy link
Contributor Author

Looks like CI is happy, just waiting for a review.

Copy link
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

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

Just some minor style improvements, otherwise that looks OK.

/* Check to make sure the UART peripheral is present */
if(!ROM_SysCtlPeripheralPresent(SYSCTL_PERIPH_UART0)){
if (!ROM_SysCtlPeripheralPresent(SYSCTL_PERIPH_UART0)){
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (!ROM_SysCtlPeripheralPresent(SYSCTL_PERIPH_UART0)){
if (!ROM_SysCtlPeripheralPresent(SYSCTL_PERIPH_UART0)) {

return UART_NODEV;
}

int res = init_base(uart, baudrate);
if(res != UART_OK){
if (res != UART_OK){
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (res != UART_OK){
if (res != UART_OK) {

Comment on lines 132 to 135
if (ulStatus & (UART_INT_RX | UART_INT_RT))
{
while(ROM_UARTCharsAvail(UART0_BASE))
while (ROM_UARTCharsAvail(UART0_BASE))
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (ulStatus & (UART_INT_RX | UART_INT_RT))
{
while(ROM_UARTCharsAvail(UART0_BASE))
while (ROM_UARTCharsAvail(UART0_BASE))
{
if (ulStatus & (UART_INT_RX | UART_INT_RT)) {
while (ROM_UARTCharsAvail(UART0_BASE)) {

(doesn't render well sorry)

@MrKevinWeiss
Copy link
Contributor Author

Thanks, will get to it tomorrow morning!

@aabadie aabadie self-assigned this Sep 9, 2020
@MrKevinWeiss
Copy link
Contributor Author

rebased and squash with the fixes (hope that is OK).

Copy link
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

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

I trust your testing on mbed_lpc1768 which has the more in-depth changes, other CPU changes are straight forward.

ACK

@aabadie aabadie merged commit 9b9ed23 into RIOT-OS:master Sep 9, 2020
@MrKevinWeiss MrKevinWeiss deleted the pr/slwstk6220a branch September 9, 2020 09:46
@MrKevinWeiss
Copy link
Contributor Author

Thanks for the review @aabadie !

@MrKevinWeiss
Copy link
Contributor Author

hmmm something happened with the mbed_lpc1768 and it seems to be broken now... let me look into it.

@MrKevinWeiss
Copy link
Contributor Author

That was annoying, OK should be fix -> #14990

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants